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

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:

quoted

Am Donnerstag, den 28.07.2016, 10:45 -0500 schrieb Rob Herring:

quoted

On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid-qgqNFa1JUf/iIABC8yteiw@public.gmane.org
au>
wrote:

quoted

On 28/07/2016 10:11, Phil Reid wrote:

quoted

G'day Karl / Rob,
On 28/07/2016 05:08, Karl-Heinz Schneider wrote:

quoted

Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob
Herring:

quoted

On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid
wrote:

quoted

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.

quoted

quoted

quoted

quoted

quoted

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.

quoted

quoted

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