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

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

[Phil Reid]

Adds SMBALERT interupt support fo AC present and battery insertation / removal.

This patch is dependent on karl-heinz@schneider-inet.de patch series:
[PATCH v3] Add support for Smart Battery System Manager

Tested with my patches to the sbs-battery driver applied:
[PATCH v2] power: sbs-battery fixup gpio detect irq handling

sbs-battery driver patch  can be applied before or after this series without
breaking anything.


Phil Reid (1):
  power: sbs-manager: Add interrupt support and battery detect gpios

 .../devicetree/bindings/power/sbs,sbs-manager.txt  |  29 ++++
 drivers/power/sbs-manager.c                        | 157 +++++++++++++++++++++
 2 files changed, 186 insertions(+)

-- 
1.8.3.1

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

[Phil Reid]

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.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 .../devicetree/bindings/power/sbs,sbs-manager.txt  |  29 ++++
 drivers/power/sbs-manager.c                        | 157 +++++++++++++++++++++
 2 files changed, 186 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
index f0e2253..624967e 100644
--- a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
+++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
@@ -3,6 +3,26 @@ Binding for sbs-manager
 Required properties:
 - compatible: should be "lltc,ltc1760" or use "sbs,sbs-manager" as fallback.
 - reg: integer, i2c address of the device. Should be <0xa>.
+Optional properties:
+- gpio-controller: Marks the port as GPIO controller.
+  See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
+- #gpio-cells: Should be <2>. The first cell is the pin number, the second cell
+  is used to specify optional parameters:
+  See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
+- interrupt-parent: Phandle for the interrupt controller that services
+  interrupts for this device.
+  See also .../devicetree/bindings/interrupt-controller/interrupts.txt.
+- interrupts: Interrupt mapping for SMBALERT IRQ.
+  See also .../devicetree/bindings/interrupt-controller/interrupts.txt.
+- #interrupt-cells: Should be <2>. The first cell is the GPIO number. The
+  second cell is used to specify flags. The following subset of flags is
+  supported:
+  - trigger type (bits[1:0]):
+      3 = low-to-high or high-to-low edge triggered
+      Valid value is 3
+  See also .../devicetree/bindings/interrupt-controller/interrupts.txt.
+- interrupt-controller: Marks the device node as an interrupt controller.
+  See also .../devicetree/bindings/interrupt-controller/interrupts.txt.
 
 From OS view the device is basically an i2c-mux used to communicate with up to
 four smart battery devices at address 0xb. The driver actually implements this
@@ -17,6 +37,15 @@ batman@0a {
     #address-cells = <1>;
     #size-cells = <0>;
 
+    gpio-controller;
+    #gpio-cells = <2>;
+
+    interrupt-controller;
+    #interrupt-cells=<2>;
+    interrupt-parent = <&parent_irq>;
+    interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+    interrupts-names = "irq";
+
     i2c@1 {
         #address-cells = <1>;
         #size-cells = <0>;
diff --git a/drivers/power/sbs-manager.c b/drivers/power/sbs-manager.c
index adf9e41..3911329 100644
--- a/drivers/power/sbs-manager.c
+++ b/drivers/power/sbs-manager.c
@@ -20,6 +20,8 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/power_supply.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
 
 #define SBSM_MAX_BATS  4
 #define SBSM_RETRY_CNT 3
@@ -30,14 +32,22 @@
 #define SBSM_CMD_BATSYSINFO      0x04
 #define SBSM_CMD_LTC             0x3c
 
+#define SBSM_BIT_AC_PRESENT      BIT(0)
+
 struct sbsm_data {
 	struct i2c_client *client;
 	struct i2c_mux_core *muxc;
 
 	struct power_supply *psy;
 
+	struct gpio_chip chip;
+
 	int cur_chan;         /* currently selected channel */
 	bool is_ltc1760;      /* special capabilities */
+
+	unsigned int irq_mask;
+	unsigned int last_state;
+	unsigned int last_state_cont;
 };
 
 static enum power_supply_property sbsm_props[] = {
@@ -206,6 +216,149 @@ static const struct power_supply_desc sbsm_default_psy_desc = {
 	.property_is_writeable = &sbsm_prop_is_writeable,
 };
 
+static int sbsm_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+	struct sbsm_data *data = gpiochip_get_data(gc);
+	int ret;
+
+	ret = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATE);
+	if (ret < 0)
+		return ret;
+
+	return ret & BIT(off);
+}
+
+/*
+ * This needs to be defined or the GPIO lib fails to register the pin.
+ * But the 'gpio' is always an input.
+ */
+static int sbsm_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+{
+	return 0;
+}
+
+static irqreturn_t sbsm_irq(int irq, void *data)
+{
+	struct sbsm_data *sbsm = data;
+	int ret, i, nhandled = 0, child_irq = 0, irq_bat = 0;
+
+	ret = sbsm_read_word(sbsm->client, SBSM_CMD_BATSYSSTATE);
+	if (ret >= 0)
+		irq_bat = (ret ^ sbsm->last_state) & sbsm->irq_mask;
+	sbsm->last_state = ret;
+
+	ret = sbsm_read_word(sbsm->client, SBSM_CMD_BATSYSSTATECONT);
+	if ((ret >= 0) &&
+	    ((ret ^ sbsm->last_state_cont) & SBSM_BIT_AC_PRESENT)) {
+		irq_bat |= sbsm->irq_mask;
+		power_supply_changed(sbsm->psy);
+	}
+	sbsm->last_state_cont = ret;
+
+	for (i = 0; i < SBSM_MAX_BATS; i++) {
+		if (irq_bat & BIT(i)) {
+			child_irq = irq_find_mapping(sbsm->chip.irqdomain, i);
+			handle_nested_irq(child_irq);
+			nhandled++;
+		}
+	}
+
+	return (nhandled > 0) ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void sbsm_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct sbsm_data *sbsm = gpiochip_get_data(gc);
+	unsigned int pos = data->hwirq;
+
+	sbsm->irq_mask &= ~BIT(pos);
+}
+
+static void sbsm_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct sbsm_data *sbsm = gpiochip_get_data(gc);
+	unsigned int pos = data->hwirq;
+
+	sbsm->irq_mask |= BIT(pos);
+}
+
+static int sbsm_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH)
+		return 0;
+
+	return -EINVAL;
+}
+
+static struct irq_chip sbsm_irq_chip = {
+	.name = "sbs-manager",
+	.irq_mask = sbsm_irq_mask,
+	.irq_unmask = sbsm_irq_unmask,
+	.irq_set_type = sbsm_irq_set_type,
+};
+
+static int sbsm_gpio_setup(struct sbsm_data *data)
+{
+	struct gpio_chip *gc = &data->chip;
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
+	struct device_node *of_node = client->dev.of_node;
+	unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_HIGH;
+	int irq = client->irq;
+	int ret;
+
+	if (!of_get_property(of_node, "gpio-controller", NULL))
+		return 0;
+
+	ret  = sbsm_read_word(client, SBSM_CMD_BATSYSSTATE);
+	if (ret < 0)
+		return ret;
+	data->last_state = ret;
+
+	ret  = sbsm_read_word(client, SBSM_CMD_BATSYSSTATECONT);
+	if (ret < 0)
+		return ret;
+	data->last_state_cont = ret;
+
+	gc->get = sbsm_gpio_get_value;
+	gc->direction_input  = sbsm_gpio_direction_input;
+	gc->can_sleep = true;
+	gc->base = -1;
+	gc->ngpio = SBSM_MAX_BATS;
+	gc->label = client->name;
+	gc->parent = dev;
+	gc->owner = THIS_MODULE;
+
+	ret = devm_gpiochip_add_data(dev, gc, data);
+	if (ret) {
+		dev_err(dev, "devm_gpiochip_add_data failed: %d\n", ret);
+		return ret;
+	}
+
+	if (irq < 0)
+		return 0;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, sbsm_irq,
+					irqflags, dev_name(dev), data);
+	if (ret != 0) {
+		dev_err(dev, "unable to request IRQ%d: %d\n", irq, ret);
+		return ret;
+	}
+
+	ret =  gpiochip_irqchip_add(gc, &sbsm_irq_chip, 0, handle_simple_irq,
+				    IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(dev, "gpiochip_irqchip_add failed: %d\n", ret);
+		return ret;
+	}
+
+	gpiochip_set_chained_irqchip(gc, &sbsm_irq_chip, irq, NULL);
+
+	return ret;
+}
+
 static int sbsm_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -283,6 +436,10 @@ static int sbsm_probe(struct i2c_client *client,
 		goto err_psy;
 	}
 
+	ret = sbsm_gpio_setup(data);
+	if (ret < 0)
+		goto err_psy;
+
 	dev_info(dev, "sbsm registered\n");
 	return 0;
 
-- 
1.8.3.1

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

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

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.

Also, I'm not even convinced you need the sbs-mgr to be an 
interrupt-controller. Is this anything more that just wire-OR'ed 
interrupt lines which can be handled as shared irq? Does reading 
SBSM_CMD_BATSYSSTATE have a side effect for example?

Rob

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

[Karl-Heinz Schneider]

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.
> 
> 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.
> 
> Also, I'm not even convinced you need the sbs-mgr to be an 
> interrupt-controller. Is this anything more that just wire-OR'ed 
> interrupt lines which can be handled as shared irq? Does reading 
> SBSM_CMD_BATSYSSTATE have a side effect for example?
Yes, according to the Datasheed it clears SMBALERT#.
> 
> Rob
SMBALERT is electrically an active low line which is pulled down by one
or more devices needing attention, isn't it? The I2C host driver or some
other instance need to implement support for it, the device driver may
register some sort of handler function (e.g by implementing struct
i2c_driver.alert function)

That's my understanding of the SMBALERT.

I once tried to implement the alert function and see if it get's called.
It didn't. At that point I didn't investigate further.

So, do I miss something or is that part incomplete/broken? I can only
find lm90 and ipmi_ssif which implement the callback.
-- 
Karl-Heinz

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

[Phil Reid]

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.

>>
>> 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.
>>
>> Also, I'm not even convinced you need the sbs-mgr to be an
>> interrupt-controller. Is this anything more that just wire-OR'ed
>> interrupt lines which can be handled as shared irq? Does reading
>> SBSM_CMD_BATSYSSTATE have a side effect for example?
> Yes, according to the Datasheed it clears SMBALERT#.

Yes you need to service the Ltc1760 for the irq to clear. reading the batteries does nothing.
Even without any batteries present  you may get an irq and need to service it.
eg: I have a system with 2 ltc1760 with a total of 4 batteries.
So if only one battery is present and AC power is connected then both ltc1760 will generate
a notification on SMBALERT. One which has no batteries present.

>>
>> Rob
> SMBALERT is electrically an active low line which is pulled down by one
> or more devices needing attention, isn't it? The I2C host driver or some
> other instance need to implement support for it, the device driver may
> register some sort of handler function (e.g by implementing struct
> i2c_driver.alert function)
>
> That's my understanding of the SMBALERT.
>
> I once tried to implement the alert function and see if it get's called.
> It didn't. At that point I didn't investigate further.
>
> So, do I miss something or is that part incomplete/broken? I can only
> find lm90 and ipmi_ssif which implement the callback.
>
I wasn't aware of that functionality. Most devices seem to use a GPIO to
obtain irq support. Which I guess is the most flexible solution.
I think SMBALERT also support a mechanism to query all devices on the bus to determine
which device address is asserting the irq. Not sure how this would work with multiplexed
address buses (eg dual ltc1760) and devices with the same address.

Open to suggestions

-- 
Regards
Phil Reid

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

[Phil Reid]

G'day Rob,

Just another thought.

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


>>>
>>> Also, I'm not even convinced you need the sbs-mgr to be an
>>> interrupt-controller. Is this anything more that just wire-OR'ed
>>> interrupt lines which can be handled as shared irq? Does reading
>>> SBSM_CMD_BATSYSSTATE have a side effect for example?
>> Yes, according to the Datasheed it clears SMBALERT#.
>
> Yes you need to service the Ltc1760 for the irq to clear. reading the batteries does nothing.
> Even without any batteries present  you may get an irq and need to service it.
> eg: I have a system with 2 ltc1760 with a total of 4 batteries.
> So if only one battery is present and AC power is connected then both ltc1760 will generate
> a notification on SMBALERT. One which has no batteries present.
>
>>>
>>> Rob
>> SMBALERT is electrically an active low line which is pulled down by one
>> or more devices needing attention, isn't it? The I2C host driver or some
>> other instance need to implement support for it, the device driver may
>> register some sort of handler function (e.g by implementing struct
>> i2c_driver.alert function)
>>
>> That's my understanding of the SMBALERT.
>>
>> I once tried to implement the alert function and see if it get's called.
>> It didn't. At that point I didn't investigate further.
>>
>> So, do I miss something or is that part incomplete/broken? I can only
>> find lm90 and ipmi_ssif which implement the callback.
>>
> I wasn't aware of that functionality. Most devices seem to use a GPIO to
> obtain irq support. Which I guess is the most flexible solution.
> I think SMBALERT also support a mechanism to query all devices on the bus to determine
> which device address is asserting the irq. Not sure how this would work with multiplexed
> address buses (eg dual ltc1760) and devices with the same address.
>
> Open to suggestions
>


-- 
Regards
Phil Reid

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

[Rob Herring]

On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid@electromag.com.au> wrote:
> G'day Rob,
>
> Just another thought.
>
> 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.

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.

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

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.

Rob

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

[Karl-Heinz Schneider]

Am Donnerstag, den 28.07.2016, 10:45 -0500 schrieb Rob Herring:
> On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid@electromag.com.au>
> wrote:
> > 
> > G'day Rob,
> > 
> > Just another thought.
> > 
> > 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.
> 
> 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.
> 
> Rob
-- 
Karl-Heinz

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

[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/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>> 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.

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

-- 
Regards
Phil Reid


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

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

[Karl-Heinz Schneider]

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

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

[Phil Reid]

On 24/08/2016 03:58, Karl-Heinz Schneider wrote:
> 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.
>

G'day Karl,

I'm planing on looking at the .notify() solution for sbsm.
Having the sbsm trigger an .notify() is an interesting solution.
It could certainly reduce a fair bit of my code and probably address the
biggest concern with the device tree.

I'm all for getting the driver submitted as you have it and I'll sort something
out later. I'm probably a couple of weeks away from getting time to look at this
again.



-- 
Regards
Phil Reid


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