[PATCH v2 0/3] bq24735-charger: allow gpio polling and sharing

[PATCH v2 0/3] bq24735-charger: allow gpio polling and sharing

[Peter Rosin]

Hi!

v1 -> v2 changes:
- use poll-interval instead of ti,poll-interval-ms in the bindings
- properly initialize the shared_lock mutex
- don't share gpios on active low/high mismatch
- don't regress users specifying the old ti,ac-detect-gpio form (without
  the trailing s)

I have a board that features a couple of parallel bq24735 chargers.
However, only one of the chargers has its ACOK pin wired to a gpio,
the other parallel chargers are expected to just behave the same
as the charger that has been singled out like this. Another thing
with the board is that the gpio is not capable of generating an
interrupt.

This series fixes things for me (ok, the first patch was just a
fix for a thing that initially had me confused for a bit, and is
totally unrelated. Ignore if you want to, it's basically just churn).

One thing that I wonder about is if anything should be done to
prevent unloading of the instance that shares its gpio? I thought
about adding a device_link_add() call, but am unsure if that would
be approprite? I'm not unloading drivers at all so it's a total
non-issue for me...

Cheers,
peda

Peter Rosin (3):
  power: supply: bq24735-charger: simplify register update to stop
    charging
  power: supply: bq24735-charger: optionally poll the ac-detect gpio
  power: supply: bq24735-charger: allow chargers to share the ac-detect
    gpio

 .../bindings/power/supply/ti,bq24735.txt           |   2 +
 drivers/power/supply/bq24735-charger.c             | 164 ++++++++++++++++++---
 2 files changed, 148 insertions(+), 18 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] power: supply: bq24735-charger: simplify register update to stop charging

[Peter Rosin]

Providing value bits outside of the mask is pointless.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/power/supply/bq24735-charger.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index eb7783b42e0a..1d5c9206e0ed 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -111,8 +111,7 @@ static inline int bq24735_enable_charging(struct bq24735 *charger)
 		return 0;
 
 	return bq24735_update_word(charger->client, BQ24735_CHG_OPT,
-				   BQ24735_CHG_OPT_CHARGE_DISABLE,
-				   ~BQ24735_CHG_OPT_CHARGE_DISABLE);
+				   BQ24735_CHG_OPT_CHARGE_DISABLE, 0);
 }
 
 static inline int bq24735_disable_charging(struct bq24735 *charger)
-- 
2.1.4

Re: [PATCH v2 1/3] power: supply: bq24735-charger: simplify register update to stop charging

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

Hi,

On Wed, Dec 14, 2016 at 12:56:43AM +0100, Peter Rosin wrote:
> Providing value bits outside of the mask is pointless.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/power/supply/bq24735-charger.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> index eb7783b42e0a..1d5c9206e0ed 100644
> --- a/drivers/power/supply/bq24735-charger.c
> +++ b/drivers/power/supply/bq24735-charger.c
> @@ -111,8 +111,7 @@ static inline int bq24735_enable_charging(struct bq24735 *charger)
>  		return 0;
>  
>  	return bq24735_update_word(charger->client, BQ24735_CHG_OPT,
> -				   BQ24735_CHG_OPT_CHARGE_DISABLE,
> -				   ~BQ24735_CHG_OPT_CHARGE_DISABLE);
> +				   BQ24735_CHG_OPT_CHARGE_DISABLE, 0);
>  }
>  
>  static inline int bq24735_disable_charging(struct bq24735 *charger)

Thanks for your patch. We are currently in the merge
window and your patch will appear in linux-next once
4.10-rc1 has been tagged by Linus Torvalds.

Until then I queued it into this branch:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next-next

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/3] power: supply: bq24735-charger: optionally poll the ac-detect gpio

[Peter Rosin]

If the ac-detect gpio does not support interrupts, provide a fallback
to poll the gpio at a configurable interval.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../bindings/power/supply/ti,bq24735.txt           |  2 +
 drivers/power/supply/bq24735-charger.c             | 50 +++++++++++++++++++---
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
index 3bf55757ceec..54f0004643e2 100644
--- a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
+++ b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
@@ -25,6 +25,8 @@ Optional properties :
  - ti,external-control : Indicates that the charger is configured externally
    and that the host should not attempt to enable/disable charging or set the
    charge voltage/current.
+ - poll-interval : In case 'interrupts' is not specified, poll AC presense
+   on the ti,ac-detect-gpios GPIO with this interval (milliseconds).
 
 Example:
 
diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index 1d5c9206e0ed..f45876927676 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -50,6 +50,8 @@ struct bq24735 {
 	struct bq24735_platform		*pdata;
 	struct mutex			lock;
 	struct gpio_desc		*status_gpio;
+	struct delayed_work		poll;
+	u32				poll_interval;
 	bool				charging;
 };
 
@@ -209,11 +211,8 @@ static int bq24735_charger_is_charging(struct bq24735 *charger)
 	return !(ret & BQ24735_CHG_OPT_CHARGE_DISABLE);
 }
 
-static irqreturn_t bq24735_charger_isr(int irq, void *devid)
+static void bq24735_update(struct bq24735 *charger)
 {
-	struct power_supply *psy = devid;
-	struct bq24735 *charger = to_bq24735(psy);
-
 	mutex_lock(&charger->lock);
 
 	if (charger->charging && bq24735_charger_is_present(charger))
@@ -223,11 +222,29 @@ static irqreturn_t bq24735_charger_isr(int irq, void *devid)
 
 	mutex_unlock(&charger->lock);
 
-	power_supply_changed(psy);
+	power_supply_changed(charger->charger);
+}
+
+static irqreturn_t bq24735_charger_isr(int irq, void *devid)
+{
+	struct power_supply *psy = devid;
+	struct bq24735 *charger = to_bq24735(psy);
+
+	bq24735_update(charger);
 
 	return IRQ_HANDLED;
 }
 
+static void bq24735_poll(struct work_struct *work)
+{
+	struct bq24735 *charger = container_of(work, struct bq24735, poll.work);
+
+	bq24735_update(charger);
+
+	schedule_delayed_work(&charger->poll,
+			      msecs_to_jiffies(charger->poll_interval));
+}
+
 static int bq24735_charger_get_property(struct power_supply *psy,
 					enum power_supply_property psp,
 					union power_supply_propval *val)
@@ -455,11 +472,33 @@ static int bq24735_charger_probe(struct i2c_client *client,
 				client->irq, ret);
 			return ret;
 		}
+	} else if (charger->status_gpio) {
+		ret = of_property_read_u32(client->dev.of_node,
+					   "poll-interval",
+					   &charger->poll_interval);
+		if (ret)
+			return 0;
+		if (!charger->poll_interval)
+			return 0;
+
+		INIT_DELAYED_WORK(&charger->poll, bq24735_poll);
+		schedule_delayed_work(&charger->poll,
+				      msecs_to_jiffies(charger->poll_interval));
 	}
 
 	return 0;
 }
 
+static int bq24735_charger_remove(struct i2c_client *client)
+{
+	struct bq24735 *charger = i2c_get_clientdata(client);
+
+	if (charger->poll_interval)
+		cancel_delayed_work_sync(&charger->poll);
+
+	return 0;
+}
+
 static const struct i2c_device_id bq24735_charger_id[] = {
 	{ "bq24735-charger", 0 },
 	{}
@@ -478,6 +517,7 @@ static struct i2c_driver bq24735_charger_driver = {
 		.of_match_table = bq24735_match_ids,
 	},
 	.probe = bq24735_charger_probe,
+	.remove = bq24735_charger_remove,
 	.id_table = bq24735_charger_id,
 };
 
-- 
2.1.4

Re: [PATCH v2 2/3] power: supply: bq24735-charger: optionally poll the ac-detect gpio

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

Hi,

On Wed, Dec 14, 2016 at 12:56:44AM +0100, Peter Rosin wrote:
> If the ac-detect gpio does not support interrupts, provide a fallback
> to poll the gpio at a configurable interval.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>
> [...]
>
> +	} else if (charger->status_gpio) {
> +		ret = of_property_read_u32(client->dev.of_node,
> +					   "poll-interval",
> +					   &charger->poll_interval);

Please use device_property_read_u32() instead.

> [...]

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3] power: supply: bq24735-charger: optionally poll the ac-detect gpio

[Peter Rosin]

If the ac-detect gpio does not support interrupts, provide a fallback
to poll the gpio at a configurable interval.

Signed-off-by: Peter Rosin <peda@axentia.se>
---

v2 -> v3 changes:
- use device_property_read_u32 instead of of_property_read_u32

v1 -> v2 changes:
- use poll-interval instead of ti,poll-interval-ms in the bindings

Cheers,
peda

 .../bindings/power/supply/ti,bq24735.txt           |  2 +
 drivers/power/supply/bq24735-charger.c             | 49 +++++++++++++++++++---
 2 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
index 3bf55757ceec..799d0e5d6f26 100644
--- a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
+++ b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
@@ -25,6 +25,8 @@ Optional properties :
  - ti,external-control : Indicates that the charger is configured externally
    and that the host should not attempt to enable/disable charging or set the
    charge voltage/current.
+ - poll-interval : In case 'interrupts' is not specified, poll AC presence
+   on the ti,ac-detect-gpios GPIO with this interval (milliseconds).
 
 Example:
 
diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index 1d5c9206e0ed..8a0242c13b7e 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -50,6 +50,8 @@ struct bq24735 {
 	struct bq24735_platform		*pdata;
 	struct mutex			lock;
 	struct gpio_desc		*status_gpio;
+	struct delayed_work		poll;
+	u32				poll_interval;
 	bool				charging;
 };
 
@@ -209,11 +211,8 @@ static int bq24735_charger_is_charging(struct bq24735 *charger)
 	return !(ret & BQ24735_CHG_OPT_CHARGE_DISABLE);
 }
 
-static irqreturn_t bq24735_charger_isr(int irq, void *devid)
+static void bq24735_update(struct bq24735 *charger)
 {
-	struct power_supply *psy = devid;
-	struct bq24735 *charger = to_bq24735(psy);
-
 	mutex_lock(&charger->lock);
 
 	if (charger->charging && bq24735_charger_is_present(charger))
@@ -223,11 +222,29 @@ static irqreturn_t bq24735_charger_isr(int irq, void *devid)
 
 	mutex_unlock(&charger->lock);
 
-	power_supply_changed(psy);
+	power_supply_changed(charger->charger);
+}
+
+static irqreturn_t bq24735_charger_isr(int irq, void *devid)
+{
+	struct power_supply *psy = devid;
+	struct bq24735 *charger = to_bq24735(psy);
+
+	bq24735_update(charger);
 
 	return IRQ_HANDLED;
 }
 
+static void bq24735_poll(struct work_struct *work)
+{
+	struct bq24735 *charger = container_of(work, struct bq24735, poll.work);
+
+	bq24735_update(charger);
+
+	schedule_delayed_work(&charger->poll,
+			      msecs_to_jiffies(charger->poll_interval));
+}
+
 static int bq24735_charger_get_property(struct power_supply *psy,
 					enum power_supply_property psp,
 					union power_supply_propval *val)
@@ -455,11 +472,32 @@ static int bq24735_charger_probe(struct i2c_client *client,
 				client->irq, ret);
 			return ret;
 		}
+	} else if (charger->status_gpio) {
+		ret = device_property_read_u32(&client->dev, "poll-interval",
+					       &charger->poll_interval);
+		if (ret)
+			return 0;
+		if (!charger->poll_interval)
+			return 0;
+
+		INIT_DELAYED_WORK(&charger->poll, bq24735_poll);
+		schedule_delayed_work(&charger->poll,
+				      msecs_to_jiffies(charger->poll_interval));
 	}
 
 	return 0;
 }
 
+static int bq24735_charger_remove(struct i2c_client *client)
+{
+	struct bq24735 *charger = i2c_get_clientdata(client);
+
+	if (charger->poll_interval)
+		cancel_delayed_work_sync(&charger->poll);
+
+	return 0;
+}
+
 static const struct i2c_device_id bq24735_charger_id[] = {
 	{ "bq24735-charger", 0 },
 	{}
@@ -478,6 +516,7 @@ static struct i2c_driver bq24735_charger_driver = {
 		.of_match_table = bq24735_match_ids,
 	},
 	.probe = bq24735_charger_probe,
+	.remove = bq24735_charger_remove,
 	.id_table = bq24735_charger_id,
 };
 
-- 
2.1.4

Re: [PATCH v3] power: supply: bq24735-charger: optionally poll the ac-detect gpio

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

Hi,

On Thu, Dec 15, 2016 at 10:28:46AM +0100, Peter Rosin wrote:
> If the ac-detect gpio does not support interrupts, provide a fallback
> to poll the gpio at a configurable interval.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Thanks for your patch. We are currently in the merge
window and your patch will appear in linux-next once
4.10-rc1 has been tagged by Linus Torvalds.

Until then I queued it into this branch:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next-next

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] power: supply: bq24735-charger: optionally poll the ac-detect gpio

[Rob Herring]

On Thu, Dec 15, 2016 at 10:28:46AM +0100, Peter Rosin wrote:
> If the ac-detect gpio does not support interrupts, provide a fallback
> to poll the gpio at a configurable interval.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> 
> v2 -> v3 changes:
> - use device_property_read_u32 instead of of_property_read_u32
> 
> v1 -> v2 changes:
> - use poll-interval instead of ti,poll-interval-ms in the bindings
> 
> Cheers,
> peda
> 
>  .../bindings/power/supply/ti,bq24735.txt           |  2 +

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/power/supply/bq24735-charger.c             | 49 +++++++++++++++++++---
>  2 files changed, 46 insertions(+), 5 deletions(-)

[PATCH v2 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio

[Peter Rosin]

If several parallel bq24735 chargers have their ac-detect gpios wired
together (or if only one of the parallel bq24735 chargers have its
ac-detect pin wired to a gpio, and the others are assumed to react the
same), then all driver instances need to check the same gpio. But the
gpio subsystem does not allow sharing gpios, so handle that locally.

However, only do this for the polling case, sharing is not supported if
the ac detection is handled with interrupts.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/power/supply/bq24735-charger.c | 111 +++++++++++++++++++++++++++++----
 1 file changed, 100 insertions(+), 11 deletions(-)

diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index f45876927676..c2403c4d5ece 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -25,6 +25,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
@@ -43,12 +44,24 @@
 #define BQ24735_MANUFACTURER_ID		0xfe
 #define BQ24735_DEVICE_ID		0xff
 
+struct bq24735;
+
+struct bq24735_shared {
+	struct list_head		list;
+	struct bq24735			*owner;
+	struct gpio_desc		*status_gpio;
+};
+
+static DEFINE_MUTEX(shared_lock);
+static LIST_HEAD(shared_list);
+
 struct bq24735 {
 	struct power_supply		*charger;
 	struct power_supply_desc	charger_desc;
 	struct i2c_client		*client;
 	struct bq24735_platform		*pdata;
 	struct mutex			lock;
+	struct bq24735_shared		*shared;
 	struct gpio_desc		*status_gpio;
 	struct delayed_work		poll;
 	u32				poll_interval;
@@ -346,6 +359,75 @@ static struct bq24735_platform *bq24735_parse_dt_data(struct i2c_client *client)
 	return pdata;
 }
 
+static struct gpio_desc *bq24735_get_status_gpio(struct bq24735 *charger)
+{
+	struct device *dev = &charger->client->dev;
+	struct bq24735_shared *shared;
+	int gpio;
+	enum of_gpio_flags flags;
+	int active_low;
+	struct list_head *pos;
+
+	if (of_property_read_bool(dev->of_node, "ti,ac-detect-gpios"))
+		gpio = of_get_named_gpio_flags(dev->of_node,
+					       "ti,ac-detect-gpios", 0, &flags);
+	else if (of_property_read_bool(dev->of_node, "ti,ac-detect-gpio"))
+		gpio = of_get_named_gpio_flags(dev->of_node,
+					       "ti,ac-detect-gpio", 0, &flags);
+	else
+		return NULL;
+
+	if (!gpio_is_valid(gpio))
+		return ERR_PTR(gpio);
+	active_low = flags & OF_GPIO_ACTIVE_LOW;
+
+	mutex_lock(&shared_lock);
+	list_for_each(pos, &shared_list) {
+		shared = list_entry(pos, struct bq24735_shared, list);
+		if (gpio != desc_to_gpio(shared->status_gpio))
+			continue;
+		if (!active_low ^ !gpiod_is_active_low(shared->status_gpio))
+			continue;
+
+		get_device(&shared->owner->client->dev);
+		dev_dbg(dev, "sharing gpio with %s\n",
+			shared->owner->pdata->name);
+		charger->shared = shared;
+		mutex_unlock(&shared_lock);
+		return shared->status_gpio;
+	}
+
+	shared = devm_kzalloc(dev, sizeof(*shared), GFP_KERNEL);
+	if (!shared) {
+		mutex_unlock(&shared_lock);
+		return ERR_PTR(-ENOMEM);
+	}
+	shared->owner = charger;
+	shared->status_gpio = gpiod_get(dev, "ti,ac-detect", GPIOD_IN);
+	if (!IS_ERR(shared->status_gpio)) {
+		charger->shared = shared;
+		list_add(&shared->list, &shared_list);
+	}
+	mutex_unlock(&shared_lock);
+
+	return shared->status_gpio;
+}
+
+static void bq24735_put_status_gpio(struct bq24735 *charger)
+{
+	if (!charger->shared)
+		return;
+
+	mutex_lock(&shared_lock);
+	if (charger->shared->owner != charger) {
+		put_device(&charger->shared->owner->client->dev);
+	} else {
+		list_del(&charger->shared->list);
+		gpiod_put(charger->shared->status_gpio);
+	}
+	mutex_unlock(&shared_lock);
+}
+
 static int bq24735_charger_probe(struct i2c_client *client,
 				 const struct i2c_device_id *id)
 {
@@ -402,9 +484,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, charger);
 
-	charger->status_gpio = devm_gpiod_get_optional(&client->dev,
-						       "ti,ac-detect",
-						       GPIOD_IN);
+	charger->status_gpio = bq24735_get_status_gpio(charger);
 	if (IS_ERR(charger->status_gpio)) {
 		ret = PTR_ERR(charger->status_gpio);
 		dev_err(&client->dev, "Getting gpio failed: %d\n", ret);
@@ -416,28 +496,30 @@ static int bq24735_charger_probe(struct i2c_client *client,
 		if (ret < 0) {
 			dev_err(&client->dev, "Failed to read manufacturer id : %d\n",
 				ret);
-			return ret;
+			goto out;
 		} else if (ret != 0x0040) {
 			dev_err(&client->dev,
 				"manufacturer id mismatch. 0x0040 != 0x%04x\n", ret);
-			return -ENODEV;
+			ret = -ENODEV;
+			goto out;
 		}
 
 		ret = bq24735_read_word(client, BQ24735_DEVICE_ID);
 		if (ret < 0) {
 			dev_err(&client->dev, "Failed to read device id : %d\n", ret);
-			return ret;
+			goto out;
 		} else if (ret != 0x000B) {
 			dev_err(&client->dev,
 				"device id mismatch. 0x000b != 0x%04x\n", ret);
-			return -ENODEV;
+			ret = -ENODEV;
+			goto out;
 		}
 	}
 
 	ret = bq24735_config_charger(charger);
 	if (ret < 0) {
 		dev_err(&client->dev, "failed in configuring charger");
-		return ret;
+		goto out;
 	}
 
 	/* check for AC adapter presence */
@@ -445,7 +527,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
 		ret = bq24735_enable_charging(charger);
 		if (ret < 0) {
 			dev_err(&client->dev, "Failed to enable charging\n");
-			return ret;
+			goto out;
 		}
 	}
 
@@ -455,7 +537,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
 		ret = PTR_ERR(charger->charger);
 		dev_err(&client->dev, "Failed to register power supply: %d\n",
 			ret);
-		return ret;
+		goto out;
 	}
 
 	if (client->irq) {
@@ -470,7 +552,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
 			dev_err(&client->dev,
 				"Unable to register IRQ %d err %d\n",
 				client->irq, ret);
-			return ret;
+			goto out;
 		}
 	} else if (charger->status_gpio) {
 		ret = of_property_read_u32(client->dev.of_node,
@@ -487,6 +569,11 @@ static int bq24735_charger_probe(struct i2c_client *client,
 	}
 
 	return 0;
+
+out:
+	bq24735_put_status_gpio(charger);
+
+	return ret;
 }
 
 static int bq24735_charger_remove(struct i2c_client *client)
@@ -496,6 +583,8 @@ static int bq24735_charger_remove(struct i2c_client *client)
 	if (charger->poll_interval)
 		cancel_delayed_work_sync(&charger->poll);
 
+	bq24735_put_status_gpio(charger);
+
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH v2 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 7445 bytes --]

Hi,

On Wed, Dec 14, 2016 at 12:56:45AM +0100, Peter Rosin wrote:
> If several parallel bq24735 chargers have their ac-detect gpios wired
> together (or if only one of the parallel bq24735 chargers have its
> ac-detect pin wired to a gpio, and the others are assumed to react the
> same), then all driver instances need to check the same gpio. But the
> gpio subsystem does not allow sharing gpios, so handle that locally.

Adding GPIO subsystem people to see if they can come up with
something in the gpiod API for this usecase.

> However, only do this for the polling case, sharing is not supported if
> the ac detection is handled with interrupts.

Why? I guess you only added the gpio polling stuff for the shared
gpio feature, so we can skip the gpio polling if we add shared irq
support instead?

> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/power/supply/bq24735-charger.c | 111 +++++++++++++++++++++++++++++----
>  1 file changed, 100 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> index f45876927676..c2403c4d5ece 100644
> --- a/drivers/power/supply/bq24735-charger.c
> +++ b/drivers/power/supply/bq24735-charger.c
> @@ -25,6 +25,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/power_supply.h>
>  #include <linux/slab.h>
> @@ -43,12 +44,24 @@
>  #define BQ24735_MANUFACTURER_ID		0xfe
>  #define BQ24735_DEVICE_ID		0xff
>  
> +struct bq24735;
> +
> +struct bq24735_shared {
> +	struct list_head		list;
> +	struct bq24735			*owner;
> +	struct gpio_desc		*status_gpio;
> +};
> +
> +static DEFINE_MUTEX(shared_lock);
> +static LIST_HEAD(shared_list);
> +
>  struct bq24735 {
>  	struct power_supply		*charger;
>  	struct power_supply_desc	charger_desc;
>  	struct i2c_client		*client;
>  	struct bq24735_platform		*pdata;
>  	struct mutex			lock;
> +	struct bq24735_shared		*shared;
>  	struct gpio_desc		*status_gpio;
>  	struct delayed_work		poll;
>  	u32				poll_interval;
> @@ -346,6 +359,75 @@ static struct bq24735_platform *bq24735_parse_dt_data(struct i2c_client *client)
>  	return pdata;
>  }
>  
> +static struct gpio_desc *bq24735_get_status_gpio(struct bq24735 *charger)
> +{
> +	struct device *dev = &charger->client->dev;
> +	struct bq24735_shared *shared;
> +	int gpio;
> +	enum of_gpio_flags flags;
> +	int active_low;
> +	struct list_head *pos;
> +
> +	if (of_property_read_bool(dev->of_node, "ti,ac-detect-gpios"))
> +		gpio = of_get_named_gpio_flags(dev->of_node,
> +					       "ti,ac-detect-gpios", 0, &flags);
> +	else if (of_property_read_bool(dev->of_node, "ti,ac-detect-gpio"))
> +		gpio = of_get_named_gpio_flags(dev->of_node,
> +					       "ti,ac-detect-gpio", 0, &flags);
> +	else
> +		return NULL;
> +
> +	if (!gpio_is_valid(gpio))
> +		return ERR_PTR(gpio);
> +	active_low = flags & OF_GPIO_ACTIVE_LOW;
> +
> +	mutex_lock(&shared_lock);
> +	list_for_each(pos, &shared_list) {
> +		shared = list_entry(pos, struct bq24735_shared, list);
> +		if (gpio != desc_to_gpio(shared->status_gpio))
> +			continue;
> +		if (!active_low ^ !gpiod_is_active_low(shared->status_gpio))
> +			continue;
> +
> +		get_device(&shared->owner->client->dev);
> +		dev_dbg(dev, "sharing gpio with %s\n",
> +			shared->owner->pdata->name);
> +		charger->shared = shared;
> +		mutex_unlock(&shared_lock);
> +		return shared->status_gpio;
> +	}
> +
> +	shared = devm_kzalloc(dev, sizeof(*shared), GFP_KERNEL);
> +	if (!shared) {
> +		mutex_unlock(&shared_lock);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	shared->owner = charger;
> +	shared->status_gpio = gpiod_get(dev, "ti,ac-detect", GPIOD_IN);
> +	if (!IS_ERR(shared->status_gpio)) {
> +		charger->shared = shared;
> +		list_add(&shared->list, &shared_list);
> +	}
> +	mutex_unlock(&shared_lock);
> +
> +	return shared->status_gpio;
> +}
> +
> +static void bq24735_put_status_gpio(struct bq24735 *charger)
> +{
> +	if (!charger->shared)
> +		return;
> +
> +	mutex_lock(&shared_lock);
> +	if (charger->shared->owner != charger) {
> +		put_device(&charger->shared->owner->client->dev);
> +	} else {
> +		list_del(&charger->shared->list);
> +		gpiod_put(charger->shared->status_gpio);
> +	}
> +	mutex_unlock(&shared_lock);
> +}
> +
>  static int bq24735_charger_probe(struct i2c_client *client,
>  				 const struct i2c_device_id *id)
>  {
> @@ -402,9 +484,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, charger);
>  
> -	charger->status_gpio = devm_gpiod_get_optional(&client->dev,
> -						       "ti,ac-detect",
> -						       GPIOD_IN);
> +	charger->status_gpio = bq24735_get_status_gpio(charger);
>  	if (IS_ERR(charger->status_gpio)) {
>  		ret = PTR_ERR(charger->status_gpio);
>  		dev_err(&client->dev, "Getting gpio failed: %d\n", ret);
> @@ -416,28 +496,30 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  		if (ret < 0) {
>  			dev_err(&client->dev, "Failed to read manufacturer id : %d\n",
>  				ret);
> -			return ret;
> +			goto out;
>  		} else if (ret != 0x0040) {
>  			dev_err(&client->dev,
>  				"manufacturer id mismatch. 0x0040 != 0x%04x\n", ret);
> -			return -ENODEV;
> +			ret = -ENODEV;
> +			goto out;
>  		}
>  
>  		ret = bq24735_read_word(client, BQ24735_DEVICE_ID);
>  		if (ret < 0) {
>  			dev_err(&client->dev, "Failed to read device id : %d\n", ret);
> -			return ret;
> +			goto out;
>  		} else if (ret != 0x000B) {
>  			dev_err(&client->dev,
>  				"device id mismatch. 0x000b != 0x%04x\n", ret);
> -			return -ENODEV;
> +			ret = -ENODEV;
> +			goto out;
>  		}
>  	}
>  
>  	ret = bq24735_config_charger(charger);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "failed in configuring charger");
> -		return ret;
> +		goto out;
>  	}
>  
>  	/* check for AC adapter presence */
> @@ -445,7 +527,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  		ret = bq24735_enable_charging(charger);
>  		if (ret < 0) {
>  			dev_err(&client->dev, "Failed to enable charging\n");
> -			return ret;
> +			goto out;
>  		}
>  	}
>  
> @@ -455,7 +537,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  		ret = PTR_ERR(charger->charger);
>  		dev_err(&client->dev, "Failed to register power supply: %d\n",
>  			ret);
> -		return ret;
> +		goto out;
>  	}
>  
>  	if (client->irq) {
> @@ -470,7 +552,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  			dev_err(&client->dev,
>  				"Unable to register IRQ %d err %d\n",
>  				client->irq, ret);
> -			return ret;
> +			goto out;
>  		}
>  	} else if (charger->status_gpio) {
>  		ret = of_property_read_u32(client->dev.of_node,
> @@ -487,6 +569,11 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  	}
>  
>  	return 0;
> +
> +out:
> +	bq24735_put_status_gpio(charger);
> +
> +	return ret;
>  }
>  
>  static int bq24735_charger_remove(struct i2c_client *client)
> @@ -496,6 +583,8 @@ static int bq24735_charger_remove(struct i2c_client *client)
>  	if (charger->poll_interval)
>  		cancel_delayed_work_sync(&charger->poll);
>  
> +	bq24735_put_status_gpio(charger);
> +
>  	return 0;
>  }
>  
> -- 
> 2.1.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 8056 bytes --]

[of course I forgot to actually add gpio people, let's try again]

On Wed, Dec 14, 2016 at 05:59:21PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Dec 14, 2016 at 12:56:45AM +0100, Peter Rosin wrote:
> > If several parallel bq24735 chargers have their ac-detect gpios wired
> > together (or if only one of the parallel bq24735 chargers have its
> > ac-detect pin wired to a gpio, and the others are assumed to react the
> > same), then all driver instances need to check the same gpio. But the
> > gpio subsystem does not allow sharing gpios, so handle that locally.
> 
> Adding GPIO subsystem people to see if they can come up with
> something in the gpiod API for this usecase.
> 
> > However, only do this for the polling case, sharing is not supported if
> > the ac detection is handled with interrupts.
> 
> Why? I guess you only added the gpio polling stuff for the shared
> gpio feature, so we can skip the gpio polling if we add shared irq
> support instead?
> 
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> > ---
> >  drivers/power/supply/bq24735-charger.c | 111 +++++++++++++++++++++++++++++----
> >  1 file changed, 100 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> > index f45876927676..c2403c4d5ece 100644
> > --- a/drivers/power/supply/bq24735-charger.c
> > +++ b/drivers/power/supply/bq24735-charger.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/power_supply.h>
> >  #include <linux/slab.h>
> > @@ -43,12 +44,24 @@
> >  #define BQ24735_MANUFACTURER_ID		0xfe
> >  #define BQ24735_DEVICE_ID		0xff
> >  
> > +struct bq24735;
> > +
> > +struct bq24735_shared {
> > +	struct list_head		list;
> > +	struct bq24735			*owner;
> > +	struct gpio_desc		*status_gpio;
> > +};
> > +
> > +static DEFINE_MUTEX(shared_lock);
> > +static LIST_HEAD(shared_list);
> > +
> >  struct bq24735 {
> >  	struct power_supply		*charger;
> >  	struct power_supply_desc	charger_desc;
> >  	struct i2c_client		*client;
> >  	struct bq24735_platform		*pdata;
> >  	struct mutex			lock;
> > +	struct bq24735_shared		*shared;
> >  	struct gpio_desc		*status_gpio;
> >  	struct delayed_work		poll;
> >  	u32				poll_interval;
> > @@ -346,6 +359,75 @@ static struct bq24735_platform *bq24735_parse_dt_data(struct i2c_client *client)
> >  	return pdata;
> >  }
> >  
> > +static struct gpio_desc *bq24735_get_status_gpio(struct bq24735 *charger)
> > +{
> > +	struct device *dev = &charger->client->dev;
> > +	struct bq24735_shared *shared;
> > +	int gpio;
> > +	enum of_gpio_flags flags;
> > +	int active_low;
> > +	struct list_head *pos;
> > +
> > +	if (of_property_read_bool(dev->of_node, "ti,ac-detect-gpios"))
> > +		gpio = of_get_named_gpio_flags(dev->of_node,
> > +					       "ti,ac-detect-gpios", 0, &flags);
> > +	else if (of_property_read_bool(dev->of_node, "ti,ac-detect-gpio"))
> > +		gpio = of_get_named_gpio_flags(dev->of_node,
> > +					       "ti,ac-detect-gpio", 0, &flags);
> > +	else
> > +		return NULL;
> > +
> > +	if (!gpio_is_valid(gpio))
> > +		return ERR_PTR(gpio);
> > +	active_low = flags & OF_GPIO_ACTIVE_LOW;
> > +
> > +	mutex_lock(&shared_lock);
> > +	list_for_each(pos, &shared_list) {
> > +		shared = list_entry(pos, struct bq24735_shared, list);
> > +		if (gpio != desc_to_gpio(shared->status_gpio))
> > +			continue;
> > +		if (!active_low ^ !gpiod_is_active_low(shared->status_gpio))
> > +			continue;
> > +
> > +		get_device(&shared->owner->client->dev);
> > +		dev_dbg(dev, "sharing gpio with %s\n",
> > +			shared->owner->pdata->name);
> > +		charger->shared = shared;
> > +		mutex_unlock(&shared_lock);
> > +		return shared->status_gpio;
> > +	}
> > +
> > +	shared = devm_kzalloc(dev, sizeof(*shared), GFP_KERNEL);
> > +	if (!shared) {
> > +		mutex_unlock(&shared_lock);
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +	shared->owner = charger;
> > +	shared->status_gpio = gpiod_get(dev, "ti,ac-detect", GPIOD_IN);
> > +	if (!IS_ERR(shared->status_gpio)) {
> > +		charger->shared = shared;
> > +		list_add(&shared->list, &shared_list);
> > +	}
> > +	mutex_unlock(&shared_lock);
> > +
> > +	return shared->status_gpio;
> > +}
> > +
> > +static void bq24735_put_status_gpio(struct bq24735 *charger)
> > +{
> > +	if (!charger->shared)
> > +		return;
> > +
> > +	mutex_lock(&shared_lock);
> > +	if (charger->shared->owner != charger) {
> > +		put_device(&charger->shared->owner->client->dev);
> > +	} else {
> > +		list_del(&charger->shared->list);
> > +		gpiod_put(charger->shared->status_gpio);
> > +	}
> > +	mutex_unlock(&shared_lock);
> > +}
> > +
> >  static int bq24735_charger_probe(struct i2c_client *client,
> >  				 const struct i2c_device_id *id)
> >  {
> > @@ -402,9 +484,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
> >  
> >  	i2c_set_clientdata(client, charger);
> >  
> > -	charger->status_gpio = devm_gpiod_get_optional(&client->dev,
> > -						       "ti,ac-detect",
> > -						       GPIOD_IN);
> > +	charger->status_gpio = bq24735_get_status_gpio(charger);
> >  	if (IS_ERR(charger->status_gpio)) {
> >  		ret = PTR_ERR(charger->status_gpio);
> >  		dev_err(&client->dev, "Getting gpio failed: %d\n", ret);
> > @@ -416,28 +496,30 @@ static int bq24735_charger_probe(struct i2c_client *client,
> >  		if (ret < 0) {
> >  			dev_err(&client->dev, "Failed to read manufacturer id : %d\n",
> >  				ret);
> > -			return ret;
> > +			goto out;
> >  		} else if (ret != 0x0040) {
> >  			dev_err(&client->dev,
> >  				"manufacturer id mismatch. 0x0040 != 0x%04x\n", ret);
> > -			return -ENODEV;
> > +			ret = -ENODEV;
> > +			goto out;
> >  		}
> >  
> >  		ret = bq24735_read_word(client, BQ24735_DEVICE_ID);
> >  		if (ret < 0) {
> >  			dev_err(&client->dev, "Failed to read device id : %d\n", ret);
> > -			return ret;
> > +			goto out;
> >  		} else if (ret != 0x000B) {
> >  			dev_err(&client->dev,
> >  				"device id mismatch. 0x000b != 0x%04x\n", ret);
> > -			return -ENODEV;
> > +			ret = -ENODEV;
> > +			goto out;
> >  		}
> >  	}
> >  
> >  	ret = bq24735_config_charger(charger);
> >  	if (ret < 0) {
> >  		dev_err(&client->dev, "failed in configuring charger");
> > -		return ret;
> > +		goto out;
> >  	}
> >  
> >  	/* check for AC adapter presence */
> > @@ -445,7 +527,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
> >  		ret = bq24735_enable_charging(charger);
> >  		if (ret < 0) {
> >  			dev_err(&client->dev, "Failed to enable charging\n");
> > -			return ret;
> > +			goto out;
> >  		}
> >  	}
> >  
> > @@ -455,7 +537,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
> >  		ret = PTR_ERR(charger->charger);
> >  		dev_err(&client->dev, "Failed to register power supply: %d\n",
> >  			ret);
> > -		return ret;
> > +		goto out;
> >  	}
> >  
> >  	if (client->irq) {
> > @@ -470,7 +552,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
> >  			dev_err(&client->dev,
> >  				"Unable to register IRQ %d err %d\n",
> >  				client->irq, ret);
> > -			return ret;
> > +			goto out;
> >  		}
> >  	} else if (charger->status_gpio) {
> >  		ret = of_property_read_u32(client->dev.of_node,
> > @@ -487,6 +569,11 @@ static int bq24735_charger_probe(struct i2c_client *client,
> >  	}
> >  
> >  	return 0;
> > +
> > +out:
> > +	bq24735_put_status_gpio(charger);
> > +
> > +	return ret;
> >  }
> >  
> >  static int bq24735_charger_remove(struct i2c_client *client)
> > @@ -496,6 +583,8 @@ static int bq24735_charger_remove(struct i2c_client *client)
> >  	if (charger->poll_interval)
> >  		cancel_delayed_work_sync(&charger->poll);
> >  
> > +	bq24735_put_status_gpio(charger);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.1.4
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio

[Peter Rosin]

On 2016-12-14 18:01, Sebastian Reichel wrote:
> [of course I forgot to actually add gpio people, let's try again]
> 
> On Wed, Dec 14, 2016 at 05:59:21PM +0100, Sebastian Reichel wrote:
>> Hi,
>>
>> On Wed, Dec 14, 2016 at 12:56:45AM +0100, Peter Rosin wrote:
>>> If several parallel bq24735 chargers have their ac-detect gpios wired
>>> together (or if only one of the parallel bq24735 chargers have its
>>> ac-detect pin wired to a gpio, and the others are assumed to react the
>>> same), then all driver instances need to check the same gpio. But the
>>> gpio subsystem does not allow sharing gpios, so handle that locally.
>>
>> Adding GPIO subsystem people to see if they can come up with
>> something in the gpiod API for this usecase.

Right, I don't like how my new code steps away from gpio descriptors.

>>> However, only do this for the polling case, sharing is not supported if
>>> the ac detection is handled with interrupts.
>>
>> Why? I guess you only added the gpio polling stuff for the shared
>> gpio feature, so we can skip the gpio polling if we add shared irq
>> support instead?

Nope, the hw really can't do interrupts on the gpio, it's just not wired
up for that. The gpio is on an expander, and the irq line from the expander
top the cpu is not there (due to a desire to minimize the number of
connections between two parts of the system). So I need both polling and
sharing.

Cheers,
peda

Re: [PATCH v2 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio

[Linus Walleij]

On Wed, Dec 14, 2016 at 6:41 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2016-12-14 18:01, Sebastian Reichel wrote:
>> [of course I forgot to actually add gpio people, let's try again]
>>
>> On Wed, Dec 14, 2016 at 05:59:21PM +0100, Sebastian Reichel wrote:
>>> Hi,
>>>
>>> On Wed, Dec 14, 2016 at 12:56:45AM +0100, Peter Rosin wrote:
>>>> If several parallel bq24735 chargers have their ac-detect gpios wired
>>>> together (or if only one of the parallel bq24735 chargers have its
>>>> ac-detect pin wired to a gpio, and the others are assumed to react the
>>>> same), then all driver instances need to check the same gpio. But the
>>>> gpio subsystem does not allow sharing gpios, so handle that locally.
>>>
>>> Adding GPIO subsystem people to see if they can come up with
>>> something in the gpiod API for this usecase.
>
> Right, I don't like how my new code steps away from gpio descriptors.

The issue of shared gpiods have come up over and over again.
For example the messy regulator code needs this too.

It is better if we implement something like gpiod_get_shared()
in the gpiolib of these cases.

Just put a refcount in struct gpio_desc in drivers/gpio/gpiolib.h
for this case I guess?

Yours,
Linus Walleij

Re: [PATCH v2 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio

[Peter Rosin]

On 2016-12-30 08:49, Linus Walleij wrote:
> On Wed, Dec 14, 2016 at 6:41 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2016-12-14 18:01, Sebastian Reichel wrote:
>>> [of course I forgot to actually add gpio people, let's try again]
>>>
>>> On Wed, Dec 14, 2016 at 05:59:21PM +0100, Sebastian Reichel wrote:
>>>> Hi,
>>>>
>>>> On Wed, Dec 14, 2016 at 12:56:45AM +0100, Peter Rosin wrote:
>>>>> If several parallel bq24735 chargers have their ac-detect gpios wired
>>>>> together (or if only one of the parallel bq24735 chargers have its
>>>>> ac-detect pin wired to a gpio, and the others are assumed to react the
>>>>> same), then all driver instances need to check the same gpio. But the
>>>>> gpio subsystem does not allow sharing gpios, so handle that locally.
>>>>
>>>> Adding GPIO subsystem people to see if they can come up with
>>>> something in the gpiod API for this usecase.
>>
>> Right, I don't like how my new code steps away from gpio descriptors.
> 
> The issue of shared gpiods have come up over and over again.
> For example the messy regulator code needs this too.
> 
> It is better if we implement something like gpiod_get_shared()
> in the gpiolib of these cases.
> 
> Just put a refcount in struct gpio_desc in drivers/gpio/gpiolib.h
> for this case I guess?

I actually tried that, but ran into atomicy issues with the
FLAG_REQUESTED bit and gave up. Didn't really try all that hard
though, but I simply didn't feel comfortable with going near such
fundamental designs...

Cheers,
peda

Re: [PATCH v2 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio

[Linus Walleij]

On Mon, Jan 2, 2017 at 9:31 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2016-12-30 08:49, Linus Walleij wrote:
>> On Wed, Dec 14, 2016 at 6:41 PM, Peter Rosin <peda@axentia.se> wrote:
>>> On 2016-12-14 18:01, Sebastian Reichel wrote:
>>>> [of course I forgot to actually add gpio people, let's try again]
>>>>
>>>> On Wed, Dec 14, 2016 at 05:59:21PM +0100, Sebastian Reichel wrote:
>>>>> Hi,
>>>>>
>>>>> On Wed, Dec 14, 2016 at 12:56:45AM +0100, Peter Rosin wrote:
>>>>>> If several parallel bq24735 chargers have their ac-detect gpios wired
>>>>>> together (or if only one of the parallel bq24735 chargers have its
>>>>>> ac-detect pin wired to a gpio, and the others are assumed to react the
>>>>>> same), then all driver instances need to check the same gpio. But the
>>>>>> gpio subsystem does not allow sharing gpios, so handle that locally.
>>>>>
>>>>> Adding GPIO subsystem people to see if they can come up with
>>>>> something in the gpiod API for this usecase.
>>>
>>> Right, I don't like how my new code steps away from gpio descriptors.
>>
>> The issue of shared gpiods have come up over and over again.
>> For example the messy regulator code needs this too.
>>
>> It is better if we implement something like gpiod_get_shared()
>> in the gpiolib of these cases.
>>
>> Just put a refcount in struct gpio_desc in drivers/gpio/gpiolib.h
>> for this case I guess?
>
> I actually tried that, but ran into atomicy issues with the
> FLAG_REQUESTED bit and gave up. Didn't really try all that hard
> though, but I simply didn't feel comfortable with going near such
> fundamental designs...

Oh I see. Well if it is of any help that would make me nervous too.

I would just remove the use of FLAG_REQUESTED altogether,
redefine the flags in gpiolib.h from 0 and add a struct kref into the struct
to deal with the refcounting.

That should do it. I think.

Yours,
Linus Walleij