Re: [PATCH] rtc: ds1307: fix ds1339 wakealarm support

[Soeren Moch]

Am 22.10.2018 um 17:52 schrieb Alexandre Belloni:
> On 22/10/2018 16:18:13+0200, Soeren Moch wrote:
>> Hi,
>>
>> Am 22.10.2018 um 15:57 schrieb Alexandre Belloni:
>>> Hello,
>>>
>>> On 22/10/2018 14:38:12+0200, Soeren Moch wrote:
>>>> Commit 51ed73eb998a1c79a2b0e9bed68f75a8a2c93b9b ("rtc: ds1340: Add support
>>>> for trickle charger.") breaks ds1339 wakealarm support by limiting
>>>> accessible registers. Fix this.
>>>>
>>>> Fixes: 51ed73eb998a ("rtc: ds1340: Add support for trickle charger.")
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Signed-off-by: Soeren Moch <smoch@xxxxxx>
>>>> --
>>>> Cc: Andrea Greco <a.greco@xxxxxxxxx>
>>>> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>
>>>> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
>>>> Cc: linux-rtc@xxxxxxxxxxxxxxx
>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>> ---
>>>>  drivers/rtc/rtc-ds1307.c | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>>>> index 4b2b4627daeb..71396b62dc52 100644
>>>> --- a/drivers/rtc/rtc-ds1307.c
>>>> +++ b/drivers/rtc/rtc-ds1307.c
>>>> @@ -1384,7 +1384,6 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
>>>>  static const struct regmap_config regmap_config = {
>>>>  	.reg_bits = 8,
>>>>  	.val_bits = 8,
>>>> -	.max_register = 0x9,
>>> I would prefer having a different regmap_config for chips that have more
>>> registers. I guess you could put a pointer to a regmap_config in chip_desc.
>>>
>> What looks strange to me, the original patch (according to its
>> description) enabled trickle charger functionality for a single chip.
>> This is something totally different than limiting the register space for
>> all chips that are supported by this driver. So this seems to be an
>> accident to me. And this causes an regression for ds1339, which was
>> supported for years.
>>
>> Are there any advantages for limiting the address space of i2c devices?
> I would think it was done as a facility to debug the feature because
> when max_register is set, regmap will allow you to dump the register
> space.
>
>> If we really need this, can we apply this partial revert as regression
>> fix (also for stable) and implement the address space limitation separately?
>>
> Fine, I'm applying this patch right now.
>
OK, thank you,
Soeren