Re: [PATCH 2/4] ARM: tegra: nyan: Use external control for bq24735 charger

[Paul Kocialkowski]

Le mercredi 21 septembre 2016 Ã 08:30 +0100, Jon Hunter a ÃcritÂ:
> On 20/09/16 19:02, Paul Kocialkowski wrote:
> > 
> > * PGP Signed by an unknown key
> > 
> > Le mardi 20 septembre 2016 Ã 18:40 +0100, Jon Hunter a Ãcrit :
> > > 
> > > On 28/08/16 18:32, Paul Kocialkowski wrote:
> > > > 
> > > > Â
> > > > Nyan boards come with an embedded controller that controls when to
> > > > enable and disable the charge. Thus, it should not be left up to the
> > > > kernel to handle that.
> > > > Â
> > > > Using the ti,external-control property allows specifying this use-case.
> > > Â
> > > So the bq24735 is populated under the EC's 'i2c-tunnel' property which
> > > is there to specifically interface it's child devices to the host. So I
> > > am a bit confused why this is expose to the host if it should not be used?
> > 
> > Well, it needs to access the information in the read-only registers provided
> > by
> > the chip, which is allowed by the setup in place that you described.
> 
> Is this to expose the current state to the kernel so we can monitor the
> battery state?

Yes, that is correct.

> > However, the EC has its internal state machine that decides when to start
> > charging, etc and so should be the only one to write registers, to avoid
> > conflicts.
> > 
> > > 
> > > Again you may right and I did find the original series [0] for this
> > > which specifically references the Acer Chromebook that needs this.
> > > However, I am not sure why this was never populated? Is there any other
> > > history here?
> > 
> > I am also confused about why it wasn't applied earlier. However, the cros
> > kernel
> > is using the very same scheme.
> 
> Do you have a reference?

Sure thing, there's a similar commit for the dts[0] and one for the driver[1]
(which was already merged in mainline).

> > > What is the actual problem you see without making this change?
> > 
> > There is a risk of conflict (even though it's probably not that
> > significant),
> > given the low variety of possible cases here. The idea is simply to say that
> > the
> > EC is in charge and to let it do its job without interfering.
> > 
> > > 
> > > The original series states ...
> > > Â
> > > "On Acer Chromebook 13 (CB5-311) this module fails to load if the
> > > charger is not inserted, and will error when it is removed."
> > 
> > I'm confused about that comment. At this point (and with this patch), it
> > works
> > normally.
> 
> Ok, I think Thierry prefers to only apply fixes for problems that can be
> reproduced. Is there a simple way to check the battery status and
> charging status via say the sysfs? If I can test that this has no
> negative impact may be it is ok.

Sure, this is exported at: /sys/class/power_supply/bq24735@5-0009
Also, note that the power-supply next branch[2] has some more fixes for the
bq24735 driver.

Cheers,

[0]:Âhttps://chromium.googlesource.com/chromiumos/third_party/kernel/+/e25a91f87af41e29012a4e2dd7a9ab725efd308e
[1]: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/6b34e53d506b44f911d0fd246ccdc8b4e942e4ae
[2]: https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next

-- 
Paul Kocialkowski, developer of low-level free software for embedded devices

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

Attachment: signature.asc
Description: This is a digitally signed message part