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