RE: gpio: aggregator: Using chips with can_sleep

[Mikko Salomäki <ms@xxxxxxxxxxxxxx>]

Hello Geert,

Thank you for your feedback.

Your proposed changes are precisely what's needed.

I really appreciate your work on gpio-aggregator, it makes life easier in exposing system level gpio to userspace.

Regards,
Mikko


-----Original Message-----
From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> 
Sent: Friday, 28 January 2022 15:41
To: Mikko Salomäki <ms@xxxxxxxxxxxxxx>
Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx
Subject: Re: gpio: aggregator: Using chips with can_sleep

 	Hi Mikko,

On Fri, 28 Jan 2022, Andy Shevchenko wrote:
> +Cc: author, maintainers

Thank you, Andy

> On Wed, Jan 26, 2022 at 10:38 PM Mikko Salomäki <ms@xxxxxxxxxxxxxx> wrote:
>
>> Trying to map gpio from i2c connected chips triggered kernel warnings from libgpiod when setting or getting values. By my understanding the get and set calls need to change to their _cansleep counterparts for chips with chip->can_sleep.
>>
>> For example:
>> gpiod_get_value() -> gpiod_get_value_cansleep()
>>
>> Is this an actual bug or my misunderstanding?

Thanks for your report!
I think this is an oversight, i.e. an actual bug.

Does the below (compile-tested only) help?

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index 869dc952cf45218b..70ccad102cb1df96 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -278,7 +278,8 @@ static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
  {
  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);

-	return gpiod_get_value(fwd->descs[offset]);
+	return chip->can_sleep ? gpiod_get_value_cansleep(fwd->descs[offset])
+			       : gpiod_get_value(fwd->descs[offset]);
  }

  static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask, @@ -293,7 +294,10 @@ static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
  	for_each_set_bit(i, mask, fwd->chip.ngpio)
  		descs[j++] = fwd->descs[i];

-	error = gpiod_get_array_value(j, descs, NULL, values);
+	if (fwd->chip.can_sleep)
+		error = gpiod_get_array_value_cansleep(j, descs, NULL, values);
+	else
+		error = gpiod_get_array_value(j, descs, NULL, values);
  	if (error)
  		return error;

@@ -328,7 +332,10 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
  {
  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);

-	gpiod_set_value(fwd->descs[offset], value);
+	if (chip->can_sleep)
+		gpiod_set_value_cansleep(fwd->descs[offset], value);
+	else
+		gpiod_set_value(fwd->descs[offset], value);
  }

  static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask, @@ -343,7 +350,10 @@ static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
  		descs[j++] = fwd->descs[i];
  	}

-	gpiod_set_array_value(j, descs, NULL, values);
+	if (fwd->chip.can_sleep)
+		gpiod_set_array_value_cansleep(j, descs, NULL, values);
+	else
+		gpiod_set_array_value(j, descs, NULL, values);
  }

  static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds