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 <[email protected]>
---
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
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
On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
<[email protected]> 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
On 16/08/2019 10:50:49-0500, Li Yang wrote:
> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> <[email protected]> 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
On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
<[email protected]> wrote:
>
> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> > On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> > <[email protected]> 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
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?
On Tue, Aug 20, 2019 at 1:24 PM Mark Brown <[email protected]> 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
On 8/16/19 10:40 PM, Li Yang wrote:
> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
> <[email protected]> wrote:
>>
>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
>>> <[email protected]> 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
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.
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
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.
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.
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
>
> On 8/16/19 10:40 PM, Li Yang wrote:
> > On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni
> > <[email protected]> wrote:
> >>
> >> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> >>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> >>> <[email protected]> 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&data=02%7C01%7Cbiwen.li%40nxp.
> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF
> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&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
>
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
>>> <[email protected]> wrote:
>>>>
>>>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
>>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
>>>>> <[email protected]> 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&data=02%7C01%7Cbiwen.li%40nxp.
>> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
>> c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF
>> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&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
>
> 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
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> >>>>> <[email protected]> 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&data=02%7C01%7Cbiwen.li%40n
> xp.
> >>
> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
> >>
> c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF
> >> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&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
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
> > >>> <[email protected]> wrote:
> > >>>>
> > >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote:
> > >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni
> > >>>>> <[email protected]> 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&data=02%7C01%7Cbiwen.li%40n
> > xp.
> > >>
> > com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99
> > >>
> > c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF
> > >> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&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