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

[Soeren Moch]

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?
If we really need this, can we apply this partial revert as regression
fix (also for stable) and implement the address space limitation separately?

Thanks,
Soeren