twl4030_charger: need changes to get probed?

twl4030_charger: need changes to get probed?

[Pali Rohár]

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...

-- 
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150306/cbf1153d/attachment.sig>

twl4030_charger: need changes to get probed?

[Grazvydas Ignotas]

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

twl4030_charger: need changes to get probed?

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

twl4030_charger: need changes to get probed?

[Pali Rohár]

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.

-- 
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150306/5841d687/attachment.sig>

twl4030_charger: need changes to get probed?

[Grazvydas Ignotas]

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

twl4030_charger: need changes to get probed?

[Pali Rohár]

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.

-- 
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150307/2b2552c3/attachment.sig>

twl4030_charger: need changes to get probed?

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

twl4030_charger: need changes to get probed?

[Sebastian Reichel]

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");
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150307/fa10e97e/attachment.sig>

twl4030_charger: need changes to get probed?

[NeilBrown]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150309/74073678/attachment.sig>

twl4030_charger: need changes to get probed?

[Sebastian Reichel]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150309/992889d1/attachment.sig>