| Message ID | 20180520133723.30547-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: pcf2127: add support for accessing internal static RAM | expand |
Hi, On 20/05/2018 15:37:23+0200, Uwe Kleine-König wrote: > static int pcf2127_probe(struct device *dev, struct regmap *regmap, > - const char *name) > + const char *name, bool has_nvmem) > { > struct pcf2127 *pcf2127; > + int ret = 0; > > dev_dbg(dev, "%s\n", __func__); > > @@ -200,8 +242,21 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap, > > pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops, > THIS_MODULE); > + if (IS_ERR(pcf2127->rtc)) > + return PTR_ERR(pcf2127->rtc); > + > + if (has_nvmem) { > + struct nvmem_config nvmem_cfg = { > + .priv = pcf2127, > + .reg_read = pcf2127_nvmem_read, > + .reg_write = pcf2127_nvmem_write, > + .size = 512, > + }; > + > + ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg); > + } > > - return PTR_ERR_OR_ZERO(pcf2127->rtc); > + return ret; You must not return an error here once devm_rtc_device_register has succeeded. You have the choice between ignoring the nvmem registration error or switching to devm_rtc_allocate_device/rtc_register_device().
Hello Alexandre, On Sun, May 20, 2018 at 08:05:20PM +0200, Alexandre Belloni wrote: > On 20/05/2018 15:37:23+0200, Uwe Kleine-König wrote: > > @@ -200,8 +242,21 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap, > > > > pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops, > > THIS_MODULE); > > + if (IS_ERR(pcf2127->rtc)) > > + return PTR_ERR(pcf2127->rtc); > > + > > + if (has_nvmem) { > > + struct nvmem_config nvmem_cfg = { > > + .priv = pcf2127, > > + .reg_read = pcf2127_nvmem_read, > > + .reg_write = pcf2127_nvmem_write, > > + .size = 512, > > + }; > > + > > + ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg); > > + } > > > > - return PTR_ERR_OR_ZERO(pcf2127->rtc); > > + return ret; > > You must not return an error here once devm_rtc_device_register has > succeeded. Why? Sounds like something that should be fixed. Best regards Uwe
On 20/05/2018 20:40:01+0200, Uwe Kleine-König wrote: > Hello Alexandre, > > On Sun, May 20, 2018 at 08:05:20PM +0200, Alexandre Belloni wrote: > > On 20/05/2018 15:37:23+0200, Uwe Kleine-König wrote: > > > @@ -200,8 +242,21 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap, > > > > > > pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops, > > > THIS_MODULE); > > > + if (IS_ERR(pcf2127->rtc)) > > > + return PTR_ERR(pcf2127->rtc); > > > + > > > + if (has_nvmem) { > > > + struct nvmem_config nvmem_cfg = { > > > + .priv = pcf2127, > > > + .reg_read = pcf2127_nvmem_read, > > > + .reg_write = pcf2127_nvmem_write, > > > + .size = 512, > > > + }; > > > + > > > + ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg); > > > + } > > > > > > - return PTR_ERR_OR_ZERO(pcf2127->rtc); > > > + return ret; > > > > You must not return an error here once devm_rtc_device_register has > > succeeded. > > Why? Sounds like something that should be fixed. > This is the explanation: http://patchwork.ozlabs.org/patch/324263/
Hello, On Sun, May 20, 2018 at 10:08:09PM +0200, Alexandre Belloni wrote: > On 20/05/2018 20:40:01+0200, Uwe Kleine-König wrote: > > On Sun, May 20, 2018 at 08:05:20PM +0200, Alexandre Belloni wrote: > > > You must not return an error here once devm_rtc_device_register has > > > succeeded. > > > > Why? Sounds like something that should be fixed. > > > > This is the explanation: > http://patchwork.ozlabs.org/patch/324263/ So open() should get a reference to the module to ensure it is not freed while the dev is accessed. The same can happen when the driver is manually unbound while the device is accessed (which is also fixed when open() takes a reference). Best regards Uwe
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c index e83be1852c2f..9f99a0966550 100644 --- a/drivers/rtc/rtc-pcf2127.c +++ b/drivers/rtc/rtc-pcf2127.c @@ -36,6 +36,11 @@ #define PCF2127_REG_MO (0x08) #define PCF2127_REG_YR (0x09) +/* the pcf2127 has 512 bytes nvmem, pcf2129 doesn't */ +#define PCF2127_REG_RAM_addr_MSB 0x1a +#define PCF2127_REG_RAM_wrt_cmd 0x1c +#define PCF2127_REG_RAM_rd_cmd 0x1d + #define PCF2127_OSF BIT(7) /* Oscillator Fail flag */ struct pcf2127 { @@ -183,10 +188,47 @@ static const struct rtc_class_ops pcf2127_rtc_ops = { .set_time = pcf2127_rtc_set_time, }; +static int pcf2127_nvmem_read(void *priv, unsigned int offset, + void *val, size_t bytes) +{ + struct pcf2127 *pcf2127 = priv; + int ret; + unsigned char offsetbuf[] = { offset >> 8, offset }; + + ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB, + offsetbuf, 2); + if (ret) + return ret; + + ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_RAM_rd_cmd, + val, bytes); + + return ret ?: bytes; +} + +static int pcf2127_nvmem_write(void *priv, unsigned int offset, + void *val, size_t bytes) +{ + struct pcf2127 *pcf2127 = priv; + int ret; + unsigned char offsetbuf[] = { offset >> 8, offset }; + + ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB, + offsetbuf, 2); + if (ret) + return ret; + + ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_wrt_cmd, + val, bytes); + + return ret ?: bytes; +} + static int pcf2127_probe(struct device *dev, struct regmap *regmap, - const char *name) + const char *name, bool has_nvmem) { struct pcf2127 *pcf2127; + int ret = 0; dev_dbg(dev, "%s\n", __func__); @@ -200,8 +242,21 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap, pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops, THIS_MODULE); + if (IS_ERR(pcf2127->rtc)) + return PTR_ERR(pcf2127->rtc); + + if (has_nvmem) { + struct nvmem_config nvmem_cfg = { + .priv = pcf2127, + .reg_read = pcf2127_nvmem_read, + .reg_write = pcf2127_nvmem_write, + .size = 512, + }; + + ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg); + } - return PTR_ERR_OR_ZERO(pcf2127->rtc); + return ret; } #ifdef CONFIG_OF @@ -309,11 +364,11 @@ static int pcf2127_i2c_probe(struct i2c_client *client, } return pcf2127_probe(&client->dev, regmap, - pcf2127_i2c_driver.driver.name); + pcf2127_i2c_driver.driver.name, id->driver_data); } static const struct i2c_device_id pcf2127_i2c_id[] = { - { "pcf2127", 0 }, + { "pcf2127", 1 }, { "pcf2129", 0 }, { } }; @@ -372,11 +427,12 @@ static int pcf2127_spi_probe(struct spi_device *spi) return PTR_ERR(regmap); } - return pcf2127_probe(&spi->dev, regmap, pcf2127_spi_driver.driver.name); + return pcf2127_probe(&spi->dev, regmap, pcf2127_spi_driver.driver.name, + spi_get_device_id(spi)->driver_data); } static const struct spi_device_id pcf2127_spi_id[] = { - { "pcf2127", 0 }, + { "pcf2127", 1 }, { "pcf2129", 0 }, { } };
The PCF2127 has 512 bytes of internal static RAM and this patch expands the driver to access this memory. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 6 deletions(-)