[v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Biwen Li]

Issue:
    - # hwclock -w
      hwclock: RTC_SET_TIME: Invalid argument

Why:
    - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch
      will always check for unwritable registers, it will compare reg
      with max_register in regmap_writeable.

    - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS
      is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
      '0x30 < 0x2f' is false,so regmap_writeable will return false.

    - Root cause: the buf[] was written to a wrong place in the file
      drivers/rtc/rtc-pcf85363.c

How:
    - Split of writing regs to two parts, first part writes control
      registers about stop_enable and resets, second part writes
      RTC time and date registers.

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v2:
	- add Why and How into commit message.
	 
 drivers/rtc/rtc-pcf85363.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index a075e77617dc..3450d615974d 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -166,7 +166,12 @@ static int pcf85363_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	buf[DT_YEARS] = bin2bcd(tm->tm_year % 100);
 
 	ret = regmap_bulk_write(pcf85363->regmap, CTRL_STOP_EN,
-				tmp, sizeof(tmp));
+				tmp, 2);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_write(pcf85363->regmap, DT_100THS,
+				buf, sizeof(tmp) - 2);
 	if (ret)
 		return ret;
 
-- 
2.17.1

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Alexandre Belloni]

On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> Issue:
>     - # hwclock -w
>       hwclock: RTC_SET_TIME: Invalid argument
> 
> Why:
>     - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch
>       will always check for unwritable registers, it will compare reg
>       with max_register in regmap_writeable.
> 
>     - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS
>       is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
>       '0x30 < 0x2f' is false,so regmap_writeable will return false.
> 
>     - Root cause: the buf[] was written to a wrong place in the file
>       drivers/rtc/rtc-pcf85363.c
> 

This is not true, the RTC wraps the register accesses properly and this
is probably something that should be handled by regmap_writable.

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

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Li Yang]

On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> > Issue:
> >     - # hwclock -w
> >       hwclock: RTC_SET_TIME: Invalid argument
> >
> > Why:
> >     - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch
> >       will always check for unwritable registers, it will compare reg
> >       with max_register in regmap_writeable.
> >
> >     - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS
> >       is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
> >       '0x30 < 0x2f' is false,so regmap_writeable will return false.
> >
> >     - Root cause: the buf[] was written to a wrong place in the file
> >       drivers/rtc/rtc-pcf85363.c
> >
>
> This is not true, the RTC wraps the register accesses properly and this

This performance hack probably deserve some explanation in the code comment.  :)

> is probably something that should be handled by regmap_writable.

The address wrapping is specific to this RTC chip.  Is it also
commonly used by other I2C devices?  I'm not sure if regmap_writable
should handle the wrapping case if it is too special.

Regards,
Leo

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Alexandre Belloni]

On 16/08/2019 10:50:49-0500, Li Yang wrote:
> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> > > Issue:
> > >     - # hwclock -w
> > >       hwclock: RTC_SET_TIME: Invalid argument
> > >
> > > Why:
> > >     - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch
> > >       will always check for unwritable registers, it will compare reg
> > >       with max_register in regmap_writeable.
> > >
> > >     - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS
> > >       is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
> > >       '0x30 < 0x2f' is false,so regmap_writeable will return false.
> > >
> > >     - Root cause: the buf[] was written to a wrong place in the file
> > >       drivers/rtc/rtc-pcf85363.c
> > >
> >
> > This is not true, the RTC wraps the register accesses properly and this
> 
> This performance hack probably deserve some explanation in the code comment.  :)
> 
> > is probably something that should be handled by regmap_writable.
> 
> The address wrapping is specific to this RTC chip.  Is it also
> commonly used by other I2C devices?  I'm not sure if regmap_writable
> should handle the wrapping case if it is too special.
> 

Most of the i2c RTCs do address wrapping which is sometimes the only way
to properly set the time.


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

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Li Yang]

On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> > On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > >
> > > On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> > > > Issue:
> > > >     - # hwclock -w
> > > >       hwclock: RTC_SET_TIME: Invalid argument
> > > >
> > > > Why:
> > > >     - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch
> > > >       will always check for unwritable registers, it will compare reg
> > > >       with max_register in regmap_writeable.
> > > >
> > > >     - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS
> > > >       is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
> > > >       '0x30 < 0x2f' is false,so regmap_writeable will return false.
> > > >
> > > >     - Root cause: the buf[] was written to a wrong place in the file
> > > >       drivers/rtc/rtc-pcf85363.c
> > > >
> > >
> > > This is not true, the RTC wraps the register accesses properly and this
> >
> > This performance hack probably deserve some explanation in the code comment.  :)
> >
> > > is probably something that should be handled by regmap_writable.
> >
> > The address wrapping is specific to this RTC chip.  Is it also
> > commonly used by other I2C devices?  I'm not sure if regmap_writable
> > should handle the wrapping case if it is too special.
> >
>
> Most of the i2c RTCs do address wrapping which is sometimes the only way
> to properly set the time.

Adding Mark and Nandor to the loop.

Regards,
Leo

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

On Fri, Aug 16, 2019 at 02:40:47PM -0500, Li Yang wrote:
> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni

> > Most of the i2c RTCs do address wrapping which is sometimes the only way
> > to properly set the time.

> Adding Mark and Nandor to the loop.

Is there a specific question or something here?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Li Yang]

On Tue, Aug 20, 2019 at 1:24 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Aug 16, 2019 at 02:40:47PM -0500, Li Yang wrote:
> > On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
>
> > > Most of the i2c RTCs do address wrapping which is sometimes the only way
> > > to properly set the time.
>
> > Adding Mark and Nandor to the loop.
>
> Is there a specific question or something here?

Some of the RTC hardware has the capability of address wrapping which
means if you access a continuous address range across a certain
boundary(could be the boundary of a regmap region) the hardware
actually wrap the access to a lower address.  But the address
violation check of regmap rejects such access.  According to
Alexcandre, the address wrapping is essential to the functionality of
some RTC devices and can improve performance for some others.  We are
wondering if it is reasonable to have regmap support this address
wrapping.

Regards,
Leo

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Nandor Han]

On 8/16/19 10:40 PM, Li Yang wrote:
> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
>>
>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
>>> <alexandre.belloni@bootlin.com> wrote:
>>>>
>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote:
>>>>> Issue:
>>>>>      - # hwclock -w
>>>>>        hwclock: RTC_SET_TIME: Invalid argument
>>>>>
>>>>> Why:
>>>>>      - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch
>>>>>        will always check for unwritable registers, it will compare reg
>>>>>        with max_register in regmap_writeable.
>>>>>
>>>>>      - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS
>>>>>        is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
>>>>>        '0x30 < 0x2f' is false,so regmap_writeable will return false.
>>>>>
>>>>>      - Root cause: the buf[] was written to a wrong place in the file
>>>>>        drivers/rtc/rtc-pcf85363.c
>>>>>
>>>>
>>>> This is not true, the RTC wraps the register accesses properly and this
>>>
>>> This performance hack probably deserve some explanation in the code comment.  :)
>>>
>>>> is probably something that should be handled by regmap_writable.
>>>
>>> The address wrapping is specific to this RTC chip.  Is it also
>>> commonly used by other I2C devices?  I'm not sure if regmap_writable
>>> should handle the wrapping case if it is too special.
>>>
>>
>> Most of the i2c RTCs do address wrapping which is sometimes the only way
>> to properly set the time.
> 
> Adding Mark and Nandor to the loop.
> 
> Regards,
> Leo
> 

Hi,
   `regmap` provides couple of ways to validate the registers: 
max_register, callback function and write table. All of these are 
optional, so it gives you the freedom to customize it as needed.

In this situation probably you could:
   1. Avoid using the wrapping feature of pcf85363 (you can just provide 
separate calls for stop, reset and time confguration). In this way the 
`max_register` validation method will work fine.
   2. Replace `max_register` method validation with `callback function` 
validation method, were you could make your own validation.


Regards,
    Nandor

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote:

> Some of the RTC hardware has the capability of address wrapping which
> means if you access a continuous address range across a certain
> boundary(could be the boundary of a regmap region) the hardware
> actually wrap the access to a lower address.  But the address
> violation check of regmap rejects such access.  According to
> Alexcandre, the address wrapping is essential to the functionality of

It's *essential*?  Will innovation never cease?

> some RTC devices and can improve performance for some others.  We are
> wondering if it is reasonable to have regmap support this address
> wrapping.

I guess, I don't see any particular reason why not unless the patches
are horrible or get in the way of other stuff.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Alexandre Belloni]

On 21/08/2019 12:21:42+0100, Mark Brown wrote:
> On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote:
> 
> > Some of the RTC hardware has the capability of address wrapping which
> > means if you access a continuous address range across a certain
> > boundary(could be the boundary of a regmap region) the hardware
> > actually wrap the access to a lower address.  But the address
> > violation check of regmap rejects such access.  According to
> > Alexcandre, the address wrapping is essential to the functionality of
> 
> It's *essential*?  Will innovation never cease?
> 

To be clear, for some RTCs, its is the only way to accurately set the
time.



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

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Wed, Aug 21, 2019 at 01:24:13PM +0200, Alexandre Belloni wrote:
> On 21/08/2019 12:21:42+0100, Mark Brown wrote:
> > On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote:

> > > violation check of regmap rejects such access.  According to
> > > Alexcandre, the address wrapping is essential to the functionality of

> > It's *essential*?  Will innovation never cease?

> To be clear, for some RTCs, its is the only way to accurately set the
> time.

What's the mechanism here?  It's a very strange thing to require.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Alexandre Belloni]

On 21/08/2019 12:30:29+0100, Mark Brown wrote:
> On Wed, Aug 21, 2019 at 01:24:13PM +0200, Alexandre Belloni wrote:
> > On 21/08/2019 12:21:42+0100, Mark Brown wrote:
> > > On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote:
> 
> > > > violation check of regmap rejects such access.  According to
> > > > Alexcandre, the address wrapping is essential to the functionality of
> 
> > > It's *essential*?  Will innovation never cease?
> 
> > To be clear, for some RTCs, its is the only way to accurately set the
> > time.
> 
> What's the mechanism here?  It's a very strange thing to require.

The clock control is on the first register, then you have sec, min,
hour, day, mon, year.

To be able to set the clock accurately, you need to first disable the
clock, then set the time and date and finally reenable the clock in the
first register. This should be done in a single i2c write.

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

Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On Wed, Aug 21, 2019 at 01:38:56PM +0200, Alexandre Belloni wrote:
> On 21/08/2019 12:30:29+0100, Mark Brown wrote:

> > What's the mechanism here?  It's a very strange thing to require.

> The clock control is on the first register, then you have sec, min,
> hour, day, mon, year.

> To be able to set the clock accurately, you need to first disable the
> clock, then set the time and date and finally reenable the clock in the
> first register. This should be done in a single i2c write.

Ugh, right.  And of course it would be silly to put the enable register
last or done some sort of strobe thing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [EXT] Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Biwen Li]

> 
> On 8/16/19 10:40 PM, Li Yang wrote:
> > On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> >>
> >> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> >>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> >>> <alexandre.belloni@bootlin.com> wrote:
> >>>>
> >>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> >>>>> Issue:
> >>>>>      - # hwclock -w
> >>>>>        hwclock: RTC_SET_TIME: Invalid argument
> >>>>>
> >>>>> Why:
> >>>>>      - Relative patch:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org
> %2Flkml%2F2019%2F4%2F3%2F55&amp;data=02%7C01%7Cbiwen.li%40nxp.
> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C637019652111923736&amp;sdata=spY6e22YOkOF
> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&amp;reserved=0 , this
> patch
> >>>>>        will always check for unwritable registers, it will compare reg
> >>>>>        with max_register in regmap_writeable.
> >>>>>
> >>>>>      - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but
> DT_100THS
> >>>>>        is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
> >>>>>        '0x30 < 0x2f' is false,so regmap_writeable will return false.
> >>>>>
> >>>>>      - Root cause: the buf[] was written to a wrong place in the file
> >>>>>        drivers/rtc/rtc-pcf85363.c
> >>>>>
> >>>>
> >>>> This is not true, the RTC wraps the register accesses properly and
> >>>> this
> >>>
> >>> This performance hack probably deserve some explanation in the code
> >>> comment.  :)
> >>>
> >>>> is probably something that should be handled by regmap_writable.
> >>>
> >>> The address wrapping is specific to this RTC chip.  Is it also
> >>> commonly used by other I2C devices?  I'm not sure if regmap_writable
> >>> should handle the wrapping case if it is too special.
> >>>
> >>
> >> Most of the i2c RTCs do address wrapping which is sometimes the only
> >> way to properly set the time.
> >
> > Adding Mark and Nandor to the loop.
> >
> > Regards,
> > Leo
> >
> 
> Hi,
>    `regmap` provides couple of ways to validate the registers:
> max_register, callback function and write table. All of these are optional, so it
> gives you the freedom to customize it as needed.
> 
> In this situation probably you could:
>    1. Avoid using the wrapping feature of pcf85363 (you can just provide
> separate calls for stop, reset and time confguration). In this way the
> `max_register` validation method will work fine.
Yes, I use this way. Path as follows:
Stop and reset - > set time > stop

>    2. Replace `max_register` method validation with `callback function`
> validation method, were you could make your own validation.
It is not work, show the code in as follows:

bool regmap_writeable(struct regmap *map, unsigned int reg)
{
        if (map->max_register && reg > map->max_register)
                return false;
Callback function (writeable_reg) will not be called.
        if (map->writeable_reg)
                return map->writeable_reg(map->dev, reg);

        if (map->wr_table)
                return regmap_check_range_table(map, reg, map->wr_table);

        return true;
}

> 
> 
> Regards,
>     Nandor
> 

Re: [EXT] Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Nandor Han]

On 8/26/19 7:29 AM, Biwen Li wrote:
>>
>> On 8/16/19 10:40 PM, Li Yang wrote:
>>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
>>> <alexandre.belloni@bootlin.com> wrote:
>>>>
>>>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
>>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
>>>>> <alexandre.belloni@bootlin.com> wrote:
>>>>>>
>>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote:
>>>>>>> Issue:
>>>>>>>       - # hwclock -w
>>>>>>>         hwclock: RTC_SET_TIME: Invalid argument
>>>>>>>
>>>>>>> Why:
>>>>>>>       - Relative patch:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org
>> %2Flkml%2F2019%2F4%2F3%2F55&amp;data=02%7C01%7Cbiwen.li%40nxp.
>> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
>> c5c301635%7C0%7C0%7C637019652111923736&amp;sdata=spY6e22YOkOF
>> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&amp;reserved=0 , this
>> patch
>>>>>>>         will always check for unwritable registers, it will compare reg
>>>>>>>         with max_register in regmap_writeable.
>>>>>>>
>>>>>>>       - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but
>> DT_100THS
>>>>>>>         is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
>>>>>>>         '0x30 < 0x2f' is false,so regmap_writeable will return false.
>>>>>>>
>>>>>>>       - Root cause: the buf[] was written to a wrong place in the file
>>>>>>>         drivers/rtc/rtc-pcf85363.c
>>>>>>>
>>>>>>
>>>>>> This is not true, the RTC wraps the register accesses properly and
>>>>>> this
>>>>>
>>>>> This performance hack probably deserve some explanation in the code
>>>>> comment.  :)
>>>>>
>>>>>> is probably something that should be handled by regmap_writable.
>>>>>
>>>>> The address wrapping is specific to this RTC chip.  Is it also
>>>>> commonly used by other I2C devices?  I'm not sure if regmap_writable
>>>>> should handle the wrapping case if it is too special.
>>>>>
>>>>
>>>> Most of the i2c RTCs do address wrapping which is sometimes the only
>>>> way to properly set the time.
>>>
>>> Adding Mark and Nandor to the loop.
>>>
>>> Regards,
>>> Leo
>>>
>>
>> Hi,
>>     `regmap` provides couple of ways to validate the registers:
>> max_register, callback function and write table. All of these are optional, so it
>> gives you the freedom to customize it as needed.
>>
>> In this situation probably you could:
>>     1. Avoid using the wrapping feature of pcf85363 (you can just provide
>> separate calls for stop, reset and time confguration). In this way the
>> `max_register` validation method will work fine.
> Yes, I use this way. Path as follows:
> Stop and reset - > set time > stop
> 

Some of the concerns regarding this method was that it might not be 
precise enough. That because you need 2 I2C operations (one for stop and 
one for time configuration). Not sure about your case if this is a 
problem or not.

>>     2. Replace `max_register` method validation with `callback function`
>> validation method, were you could make your own validation.
> It is not work, show the code in as follows:
> 
> bool regmap_writeable(struct regmap *map, unsigned int reg)
> {
>          if (map->max_register && reg > map->max_register)
>                  return false;
> Callback function (writeable_reg) will not be called.
>          if (map->writeable_reg)
>                  return map->writeable_reg(map->dev, reg);

Hi Li,
    If you *replace* the `max_register` method with `callback function` 
it should work. The code above will use every method *if provided*. In 
other words if `map->max_register` is 0 will go to the next step and 
check `map->writeable_reg`. Right?



Regards,
     Nandor

RE: [EXT] Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Biwen Li]

> 
> On 8/26/19 7:29 AM, Biwen Li wrote:
> >>
> >> On 8/16/19 10:40 PM, Li Yang wrote:
> >>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
> >>> <alexandre.belloni@bootlin.com> wrote:
> >>>>
> >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> >>>>> <alexandre.belloni@bootlin.com> wrote:
> >>>>>>
> >>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> >>>>>>> Issue:
> >>>>>>>       - # hwclock -w
> >>>>>>>         hwclock: RTC_SET_TIME: Invalid argument
> >>>>>>>
> >>>>>>> Why:
> >>>>>>>       - Relative patch:
> >> https://lkml.org
> >> %2Flkml%2F2019%2F4%2F3%2F55&amp;data=02%7C01%7Cbiwen.li%40n
> xp.
> >>
> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
> >>
> c5c301635%7C0%7C0%7C637019652111923736&amp;sdata=spY6e22YOkOF
> >> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&amp;reserved=0 ,
> this patch
> >>>>>>>         will always check for unwritable registers, it will compare reg
> >>>>>>>         with max_register in regmap_writeable.
> >>>>>>>
> >>>>>>>       - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but
> >> DT_100THS
> >>>>>>>         is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
> >>>>>>>         '0x30 < 0x2f' is false,so regmap_writeable will return false.
> >>>>>>>
> >>>>>>>       - Root cause: the buf[] was written to a wrong place in the file
> >>>>>>>         drivers/rtc/rtc-pcf85363.c
> >>>>>>>
> >>>>>>
> >>>>>> This is not true, the RTC wraps the register accesses properly
> >>>>>> and this
> >>>>>
> >>>>> This performance hack probably deserve some explanation in the
> >>>>> code comment.  :)
> >>>>>
> >>>>>> is probably something that should be handled by regmap_writable.
> >>>>>
> >>>>> The address wrapping is specific to this RTC chip.  Is it also
> >>>>> commonly used by other I2C devices?  I'm not sure if
> >>>>> regmap_writable should handle the wrapping case if it is too special.
> >>>>>
> >>>>
> >>>> Most of the i2c RTCs do address wrapping which is sometimes the
> >>>> only way to properly set the time.
> >>>
> >>> Adding Mark and Nandor to the loop.
> >>>
> >>> Regards,
> >>> Leo
> >>>
> >>
> >> Hi,
> >>     `regmap` provides couple of ways to validate the registers:
> >> max_register, callback function and write table. All of these are
> >> optional, so it gives you the freedom to customize it as needed.
> >>
> >> In this situation probably you could:
> >>     1. Avoid using the wrapping feature of pcf85363 (you can just
> >> provide separate calls for stop, reset and time confguration). In
> >> this way the `max_register` validation method will work fine.
> > Yes, I use this way. Path as follows:
> > Stop and reset - > set time > stop
> >
> 
> Some of the concerns regarding this method was that it might not be precise
> enough. That because you need 2 I2C operations (one for stop and one for time
> configuration). Not sure about your case if this is a problem or not.
Ok, got it, thanks.
> 
> >>     2. Replace `max_register` method validation with `callback
> >> function` validation method, were you could make your own validation.
> > It is not work, show the code in as follows:
> >
> > bool regmap_writeable(struct regmap *map, unsigned int reg) {
> >          if (map->max_register && reg > map->max_register)
> >                  return false;
> > Callback function (writeable_reg) will not be called.
> >          if (map->writeable_reg)
> >                  return map->writeable_reg(map->dev, reg);
> 
> Hi Li,
>     If you *replace* the `max_register` method with `callback function` it
> should work. The code above will use every method *if provided*. In other
> words if `map->max_register` is 0 will go to the next step and check
> `map->writeable_reg`. Right?
Yes, you are right. Thanks.
> 
> 
> 
> Regards,
>      Nandor

Re: [EXT] Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Alexandre Belloni]

Hi,

On 26/08/2019 09:49:49+0000, Biwen Li wrote:
> > 
> > On 8/26/19 7:29 AM, Biwen Li wrote:
> > >>
> > >> On 8/16/19 10:40 PM, Li Yang wrote:
> > >>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
> > >>> <alexandre.belloni@bootlin.com> wrote:
> > >>>>
> > >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> > >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> > >>>>> <alexandre.belloni@bootlin.com> wrote:
> > >>>>>>
> > >>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> > >>>>>>> Issue:
> > >>>>>>>       - # hwclock -w
> > >>>>>>>         hwclock: RTC_SET_TIME: Invalid argument
> > >>>>>>>
> > >>>>>>> Why:
> > >>>>>>>       - Relative patch:
> > >> https://lkml.org
> > >> %2Flkml%2F2019%2F4%2F3%2F55&amp;data=02%7C01%7Cbiwen.li%40n
> > xp.
> > >>
> > com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
> > >>
> > c5c301635%7C0%7C0%7C637019652111923736&amp;sdata=spY6e22YOkOF
> > >> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&amp;reserved=0 ,
> > this patch
> > >>>>>>>         will always check for unwritable registers, it will compare reg
> > >>>>>>>         with max_register in regmap_writeable.
> > >>>>>>>
> > >>>>>>>       - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but
> > >> DT_100THS
> > >>>>>>>         is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
> > >>>>>>>         '0x30 < 0x2f' is false,so regmap_writeable will return false.
> > >>>>>>>
> > >>>>>>>       - Root cause: the buf[] was written to a wrong place in the file
> > >>>>>>>         drivers/rtc/rtc-pcf85363.c
> > >>>>>>>
> > >>>>>>
> > >>>>>> This is not true, the RTC wraps the register accesses properly
> > >>>>>> and this
> > >>>>>
> > >>>>> This performance hack probably deserve some explanation in the
> > >>>>> code comment.  :)
> > >>>>>
> > >>>>>> is probably something that should be handled by regmap_writable.
> > >>>>>
> > >>>>> The address wrapping is specific to this RTC chip.  Is it also
> > >>>>> commonly used by other I2C devices?  I'm not sure if
> > >>>>> regmap_writable should handle the wrapping case if it is too special.
> > >>>>>
> > >>>>
> > >>>> Most of the i2c RTCs do address wrapping which is sometimes the
> > >>>> only way to properly set the time.
> > >>>
> > >>> Adding Mark and Nandor to the loop.
> > >>>
> > >>> Regards,
> > >>> Leo
> > >>>
> > >>
> > >> Hi,
> > >>     `regmap` provides couple of ways to validate the registers:
> > >> max_register, callback function and write table. All of these are
> > >> optional, so it gives you the freedom to customize it as needed.
> > >>
> > >> In this situation probably you could:
> > >>     1. Avoid using the wrapping feature of pcf85363 (you can just
> > >> provide separate calls for stop, reset and time confguration). In
> > >> this way the `max_register` validation method will work fine.
> > > Yes, I use this way. Path as follows:
> > > Stop and reset - > set time > stop
> > >
> > 
> > Some of the concerns regarding this method was that it might not be precise
> > enough. That because you need 2 I2C operations (one for stop and one for time
> > configuration). Not sure about your case if this is a problem or not.
> Ok, got it, thanks.

To be clear, for this RTC it is fine to separate both writes. Want I
want is a corrected commit message with a proper reference to
8b9f9d4dc511309918c4f6793bae7387c0c638af instead of a link to lkml.org
and a proper explanation.

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

RE: [EXT] Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w

[Biwen Li]

> 
> Hi,
> 
> On 26/08/2019 09:49:49+0000, Biwen Li wrote:
> > >
> > > On 8/26/19 7:29 AM, Biwen Li wrote:
> > > >>
> > > >> On 8/16/19 10:40 PM, Li Yang wrote:
> > > >>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
> > > >>> <alexandre.belloni@bootlin.com> wrote:
> > > >>>>
> > > >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> > > >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> > > >>>>> <alexandre.belloni@bootlin.com> wrote:
> > > >>>>>>
> > > >>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote:
> > > >>>>>>> Issue:
> > > >>>>>>>       - # hwclock -w
> > > >>>>>>>         hwclock: RTC_SET_TIME: Invalid argument
> > > >>>>>>>
> > > >>>>>>> Why:
> > > >>>>>>>       - Relative patch:
> > > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2
> > > >>
> Flkml.org&amp;data=02%7C01%7Cbiwen.li%40nxp.com%7C03141ff7858343
> 3
> > > >>
> 20be408d72a0d1e10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C63
> > > >>
> 7024108138794294&amp;sdata=QrALkFN6heF%2B7S73FQ9c%2FyKNRHyBuL
> %2B6
> > > >> %2B4PDM9hYRyM%3D&amp;reserved=0
> > > >> %2Flkml%2F2019%2F4%2F3%2F55&amp;data=02%7C01%7Cbiwen.li%
> 40n
> > > xp.
> > > >>
> > >
> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
> > > >>
> > >
> c5c301635%7C0%7C0%7C637019652111923736&amp;sdata=spY6e22YOkOF
> > > >>
> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&amp;reserved=0 ,
> > > this patch
> > > >>>>>>>         will always check for unwritable registers, it will compare
> reg
> > > >>>>>>>         with max_register in regmap_writeable.
> > > >>>>>>>
> > > >>>>>>>       - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e,
> > > >>>>>>> but
> > > >> DT_100THS
> > > >>>>>>>         is 0, max_regiter is 0x2f, then reg will be equal to 0x30,
> > > >>>>>>>         '0x30 < 0x2f' is false,so regmap_writeable will return
> false.
> > > >>>>>>>
> > > >>>>>>>       - Root cause: the buf[] was written to a wrong place in the
> file
> > > >>>>>>>         drivers/rtc/rtc-pcf85363.c
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>> This is not true, the RTC wraps the register accesses
> > > >>>>>> properly and this
> > > >>>>>
> > > >>>>> This performance hack probably deserve some explanation in the
> > > >>>>> code comment.  :)
> > > >>>>>
> > > >>>>>> is probably something that should be handled by regmap_writable.
> > > >>>>>
> > > >>>>> The address wrapping is specific to this RTC chip.  Is it also
> > > >>>>> commonly used by other I2C devices?  I'm not sure if
> > > >>>>> regmap_writable should handle the wrapping case if it is too special.
> > > >>>>>
> > > >>>>
> > > >>>> Most of the i2c RTCs do address wrapping which is sometimes the
> > > >>>> only way to properly set the time.
> > > >>>
> > > >>> Adding Mark and Nandor to the loop.
> > > >>>
> > > >>> Regards,
> > > >>> Leo
> > > >>>
> > > >>
> > > >> Hi,
> > > >>     `regmap` provides couple of ways to validate the registers:
> > > >> max_register, callback function and write table. All of these are
> > > >> optional, so it gives you the freedom to customize it as needed.
> > > >>
> > > >> In this situation probably you could:
> > > >>     1. Avoid using the wrapping feature of pcf85363 (you can just
> > > >> provide separate calls for stop, reset and time confguration). In
> > > >> this way the `max_register` validation method will work fine.
> > > > Yes, I use this way. Path as follows:
> > > > Stop and reset - > set time > stop
> > > >
> > >
> > > Some of the concerns regarding this method was that it might not be
> > > precise enough. That because you need 2 I2C operations (one for stop
> > > and one for time configuration). Not sure about your case if this is a problem
> or not.
> > Ok, got it, thanks.
> 
> To be clear, for this RTC it is fine to separate both writes. Want I want is a
> corrected commit message with a proper reference to
> 8b9f9d4dc511309918c4f6793bae7387c0c638af instead of a link to lkml.org
> and a proper explanation.
Ok, got it, thanks.I will replace link to lkml.org with
8b9f9d4dc511309918c4f6793bae7387c0c638af and add a proper explanation
to the commit message in v4.
> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin.
> com&amp;data=02%7C01%7Cbiwen.li%40nxp.com%7C03141ff7858343320b
> e408d72a0d1e10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 37024108138794294&amp;sdata=XnAxJmOkh1VVA9ed%2FLr%2BbvWbVpLD
> bwLjJrdaFidRtDk%3D&amp;reserved=0