| Message ID | 20181022123812.5782-1-smoch@web.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: ds1307: fix ds1339 wakealarm support | expand |
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.
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
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.
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
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,
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(-)