| Message ID | 20150306212417.GA24169@amd (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Friday 06 March 2015 22:24:17 Pavel Machek wrote: > Hi! > > According to n900 dts, twl4030-bci (aka charger) should be > included. > AFAIK it is not present on n900...
On Fri, Mar 6, 2015 at 11:57 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Friday 06 March 2015 22:24:17 Pavel Machek wrote: >> Hi! >> >> According to n900 dts, twl4030-bci (aka charger) should be >> included. >> > > AFAIK it is not present on n900... Right, it uses twl5030 variant without the charger, charging on n900 is provided by separate chip and for a good reason as twl's charger is not that good. Forcing the driver to load just ends up with it accessing non-existent registers over i2c. Gražvydas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat 2015-03-07 00:12:07, Grazvydas Ignotas wrote: > On Fri, Mar 6, 2015 at 11:57 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Friday 06 March 2015 22:24:17 Pavel Machek wrote: > >> Hi! > >> > >> According to n900 dts, twl4030-bci (aka charger) should be > >> included. > >> > > > > AFAIK it is not present on n900... > > Right, it uses twl5030 variant without the charger, charging on n900 > is provided by separate chip and for a good reason as twl's charger is > not that good. Forcing the driver to load just ends up with it > accessing non-existent registers over i2c. Ok, but: 1) Why is the twl4030-bci enabled in n900's dts, then and 2) When it is enabled, why it does not load? (I guess there's no way to get to input voltage on n900...?) Pavel
On Friday 06 March 2015 23:40:34 Pavel Machek wrote: > On Sat 2015-03-07 00:12:07, Grazvydas Ignotas wrote: > > On Fri, Mar 6, 2015 at 11:57 PM, Pali Rohár > > <pali.rohar@gmail.com> wrote: > > > On Friday 06 March 2015 22:24:17 Pavel Machek wrote: > > >> Hi! > > >> > > >> According to n900 dts, twl4030-bci (aka charger) should > > >> be included. > > > > > > AFAIK it is not present on n900... > > > > Right, it uses twl5030 variant without the charger, charging > > on n900 is provided by separate chip and for a good reason > > as twl's charger is not that good. Forcing the driver to > > load just ends up with it accessing non-existent registers > > over i2c. > > Ok, but: > > 1) Why is the twl4030-bci enabled in n900's dts, then > maybe it is bug in n900 dts... Grazvydas, is there some runtime check if twl4030/twl5030 chip has charger or not? or do we need to explicitly disable device twl4030-bci in DT? > and > > 2) When it is enabled, why it does not load? > > (I guess there's no way to get to input voltage on n900...?) > > Pavel you can read voltage only from rx51_battery.ko (TWL ADC) or bq27x00_battery.ko look for Nokia_N900_RX-51_Schematics.pdf file where you can find information what is connected to ADC and bq27200 chip.
On Sat, Mar 7, 2015 at 12:56 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Friday 06 March 2015 23:40:34 Pavel Machek wrote: >> On Sat 2015-03-07 00:12:07, Grazvydas Ignotas wrote: >> > On Fri, Mar 6, 2015 at 11:57 PM, Pali Rohár >> > <pali.rohar@gmail.com> wrote: >> > > On Friday 06 March 2015 22:24:17 Pavel Machek wrote: >> > >> Hi! >> > >> >> > >> According to n900 dts, twl4030-bci (aka charger) should >> > >> be included. >> > > >> > > AFAIK it is not present on n900... >> > >> > Right, it uses twl5030 variant without the charger, charging >> > on n900 is provided by separate chip and for a good reason >> > as twl's charger is not that good. Forcing the driver to >> > load just ends up with it accessing non-existent registers >> > over i2c. >> >> Ok, but: >> >> 1) Why is the twl4030-bci enabled in n900's dts, then >> > > maybe it is bug in n900 dts... > > Grazvydas, is there some runtime check if twl4030/twl5030 chip > has charger or not? or do we need to explicitly disable device > twl4030-bci in DT? Actually from looking at the schematics, it looks like the charger pins are still there but all connected to ground. So it probably has the charger after all, it's just not connected or used. I'm not aware or any registers for direct detection, and indirect detection is difficult because BCI mostly disables itself when no charger is connected and most registers read as 0 or have old values from last charging session (which will never happen on n900). There is IDCODE register on twl4030 itself, but it's documented as not meaningful when accessed over i2c (when is it meaningful then??). drivers/mfd/twl-core.c has a i2c_device_id table of various twl4030 variants, some of which have no charger. N900 has GAIA/twl5030, which differs from twl4030 only by vaux2 regulator according to that file. N900's old board files specify 5030, but .dts does not. Gražvydas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 07 March 2015 16:56:01 Grazvydas Ignotas wrote:
> N900's old board files specify 5030, but .dts does not.
I would guess this is bug and DTS file needs to be fixed.
Hi, On Fri, Mar 06, 2015 at 10:24:17PM +0100, Pavel Machek wrote: > According to n900 dts, twl4030-bci (aka charger) should be > included. its part of twl, but not used on N900 afaik. > (But it does not seem to do anything useful on n900. I was hoping for > measurement of input voltage, but .. no.) check for rx51-battery. > Any ideas why the patch below is needed? platform_driver_probe() does not support deferred probing. Neil, can you take this patch into your series for the next round? -- Sebastian > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c > index d35b83e..96bbbe9 100644 > --- a/drivers/power/twl4030_charger.c > +++ b/drivers/power/twl4030_charger.c > @@ -714,6 +722,7 @@ static const struct of_device_id twl_bci_of_match[] = { > MODULE_DEVICE_TABLE(of, twl_bci_of_match); > > static struct platform_driver twl4030_bci_driver = { > + .probe = twl4030_bci_probe, > .driver = { > .name = "twl4030_bci", > .of_match_table = of_match_ptr(twl_bci_of_match), > @@ -721,7 +730,7 @@ static struct platform_driver twl4030_bci_driver = { > .remove = __exit_p(twl4030_bci_remove), > }; > > -module_platform_driver_probe(twl4030_bci_driver, twl4030_bci_probe); > +module_platform_driver(twl4030_bci_driver); > > MODULE_AUTHOR("Gražvydas Ignotas"); > MODULE_DESCRIPTION("TWL4030 Battery Charger Interface driver");
On Sat, 7 Mar 2015 22:01:02 +0100 Sebastian Reichel <sre@debian.org> wrote: > Hi, > > On Fri, Mar 06, 2015 at 10:24:17PM +0100, Pavel Machek wrote: > > According to n900 dts, twl4030-bci (aka charger) should be > > included. > > its part of twl, but not used on N900 afaik. > > > (But it does not seem to do anything useful on n900. I was hoping for > > measurement of input voltage, but .. no.) > > check for rx51-battery. > > > Any ideas why the patch below is needed? > > platform_driver_probe() does not support deferred probing. > > Neil, can you take this patch into your series for the next round? I could, but I do wonder if it is the right thing to do. Shouldn't we fix platform_driver_probe() to support deferred probing. As I understand it, it refused to retry a probe if there is an error, and the comments suggest that such retrying is avoided because it would be a waste of time: /* * Prevent driver from requesting probe deferral to avoid further * futile probe attempts. */ In this case, it isn't futile. Earlier there is a comment saying: * Use this instead of platform_driver_register() when you know the device * is not hotpluggable and has already been registered, and you want to * remove its run-once probe() infrastructure from memory after the driver * has bound to the device. I presume all this applies. I assume that the only problem is a probe-order thing. So maybe we should fix platform_driver_probe() to do the right thing with -EPROBEDEFER?? Trouble is, I really don't understand the point or mechanism for platform_driver_probe(), so I cannot suggest anything. But I have been annoyed before that platform_driver_probe doesn't cope with EPROBEDEFER, so I would like it fixed. NeilBrown
Hi, On Mon, Mar 09, 2015 at 11:06:53AM +1100, NeilBrown wrote: > On Sat, 7 Mar 2015 22:01:02 +0100 Sebastian Reichel <sre@debian.org> wrote: > > platform_driver_probe() does not support deferred probing. > > > > Neil, can you take this patch into your series for the next round? > > I could, but I do wonder if it is the right thing to do. > > Shouldn't we fix platform_driver_probe() to support deferred probing. well most drivers use platform_driver_register anyways. Other subsystems, like e.g. i2c have converted all drivers already. In drivers/power/ there are only three drivers using platform_driver_probe: drivers/power/avs/smartreflex.c - ok here drivers/power/reset/brcmstb-reboot.c - looks ok, too drivers/power/twl4030_charger.c - should probably be converted > As I understand it, it refused to retry a probe if there is an error, and the > comments suggest that such retrying is avoided because it would be a waste > of time: > > /* > * Prevent driver from requesting probe deferral to avoid further > * futile probe attempts. > */ > > In this case, it isn't futile. All drivers would benefit of being probed again if they returned EPROBEDEFER, but their probe function can't be called again if they use driver_platform_probe, since the probe function will be unloaded when it should be called again. Apart from that the .probe function pointer is not set. Thus trying to probe the driver again at a later point is "a waste of time" and "futile", since it will definitely fail. > Earlier there is a comment saying: > > * Use this instead of platform_driver_register() when you know the device > * is not hotpluggable and has already been registered, and you want to > * remove its run-once probe() infrastructure from memory after the driver > * has bound to the device. > > I presume all this applies. I assume that the only problem is a probe-order > thing. So maybe we should fix platform_driver_probe() to do the right thing > with -EPROBEDEFER?? > > Trouble is, I really don't understand the point or mechanism for > platform_driver_probe(), so I cannot suggest anything. > But I have been annoyed before that platform_driver_probe doesn't cope with > EPROBEDEFER, so I would like it fixed. platform_driver_probe is not about probe-order, but about not having the probe function in memory once the driver is loaded. So the probe function cannot be called again. If you don't want this use platform_driver_register, as most drivers actually do. I guess we should add some coccinelle scripts for detection of potentially broken drivers (e.g. everything requesting gpios/pinctrl together with platform_driver_register). -- Sebastian
Hi! > >> Ok, but: > >> > >> 1) Why is the twl4030-bci enabled in n900's dts, then > >> > > > > maybe it is bug in n900 dts... > > > > Grazvydas, is there some runtime check if twl4030/twl5030 chip > > has charger or not? or do we need to explicitly disable device > > twl4030-bci in DT? > > Actually from looking at the schematics, it looks like the charger > pins are still there but all connected to ground. So it probably has > the charger after all, it's just not connected or used. > > I'm not aware or any registers for direct detection, and indirect > detection is difficult because BCI mostly disables itself when no > charger is connected and most registers read as 0 or have old values > from last charging session (which will never happen on n900). > > There is IDCODE register on twl4030 itself, but it's documented as not > meaningful when accessed over i2c (when is it meaningful then??). > > drivers/mfd/twl-core.c has a i2c_device_id table of various twl4030 > variants, some of which have no charger. N900 has GAIA/twl5030, which > differs from twl4030 only by vaux2 regulator according to that file. > N900's old board files specify 5030, but .dts does not. I have enabled the (not-too-useful) twl4030 charger on my n900, and it seems to break boot with kernels 34c9a0ffc75ad25b6a60f61e27c4a4b1189b8085 and newer. I'll probably not have time to investigate it further (charger not connected to anything is not too useful), but maybe Neil wants to test on his phone...? Pavel
diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c index d35b83e..96bbbe9 100644 --- a/drivers/power/twl4030_charger.c +++ b/drivers/power/twl4030_charger.c @@ -714,6 +722,7 @@ static const struct of_device_id twl_bci_of_match[] = { MODULE_DEVICE_TABLE(of, twl_bci_of_match); static struct platform_driver twl4030_bci_driver = { + .probe = twl4030_bci_probe, .driver = { .name = "twl4030_bci", .of_match_table = of_match_ptr(twl_bci_of_match), @@ -721,7 +730,7 @@ static struct platform_driver twl4030_bci_driver = { .remove = __exit_p(twl4030_bci_remove), }; -module_platform_driver_probe(twl4030_bci_driver, twl4030_bci_probe); +module_platform_driver(twl4030_bci_driver); MODULE_AUTHOR("Gražvydas Ignotas"); MODULE_DESCRIPTION("TWL4030 Battery Charger Interface driver");
Hi! According to n900 dts, twl4030-bci (aka charger) should be included. (But it does not seem to do anything useful on n900. I was hoping for measurement of input voltage, but .. no.) Any ideas why the patch below is needed? Signed-off-by: Pavel Machek <pavel@ucw.cz>