Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework
On 4/6/19 12:44 AM, Alexandre Belloni wrote: Hi, On 05/04/2019 11:14:35+, Han Nandor wrote: ` # hexdump -n 10 -C /sys/bus/nvmem/devices/ds3232_sram0/nvmem 74 65 73 74 69 6e 67 0a 00 00|testing...| 000a ` Thanks for that nice description! Glad that you like it. :) drivers/rtc/rtc-ds3232.c | 41 ++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c index 7184e5145f12..fe615aedaa9a 100644 --- a/drivers/rtc/rtc-ds3232.c +++ b/drivers/rtc/rtc-ds3232.c @@ -24,6 +24,8 @@ #include #include +#include "rtc-core.h" + The drivers should not have to include that header, see fd5cd21d995e67f87b3eb4adf938be85fe83ef4b (from 4.16). Ahh...right. I did the implementation on 4.14 and it was needed there. I will change the implementation but I'll not be able to fully test it. struct ds3232 { struct device *dev; struct regmap *regmap; int irq; struct rtc_device *rtc; + struct nvmem_config nvmem_cfg; You don't actually need to keep that structure for the whole life of the device as it is copied as soon as it is registered. I usually prefer to have it on the stack. Very true. Done + ds3232->nvmem_cfg.stride = 1; + ds3232->nvmem_cfg.size = DS3232_REG_SRAM_SIZE; + ds3232->nvmem_cfg.word_size = 1; + ds3232->nvmem_cfg.reg_read = ds3232_nvmem_read; + ds3232->nvmem_cfg.reg_write = ds3232_nvmem_write; + ds3232->nvmem_cfg.priv = ds3232; Please also set the type (battery backed). Done. + ds3232->rtc->dev.of_node = dev->of_node; Please don't mess with rtc->dev. In my use-case I do use the device tree to declare a nvmem cell in this RTC node, which is used by another driver. Without this change finding the cell (using `devm_nvmem_cell_get` function) was failing because of missing `of_node` (again this was on 4.14). I can do a bit of digging to see if something was changed, but I will really appreciate your advice, because I'm not be able to test the fix on linux-next. + ds3232->rtc->nvmem_config = &ds3232->nvmem_cfg; + rtc_nvmem_register(ds3232->rtc); This part will not compile on v5.1-rc1. Sorry. I will fix this, and I will at least see that it builds on linux-next. Thanks for review, Nandor
Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework
Hi, On 05/04/2019 11:14:35+, Han Nandor wrote: > DS3232 RTC has 236 bytes of persistent memory. > > Add RTC SRAM read and write access using > the NVMEM Framework. > > Signed-off-by: Nandor Han > --- > > Description > --- > Provides DS3232 RTC SRAM access using NVMEM framework. > > Testing > --- > The test was done on a custom board which contains a > DS3232 RTC device. > Kernel Version: 4.14.60 (Just for clarity, the patch is against master) > > 1. Verify that SRAM is accessible using NVMEM interface: PASS > ` > # hexdump /sys/bus/nvmem/devices/ds3232_sram0/nvmem > 000 > * > 0e0 > ` > 2. Modify the content. > ` > # echo testing > /sys/bus/nvmem/devices/ds3232_sram0/nvmem > # > ` > 3. Power cycle the board and verify that contents are preserved: PASS > ` > # hexdump -n 10 -C /sys/bus/nvmem/devices/ds3232_sram0/nvmem > 74 65 73 74 69 6e 67 0a 00 00|testing...| > 000a > ` > Thanks for that nice description! > drivers/rtc/rtc-ds3232.c | 41 ++-- > 1 file changed, 39 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c > index 7184e5145f12..fe615aedaa9a 100644 > --- a/drivers/rtc/rtc-ds3232.c > +++ b/drivers/rtc/rtc-ds3232.c > @@ -24,6 +24,8 @@ > #include > #include > > +#include "rtc-core.h" > + The drivers should not have to include that header, see fd5cd21d995e67f87b3eb4adf938be85fe83ef4b (from 4.16). > #define DS3232_REG_SECONDS 0x00 > #define DS3232_REG_MINUTES 0x01 > #define DS3232_REG_HOURS0x02 > @@ -48,12 +50,17 @@ > # define DS3232_REG_SR_A1F 0x01 > > #define DS3232_REG_TEMPERATURE 0x11 > +#define DS3232_REG_SRAM_START 0x14 > +#define DS3232_REG_SRAM_END 0xFF > + > +#define DS3232_REG_SRAM_SIZE236 > > struct ds3232 { > struct device *dev; > struct regmap *regmap; > int irq; > struct rtc_device *rtc; > + struct nvmem_config nvmem_cfg; > You don't actually need to keep that structure for the whole life of the device as it is copied as soon as it is registered. I usually prefer to have it on the stack. > bool suspended; > }; > @@ -461,6 +468,24 @@ static const struct rtc_class_ops ds3232_rtc_ops = { > .alarm_irq_enable = ds3232_alarm_irq_enable, > }; > > +static int ds3232_nvmem_read(void *priv, unsigned int offset, void *val, > + size_t bytes) > +{ > + struct ds3232 *ds3232 = (struct ds3232 *)priv; > + > + return regmap_bulk_read(ds3232->regmap, DS3232_REG_SRAM_START + offset, > + val, bytes); > +} > + > +static int ds3232_nvmem_write(void *priv, unsigned int offset, void *val, > + size_t bytes) > +{ > + struct ds3232 *ds3232 = (struct ds3232 *)priv; > + > + return regmap_bulk_write(ds3232->regmap, DS3232_REG_SRAM_START + offset, > + val, bytes); > +} > + > static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, > const char *name) > { > @@ -476,6 +501,14 @@ static int ds3232_probe(struct device *dev, struct > regmap *regmap, int irq, > ds3232->dev = dev; > dev_set_drvdata(dev, ds3232); > > + ds3232->nvmem_cfg.name = "ds3232_sram"; > + ds3232->nvmem_cfg.stride = 1; > + ds3232->nvmem_cfg.size = DS3232_REG_SRAM_SIZE; > + ds3232->nvmem_cfg.word_size = 1; > + ds3232->nvmem_cfg.reg_read = ds3232_nvmem_read; > + ds3232->nvmem_cfg.reg_write = ds3232_nvmem_write; > + ds3232->nvmem_cfg.priv = ds3232; Please also set the type (battery backed). > + > ret = ds3232_check_rtc_status(dev); > if (ret) > return ret; > @@ -490,6 +523,10 @@ static int ds3232_probe(struct device *dev, struct > regmap *regmap, int irq, > if (IS_ERR(ds3232->rtc)) > return PTR_ERR(ds3232->rtc); > > + ds3232->rtc->dev.of_node = dev->of_node; Please don't mess with rtc->dev. > + ds3232->rtc->nvmem_config = &ds3232->nvmem_cfg; > + rtc_nvmem_register(ds3232->rtc); This part will not compile on v5.1-rc1. > + > if (ds3232->irq > 0) { > ret = devm_request_threaded_irq(dev, ds3232->irq, NULL, > ds3232_irq, > @@ -542,7 +579,7 @@ static int ds3232_i2c_probe(struct i2c_client *client, > static const struct regmap_config config = { > .reg_bits = 8, > .val_bits = 8, > - .max_register = 0x13, > + .max_register = DS3232_REG_SRAM_END, > }; > > regmap = devm_regmap_init_i2c(client, &config); > @@ -609,7 +646,7 @@ static int ds3234_probe(struct spi_device *spi) > static const struct regmap_config config = { > .reg_bits = 8, > .val_bits = 8, > - .max_register = 0x13, > + .max_register = DS3232_
[PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework
DS3232 RTC has 236 bytes of persistent memory. Add RTC SRAM read and write access using the NVMEM Framework. Signed-off-by: Nandor Han --- Description --- Provides DS3232 RTC SRAM access using NVMEM framework. Testing --- The test was done on a custom board which contains a DS3232 RTC device. Kernel Version: 4.14.60 (Just for clarity, the patch is against master) 1. Verify that SRAM is accessible using NVMEM interface: PASS ` # hexdump /sys/bus/nvmem/devices/ds3232_sram0/nvmem 000 * 0e0 ` 2. Modify the content. ` # echo testing > /sys/bus/nvmem/devices/ds3232_sram0/nvmem # ` 3. Power cycle the board and verify that contents are preserved: PASS ` # hexdump -n 10 -C /sys/bus/nvmem/devices/ds3232_sram0/nvmem 74 65 73 74 69 6e 67 0a 00 00|testing...| 000a ` drivers/rtc/rtc-ds3232.c | 41 ++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c index 7184e5145f12..fe615aedaa9a 100644 --- a/drivers/rtc/rtc-ds3232.c +++ b/drivers/rtc/rtc-ds3232.c @@ -24,6 +24,8 @@ #include #include +#include "rtc-core.h" + #define DS3232_REG_SECONDS 0x00 #define DS3232_REG_MINUTES 0x01 #define DS3232_REG_HOURS0x02 @@ -48,12 +50,17 @@ # define DS3232_REG_SR_A1F 0x01 #define DS3232_REG_TEMPERATURE 0x11 +#define DS3232_REG_SRAM_START 0x14 +#define DS3232_REG_SRAM_END 0xFF + +#define DS3232_REG_SRAM_SIZE236 struct ds3232 { struct device *dev; struct regmap *regmap; int irq; struct rtc_device *rtc; + struct nvmem_config nvmem_cfg; bool suspended; }; @@ -461,6 +468,24 @@ static const struct rtc_class_ops ds3232_rtc_ops = { .alarm_irq_enable = ds3232_alarm_irq_enable, }; +static int ds3232_nvmem_read(void *priv, unsigned int offset, void *val, +size_t bytes) +{ + struct ds3232 *ds3232 = (struct ds3232 *)priv; + + return regmap_bulk_read(ds3232->regmap, DS3232_REG_SRAM_START + offset, + val, bytes); +} + +static int ds3232_nvmem_write(void *priv, unsigned int offset, void *val, + size_t bytes) +{ + struct ds3232 *ds3232 = (struct ds3232 *)priv; + + return regmap_bulk_write(ds3232->regmap, DS3232_REG_SRAM_START + offset, +val, bytes); +} + static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, const char *name) { @@ -476,6 +501,14 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, ds3232->dev = dev; dev_set_drvdata(dev, ds3232); + ds3232->nvmem_cfg.name = "ds3232_sram"; + ds3232->nvmem_cfg.stride = 1; + ds3232->nvmem_cfg.size = DS3232_REG_SRAM_SIZE; + ds3232->nvmem_cfg.word_size = 1; + ds3232->nvmem_cfg.reg_read = ds3232_nvmem_read; + ds3232->nvmem_cfg.reg_write = ds3232_nvmem_write; + ds3232->nvmem_cfg.priv = ds3232; + ret = ds3232_check_rtc_status(dev); if (ret) return ret; @@ -490,6 +523,10 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, if (IS_ERR(ds3232->rtc)) return PTR_ERR(ds3232->rtc); + ds3232->rtc->dev.of_node = dev->of_node; + ds3232->rtc->nvmem_config = &ds3232->nvmem_cfg; + rtc_nvmem_register(ds3232->rtc); + if (ds3232->irq > 0) { ret = devm_request_threaded_irq(dev, ds3232->irq, NULL, ds3232_irq, @@ -542,7 +579,7 @@ static int ds3232_i2c_probe(struct i2c_client *client, static const struct regmap_config config = { .reg_bits = 8, .val_bits = 8, - .max_register = 0x13, + .max_register = DS3232_REG_SRAM_END, }; regmap = devm_regmap_init_i2c(client, &config); @@ -609,7 +646,7 @@ static int ds3234_probe(struct spi_device *spi) static const struct regmap_config config = { .reg_bits = 8, .val_bits = 8, - .max_register = 0x13, + .max_register = DS3232_REG_SRAM_END, .write_flag_mask = 0x80, }; struct regmap *regmap; -- 2.17.2