[PATCH] rtc: ds1307: fix ds1339 wakealarm support

[PATCH] rtc: ds1307: fix ds1339 wakealarm support

[Soeren Moch]

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@vger.kernel.org
Signed-off-by: Soeren Moch <smoch@web.de>
--
Cc: Andrea Greco <a.greco@4sigma.it>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: linux-rtc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 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,
 };
 
 static int ds1307_probe(struct i2c_client *client,
-- 
2.17.1

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

[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@vger.kernel.org
> Signed-off-by: Soeren Moch <smoch@web.de>
> --
> Cc: Andrea Greco <a.greco@4sigma.it>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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@vger.kernel.org
>> Signed-off-by: Soeren Moch <smoch@web.de>
>> --
>> Cc: Andrea Greco <a.greco@4sigma.it>
>> Cc: Alessandro Zummo <a.zummo@towertech.it>
>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Cc: linux-rtc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  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

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

[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@vger.kernel.org
> >> Signed-off-by: Soeren Moch <smoch@web.de>
> >> --
> >> Cc: Andrea Greco <a.greco@4sigma.it>
> >> Cc: Alessandro Zummo <a.zummo@towertech.it>
> >> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> >> Cc: linux-rtc@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >>  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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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@vger.kernel.org
>>>> Signed-off-by: Soeren Moch <smoch@web.de>
>>>> --
>>>> Cc: Andrea Greco <a.greco@4sigma.it>
>>>> Cc: Alessandro Zummo <a.zummo@towertech.it>
>>>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>>> Cc: linux-rtc@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>  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