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 = >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 = >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, );
> @@ -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,
[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 = >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, );
@@ -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
