[PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework

[PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework

[Han Nandor]

DS3232 RTC has 236 bytes of persistent memory.

Add RTC SRAM read and write access using
the NVMEM Framework.

Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---

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
0000000 0000 0000 0000 0000 0000 0000 0000 0000
*
00000e0
`
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
00000000  74 65 73 74 69 6e 67 0a  00 00                    |testing...|
0000000a
`

 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 <linux/regmap.h>
 #include <linux/hwmon.h>
 
+#include "rtc-core.h"
+
 #define DS3232_REG_SECONDS      0x00
 #define DS3232_REG_MINUTES      0x01
 #define DS3232_REG_HOURS        0x02
@@ -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_SIZE    236
 
 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

Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework

[Alexandre Belloni]

Hi,

On 05/04/2019 11:14:35+0000, 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 <nandor.han@vaisala.com>
> ---
> 
> 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
> 0000000 0000 0000 0000 0000 0000 0000 0000 0000
> *
> 00000e0
> `
> 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
> 00000000  74 65 73 74 69 6e 67 0a  00 00                    |testing...|
> 0000000a
> `
> 

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 <linux/regmap.h>
>  #include <linux/hwmon.h>
>  
> +#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_HOURS        0x02
> @@ -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_SIZE    236
>  

>  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_REG_SRAM_END,
>  		.write_flag_mask = 0x80,
>  	};
>  	struct regmap *regmap;
> -- 
> 2.17.2
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework

[Nandor Han]

On 4/6/19 12:44 AM, Alexandre Belloni wrote:
> Hi,
> 
> On 05/04/2019 11:14:35+0000, Han Nandor wrote:

<snip>

>> `
>> # hexdump -n 10 -C /sys/bus/nvmem/devices/ds3232_sram0/nvmem
>> 00000000  74 65 73 74 69 6e 67 0a  00 00                    |testing...|
>> 0000000a
>> `
>>
> 
> 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 <linux/regmap.h>
>>   #include <linux/hwmon.h>
>>   
>> +#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.

<snip>

>>   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

<snip>

>> +	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.

<snip>

>>   
>> +	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