Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc:     "a.zummo@towertech.it" <a.zummo@towertech.it>,
        "linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20190405111348.28935-1-nandor.han@vaisala.com>
 <20190405214454.GJ22216@piout.net>
From:   Nandor Han <nandor.han@vaisala.com>
Message-ID: <ca262241-16f3-ab43-f63e-fb6293798d21@vaisala.com>
Date:   Mon, 8 Apr 2019 11:51:15 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190405214454.GJ22216@piout.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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