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

[Biwen Li <biwen.li@nxp.com>]

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