[PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Andrew Jeffery]

Hello,

This is a rework of a Rube Goldberg-inspired RFC I posted previously:

https://lore.kernel.org/lkml/20210723075858.376378-1-andrew@aj.id.au/

This time around there's a lot less Rube - the series:

1. Contains no (ab)use of pinctrl
2. Always exposes all pins as GPIOs
3. Internally tracks the active pins

Without these patches the driver limits the number of pins exposed on
the gpiochip to the number of pins specified as GPIO in the devicetree,
but doesn't map between the GPIO and pin number spaces. The result is
that specifying offset or interleaved GPIOs in the devicetree gives
unexpected behaviour in userspace.

By always exposing all pins as GPIOs the patches resolve the lack of
mapping between GPIO offsets and pins on the package in the driver by
ensuring we always have a 1-to-1 mapping.

The issue is primarily addressed by patch 1/2. Patch 2/2 makes it
possible to not expose any pins as LEDs (and therefore make them all
accessible as GPIOs). This has a follow-on effect of allowing the driver
to bind to a device instantiated at runtime without requiring a
description in the devicetree.

I've tested the series under qemu to inspect the various interactions
between LEDs vs GPIOs as well as conflicting GPIO requests.

Please review!

Andrew

Andrew Jeffery (2):
  leds: pca955x: Make the gpiochip always expose all pins
  leds: pca955x: Allow zero LEDs to be specified

 drivers/leds/leds-pca955x.c | 65 +++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 31 deletions(-)


base-commit: 239f32b4f161c1584cd4b386d6ab8766432a6ede
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] leds: pca955x: Make the gpiochip always expose all pins

[Andrew Jeffery]

The devicetree binding allows specifying which pins are GPIO vs LED.
Limiting the instantiated gpiochip to just these pins as the driver
currently does requires an arbitrary mapping between pins and GPIOs, but
such a mapping is not implemented by the driver. As a result,
specifying GPIOs in such a way that they don't map 1-to-1 to pin indexes
does not function as expected.

Establishing such a mapping is more complex than not and even if we did,
doing so leads to a slightly hairy userspace experience as the behaviour
of the PCA955x gpiochip would depend on how the pins are assigned in the
devicetree. Instead, always expose all pins via the gpiochip to provide
a stable interface and track which pins are in use.

Specifying a pin as `type = <PCA955X_TYPE_GPIO>;` in the devicetree
becomes a no-op.

I've assessed the impact of this change by looking through all of the
affected devicetrees as of the tag leds-5.15-rc1:

```
$ git grep -l 'pca955[0123]' $(find . -name dts -type d)
arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
arch/arm/boot/dts/aspeed-bmc-opp-mowgli.dts
arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
```

These are all IBM-associated platforms. I've analysed both the
devicetrees and schematics where necessary to determine whether any
systems hit the hazard of the current broken behaviour. For the most
part, the systems specify the pins as either all LEDs or all GPIOs, or
at least do so in a way such that the broken behaviour isn't exposed.

The main counter-point to this observation is the Everest system whose
devicetree describes a large number of PCA955x devices and in some cases
has pin assignments that hit the hazard. However, there does not seem to
be any use of the affected GPIOs in the userspace associated with
Everest.

Regardless, any use of the hazardous GPIOs in Everest is already broken,
so let's fix the interface and then fix any already broken userspace
with it.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/leds/leds-pca955x.c | 63 +++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index a6b5699aeae4..77c0f461ab95 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -37,6 +37,7 @@
  *  bits the chip supports.
  */
 
+#include <linux/bitops.h>
 #include <linux/ctype.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -118,6 +119,7 @@ struct pca955x {
 	struct pca955x_led *leds;
 	struct pca955x_chipdef	*chipdef;
 	struct i2c_client	*client;
+	unsigned long active_pins;
 #ifdef CONFIG_LEDS_PCA955X_GPIO
 	struct gpio_chip gpio;
 #endif
@@ -360,12 +362,15 @@ static int pca955x_read_input(struct i2c_client *client, int n, u8 *val)
 static int pca955x_gpio_request_pin(struct gpio_chip *gc, unsigned int offset)
 {
 	struct pca955x *pca955x = gpiochip_get_data(gc);
-	struct pca955x_led *led = &pca955x->leds[offset];
 
-	if (led->type == PCA955X_TYPE_GPIO)
-		return 0;
+	return test_and_set_bit(offset, &pca955x->active_pins) ? -EBUSY : 0;
+}
 
-	return -EBUSY;
+static void pca955x_gpio_free_pin(struct gpio_chip *gc, unsigned int offset)
+{
+	struct pca955x *pca955x = gpiochip_get_data(gc);
+
+	clear_bit(offset, &pca955x->active_pins);
 }
 
 static int pca955x_set_value(struct gpio_chip *gc, unsigned int offset,
@@ -489,7 +494,6 @@ static int pca955x_probe(struct i2c_client *client)
 	struct i2c_adapter *adapter;
 	int i, err;
 	struct pca955x_platform_data *pdata;
-	int ngpios = 0;
 	bool set_default_label = false;
 	bool keep_pwm = false;
 	char default_label[8];
@@ -567,9 +571,7 @@ static int pca955x_probe(struct i2c_client *client)
 
 		switch (pca955x_led->type) {
 		case PCA955X_TYPE_NONE:
-			break;
 		case PCA955X_TYPE_GPIO:
-			ngpios++;
 			break;
 		case PCA955X_TYPE_LED:
 			led = &pca955x_led->led_cdev;
@@ -613,6 +615,8 @@ static int pca955x_probe(struct i2c_client *client)
 			if (err)
 				return err;
 
+			set_bit(i, &pca955x->active_pins);
+
 			/*
 			 * For default-state == "keep", let the core update the
 			 * brightness from the hardware, then check the
@@ -650,31 +654,30 @@ static int pca955x_probe(struct i2c_client *client)
 		return err;
 
 #ifdef CONFIG_LEDS_PCA955X_GPIO
-	if (ngpios) {
-		pca955x->gpio.label = "gpio-pca955x";
-		pca955x->gpio.direction_input = pca955x_gpio_direction_input;
-		pca955x->gpio.direction_output = pca955x_gpio_direction_output;
-		pca955x->gpio.set = pca955x_gpio_set_value;
-		pca955x->gpio.get = pca955x_gpio_get_value;
-		pca955x->gpio.request = pca955x_gpio_request_pin;
-		pca955x->gpio.can_sleep = 1;
-		pca955x->gpio.base = -1;
-		pca955x->gpio.ngpio = ngpios;
-		pca955x->gpio.parent = &client->dev;
-		pca955x->gpio.owner = THIS_MODULE;
+	pca955x->gpio.label = "gpio-pca955x";
+	pca955x->gpio.direction_input = pca955x_gpio_direction_input;
+	pca955x->gpio.direction_output = pca955x_gpio_direction_output;
+	pca955x->gpio.set = pca955x_gpio_set_value;
+	pca955x->gpio.get = pca955x_gpio_get_value;
+	pca955x->gpio.request = pca955x_gpio_request_pin;
+	pca955x->gpio.free = pca955x_gpio_free_pin;
+	pca955x->gpio.can_sleep = 1;
+	pca955x->gpio.base = -1;
+	pca955x->gpio.ngpio = chip->bits;
+	pca955x->gpio.parent = &client->dev;
+	pca955x->gpio.owner = THIS_MODULE;
 
-		err = devm_gpiochip_add_data(&client->dev, &pca955x->gpio,
-					     pca955x);
-		if (err) {
-			/* Use data->gpio.dev as a flag for freeing gpiochip */
-			pca955x->gpio.parent = NULL;
-			dev_warn(&client->dev, "could not add gpiochip\n");
-			return err;
-		}
-		dev_info(&client->dev, "gpios %i...%i\n",
-			 pca955x->gpio.base, pca955x->gpio.base +
-			 pca955x->gpio.ngpio - 1);
+	err = devm_gpiochip_add_data(&client->dev, &pca955x->gpio,
+				     pca955x);
+	if (err) {
+		/* Use data->gpio.dev as a flag for freeing gpiochip */
+		pca955x->gpio.parent = NULL;
+		dev_warn(&client->dev, "could not add gpiochip\n");
+		return err;
 	}
+	dev_info(&client->dev, "gpios %i...%i\n",
+		 pca955x->gpio.base, pca955x->gpio.base +
+		 pca955x->gpio.ngpio - 1);
 #endif
 
 	return 0;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] leds: pca955x: Make the gpiochip always expose all pins

[Andrew Jeffery]

On Tue, 21 Sep 2021, at 14:09, Andrew Jeffery wrote:
> The devicetree binding allows specifying which pins are GPIO vs LED.
> Limiting the instantiated gpiochip to just these pins as the driver
> currently does requires an arbitrary mapping between pins and GPIOs, but
> such a mapping is not implemented by the driver. As a result,
> specifying GPIOs in such a way that they don't map 1-to-1 to pin indexes
> does not function as expected.
>
> Establishing such a mapping is more complex than not and even if we did,
> doing so leads to a slightly hairy userspace experience as the behaviour
> of the PCA955x gpiochip would depend on how the pins are assigned in the
> devicetree. Instead, always expose all pins via the gpiochip to provide
> a stable interface and track which pins are in use.
>
> Specifying a pin as `type = <PCA955X_TYPE_GPIO>;` in the devicetree
> becomes a no-op.
>
> I've assessed the impact of this change by looking through all of the
> affected devicetrees as of the tag leds-5.15-rc1:
>
> ```
> $ git grep -l 'pca955[0123]' $(find . -name dts -type d)
> arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
> arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
> arch/arm/boot/dts/aspeed-bmc-opp-mowgli.dts
> arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
> arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> ```
>
> These are all IBM-associated platforms. I've analysed both the
> devicetrees and schematics where necessary to determine whether any
> systems hit the hazard of the current broken behaviour. For the most
> part, the systems specify the pins as either all LEDs or all GPIOs, or
> at least do so in a way such that the broken behaviour isn't exposed.
>
> The main counter-point to this observation is the Everest system whose
> devicetree describes a large number of PCA955x devices and in some cases
> has pin assignments that hit the hazard. However, there does not seem to
> be any use of the affected GPIOs in the userspace associated with
> Everest.
>
> Regardless, any use of the hazardous GPIOs in Everest is already broken,
> so let's fix the interface and then fix any already broken userspace
> with it.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Hello LED maintainers,

Just checking in on the state of this as it hasn't appeared in for-next.

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] leds: pca955x: Make the gpiochip always expose all pins

[Linus Walleij]

On Tue, Sep 21, 2021 at 6:40 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> The devicetree binding allows specifying which pins are GPIO vs LED.
> Limiting the instantiated gpiochip to just these pins as the driver
> currently does requires an arbitrary mapping between pins and GPIOs, but
> such a mapping is not implemented by the driver. As a result,
> specifying GPIOs in such a way that they don't map 1-to-1 to pin indexes
> does not function as expected.
>
> Establishing such a mapping is more complex than not and even if we did,
> doing so leads to a slightly hairy userspace experience as the behaviour
> of the PCA955x gpiochip would depend on how the pins are assigned in the
> devicetree. Instead, always expose all pins via the gpiochip to provide
> a stable interface and track which pins are in use.
>
> Specifying a pin as `type = <PCA955X_TYPE_GPIO>;` in the devicetree
> becomes a no-op.
>
> I've assessed the impact of this change by looking through all of the
> affected devicetrees as of the tag leds-5.15-rc1:
>
> ```
> $ git grep -l 'pca955[0123]' $(find . -name dts -type d)
> arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
> arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
> arch/arm/boot/dts/aspeed-bmc-opp-mowgli.dts
> arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
> arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> ```
>
> These are all IBM-associated platforms. I've analysed both the
> devicetrees and schematics where necessary to determine whether any
> systems hit the hazard of the current broken behaviour. For the most
> part, the systems specify the pins as either all LEDs or all GPIOs, or
> at least do so in a way such that the broken behaviour isn't exposed.
>
> The main counter-point to this observation is the Everest system whose
> devicetree describes a large number of PCA955x devices and in some cases
> has pin assignments that hit the hazard. However, there does not seem to
> be any use of the affected GPIOs in the userspace associated with
> Everest.
>
> Regardless, any use of the hazardous GPIOs in Everest is already broken,
> so let's fix the interface and then fix any already broken userspace
> with it.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] leds: pca955x: Make the gpiochip always expose all pins

[Joel Stanley]

Hello Pavel and Arnd,

This one has slipped through the cracks. Andrew asked for a follow up
and Linus sent a review, but we haven't heard from Pavel at all.

We've merged device tree changes through the soc tree in v5.16 that
depend on this patch. Ideally I would like to see it applied to fix
those device trees, instead of sending reverts for the device trees.

Additionally, I'm now reviewing changes for v5.17 and want to decide
which direction we should take.

Pavel, are you happy with the change?

If so, would you consider merging it as a fix for v5.16?

Cheers,

Joel

On Tue, 9 Nov 2021 at 11:03, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 21, 2021 at 6:40 AM Andrew Jeffery <andrew@aj.id.au> wrote:
>
> > The devicetree binding allows specifying which pins are GPIO vs LED.
> > Limiting the instantiated gpiochip to just these pins as the driver
> > currently does requires an arbitrary mapping between pins and GPIOs, but
> > such a mapping is not implemented by the driver. As a result,
> > specifying GPIOs in such a way that they don't map 1-to-1 to pin indexes
> > does not function as expected.
> >
> > Establishing such a mapping is more complex than not and even if we did,
> > doing so leads to a slightly hairy userspace experience as the behaviour
> > of the PCA955x gpiochip would depend on how the pins are assigned in the
> > devicetree. Instead, always expose all pins via the gpiochip to provide
> > a stable interface and track which pins are in use.
> >
> > Specifying a pin as `type = <PCA955X_TYPE_GPIO>;` in the devicetree
> > becomes a no-op.
> >
> > I've assessed the impact of this change by looking through all of the
> > affected devicetrees as of the tag leds-5.15-rc1:
> >
> > ```
> > $ git grep -l 'pca955[0123]' $(find . -name dts -type d)
> > arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
> > arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> > arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
> > arch/arm/boot/dts/aspeed-bmc-opp-mowgli.dts
> > arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
> > arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> > arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> > ```
> >
> > These are all IBM-associated platforms. I've analysed both the
> > devicetrees and schematics where necessary to determine whether any
> > systems hit the hazard of the current broken behaviour. For the most
> > part, the systems specify the pins as either all LEDs or all GPIOs, or
> > at least do so in a way such that the broken behaviour isn't exposed.
> >
> > The main counter-point to this observation is the Everest system whose
> > devicetree describes a large number of PCA955x devices and in some cases
> > has pin assignments that hit the hazard. However, there does not seem to
> > be any use of the affected GPIOs in the userspace associated with
> > Everest.
> >
> > Regardless, any use of the hazardous GPIOs in Everest is already broken,
> > so let's fix the interface and then fix any already broken userspace
> > with it.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] leds: pca955x: Allow zero LEDs to be specified

[Andrew Jeffery]

It's valid to use the PCA955x devices just for GPIOs and not for LEDs.
In this case, as PCA955X_TYPE_GPIO is now equivalent to
PCA955X_TYPE_NONE, remove the test for whether we have any child nodes
specified in the devicetree.

A consequence of this is it's now possible to bind the driver to a
PCA955x device when dynamically instantiated through the I2C subsystem's
`new_device` interface.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/leds/leds-pca955x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 77c0f461ab95..81aaf21212d7 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -429,7 +429,7 @@ pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
 	int count;
 
 	count = device_get_child_node_count(&client->dev);
-	if (!count || count > chip->bits)
+	if (count > chip->bits)
 		return ERR_PTR(-ENODEV);
 
 	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Andy Shevchenko]

On Tue, Sep 21, 2021 at 7:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Hello,
>
> This is a rework of a Rube Goldberg-inspired RFC I posted previously:
>
> https://lore.kernel.org/lkml/20210723075858.376378-1-andrew@aj.id.au/
>
> This time around there's a lot less Rube - the series:
>
> 1. Contains no (ab)use of pinctrl
> 2. Always exposes all pins as GPIOs
> 3. Internally tracks the active pins

Thanks for the rework! Briefly looking it looks very nice to me, hence, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Without these patches the driver limits the number of pins exposed on
> the gpiochip to the number of pins specified as GPIO in the devicetree,
> but doesn't map between the GPIO and pin number spaces. The result is
> that specifying offset or interleaved GPIOs in the devicetree gives
> unexpected behaviour in userspace.
>
> By always exposing all pins as GPIOs the patches resolve the lack of
> mapping between GPIO offsets and pins on the package in the driver by
> ensuring we always have a 1-to-1 mapping.
>
> The issue is primarily addressed by patch 1/2. Patch 2/2 makes it
> possible to not expose any pins as LEDs (and therefore make them all
> accessible as GPIOs). This has a follow-on effect of allowing the driver
> to bind to a device instantiated at runtime without requiring a
> description in the devicetree.
>
> I've tested the series under qemu to inspect the various interactions
> between LEDs vs GPIOs as well as conflicting GPIO requests.
>
> Please review!
>
> Andrew
>
> Andrew Jeffery (2):
>   leds: pca955x: Make the gpiochip always expose all pins
>   leds: pca955x: Allow zero LEDs to be specified
>
>  drivers/leds/leds-pca955x.c | 65 +++++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
>
>
> base-commit: 239f32b4f161c1584cd4b386d6ab8766432a6ede
> --
> 2.30.2
>


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Linus Walleij]

On Tue, Sep 21, 2021 at 6:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> 1. Contains no (ab)use of pinctrl
> 2. Always exposes all pins as GPIOs
> 3. Internally tracks the active pins

Looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Joel Stanley]

On Tue, 21 Sept 2021 at 04:39, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Hello,
>
> This is a rework of a Rube Goldberg-inspired RFC I posted previously:
>
> https://lore.kernel.org/lkml/20210723075858.376378-1-andrew@aj.id.au/
>
> This time around there's a lot less Rube - the series:
>
> 1. Contains no (ab)use of pinctrl
> 2. Always exposes all pins as GPIOs
> 3. Internally tracks the active pins
>
> Without these patches the driver limits the number of pins exposed on
> the gpiochip to the number of pins specified as GPIO in the devicetree,
> but doesn't map between the GPIO and pin number spaces. The result is
> that specifying offset or interleaved GPIOs in the devicetree gives
> unexpected behaviour in userspace.
>
> By always exposing all pins as GPIOs the patches resolve the lack of
> mapping between GPIO offsets and pins on the package in the driver by
> ensuring we always have a 1-to-1 mapping.
>
> The issue is primarily addressed by patch 1/2. Patch 2/2 makes it
> possible to not expose any pins as LEDs (and therefore make them all
> accessible as GPIOs). This has a follow-on effect of allowing the driver
> to bind to a device instantiated at runtime without requiring a
> description in the devicetree.
>
> I've tested the series under qemu to inspect the various interactions
> between LEDs vs GPIOs as well as conflicting GPIO requests.
>
> Please review!

Reviewed-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Cédric Le Goater]

On 9/21/21 06:39, Andrew Jeffery wrote:
> Hello,
> 
> This is a rework of a Rube Goldberg-inspired RFC I posted previously:
> 
> https://lore.kernel.org/lkml/20210723075858.376378-1-andrew@aj.id.au/
> 
> This time around there's a lot less Rube - the series:
> 
> 1. Contains no (ab)use of pinctrl
> 2. Always exposes all pins as GPIOs
> 3. Internally tracks the active pins
> 
> Without these patches the driver limits the number of pins exposed on
> the gpiochip to the number of pins specified as GPIO in the devicetree,
> but doesn't map between the GPIO and pin number spaces. The result is
> that specifying offset or interleaved GPIOs in the devicetree gives
> unexpected behaviour in userspace.
> 
> By always exposing all pins as GPIOs the patches resolve the lack of
> mapping between GPIO offsets and pins on the package in the driver by
> ensuring we always have a 1-to-1 mapping.
> 
> The issue is primarily addressed by patch 1/2. Patch 2/2 makes it
> possible to not expose any pins as LEDs (and therefore make them all
> accessible as GPIOs). This has a follow-on effect of allowing the driver
> to bind to a device instantiated at runtime without requiring a
> description in the devicetree.
> 
> I've tested the series under qemu to inspect the various interactions
> between LEDs vs GPIOs as well as conflicting GPIO requests.

I was wondering what kind of tests you did.

Did you cold plug 'pca995x' devices in a QEMU Aspeed EVB* machine running
with a custom DTB adding different layouts of the pca955x ?

> Please review!

This is simpler than the 'ngpio' business we had before.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> Andrew
> 
> Andrew Jeffery (2):
>    leds: pca955x: Make the gpiochip always expose all pins
>    leds: pca955x: Allow zero LEDs to be specified
> 
>   drivers/leds/leds-pca955x.c | 65 +++++++++++++++++++------------------
>   1 file changed, 34 insertions(+), 31 deletions(-)
> 
> 
> base-commit: 239f32b4f161c1584cd4b386d6ab8766432a6ede
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Joel Stanley]

Hello Pavel,



On Fri, 24 Sept 2021 at 06:41, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 9/21/21 06:39, Andrew Jeffery wrote:
> > Without these patches the driver limits the number of pins exposed on
> > the gpiochip to the number of pins specified as GPIO in the devicetree,
> > but doesn't map between the GPIO and pin number spaces. The result is
> > that specifying offset or interleaved GPIOs in the devicetree gives
> > unexpected behaviour in userspace.
> >
> > By always exposing all pins as GPIOs the patches resolve the lack of
> > mapping between GPIO offsets and pins on the package in the driver by
> > ensuring we always have a 1-to-1 mapping.
> >
> > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it
> > possible to not expose any pins as LEDs (and therefore make them all
> > accessible as GPIOs). This has a follow-on effect of allowing the driver
> > to bind to a device instantiated at runtime without requiring a
> > description in the devicetree.
> >
> > I've tested the series under qemu to inspect the various interactions
> > between LEDs vs GPIOs as well as conflicting GPIO requests.

> > Please review!
>
> This is simpler than the 'ngpio' business we had before.
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

I saw that you recently merged some LED patches. I was wondering if
you could consider this series for v5.18. It still applies cleanly,
and we've been running it for a while now, so it's very well tested.

Cheers,

Joel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1815 bytes --]

Hi!

> > > Without these patches the driver limits the number of pins exposed on
> > > the gpiochip to the number of pins specified as GPIO in the devicetree,
> > > but doesn't map between the GPIO and pin number spaces. The result is
> > > that specifying offset or interleaved GPIOs in the devicetree gives
> > > unexpected behaviour in userspace.
> > >
> > > By always exposing all pins as GPIOs the patches resolve the lack of
> > > mapping between GPIO offsets and pins on the package in the driver by
> > > ensuring we always have a 1-to-1 mapping.
> > >
> > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it
> > > possible to not expose any pins as LEDs (and therefore make them all
> > > accessible as GPIOs). This has a follow-on effect of allowing the driver
> > > to bind to a device instantiated at runtime without requiring a
> > > description in the devicetree.
> > >
> > > I've tested the series under qemu to inspect the various interactions
> > > between LEDs vs GPIOs as well as conflicting GPIO requests.
> 
> > > Please review!
> >
> > This is simpler than the 'ngpio' business we had before.
> >
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> I saw that you recently merged some LED patches. I was wondering if
> you could consider this series for v5.18. It still applies cleanly,
> and we've been running it for a while now, so it's very well tested.

Thanks, applied. I must say this is really ninja-mutant driver, but I
see no better way.

+++ b/drivers/leds/leds-pca955x.c
@@ -429,7 +429,7 @@ pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
        int count;

This really should be unsigned. Care to fix/submit a patch?

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] leds: pca955x: Expose GPIOs for all pins

[Andrew Jeffery]

On Wed, 2 Mar 2022, at 19:24, Pavel Machek wrote:
> Hi!
>
>> > > Without these patches the driver limits the number of pins exposed on
>> > > the gpiochip to the number of pins specified as GPIO in the devicetree,
>> > > but doesn't map between the GPIO and pin number spaces. The result is
>> > > that specifying offset or interleaved GPIOs in the devicetree gives
>> > > unexpected behaviour in userspace.
>> > >
>> > > By always exposing all pins as GPIOs the patches resolve the lack of
>> > > mapping between GPIO offsets and pins on the package in the driver by
>> > > ensuring we always have a 1-to-1 mapping.
>> > >
>> > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it
>> > > possible to not expose any pins as LEDs (and therefore make them all
>> > > accessible as GPIOs). This has a follow-on effect of allowing the driver
>> > > to bind to a device instantiated at runtime without requiring a
>> > > description in the devicetree.
>> > >
>> > > I've tested the series under qemu to inspect the various interactions
>> > > between LEDs vs GPIOs as well as conflicting GPIO requests.
>> 
>> > > Please review!
>> >
>> > This is simpler than the 'ngpio' business we had before.
>> >
>> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> 
>> I saw that you recently merged some LED patches. I was wondering if
>> you could consider this series for v5.18. It still applies cleanly,
>> and we've been running it for a while now, so it's very well tested.
>
> Thanks, applied. I must say this is really ninja-mutant driver, but I
> see no better way.
>
> +++ b/drivers/leds/leds-pca955x.c
> @@ -429,7 +429,7 @@ pca955x_get_pdata(struct i2c_client *client, struct 
> pca955x_chipdef *chip)
>         int count;
>
> This really should be unsigned. Care to fix/submit a patch?

I'll send a fix.

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel