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

From: Nandor Han
Date: Mon Aug 26 2019 - 05:17:24 EST

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@xxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:

On 16/08/2019 10:46:36+0800, Biwen Li wrote:
- # hwclock -w
hwclock: RTC_SET_TIME: Invalid argument

- Relative patch:
3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&amp;reserved=0 , this
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
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

This is not true, the RTC wraps the register accesses properly and

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.


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