diff mbox series

[RFC,v2,4/6] gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd

Message ID 20250317-aaeon-up-board-pinctrl-support-v2-4-36126e30aa62@bootlin.com
State New
Headers show
Series Add pinctrl support for the AAEON UP board FPGA | expand

Commit Message

Thomas Richard March 17, 2025, 3:38 p.m. UTC 
Add request() callback to check if the GPIO descriptor was well registered
in the gpiochip_fwd before to use it. This is done to handle the case
where GPIO descriptor is added at runtime in the forwarder.

If at least one GPIO descriptor was not added before the forwarder
registration, we assume the forwarder can sleep as if a GPIO is added at
runtime it may sleep.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 29 ++++++++++++++++++++++-------
 include/linux/gpio/gpio-fwd.h  |  2 ++
 2 files changed, 24 insertions(+), 7 deletions(-)

Comments

Thomas Richard April 9, 2025, 2:50 p.m. UTC | #1 
On 3/17/25 18:13, Andy Shevchenko wrote:
> On Mon, Mar 17, 2025 at 04:38:02PM +0100, Thomas Richard wrote:
>> Add request() callback to check if the GPIO descriptor was well registered
>> in the gpiochip_fwd before to use it. This is done to handle the case
>> where GPIO descriptor is added at runtime in the forwarder.
>>
>> If at least one GPIO descriptor was not added before the forwarder
>> registration, we assume the forwarder can sleep as if a GPIO is added at
>> runtime it may sleep.
> 
> Hmm... This should rather be reformatted each time a new descriptor is added,
> no?
I used the easiest solution.

Otherwise to switch to can_sleep mode:
- gpio_fwd_add_gpio_desc() inits the mutex if a sleeping GPIO line is added.
- gpio_fwd_add_gpio_desc() locks the mutex if the spinlock is locked
(gpio_fwd_get_multiple_locked() or gpio_fwd_set_multiple_locked() was
called).
- gpio_fwd_add_gpio_desc() set can_sleep mode to true
- gpio_fwd_get_multiple_locked() or gpio_fwd_set_multiple_locked() shall
unlock the spinlock and the mutex at end.

I think it is a bit over-engineered, and I probably do not handle all
corner cases.

Regards,

Thomas
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 7d00247f5268c..b9026ff2bfdc1 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -260,6 +260,14 @@  static void __exit gpio_aggregator_remove_all(void)
 
 #define fwd_tmp_size(ngpios)	(BITS_TO_LONGS((ngpios)) + (ngpios))
 
+int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return fwd->descs[offset] ? 0 : -ENXIO;
+}
+EXPORT_SYMBOL_GPL(gpio_fwd_request);
+
 int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
@@ -505,6 +513,7 @@  struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
 	chip->label = label;
 	chip->parent = dev;
 	chip->owner = THIS_MODULE;
+	chip->request = gpio_fwd_request;
 	chip->get_direction = gpio_fwd_get_direction;
 	chip->direction_input = gpio_fwd_direction_input;
 	chip->direction_output = gpio_fwd_direction_output;
@@ -524,7 +533,6 @@  int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 			       unsigned int offset)
 {
 	struct gpio_chip *chip = &fwd->chip;
-	struct gpio_chip *parent = gpiod_to_chip(desc);
 
 	if (offset > chip->ngpio)
 		return -EINVAL;
@@ -535,15 +543,10 @@  int gpiochip_fwd_add_gpio_desc(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
 	/*
 	 * If any of the GPIO lines are sleeping, then the entire forwarder
 	 * will be sleeping.
-	 * If any of the chips support .set_config(), then the forwarder will
-	 * support setting configs.
 	 */
 	if (gpiod_cansleep(desc))
 		chip->can_sleep = true;
 
-	if (parent && parent->set_config)
-		chip->set_config = gpio_fwd_set_config;
-
 	fwd->descs[offset] = desc;
 
 	dev_dbg(fwd->dev, "%u => gpio %d irq %d\n", offset,
@@ -557,7 +560,19 @@  int gpiochip_fwd_register(struct gpiochip_fwd *fwd)
 {
 	struct gpio_chip *chip = &fwd->chip;
 	struct device *dev = fwd->dev;
-	int error;
+	int ndescs = 0;
+	int error, i;
+
+	for (i = 0; i < chip->ngpio; i++)
+		if (fwd->descs[i])
+			ndescs++;
+
+	/*
+	 * Some gpio_desc were not registers. They will be registered at runtime
+	 * but we have to suppose they can sleep.
+	 */
+	if (ndescs != chip->ngpio)
+		chip->can_sleep = true;
 
 	if (chip->can_sleep)
 		mutex_init(&fwd->mlock);
diff --git a/include/linux/gpio/gpio-fwd.h b/include/linux/gpio/gpio-fwd.h
index d705b6d7ad76a..80ec34ee282fc 100644
--- a/include/linux/gpio/gpio-fwd.h
+++ b/include/linux/gpio/gpio-fwd.h
@@ -19,6 +19,8 @@  struct gpiochip_fwd {
 	unsigned long tmp[];		/* values and descs for multiple ops */
 };
 
+int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset);
+
 int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
 
 int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);