[RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andrew Jeffery]

Hello,

This series does a bunch of crimes, so it's an RFC. I'm cross-posting to the
pinctrl/GPIO and LEDs lists because the PCA955x devices impact all of them. What
needs fixing is the leds-pca955x driver's failure to map the GPIO numberspace to
the pin numberspace of the PCA955x devices. The series solves that by
implementing pinctrl and pinmux in the leds-pca955x driver.

Things I'm unsure about:

1. Patch 1: The pinctrl_gpio_as_pin() API feels a bit dirty, not sure what
   others thoughts are on that (Linus?).

2. Patch 2: I've added a new callback to hook the entirety of the pinctrl map
   parsing rather than supplying a subnode-specific callback. This was necessary
   to handle the PCA955x devicetree binding in a backwards compatible way.

3. Patch 4: The PCA955x devices don't actually have any pinmux hardware, but the
   properties of the pinctrl/pinmux subsystems in the kernel map nicely onto the
   problem we have. But it's quite a bit of code...

4. Patch 6: I also lost a bunch of time to overlooking the get_group_pins()
   callback for pinctrl, and it seems odd to me that it isn't required.

Please review!

Andrew

Andrew Jeffery (6):
  pinctrl: Add pinctrl_gpio_as_pin()
  pinctrl: Add hook for device-specific map parsing
  leds: pca955x: Relocate chipdef-related descriptors
  leds: pca955x: Use pinctrl to map GPIOs to pins
  ARM: dts: rainier: Add presence-detect and fault indictor GPIO
    expander
  pinctrl: Check get_group_pins callback on init

 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts |  76 +++
 drivers/leds/leds-pca955x.c                  | 554 +++++++++++++++----
 drivers/pinctrl/core.c                       |  28 +-
 include/linux/pinctrl/pinctrl.h              |   4 +
 4 files changed, 566 insertions(+), 96 deletions(-)

-- 
2.30.2

[RFC PATCH 1/6] pinctrl: Add pinctrl_gpio_as_pin()

[Andrew Jeffery]

Allow gpiochips to map the GPIO numberspace onto a pin numberspace when
the register layout for GPIO control is implemented in terms of the
pin numberspace.

This requirement sounds kind of strange, but the patch is driven by
trying to resolve a bug in the leds-pca955x driver where this mapping is
not correctly performed.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/pinctrl/core.c          | 19 +++++++++++++++++++
 include/linux/pinctrl/pinctrl.h |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index a4ac87c8b4f8..9c788f0e2844 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -738,6 +738,25 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
 	return -EINVAL;
 }
 
+int pinctrl_gpio_as_pin(struct pinctrl_dev *pctldev, unsigned int gpio)
+{
+	struct pinctrl_gpio_range *range;
+	int pin;
+
+	range = pinctrl_match_gpio_range(pctldev, gpio);
+	if (!range)
+		return -ENODEV;
+
+	mutex_lock(&pctldev->mutex);
+
+	pin = gpio_to_pin(range, gpio);
+
+	mutex_unlock(&pctldev->mutex);
+
+	return pin;
+}
+EXPORT_SYMBOL_GPL(pinctrl_find_gpio_range_from_pin);
+
 bool pinctrl_gpio_can_use_line(unsigned gpio)
 {
 	struct pinctrl_dev *pctldev;
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 70b45d28e7a9..1ceebc499cc4 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -182,6 +182,9 @@ extern struct pinctrl_dev *pinctrl_find_and_add_gpio_range(const char *devname,
 extern struct pinctrl_gpio_range *
 pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
 				 unsigned int pin);
+
+extern int pinctrl_gpio_as_pin(struct pinctrl_dev *pctldev, unsigned int gpio);
+
 extern int pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
 				const char *pin_group, const unsigned **pins,
 				unsigned *num_pins);
-- 
2.30.2

Re: [RFC PATCH 1/6] pinctrl: Add pinctrl_gpio_as_pin()

[Linus Walleij]

On Fri, Jul 23, 2021 at 9:59 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> Allow gpiochips to map the GPIO numberspace onto a pin numberspace when
> the register layout for GPIO control is implemented in terms of the
> pin numberspace.
>
> This requirement sounds kind of strange, but the patch is driven by
> trying to resolve a bug in the leds-pca955x driver where this mapping is
> not correctly performed.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

(...)

Hm  this looks a bit strange...

> +int pinctrl_gpio_as_pin(struct pinctrl_dev *pctldev, unsigned int gpio)

This is not a good name for this function. Try to come up with
a name that says exactly what the function does.

E.g. "apple pear as apple slice" isn't very helpful, the use case for
this is really hard to understand.

> +EXPORT_SYMBOL_GPL(pinctrl_find_gpio_range_from_pin);

This looks completely wrong.

Yours,
Linus Walleij

Re: [RFC PATCH 1/6] pinctrl: Add pinctrl_gpio_as_pin()

[Andrew Jeffery]

On Tue, 10 Aug 2021, at 23:04, Linus Walleij wrote:
> On Fri, Jul 23, 2021 at 9:59 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> 
> > Allow gpiochips to map the GPIO numberspace onto a pin numberspace when
> > the register layout for GPIO control is implemented in terms of the
> > pin numberspace.
> >
> > This requirement sounds kind of strange, but the patch is driven by
> > trying to resolve a bug in the leds-pca955x driver where this mapping is
> > not correctly performed.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> (...)
> 
> Hm  this looks a bit strange...
> 
> > +int pinctrl_gpio_as_pin(struct pinctrl_dev *pctldev, unsigned int gpio)
> 
> This is not a good name for this function. Try to come up with
> a name that says exactly what the function does.
> 
> E.g. "apple pear as apple slice" isn't very helpful, the use case for
> this is really hard to understand.

That's probably because I shouldn't be trying to do what I'm doing :)

I'll stop doing that (i.e. rework patch 4/6) and this will go away entirely.

> 
> > +EXPORT_SYMBOL_GPL(pinctrl_find_gpio_range_from_pin);
> 
> This looks completely wrong.

Yeah, whoops. That was an oversight while iterating on the patch.

Andrew

[RFC PATCH 2/6] pinctrl: Add hook for device-specific map parsing

[Andrew Jeffery]

The devicetree binding for the PCA955x LED/GPIO expanders was not
written with pinctrl in mind. To maintain compatibility with existing
devicetrees while implementing pinctrl support for the PCA955x devices,
add the ability to parse a custom device node layout to pinctrl.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/pinctrl/core.c          | 6 +++++-
 include/linux/pinctrl/pinctrl.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 9c788f0e2844..e4862552eb9b 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1063,7 +1063,11 @@ static struct pinctrl *create_pinctrl(struct device *dev,
 	INIT_LIST_HEAD(&p->states);
 	INIT_LIST_HEAD(&p->dt_maps);
 
-	ret = pinctrl_dt_to_map(p, pctldev);
+	if (pctldev && pctldev->desc->pctlops->dt_dev_to_map) {
+		ret = pctldev->desc->pctlops->dt_dev_to_map(pctldev, dev);
+	} else {
+		ret = pinctrl_dt_to_map(p, pctldev);
+	}
 	if (ret < 0) {
 		kfree(p);
 		return ERR_PTR(ret);
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 1ceebc499cc4..2eeec0af61fe 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -95,6 +95,7 @@ struct pinctrl_ops {
 			       unsigned *num_pins);
 	void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s,
 			  unsigned offset);
+	int (*dt_dev_to_map) (struct pinctrl_dev *pctldev, struct device *dev);
 	int (*dt_node_to_map) (struct pinctrl_dev *pctldev,
 			       struct device_node *np_config,
 			       struct pinctrl_map **map, unsigned *num_maps);
-- 
2.30.2

[RFC PATCH 3/6] leds: pca955x: Relocate chipdef-related descriptors

[Andrew Jeffery]

Reduce code-motion noise when implementing pinctrl.

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

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 7087ca4592fc..6614291b288e 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -75,44 +75,6 @@ struct pca955x_chipdef {
 	int			slv_addr_shift;	/* Number of bits to ignore */
 };
 
-static struct pca955x_chipdef pca955x_chipdefs[] = {
-	[pca9550] = {
-		.bits		= 2,
-		.slv_addr	= /* 110000x */ 0x60,
-		.slv_addr_shift	= 1,
-	},
-	[pca9551] = {
-		.bits		= 8,
-		.slv_addr	= /* 1100xxx */ 0x60,
-		.slv_addr_shift	= 3,
-	},
-	[pca9552] = {
-		.bits		= 16,
-		.slv_addr	= /* 1100xxx */ 0x60,
-		.slv_addr_shift	= 3,
-	},
-	[ibm_pca9552] = {
-		.bits		= 16,
-		.slv_addr	= /* 0110xxx */ 0x30,
-		.slv_addr_shift	= 3,
-	},
-	[pca9553] = {
-		.bits		= 4,
-		.slv_addr	= /* 110001x */ 0x62,
-		.slv_addr_shift	= 1,
-	},
-};
-
-static const struct i2c_device_id pca955x_id[] = {
-	{ "pca9550", pca9550 },
-	{ "pca9551", pca9551 },
-	{ "pca9552", pca9552 },
-	{ "ibm-pca9552", ibm_pca9552 },
-	{ "pca9553", pca9553 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, pca955x_id);
-
 struct pca955x {
 	struct mutex lock;
 	struct pca955x_led *leds;
@@ -415,6 +377,44 @@ pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
 	return pdata;
 }
 
+static struct pca955x_chipdef pca955x_chipdefs[] = {
+	[pca9550] = {
+		.bits		= 2,
+		.slv_addr	= /* 110000x */ 0x60,
+		.slv_addr_shift	= 1,
+	},
+	[pca9551] = {
+		.bits		= 8,
+		.slv_addr	= /* 1100xxx */ 0x60,
+		.slv_addr_shift	= 3,
+	},
+	[pca9552] = {
+		.bits		= 16,
+		.slv_addr	= /* 1100xxx */ 0x60,
+		.slv_addr_shift	= 3,
+	},
+	[ibm_pca9552] = {
+		.bits		= 16,
+		.slv_addr	= /* 0110xxx */ 0x30,
+		.slv_addr_shift	= 3,
+	},
+	[pca9553] = {
+		.bits		= 4,
+		.slv_addr	= /* 110001x */ 0x62,
+		.slv_addr_shift	= 1,
+	},
+};
+
+static const struct i2c_device_id pca955x_id[] = {
+	{ "pca9550", pca9550 },
+	{ "pca9551", pca9551 },
+	{ "pca9552", pca9552 },
+	{ "ibm-pca9552", ibm_pca9552 },
+	{ "pca9553", pca9553 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, pca955x_id);
+
 static const struct of_device_id of_pca955x_match[] = {
 	{ .compatible = "nxp,pca9550", .data = (void *)pca9550 },
 	{ .compatible = "nxp,pca9551", .data = (void *)pca9551 },
-- 
2.30.2

[RFC PATCH 4/6] leds: pca955x: Use pinctrl to map GPIOs to pins

[Andrew Jeffery]

The leds-pca955x driver currently assumes that the GPIO numberspace and
the pin numberspace are the same. This quickly falls apart with a
devicetree binding such as the following:

	pca0: pca9552@61 {
		compatible = "nxp,pca9552";
		reg = <0x61>;
		#address-cells = <1>;
		#size-cells = <0>;

		gpio-controller;
		#gpio-cells = <2>;

		led@0 {
			reg = <0>;
			type = <PCA955X_TYPE_LED>;
		};

		gpio@1 {
			reg = <1>;
			type = <PCA955X_TYPE_GPIO>;
		};
	};

This results in a gpiochip with 1 gpio at offset 0 that should control a
pin at offset 1 on the pca9552. Instead the accessing GPIO 0 of the
gpiochip attempts to drive pin 0.

Making the GPIO controller cover the entire chip opens up conflicts
between the LED and GPIO controllers for the pins. What's needed is a
mapping between the GPIO numberspace and the pin numberspace, with
mutually exclusive access to each pin for the LED and GPIO controllers.
Broadly, this is the responsibility of the pinctrl and pinmux
subsystems.

Rework the driver to implement pinctrl and pinmux (despite the lack of
actual mux hardware), and back the gpiochip and LED controller on to the
resulting pin controller.

This effort implements a custom devicetree device mapping callback for
pinctrl to parse maps from the existing devicetree binding for the
PCA955x devices. Subnodes that set `type = <PCA955X_TYPE_LED>`
automatically have a mux group map generated for the default state. This
causes probe() to claim the appropriate LED pins for the LED controller
from the pinctroller. If subnodes of `type = <PCA955X_TYPE_GPIO>` are
specified then the gpiochip implements the correct behaviour for what
the binding intended - an appropriate GPIO pin mapping is generated and
applied to the pincontroller for the specified number of pins.

For future PCA955x devicetree nodes, devices that require a mix of LED
and GPIO pins can specify the number and numberspace mappings with the
`ngpios` and `gpio-ranges` properties from generic GPIO binding. Thus
the only subnodes that need to be specified are those for the LEDs.

The result is that we now have an accurate mapping between GPIO and pin
numberspaces with mutually exclusive access to the pins. In addition,
any pin conflict issues can be resolved through the usual debugfs
properties exposed by the pinctrl subsystem.

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

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 6614291b288e..9e08dbb05bc5 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -45,6 +45,8 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -73,6 +75,7 @@ struct pca955x_chipdef {
 	int			bits;
 	u8			slv_addr;	/* 7-bit slave address mask */
 	int			slv_addr_shift;	/* Number of bits to ignore */
+	struct pinctrl_desc	*pinctrl;
 };
 
 struct pca955x {
@@ -80,6 +83,8 @@ struct pca955x {
 	struct pca955x_led *leds;
 	struct pca955x_chipdef	*chipdef;
 	struct i2c_client	*client;
+	struct pinctrl_desc	*pctldesc;
+	struct pinctrl_dev	*pctldev;
 #ifdef CONFIG_LEDS_PCA955X_GPIO
 	struct gpio_chip gpio;
 #endif
@@ -253,6 +258,15 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 	return ret;
 }
 
+static int
+pca955x_set_pin_value(struct pca955x *pca955x, unsigned int pin, int val)
+{
+	struct led_classdev *cdev = &pca955x->leds[pin].led_cdev;
+	int state = val ? PCA955X_GPIO_HIGH : PCA955X_GPIO_LOW;
+
+	return pca955x_led_set(cdev, state);
+}
+
 #ifdef CONFIG_LEDS_PCA955X_GPIO
 /*
  * Read the INPUT register, which contains the state of LEDs.
@@ -271,64 +285,348 @@ 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)
+static void
+pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset, int val)
 {
 	struct pca955x *pca955x = gpiochip_get_data(gc);
-	struct pca955x_led *led = &pca955x->leds[offset];
+	int pin;
 
-	if (led->type == PCA955X_TYPE_GPIO)
-		return 0;
+	pin = pinctrl_gpio_as_pin(pca955x->pctldev, gc->base + offset);
+	if (pin < 0) {
+		dev_err(gc->parent, "Failed to look up pin for GPIO %d\n",
+			offset);
+		return;
+	}
 
-	return -EBUSY;
-}
-
-static int pca955x_set_value(struct gpio_chip *gc, unsigned int offset,
-			     int val)
-{
-	struct pca955x *pca955x = gpiochip_get_data(gc);
-	struct pca955x_led *led = &pca955x->leds[offset];
-
-	if (val)
-		return pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);
-
-	return pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);
-}
-
-static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
-				   int val)
-{
-	pca955x_set_value(gc, offset, val);
+	pca955x_set_pin_value(pca955x, pin, val);
 }
 
 static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
 {
 	struct pca955x *pca955x = gpiochip_get_data(gc);
-	struct pca955x_led *led = &pca955x->leds[offset];
 	u8 reg = 0;
+	int pin;
+
+	pin = pinctrl_gpio_as_pin(pca955x->pctldev, gc->base + offset);
+	if (pin < 0)
+		return pin;
 
 	/* There is nothing we can do about errors */
-	pca955x_read_input(pca955x->client, led->led_num / 8, &reg);
+	pca955x_read_input(pca955x->client, pin / 8, &reg);
 
-	return !!(reg & (1 << (led->led_num % 8)));
+	return !!(reg & (1 << (pin % 8)));
 }
 
-static int pca955x_gpio_direction_input(struct gpio_chip *gc,
-					unsigned int offset)
+static int
+pca955x_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
 {
 	struct pca955x *pca955x = gpiochip_get_data(gc);
-	struct pca955x_led *led = &pca955x->leds[offset];
+	struct led_classdev *cdev;
+	int pin;
 
-	/* To use as input ensure pin is not driven. */
-	return pca955x_led_set(&led->led_cdev, PCA955X_GPIO_INPUT);
+	pin = pinctrl_gpio_as_pin(pca955x->pctldev, gc->base + offset);
+	if (pin < 0)
+		return pin;
+
+	cdev = &pca955x->leds[pin].led_cdev;
+
+	return pca955x_led_set(cdev, PCA955X_GPIO_INPUT);
 }
 
-static int pca955x_gpio_direction_output(struct gpio_chip *gc,
-					 unsigned int offset, int val)
+static int
+pca955x_gpio_direction_output(struct gpio_chip *gc, unsigned int offset,
+			      int val)
 {
-	return pca955x_set_value(gc, offset, val);
+	struct pca955x *pca955x = gpiochip_get_data(gc);
+	int pin;
+
+	pin = pinctrl_gpio_as_pin(pca955x->pctldev, gc->base + offset);
+	if (pin < 0)
+		return pin;
+
+	return pca955x_set_pin_value(pca955x, pin, val);
+}
+
+static int
+pca955x_gpio_request_pin(struct gpio_chip *gc, unsigned int offset)
+{
+	return pinctrl_gpio_request(gc->base + offset);
+}
+
+static void
+pca955x_gpio_free_pin(struct gpio_chip *gc, unsigned int offset)
+{
+	int rc;
+
+	/* Go high-impedance */
+	rc = pca955x_gpio_direction_input(gc, offset);
+	if (rc < 0)
+		dev_err(gc->parent, "Failed to set direction for GPIO %u:%u\n", gc->base, offset);
+
+	pinctrl_gpio_free(gc->base + offset);
 }
 #endif /* CONFIG_LEDS_PCA955X_GPIO */
 
+static const struct pinctrl_pin_desc pca9552_pinctrl_pins[] = {
+	PINCTRL_PIN(0, "LED0"),
+	PINCTRL_PIN(1, "LED1"),
+	PINCTRL_PIN(2, "LED2"),
+	PINCTRL_PIN(3, "LED3"),
+	PINCTRL_PIN(4, "LED4"),
+	PINCTRL_PIN(5, "LED5"),
+	PINCTRL_PIN(6, "LED6"),
+	PINCTRL_PIN(7, "LED7"),
+	PINCTRL_PIN(8, "LED8"),
+	PINCTRL_PIN(9, "LED9"),
+	PINCTRL_PIN(10, "LED10"),
+	PINCTRL_PIN(11, "LED11"),
+	PINCTRL_PIN(12, "LED12"),
+	PINCTRL_PIN(13, "LED13"),
+	PINCTRL_PIN(14, "LED14"),
+	PINCTRL_PIN(15, "LED15"),
+};
+
+static const char * const pca9552_groups[] = {
+	"LED0", "LED1", "LED2",  "LED3",  "LED4",  "LED5",  "LED6",  "LED7",
+	"LED8", "LED9", "LED10", "LED11", "LED12", "LED13", "LED14", "LED15",
+};
+
+static const unsigned int pca9552_group_pins[] = {
+	0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
+};
+
+static const char *pca955x_pinctrl_dev_name(struct pca955x *pca955x)
+{
+	/* The controller is its only consumer via leds and gpios */
+	return dev_name(&pca955x->client->dev);
+}
+
+static int pca955x_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct pca955x *pca955x = pinctrl_dev_get_drvdata(pctldev);
+
+	/* We have as many groups as we have LEDs */
+	return pca955x->chipdef->bits;
+}
+
+static const char *
+pca955x_pinctrl_get_group_name(struct pinctrl_dev *pctldev, unsigned int selector)
+{
+	struct pca955x *pca955x = pinctrl_dev_get_drvdata(pctldev);
+
+	if (unlikely(selector > pca955x->chipdef->bits)) {
+		dev_err(&pca955x->client->dev,
+			"Group selector (%u) exceeds groups count (%u)\n",
+			selector, pca955x->chipdef->bits);
+		return NULL;
+	}
+
+	if (unlikely(selector > ARRAY_SIZE(pca9552_groups))) {
+		dev_err(&pca955x->client->dev,
+			"Group selector (%u) exceeds the supported group count (%u)\n",
+			selector, ARRAY_SIZE(pca9552_groups));
+		return NULL;
+	}
+
+	return pca9552_groups[selector];
+}
+
+static int
+pca955x_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, unsigned int  selector,
+			       const unsigned int **pins, unsigned int *num_pins)
+{
+	struct pca955x *pca955x = pinctrl_dev_get_drvdata(pctldev);
+
+	if (unlikely(selector > pca955x->chipdef->bits)) {
+		dev_err(&pca955x->client->dev,
+			"Group selector (%u) exceeds groups count (%u)\n",
+			selector, pca955x->chipdef->bits);
+		return -EINVAL;
+	}
+
+	if (unlikely(selector > ARRAY_SIZE(pca9552_group_pins))) {
+		dev_err(&pca955x->client->dev,
+			"Group selector (%u) exceeds the supported group count (%u)\n",
+			selector, ARRAY_SIZE(pca9552_groups));
+		return -EINVAL;
+	}
+
+	*pins = &pca9552_group_pins[selector];
+	*num_pins = 1;
+
+	return 0;
+}
+
+static int pca955x_pinmux_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return 1;
+}
+
+static const char *
+pca955x_pinmux_get_function_name(struct pinctrl_dev *pctldev, unsigned int selector)
+{
+	struct pca955x *pca955x = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector != 0)
+		dev_err(&pca955x->client->dev, "Only the 'LED' function is supported");
+
+	return "LED";
+}
+
+static int pca955x_pinmux_get_function_groups(struct pinctrl_dev *pctldev,
+					      unsigned int selector,
+					      const char * const **groups,
+					      unsigned int *num_groups)
+{
+	struct pca955x *pca955x = pinctrl_dev_get_drvdata(pctldev);
+
+	if (unlikely(pca955x->chipdef->bits > ARRAY_SIZE(pca9552_groups))) {
+		dev_warn(&pca955x->client->dev,
+			 "Unsupported PCA955x configuration, LED count exceed LED group count\n");
+		return -EINVAL;
+	}
+
+	if (selector != 0)
+		dev_err(&pca955x->client->dev, "Only the 'LED' function is supported");
+
+	*groups = &pca9552_groups[0];
+	*num_groups = pca955x->chipdef->bits;
+
+	return 0;
+}
+
+static int
+pca955x_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
+		       unsigned int group_selector)
+{
+	/* There's no actual mux as such. */
+	return 0;
+}
+
+/*
+ * Implement pinctrl map parsing in a way that's backwards compatible with the
+ * existing devicetree binding.
+ */
+static int
+pca955x_dt_dev_to_map(struct pinctrl_dev *pctldev, struct device *dev)
+{
+	struct pca955x *pca955x = pinctrl_dev_get_drvdata(pctldev);
+	struct pinctrl_desc *pctldesc = pca955x->pctldesc;
+	struct fwnode_handle *child;
+	struct pinctrl_map *maps;
+	unsigned int i = 0;
+	int rc;
+
+	if (WARN_ON(dev != &pca955x->client->dev))
+		return -EINVAL;
+
+	/* Only 1 possible mux config per LED, no further allocations needed */
+	maps = devm_kmalloc_array(dev, pca955x->chipdef->bits, sizeof(*maps), GFP_KERNEL);
+	if (!maps)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		struct pinctrl_map *m;
+		u32 type;
+		u32 reg;
+
+		/* Default to PCA955X_TYPE_LED as we do in pca955x_get_pdata */
+		rc = fwnode_property_read_u32(child, "type", &type);
+		if (rc == -EINVAL)
+			type = PCA955X_TYPE_LED;
+		else if (rc < 0)
+			goto cleanup_maps;
+
+		if (type != PCA955X_TYPE_LED)
+			continue;
+
+		rc = fwnode_property_read_u32(child, "reg", &reg);
+		if (rc < 0)
+			goto cleanup_maps;
+
+		if (i >= pca955x->chipdef->bits) {
+			dev_err(dev,
+				"The number of pin configuration nodes exceeds the number of available pins (%u)\n",
+				pca955x->chipdef->bits);
+			break;
+		}
+
+		m = &maps[i];
+
+		m->dev_name = pctldesc->name;
+		m->name = PINCTRL_STATE_DEFAULT;
+		m->type = PIN_MAP_TYPE_MUX_GROUP;
+		m->ctrl_dev_name = pctldesc->name;
+		m->data.mux.function = "LED";
+		m->data.mux.group = devm_kasprintf(dev, GFP_KERNEL, "LED%d", reg);
+		if (!m->data.mux.group) {
+			rc = -ENOMEM;
+			goto cleanup_maps;
+		}
+
+		i++;
+	}
+
+	/* Trim the map allocation as required */
+	if (i < pca955x->chipdef->bits) {
+		struct pinctrl_map *trimmed;
+
+		trimmed = devm_krealloc(dev, maps, i * sizeof(*maps), GFP_KERNEL);
+		if (trimmed)
+			maps = trimmed;
+		else
+			dev_warn(dev, "Failed to trim pinctrl maps\n");
+	}
+
+	pinctrl_register_mappings(maps, i);
+
+	return 0;
+
+cleanup_maps:
+	while (i--)
+		devm_kfree(dev, maps[i].data.mux.group);
+
+	devm_kfree(dev, maps);
+
+	return rc;
+}
+
+static void
+pca955x_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map,
+		    unsigned int num_maps)
+{
+	struct pca955x *pca955x = pinctrl_dev_get_drvdata(pctldev);
+	struct device *dev = &pca955x->client->dev;
+	struct pinctrl_map *iter = map;
+
+	if (!iter)
+		return;
+
+	while (num_maps) {
+		devm_kfree(dev, iter->data.mux.group);
+		iter++;
+		num_maps--;
+	}
+
+	devm_kfree(dev, map);
+}
+
+static const struct pinctrl_ops pca955x_pinctrl_ops = {
+	.get_groups_count	= pca955x_pinctrl_get_groups_count,
+	.get_group_name		= pca955x_pinctrl_get_group_name,
+	.get_group_pins		= pca955x_pinctrl_get_group_pins,
+	.dt_dev_to_map		= pca955x_dt_dev_to_map,
+	.dt_free_map		= pca955x_dt_free_map,
+};
+
+static const struct pinmux_ops pca955x_pinmux_ops = {
+	.get_functions_count	= pca955x_pinmux_get_functions_count,
+	.get_function_name	= pca955x_pinmux_get_function_name,
+	.get_function_groups	= pca955x_pinmux_get_function_groups,
+	.set_mux		= pca955x_pinmux_set_mux,
+	.strict = true,
+};
+
 static struct pca955x_platform_data *
 pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
 {
@@ -434,7 +732,12 @@ static int pca955x_probe(struct i2c_client *client,
 	struct i2c_adapter *adapter;
 	int i, err;
 	struct pca955x_platform_data *pdata;
-	int ngpios = 0;
+	u32 ngpios = 0;
+	struct fwnode_handle *fwnode;
+
+	fwnode = dev_fwnode(&client->dev);
+	if (!fwnode)
+		return -ENODATA;
 
 	chip = &pca955x_chipdefs[id->driver_data];
 	adapter = client->adapter;
@@ -476,12 +779,35 @@ static int pca955x_probe(struct i2c_client *client,
 	if (!pca955x->leds)
 		return -ENOMEM;
 
+	pca955x->pctldesc = devm_kzalloc(&client->dev,
+			sizeof(*pca955x->pctldesc), GFP_KERNEL);
+	if (!pca955x->pctldesc)
+		return -ENOMEM;
+
 	i2c_set_clientdata(client, pca955x);
 
 	mutex_init(&pca955x->lock);
 	pca955x->client = client;
 	pca955x->chipdef = chip;
 
+	/* pinctrl */
+	pca955x->pctldesc->name = pca955x_pinctrl_dev_name(pca955x);
+	if (!pca955x->pctldesc->name)
+		return -ENOMEM;
+
+	pca955x->pctldesc->pins = &pca9552_pinctrl_pins[0];
+	pca955x->pctldesc->npins = chip->bits;
+	pca955x->pctldesc->pctlops = &pca955x_pinctrl_ops;
+	pca955x->pctldesc->pmxops = &pca955x_pinmux_ops;
+	pca955x->pctldesc->owner = THIS_MODULE;
+
+	err = devm_pinctrl_register_and_init(&client->dev, pca955x->pctldesc,
+					     pca955x, &pca955x->pctldev);
+	if (err) {
+		dev_err(&client->dev, "Failed to register pincontroller: %d\n", err);
+		return err;
+	}
+
 	for (i = 0; i < chip->bits; i++) {
 		pca955x_led = &pca955x->leds[i];
 		pca955x_led->led_num = i;
@@ -527,6 +853,12 @@ static int pca955x_probe(struct i2c_client *client,
 		}
 	}
 
+	err = pinctrl_enable(pca955x->pctldev);
+	if (err) {
+		dev_err(&client->dev, "Failed to enable pincontroller: %d\n", err);
+		return err;
+	}
+
 	/* PWM0 is used for half brightness or 50% duty cycle */
 	err = pca955x_write_pwm(client, 0, 255 - LED_HALF);
 	if (err)
@@ -546,30 +878,64 @@ 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;
+	/* Always register the gpiochip, no-longer conditional on 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.free = pca955x_gpio_free_pin;
+	pca955x->gpio.can_sleep = 1;
+	pca955x->gpio.base = -1;
+	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");
+	if (!ngpios) {
+		err = fwnode_property_read_u32(fwnode, "ngpios", &ngpios);
+		if (err < 0 && err != -EINVAL)
 			return err;
+	}
+
+	if (!ngpios)
+		ngpios = chip->bits;
+
+
+	pca955x->gpio.ngpio = ngpios;
+
+	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;
+	}
+
+	if (!device_property_present(&client->dev, "gpio-ranges")) {
+		struct fwnode_handle *child;
+		unsigned int i = 0;
+
+		device_for_each_child_node(&client->dev, child) {
+			u32 type;
+			u32 reg;
+
+			err = fwnode_property_read_u32(child, "reg", &reg);
+			if (err < 0)
+				return err;
+
+			err = fwnode_property_read_u32(child, "type", &type);
+			if (err < 0)
+				continue;
+
+			/* XXX: Do something less wasteful? */
+			err = gpiochip_add_pin_range(&pca955x->gpio,
+					pca955x_pinctrl_dev_name(pca955x),
+					i, reg, 1);
+			if (err)
+				return err;
+
+			i++;
 		}
-		dev_info(&client->dev, "gpios %i...%i\n",
-			 pca955x->gpio.base, pca955x->gpio.base +
-			 pca955x->gpio.ngpio - 1);
 	}
 #endif
 
-- 
2.30.2

Re: [RFC PATCH 4/6] leds: pca955x: Use pinctrl to map GPIOs to pins

[Linus Walleij]

On Fri, Jul 23, 2021 at 9:59 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> The leds-pca955x driver currently assumes that the GPIO numberspace and
> the pin numberspace are the same. This quickly falls apart with a
> devicetree binding such as the following:
(...)

Honestly I do not understand this patch. It seems to implement a pin
controller and using it in nonstandard ways.

If something implements the pin controller driver API it should be
used as such IMO, externally. This seems to be using it do relay
calls to itself which seems complicated, just invent something
locally in the driver in that case? No need to use pin control?

Can you explain why this LED driver needs to implement a pin
controller?

Yours,
Linus Walleij

Re: [RFC PATCH 4/6] leds: pca955x: Use pinctrl to map GPIOs to pins

[Andrew Jeffery]

On Tue, 10 Aug 2021, at 23:24, Linus Walleij wrote:
> On Fri, Jul 23, 2021 at 9:59 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> 
> > The leds-pca955x driver currently assumes that the GPIO numberspace and
> > the pin numberspace are the same. This quickly falls apart with a
> > devicetree binding such as the following:
> (...)
> 
> Honestly I do not understand this patch. It seems to implement a pin
> controller and using it in nonstandard ways.

Yeah, it's a bit abusive, hence RFC :)

> 
> If something implements the pin controller driver API it should be
> used as such IMO, externally. This seems to be using it do relay
> calls to itself which seems complicated, just invent something
> locally in the driver in that case? No need to use pin control?

Right. After discussions with Andy I'm going to rework the approach to 
GPIOs which will remove a lot of complexity.

The thought was to try to maintain the intent of the devicetree binding 
and use existing APIs, but all-in-all it's ended up twisting things up 
in knots a fair bit. We discard a lot of it by making the gpiochip 
always cover all pins and track use directly in the driver.

> 
> Can you explain why this LED driver needs to implement a pin
> controller?

The short answer is it doesn't as it has none of the associated 
hardware.

I'll cook up something simpler with the aim to avoid non-standard (or 
any) pinctrl.

Andrew

[RFC PATCH 5/6] ARM: dts: rainier: Add presence-detect and fault indictor GPIO expander

[Andrew Jeffery]

Expose the ability for the hardware to indicate that it is present, and
if it is present, for the BMC to mark it as faulty.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 76 ++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
index 941c0489479a..84651d090965 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -1685,6 +1685,82 @@ eeprom@50 {
 		compatible = "atmel,24c64";
 		reg = <0x50>;
 	};
+
+	dbp0: led-controller@60 {
+		compatible = "nxp,pca9552";
+		reg = <0x60>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+		ngpios = <8>;
+		gpio-ranges = <&dbp0 0 8 8>;
+
+		led@0 {
+			label = "led-fault-0";
+			reg = <0>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@1 {
+			label = "led-fault-1";
+			reg = <1>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@2 {
+			label = "led-fault-2";
+			reg = <2>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@3 {
+			label = "led-fault-3";
+			reg = <3>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@4 {
+			label = "led-fault-4";
+			reg = <4>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@5 {
+			label = "led-fault-5";
+			reg = <5>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@6 {
+			label = "led-fault-6";
+			reg = <6>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@7 {
+			label = "led-fault-7";
+			reg = <7>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+	};
 };
 
 &i2c14 {
-- 
2.30.2

[RFC PATCH 6/6] pinctrl: Check get_group_pins callback on init

[Andrew Jeffery]

Do configurations exist where this doesn't make sense? I lost some time
to debugging the fact that I was missing the callback :(

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

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index e4862552eb9b..4c436a419856 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1994,7 +1994,8 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev)
 
 	if (!ops ||
 	    !ops->get_groups_count ||
-	    !ops->get_group_name)
+	    !ops->get_group_name ||
+	    !ops->get_group_pins)
 		return -EINVAL;
 
 	return 0;
-- 
2.30.2

Re: [RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andrew Jeffery]

On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:
> 
> 
> On Friday, July 23, 2021, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Hello,
> > 
> > This series does a bunch of crimes, so it's an RFC. I'm cross-posting to the
> > pinctrl/GPIO and LEDs lists because the PCA955x devices impact all of them. What
> > needs fixing is the leds-pca955x driver's failure to map the GPIO numberspace to
> > the pin numberspace of the PCA955x devices. The series solves that by
> > implementing pinctrl and pinmux in the leds-pca955x driver.
> > 
> > Things I'm unsure about:
> > 
> > 1. Patch 1: The pinctrl_gpio_as_pin() API feels a bit dirty, not sure what
> >    others thoughts are on that (Linus?).
> > 
> > 2. Patch 2: I've added a new callback to hook the entirety of the pinctrl map
> >    parsing rather than supplying a subnode-specific callback. This was necessary
> >    to handle the PCA955x devicetree binding in a backwards compatible way.
> > 
> > 3. Patch 4: The PCA955x devices don't actually have any pinmux hardware, but the
> >    properties of the pinctrl/pinmux subsystems in the kernel map nicely onto the
> >    problem we have. But it's quite a bit of code...
> > 
> > 4. Patch 6: I also lost a bunch of time to overlooking the get_group_pins()
> >    callback for pinctrl, and it seems odd to me that it isn't required.
> > 
> > Please review!
> 
> 
> Sounds like a hack.

Yes, possibly. Feedback like this is why I sent the series as an RFC.

> I was briefly looking into patches 1-4 and suddenly 
> realized that the fix can be similar as in PCA9685 (PWM), I.e. we 
> always have chips for the entire pin space and one may map them 
> accordingly, requested in one realm (LED) in the other (GPIO) 
> automatically is BUSY. Or I missed the point?

No, you haven't missed the point. I will look at the PCA9685 driver.

That said, my goal was to implement the behaviour intended by the 
existing binding (i.e. fix a bug). However, userspace would never have 
got the results it expected with the existing driver implementation, so 
I guess you could argue that no such (useful) userspace exists. Given 
that, we could adopt the strategy of always defining a gpiochip 
covering the whole pin space, and parts of the devicetree binding just 
become redundant.

Andrew

Re: [RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andy Shevchenko]

On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:
> > On Friday, July 23, 2021, Andrew Jeffery <andrew@aj.id.au> wrote:

> > > This series does a bunch of crimes, so it's an RFC. I'm cross-posting to the
> > > pinctrl/GPIO and LEDs lists because the PCA955x devices impact all of them. What
> > > needs fixing is the leds-pca955x driver's failure to map the GPIO numberspace to
> > > the pin numberspace of the PCA955x devices. The series solves that by
> > > implementing pinctrl and pinmux in the leds-pca955x driver.
> > >
> > > Things I'm unsure about:
> > >
> > > 1. Patch 1: The pinctrl_gpio_as_pin() API feels a bit dirty, not sure what
> > >    others thoughts are on that (Linus?).
> > >
> > > 2. Patch 2: I've added a new callback to hook the entirety of the pinctrl map
> > >    parsing rather than supplying a subnode-specific callback. This was necessary
> > >    to handle the PCA955x devicetree binding in a backwards compatible way.
> > >
> > > 3. Patch 4: The PCA955x devices don't actually have any pinmux hardware, but the
> > >    properties of the pinctrl/pinmux subsystems in the kernel map nicely onto the
> > >    problem we have. But it's quite a bit of code...
> > >
> > > 4. Patch 6: I also lost a bunch of time to overlooking the get_group_pins()
> > >    callback for pinctrl, and it seems odd to me that it isn't required.
> > >
> > > Please review!
> >
> >
> > Sounds like a hack.
>
> Yes, possibly. Feedback like this is why I sent the series as an RFC.
>
> > I was briefly looking into patches 1-4 and suddenly
> > realized that the fix can be similar as in PCA9685 (PWM), I.e. we
> > always have chips for the entire pin space and one may map them
> > accordingly, requested in one realm (LED) in the other (GPIO)
> > automatically is BUSY. Or I missed the point?
>
> No, you haven't missed the point. I will look at the PCA9685 driver.
>
> That said, my goal was to implement the behaviour intended by the
> existing binding (i.e. fix a bug).

Okay, so it implies that this used to work at some point. What has
changed from that point? Why can't we simply fix the culprit commit?

> However, userspace would never have
> got the results it expected with the existing driver implementation, so
> I guess you could argue that no such (useful) userspace exists. Given
> that, we could adopt the strategy of always defining a gpiochip
> covering the whole pin space, and parts of the devicetree binding just
> become redundant.

I'm lost now. GPIO has its own userspace ABI, how does it work right
now in application to this chip?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andrew Jeffery]

On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:
> On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:
> >
> > > I was briefly looking into patches 1-4 and suddenly
> > > realized that the fix can be similar as in PCA9685 (PWM), I.e. we
> > > always have chips for the entire pin space and one may map them
> > > accordingly, requested in one realm (LED) in the other (GPIO)
> > > automatically is BUSY. Or I missed the point?
> >
> > No, you haven't missed the point. I will look at the PCA9685 driver.
> >
> > That said, my goal was to implement the behaviour intended by the
> > existing binding (i.e. fix a bug).
> 
> Okay, so it implies that this used to work at some point. 

I don't think this is true. It only "works" if the lines specified as 
GPIO in the devicetree are contiguous from line 0. That way the pin and 
GPIO number spaces align. I suspect that's all that's been tested up 
until this point.

We now have a board with a PCA9552 where the first 8 pins are LED and 
the last 8 pins are GPIO, and if you specify this in the devicetree 
according to the binding you hit the failure to map between the two 
number spaces.

> What has
> changed from that point? Why can't we simply fix the culprit commit?

As such nothing has changed, I think it's always been broken, just we 
haven't had hardware configurations that demonstrated the failure.

> 
> > However, userspace would never have
> > got the results it expected with the existing driver implementation, so
> > I guess you could argue that no such (useful) userspace exists. Given
> > that, we could adopt the strategy of always defining a gpiochip
> > covering the whole pin space, and parts of the devicetree binding just
> > become redundant.
> 
> I'm lost now. GPIO has its own userspace ABI, how does it work right
> now in application to this chip?

As above, it "works" if the GPIOs specified in the devicetree are 
contiguous from line 0. It's broken if they're not.

Andrew

Re: [RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andy Shevchenko]

On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:
> > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:
> > >
> > > > I was briefly looking into patches 1-4 and suddenly
> > > > realized that the fix can be similar as in PCA9685 (PWM), I.e. we
> > > > always have chips for the entire pin space and one may map them
> > > > accordingly, requested in one realm (LED) in the other (GPIO)
> > > > automatically is BUSY. Or I missed the point?
> > >
> > > No, you haven't missed the point. I will look at the PCA9685 driver.
> > >
> > > That said, my goal was to implement the behaviour intended by the
> > > existing binding (i.e. fix a bug).
> >
> > Okay, so it implies that this used to work at some point.
>
> I don't think this is true. It only "works" if the lines specified as
> GPIO in the devicetree are contiguous from line 0. That way the pin and
> GPIO number spaces align. I suspect that's all that's been tested up
> until this point.
>
> We now have a board with a PCA9552 where the first 8 pins are LED and
> the last 8 pins are GPIO, and if you specify this in the devicetree
> according to the binding you hit the failure to map between the two
> number spaces.
>
> > What has
> > changed from that point? Why can't we simply fix the culprit commit?
>
> As such nothing has changed, I think it's always been broken, just we
> haven't had hardware configurations that demonstrated the failure.
>
> >
> > > However, userspace would never have
> > > got the results it expected with the existing driver implementation, so
> > > I guess you could argue that no such (useful) userspace exists. Given
> > > that, we could adopt the strategy of always defining a gpiochip
> > > covering the whole pin space, and parts of the devicetree binding just
> > > become redundant.
> >
> > I'm lost now. GPIO has its own userspace ABI, how does it work right
> > now in application to this chip?
>
> As above, it "works" if the GPIOs specified in the devicetree are
> contiguous from line 0. It's broken if they're not.

So, "it never works" means there is no bug. Now, what we need is to
keep the same enumeration scheme, but if you wish to be used half/half
(or any other ratio), the driver should do like the above mentioned
PWM, i.e. register entire space and depending on the requestor either
proceed with a line or mark it as BUSY.

Ideally, looking into what the chip can do, this should be indeed
converted to some like pin control + PWM + LED + GPIO drivers. Then
the function in pin mux configuration can show what exactly is enabled
on the certain line(s).


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andrew Jeffery]

On Thu, 29 Jul 2021, at 17:10, Andy Shevchenko wrote:
> On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:
> > > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > However, userspace would never have
> > > > got the results it expected with the existing driver implementation, so
> > > > I guess you could argue that no such (useful) userspace exists. Given
> > > > that, we could adopt the strategy of always defining a gpiochip
> > > > covering the whole pin space, and parts of the devicetree binding just
> > > > become redundant.
> > >
> > > I'm lost now. GPIO has its own userspace ABI, how does it work right
> > > now in application to this chip?
> >
> > As above, it "works" if the GPIOs specified in the devicetree are
> > contiguous from line 0. It's broken if they're not.
> 
> So, "it never works" means there is no bug. Now, what we need is to
> keep the same enumeration scheme, but if you wish to be used half/half
> (or any other ratio), the driver should do like the above mentioned
> PWM, i.e. register entire space and depending on the requestor either
> proceed with a line or mark it as BUSY.
> 
> Ideally, looking into what the chip can do, this should be indeed
> converted to some like pin control + PWM + LED + GPIO drivers. Then
> the function in pin mux configuration can show what exactly is enabled
> on the certain line(s).

So just to clarify, you want both solutions here?

1. A gpiochip that covers the entire pin space
2. A pinmux implementation that manages pin allocation to the different drivers

In that case we can largely leave this series as is? We only need to 
adjust how we configure the gpiochip by dropping the pin-mapping 
implementation?

I don't have a need to implement a PWM driver for it right now but that 
would make sense to do at some point.

Andrew

Re: [RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andy Shevchenko]

On Tue, Aug 3, 2021 at 7:07 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 29 Jul 2021, at 17:10, Andy Shevchenko wrote:
> > On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:
> > > > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > > However, userspace would never have
> > > > > got the results it expected with the existing driver implementation, so
> > > > > I guess you could argue that no such (useful) userspace exists. Given
> > > > > that, we could adopt the strategy of always defining a gpiochip
> > > > > covering the whole pin space, and parts of the devicetree binding just
> > > > > become redundant.
> > > >
> > > > I'm lost now. GPIO has its own userspace ABI, how does it work right
> > > > now in application to this chip?
> > >
> > > As above, it "works" if the GPIOs specified in the devicetree are
> > > contiguous from line 0. It's broken if they're not.
> >
> > So, "it never works" means there is no bug. Now, what we need is to
> > keep the same enumeration scheme, but if you wish to be used half/half
> > (or any other ratio), the driver should do like the above mentioned
> > PWM, i.e. register entire space and depending on the requestor either
> > proceed with a line or mark it as BUSY.
> >
> > Ideally, looking into what the chip can do, this should be indeed
> > converted to some like pin control + PWM + LED + GPIO drivers. Then
> > the function in pin mux configuration can show what exactly is enabled
> > on the certain line(s).
>
> So just to clarify, you want both solutions here?
>
> 1. A gpiochip that covers the entire pin space
> 2. A pinmux implementation that manages pin allocation to the different drivers
>
> In that case we can largely leave this series as is? We only need to
> adjust how we configure the gpiochip by dropping the pin-mapping
> implementation?

Nope. It's far from what I think of. Re-reading again your cover
letter it points out that pin mux per se does not exist in the
hardware. In this case things become a bit too complicated, but we
still may manage to handle them. Before I was thinking about this
hierarchy

1. pinmux driver (which is actually the main driver here)
2. LED driver (using regmap API)
3. GPIO driver (via gpio-regmap)
4. PWM driver.

Now what we need here is some kind of "virtual" pinmux. Do I
understand correctly?

To be clear: I do not like putting everything into one driver when the
logical parts may be separated.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/6] leds: Fix pca955x GPIO pin mappings

[Andrew Jeffery]

On Tue, 3 Aug 2021, at 20:03, Andy Shevchenko wrote:
> On Tue, Aug 3, 2021 at 7:07 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > On Thu, 29 Jul 2021, at 17:10, Andy Shevchenko wrote:
> > > On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:
> > > > > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > > > However, userspace would never have
> > > > > > got the results it expected with the existing driver implementation, so
> > > > > > I guess you could argue that no such (useful) userspace exists. Given
> > > > > > that, we could adopt the strategy of always defining a gpiochip
> > > > > > covering the whole pin space, and parts of the devicetree binding just
> > > > > > become redundant.
> > > > >
> > > > > I'm lost now. GPIO has its own userspace ABI, how does it work right
> > > > > now in application to this chip?
> > > >
> > > > As above, it "works" if the GPIOs specified in the devicetree are
> > > > contiguous from line 0. It's broken if they're not.
> > >
> > > So, "it never works" means there is no bug. Now, what we need is to
> > > keep the same enumeration scheme, but if you wish to be used half/half
> > > (or any other ratio), the driver should do like the above mentioned
> > > PWM, i.e. register entire space and depending on the requestor either
> > > proceed with a line or mark it as BUSY.
> > >
> > > Ideally, looking into what the chip can do, this should be indeed
> > > converted to some like pin control + PWM + LED + GPIO drivers. Then
> > > the function in pin mux configuration can show what exactly is enabled
> > > on the certain line(s).
> >
> > So just to clarify, you want both solutions here?
> >
> > 1. A gpiochip that covers the entire pin space
> > 2. A pinmux implementation that manages pin allocation to the different drivers
> >
> > In that case we can largely leave this series as is? We only need to
> > adjust how we configure the gpiochip by dropping the pin-mapping
> > implementation?
> 
> Nope. It's far from what I think of. Re-reading again your cover
> letter it points out that pin mux per se does not exist in the
> hardware. In this case things become a bit too complicated, but we
> still may manage to handle them. Before I was thinking about this
> hierarchy
> 
> 1. pinmux driver (which is actually the main driver here)
> 2. LED driver (using regmap API)
> 3. GPIO driver (via gpio-regmap)
> 4. PWM driver.

Okay - I need to look at gpio-regmap, this wasn't something I was aware 
of.

> 
> Now what we need here is some kind of "virtual" pinmux. Do I
> understand correctly?

Possibly. My thoughts went to pinctrl as part of its job is mutual 
exclusion *and* pin mapping, plus we get the nice debugfs interface 
with the pin allocation details. The need for pin mapping came from 
trying to stay true to the intent of the existing devicetree binding. 
If we throw that out and have the gpiochip cover the pin space for the 
chip then using pinctrl only gives us mutual exclusion and the debugfs 
interface. pinctrl seems pretty heavy-weight to use *just* for mutual 
exclusion - with no requirement for pin mapping I feel whether or not 
we go this way hinges on the utility of debugfs.

As outlined earlier, there's no mux hardware, the only thing that 
changes is software's intent.

> 
> To be clear: I do not like putting everything into one driver when the
> logical parts may be separated.

Right, its already a bit unwieldy.

Andrew