Re: [PATCH 1/1] power: sbs-manager: Add interrupt support and battery detect gpios

[Karl-Heinz Schneider <karl-heinz-X5L7DgJ4l23oE99TX8zNy7NAH6kLmebB@public.gmane.org>]

Hi Phil, Hi Rob,

Am Montag, den 01.08.2016, 14:52 +0800 schrieb Phil Reid:
> On 31/07/2016 22:04, Karl-Heinz Schneider wrote:
> > 
> > Am Donnerstag, den 28.07.2016, 10:45 -0500 schrieb Rob Herring:
> > > 
> > > On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid-qgqNFa1JUf/iIABC8yteiw@public.gmane.org
> > > au>
> > > wrote:
> > > > 
> > > > On 28/07/2016 10:11, Phil Reid wrote:
> > > > > 
> > > > > G'day Karl / Rob,
> > > > > On 28/07/2016 05:08, Karl-Heinz Schneider wrote:
> > > > > > 
> > > > > > Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob
> > > > > > Herring:
> > > > > > > 
> > > > > > > On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid
> > > > > > > wrote:
> > > > > > > > 
> > > > > > > > This patch added irq support for the SMBALERT pin and
> > > > > > > > notification of
> > > > > > > > the battery removal / insertion. The sbs manager would
> > > > > > > > typically be
> > > > > > > > used with the corresponding sbs-battery driver that
> > > > > > > > currently uses a
> > > > > > > > gpio input for battery presence and interrupt. To
> > > > > > > > remain
> > > > > > > > compatible
> > > > > > > > with
> > > > > > > > that existing driver this patch implements GPIO inputs
> > > > > > > > with
> > > > > > > > interrupt
> > > > > > > > support. IRQ masking is performed in software as the
> > > > > > > > hardware does not
> > > > > > > > support masking of notifications from each battery.
> > > > > > > > In addition a power_supply change notification is
> > > > > > > > generated
> > > > > > > > for the sbs
> > > > > > > > manager device when the AC present flag is changed.
> > > > > > > > Tested with LTC1760 and dual sbs compatible batteries.
> > > > > > > Please don't submit a binding and immediately turn around
> > > > > > > and
> > > > > > > add to it.
> > > > > > > While that is often preferred for drivers or kernel
> > > > > > > features,
> > > > > > > bindings
> > > > > > > should be complete as possible and not evolve. Combine
> > > > > > > this
> > > > > > > with your
> > > > > > > previous patch add sbs-mgr if that hasn't been accepted
> > > > > > > yet.
> > > > > That was from Karl. I'm happy to have this combined if he
> > > > > is.Me
> > > Indeed it was. Still, it should be combined. You can leave the
> > > driver
> > > implementation as multiple patches, just combine all the binding
> > > to a
> > > single patch by itself.
> > Will do that.
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > IMO, the sbs-bat should just be a interrupt and making it
> > > > > > > and
> > > > > > > this
> > > > > > > binding a GPIO is overkill. Since batteries nodes using
> > > > > > > this
> > > > > > > will be
> > > > > > > new, there's no reason the driver can't be updated to
> > > > > > > support
> > > > > > > interrupts.
> > > > I did consider this and didn't go that way as:
> > > > 1) the current sbs-battery driver didn't support it.
> > > > 2) Then there's also the problem of reporting  the presence of
> > > > the
> > > > battery
> > > > to the battery driver.
> > > > Without the GPIO support you resort to polling and looking for
> > > > a
> > > > NACK. Which
> > > > didn't seem that nice.
> > > The ability to read the current irq signal state with the irq api
> > > would be nice. Of course you can't really count on being able to
> > > on
> > > all irqs. Anyway, we shouldn't really design the binding around
> > > kernel
> > > limitations.
> > > 
> > > As I understand how SMBALERT works, you have to poll the devices
> > > anyway to determine which device is asserting the irq and to ack
> > > the
> > > irq. Of course, here you are doing point to point connections,
> > > but
> > > you
> > > still need to know which batteries are present by reading the
> > > sbs-mgr
> > > registers.
> > The more interesting point for me is, that SMBALERT seems to
> > provide an
> > easy to use driver side interface. The code Registering the IRQ
> > would
> > vanish...
> > Anyways I'm not very familiar with Linux IRQ handling, so I will
> > follow
> > your advise.
> Just had a look at driver/i2c/i2c-smbus.c
> This appears to be a generic driver to handle smbalert. Currently
> used by the i2c-parport driver.
> Doesn't look to support device trees at the moment thou.
> Patch series "RFC: I2C: i2c-smbus: add device tree support" from
> Andrea Merello looked to be adding generic support for it.
> https://lkml.org/lkml/2016/4/13/157
> Not sure where that's got to thou.
Did see this one.
> 
> > 
> > > 
> > > 
> > > Wouldn't switching the driver to level irq allow you to get the
> > > state
> > > immediately without a gpio read? After all, the gpio read and irq
> > > handler are reading the same register.
> If it's asserted continuously how do you stop the irq firing all the
> time?
> I guess you could register a high and low level irq and disable them
> alternatively.
> Unless I'm missing something.
> 
> Keeping the gpio functionality also allows the battery detect to work
> without the irq
> connected to the sbsm thru polling.
> 
So what to do now? I can simply take Phil's patch to add IRQ and GPIO
support to the sbsm driver. It needs some fixups though, at least
Kconfig things.

IMHO adding irq support adds complexity to the driver as well as to the
device tree binding, which shouldn't be necessary. Could also be true
for the GPIO part, though (sbsm could issue a call to .notify() for
it's slaves).

So anyway. Will take Phils patch and resubmit the sbs-manager patch
series if making the sbsm an IRQ and GPIO controller is really desired.
-- 
Karl-Heinz



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html