[PATCH v2 00/13] gpiolib: add gpiod_multi_set_value_cansleep

[PATCH v2 00/13] gpiolib: add gpiod_multi_set_value_cansleep

[David Lechner]

This series was inspired by some minor annoyance I have experienced a
few times in recent reviews.

Calling gpiod_set_array_value_cansleep() can be quite verbose due to
having so many parameters. In most cases, we already have a struct
gpio_descs that contains the first 3 parameters so we end up with 3 (or
often even 6) pointer indirections at each call site. Also, people have
a tendency to want to hard-code the first argument instead of using
struct gpio_descs.ndescs, often without checking that ndescs >= the
hard-coded value.

So I'm proposing that we add a gpiod_multi_set_value_cansleep()
function that is a wrapper around gpiod_set_array_value_cansleep()
that has struct gpio_descs as the first parameter to make it a bit
easier to read the code and avoid the hard-coding temptation.

I've just done gpiod_multi_set_value_cansleep() for now since there
were over 10 callers of this one. There aren't as many callers of
the get and atomic variants, but we can add those too if this seems
like a useful thing to do.

Maintainers, if you prefer to have this go through the gpio tree, please
give your Acked-by:, otherwise I will resend what is left after the next
kernel release.

---
Changes in v2:
- Renamed new function from gpiods_multi_set_value_cansleep() to
  gpiod_multi_set_value_cansleep()
- Fixed typo in name of replaced function in all commit messages.
- Picked up trailers.
- Link to v1: https://lore.kernel.org/r/20250131-gpio-set-array-helper-v1-0-991c8ccb4d6e@baylibre.com

---
David Lechner (13):
      gpiolib: add gpiod_multi_set_value_cansleep()
      auxdisplay: seg-led-gpio: use gpiod_multi_set_value_cansleep
      bus: ts-nbus: validate ts,data-gpios array size
      bus: ts-nbus: use gpiod_multi_set_value_cansleep
      gpio: max3191x: use gpiod_multi_set_value_cansleep
      iio: adc: ad7606: use gpiod_multi_set_value_cansleep
      iio: amplifiers: hmc425a: use gpiod_multi_set_value_cansleep
      iio: resolver: ad2s1210: use gpiod_multi_set_value_cansleep
      mmc: pwrseq_simple: use gpiod_multi_set_value_cansleep
      mux: gpio: use gpiod_multi_set_value_cansleep
      net: mdio: mux-gpio: use gpiod_multi_set_value_cansleep
      phy: mapphone-mdm6600: use gpiod_multi_set_value_cansleep
      ASoC: adau1701: use gpiod_multi_set_value_cansleep

 drivers/auxdisplay/seg-led-gpio.c           |  3 +--
 drivers/bus/ts-nbus.c                       | 10 ++++++----
 drivers/gpio/gpio-max3191x.c                | 18 +++++++-----------
 drivers/iio/adc/ad7606.c                    |  3 +--
 drivers/iio/adc/ad7606_spi.c                |  3 +--
 drivers/iio/amplifiers/hmc425a.c            |  3 +--
 drivers/iio/resolver/ad2s1210.c             |  8 ++------
 drivers/mmc/core/pwrseq_simple.c            |  3 +--
 drivers/mux/gpio.c                          |  4 +---
 drivers/net/mdio/mdio-mux-gpio.c            |  3 +--
 drivers/phy/motorola/phy-mapphone-mdm6600.c |  4 +---
 include/linux/gpio/consumer.h               |  7 +++++++
 sound/soc/codecs/adau1701.c                 |  4 +---
 13 files changed, 31 insertions(+), 42 deletions(-)
---
base-commit: df4b2bbff898227db0c14264ac7edd634e79f755
change-id: 20250131-gpio-set-array-helper-bd4a328370d3

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH v2 01/13] gpiolib: add gpiod_multi_set_value_cansleep()

[David Lechner]

Add a new gpiod_multi_set_value_cansleep() helper function with fewer
parameters than gpiod_set_array_value_cansleep().

Calling gpiod_set_array_value_cansleep() can get quite verbose. In many
cases, the first arguments all come from the same struct gpio_descs, so
having a separate function where we can just pass that cuts down on the
boilerplate.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 include/linux/gpio/consumer.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index db2dfbae8edbd12059826183b1c0f73c7a58ff40..278a197a9deba11dadbff0b58507df91be658f34 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -655,4 +655,11 @@ static inline void gpiod_unexport(struct gpio_desc *desc)
 
 #endif /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
 
+static inline int gpiod_multi_set_value_cansleep(struct gpio_descs *descs,
+						 unsigned long *value_bitmap)
+{
+	return gpiod_set_array_value_cansleep(descs->ndescs, descs->desc,
+					      descs->info, value_bitmap);
+}
+
 #endif

-- 
2.43.0

[PATCH v2 02/13] auxdisplay: seg-led-gpio: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

Acked-by: Andy Shevchenko <andy@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/auxdisplay/seg-led-gpio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/seg-led-gpio.c b/drivers/auxdisplay/seg-led-gpio.c
index f10c25e6bf126cfaac3e4c353f8bfc6639d94a60..dfb62e9ce9b407fe356c3a7d2d25319b91a11a75 100644
--- a/drivers/auxdisplay/seg-led-gpio.c
+++ b/drivers/auxdisplay/seg-led-gpio.c
@@ -36,8 +36,7 @@ static void seg_led_update(struct work_struct *work)
 
 	bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
 
-	gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
-				       priv->segment_gpios->info, values);
+	gpiod_multi_set_value_cansleep(priv->segment_gpios, values);
 }
 
 static int seg_led_linedisp_get_map_type(struct linedisp *linedisp)

-- 
2.43.0

[PATCH v2 03/13] bus: ts-nbus: validate ts,data-gpios array size

[David Lechner]

Add validation of ts,data-gpios array size during probe. The driver
later hard-codes 8 as the size of the array when using it, so we should
be validating that the array is actually that big to prevent possible
out of bounds accesses.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/bus/ts-nbus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
index 2328c48b9b1260e805c631f2aa7379d620084537..d3ee102a13893c83c50e41f7298821f4d7ae3487 100644
--- a/drivers/bus/ts-nbus.c
+++ b/drivers/bus/ts-nbus.c
@@ -48,6 +48,10 @@ static int ts_nbus_init_pdata(struct platform_device *pdev,
 		return dev_err_probe(&pdev->dev, PTR_ERR(ts_nbus->data),
 				     "failed to retrieve ts,data-gpio from dts\n");
 
+	if (ts_nbus->data->ndescs != 8)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "invalid number of ts,data-gpios\n");
+
 	ts_nbus->csn = devm_gpiod_get(&pdev->dev, "ts,csn", GPIOD_OUT_HIGH);
 	if (IS_ERR(ts_nbus->csn))
 		return dev_err_probe(&pdev->dev, PTR_ERR(ts_nbus->csn),

-- 
2.43.0

[PATCH v2 04/13] bus: ts-nbus: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

ts_nbus->data->ndescs is validated to be 8 during probe, so will have
the same value as the hard-coded 8 that is removed by this change.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/bus/ts-nbus.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
index d3ee102a13893c83c50e41f7298821f4d7ae3487..b4c9308caf0647a3261071d9527fffce77784af2 100644
--- a/drivers/bus/ts-nbus.c
+++ b/drivers/bus/ts-nbus.c
@@ -109,8 +109,7 @@ static void ts_nbus_reset_bus(struct ts_nbus *ts_nbus)
 
 	values[0] = 0;
 
-	gpiod_set_array_value_cansleep(8, ts_nbus->data->desc,
-				       ts_nbus->data->info, values);
+	gpiod_multi_set_value_cansleep(ts_nbus->data, values);
 	gpiod_set_value_cansleep(ts_nbus->csn, 0);
 	gpiod_set_value_cansleep(ts_nbus->strobe, 0);
 	gpiod_set_value_cansleep(ts_nbus->ale, 0);
@@ -150,12 +149,11 @@ static int ts_nbus_read_byte(struct ts_nbus *ts_nbus, u8 *val)
  */
 static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
 {
-	struct gpio_descs *gpios = ts_nbus->data;
 	DECLARE_BITMAP(values, 8);
 
 	values[0] = byte;
 
-	gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
+	gpiod_multi_set_value_cansleep(ts_nbus->data, values);
 }
 
 /*

-- 
2.43.0

[PATCH v2 05/13] gpio: max3191x: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/gpio/gpio-max3191x.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-max3191x.c b/drivers/gpio/gpio-max3191x.c
index bbacc714632b70e672a3d8494636fbc40dfea8ec..36ca07be3e1811fd3f0b27f41bfd307de50ec5b4 100644
--- a/drivers/gpio/gpio-max3191x.c
+++ b/drivers/gpio/gpio-max3191x.c
@@ -309,23 +309,21 @@ static int max3191x_set_config(struct gpio_chip *gpio, unsigned int offset,
 	return 0;
 }
 
-static void gpiod_set_array_single_value_cansleep(unsigned int ndescs,
-						  struct gpio_desc **desc,
-						  struct gpio_array *info,
+static void gpiod_set_array_single_value_cansleep(struct gpio_descs *descs,
 						  int value)
 {
 	unsigned long *values;
 
-	values = bitmap_alloc(ndescs, GFP_KERNEL);
+	values = bitmap_alloc(descs->ndescs, GFP_KERNEL);
 	if (!values)
 		return;
 
 	if (value)
-		bitmap_fill(values, ndescs);
+		bitmap_fill(values, descs->ndescs);
 	else
-		bitmap_zero(values, ndescs);
+		bitmap_zero(values, descs->ndescs);
 
-	gpiod_set_array_value_cansleep(ndescs, desc, info, values);
+	gpiod_multi_set_value_cansleep(descs, values);
 	bitmap_free(values);
 }
 
@@ -396,10 +394,8 @@ static int max3191x_probe(struct spi_device *spi)
 	max3191x->mode = device_property_read_bool(dev, "maxim,modesel-8bit")
 				 ? STATUS_BYTE_DISABLED : STATUS_BYTE_ENABLED;
 	if (max3191x->modesel_pins)
-		gpiod_set_array_single_value_cansleep(
-				 max3191x->modesel_pins->ndescs,
-				 max3191x->modesel_pins->desc,
-				 max3191x->modesel_pins->info, max3191x->mode);
+		gpiod_set_array_single_value_cansleep(max3191x->modesel_pins,
+						      max3191x->mode);
 
 	max3191x->ignore_uv = device_property_read_bool(dev,
 						  "maxim,ignore-undervoltage");

-- 
2.43.0

[PATCH v2 06/13] iio: adc: ad7606: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value().

These are not called in an atomic context, so changing to the cansleep
variant is fine.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7606.c     | 3 +--
 drivers/iio/adc/ad7606_spi.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index d8e3c7a43678c57470a5118715637a68b39125c1..9a124139924e4a4fbbbd234a8514eb77024442b3 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -818,8 +818,7 @@ static int ad7606_write_os_hw(struct iio_dev *indio_dev, int val)
 
 	values[0] = val & GENMASK(2, 0);
 
-	gpiod_set_array_value(st->gpio_os->ndescs, st->gpio_os->desc,
-			      st->gpio_os->info, values);
+	gpiod_multi_set_value_cansleep(st->gpio_os, values);
 
 	/* AD7616 requires a reset to update value */
 	if (st->chip_info->os_req_reset)
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index e2c1475257065c98bf8e2512bda921d6d88a3002..0fc41275e79f3af282c0ee4f31e222bbe8d8c1d2 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -297,8 +297,7 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
 	 * otherwise, they must be hardwired to VDD
 	 */
 	if (st->gpio_os) {
-		gpiod_set_array_value(st->gpio_os->ndescs,
-				      st->gpio_os->desc, st->gpio_os->info, os);
+		gpiod_multi_set_value_cansleep(st->gpio_os, os);
 	}
 	/* OS of 128 and 256 are available only in software mode */
 	st->oversampling_avail = ad7606B_oversampling_avail;

-- 
2.43.0

[PATCH v2 07/13] iio: amplifiers: hmc425a: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

Passing NULL as the 3rd argument to gpiod_set_array_value_cansleep()
only needs to be done if the array was constructed manually, which is
not the case here. This change effectively replaces that argument with
st->gpios->array_info. The possible side effect of this change is that
it could make setting the GPIOs more efficient.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/amplifiers/hmc425a.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index 2ee4c0d70281e24c1c818249b86d89ebe06d4876..d9a359e1388a0f3eb5909bf668ff82102286542b 100644
--- a/drivers/iio/amplifiers/hmc425a.c
+++ b/drivers/iio/amplifiers/hmc425a.c
@@ -161,8 +161,7 @@ static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
 
 	values[0] = value;
 
-	gpiod_set_array_value_cansleep(st->gpios->ndescs, st->gpios->desc,
-				       NULL, values);
+	gpiod_multi_set_value_cansleep(st->gpios, values);
 	return 0;
 }
 

-- 
2.43.0

[PATCH v2 08/13] iio: resolver: ad2s1210: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value().

These are not called in an atomic context, so changing to the cansleep
variant is fine.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/resolver/ad2s1210.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
index b681129a99b6cf399668bf01a1f5a15fbc4f95b8..7f18df790157f1e411fb70de193a49f0677c999f 100644
--- a/drivers/iio/resolver/ad2s1210.c
+++ b/drivers/iio/resolver/ad2s1210.c
@@ -182,8 +182,7 @@ static int ad2s1210_set_mode(struct ad2s1210_state *st, enum ad2s1210_mode mode)
 
 	bitmap[0] = mode;
 
-	return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
-				     bitmap);
+	return gpiod_multi_set_value_cansleep(gpios, bitmap);
 }
 
 /*
@@ -1473,10 +1472,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
 
 		bitmap[0] = st->resolution;
 
-		ret = gpiod_set_array_value(resolution_gpios->ndescs,
-					    resolution_gpios->desc,
-					    resolution_gpios->info,
-					    bitmap);
+		ret = gpiod_multi_set_value_cansleep(resolution_gpios, bitmap);
 		if (ret < 0)
 			return dev_err_probe(dev, ret,
 					     "failed to set resolution gpios\n");

-- 
2.43.0

[PATCH v2 09/13] mmc: pwrseq_simple: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/mmc/core/pwrseq_simple.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 37cd858df0f4d7123683e1fe23a4c3fcd7817d13..4b47e6c3b04b99dc328a8b063665a76340a8e0d0 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -54,8 +54,7 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
 		else
 			bitmap_zero(values, nvalues);
 
-		gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
-					       reset_gpios->info, values);
+		gpiod_multi_set_value_cansleep(reset_gpios, values);
 
 		bitmap_free(values);
 	}

-- 
2.43.0

[PATCH v2 10/13] mux: gpio: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

Acked-by: Peter Rosin <peda@axentia.se>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/mux/gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
index cc5f2c1861d4a22d984bcd37efb98dd3561ee765..5710879cd47f89b6ef4458d6b4419a1fe9ad349f 100644
--- a/drivers/mux/gpio.c
+++ b/drivers/mux/gpio.c
@@ -28,9 +28,7 @@ static int mux_gpio_set(struct mux_control *mux, int state)
 
 	bitmap_from_arr32(values, &value, BITS_PER_TYPE(value));
 
-	gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
-				       mux_gpio->gpios->desc,
-				       mux_gpio->gpios->info, values);
+	gpiod_multi_set_value_cansleep(mux_gpio->gpios, values);
 
 	return 0;
 }

-- 
2.43.0

[PATCH v2 11/13] net: mdio: mux-gpio: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/net/mdio/mdio-mux-gpio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/mdio/mdio-mux-gpio.c b/drivers/net/mdio/mdio-mux-gpio.c
index ef77bd1abae984e5b1e51315de39cae33e0d063d..fefa40ea5227c5a35d89ec2c6f95c6668a2470f6 100644
--- a/drivers/net/mdio/mdio-mux-gpio.c
+++ b/drivers/net/mdio/mdio-mux-gpio.c
@@ -30,8 +30,7 @@ static int mdio_mux_gpio_switch_fn(int current_child, int desired_child,
 
 	values[0] = desired_child;
 
-	gpiod_set_array_value_cansleep(s->gpios->ndescs, s->gpios->desc,
-				       s->gpios->info, values);
+	gpiod_multi_set_value_cansleep(s->gpios, values);
 
 	return 0;
 }

-- 
2.43.0

[PATCH v2 12/13] phy: mapphone-mdm6600: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

ddata->cmd_gpios->ndescs is validated to be equal to
PHY_MDM6600_NR_CMD_LINES during driver probe, so it will have the same
value as the previously hard-coded argument.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/phy/motorola/phy-mapphone-mdm6600.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c b/drivers/phy/motorola/phy-mapphone-mdm6600.c
index 152344e4f7e44de0f8ab1cae6ae01a1f1c5408e9..fd0e0cd1c1cfb10fb55ed271e47b6a0bf857028e 100644
--- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
+++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
@@ -177,9 +177,7 @@ static void phy_mdm6600_cmd(struct phy_mdm6600 *ddata, int val)
 
 	values[0] = val;
 
-	gpiod_set_array_value_cansleep(PHY_MDM6600_NR_CMD_LINES,
-				       ddata->cmd_gpios->desc,
-				       ddata->cmd_gpios->info, values);
+	gpiod_multi_set_value_cansleep(ddata->cmd_gpios, values);
 }
 
 /**

-- 
2.43.0

[PATCH v2 13/13] ASoC: adau1701: use gpiod_multi_set_value_cansleep

[David Lechner]

Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 sound/soc/codecs/adau1701.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c
index 291249e0a2a32df7dde81904dce2f6be143fc2d7..6876462d8bdbb41d551f776c2d7fe6ed46115fa1 100644
--- a/sound/soc/codecs/adau1701.c
+++ b/sound/soc/codecs/adau1701.c
@@ -325,9 +325,7 @@ static int adau1701_reset(struct snd_soc_component *component, unsigned int clkd
 			__assign_bit(1, values, 1);
 			break;
 		}
-		gpiod_set_array_value_cansleep(adau1701->gpio_pll_mode->ndescs,
-				adau1701->gpio_pll_mode->desc, adau1701->gpio_pll_mode->info,
-				values);
+		gpiod_multi_set_value_cansleep(adau1701->gpio_pll_mode, values);
 	}
 
 	adau1701->pll_clkdiv = clkdiv;

-- 
2.43.0

Re: [PATCH v2 00/13] gpiolib: add gpiod_multi_set_value_cansleep

[Bartosz Golaszewski]

On Thu, Feb 6, 2025 at 11:48 PM David Lechner <dlechner@baylibre.com> wrote:
>
> This series was inspired by some minor annoyance I have experienced a
> few times in recent reviews.
>
> Calling gpiod_set_array_value_cansleep() can be quite verbose due to
> having so many parameters. In most cases, we already have a struct
> gpio_descs that contains the first 3 parameters so we end up with 3 (or
> often even 6) pointer indirections at each call site. Also, people have
> a tendency to want to hard-code the first argument instead of using
> struct gpio_descs.ndescs, often without checking that ndescs >= the
> hard-coded value.
>
> So I'm proposing that we add a gpiod_multi_set_value_cansleep()
> function that is a wrapper around gpiod_set_array_value_cansleep()
> that has struct gpio_descs as the first parameter to make it a bit
> easier to read the code and avoid the hard-coding temptation.
>
> I've just done gpiod_multi_set_value_cansleep() for now since there
> were over 10 callers of this one. There aren't as many callers of
> the get and atomic variants, but we can add those too if this seems
> like a useful thing to do.
>
> Maintainers, if you prefer to have this go through the gpio tree, please
> give your Acked-by:, otherwise I will resend what is left after the next
> kernel release.
>
> ---
> Changes in v2:
> - Renamed new function from gpiods_multi_set_value_cansleep() to
>   gpiod_multi_set_value_cansleep()
> - Fixed typo in name of replaced function in all commit messages.
> - Picked up trailers.
> - Link to v1: https://lore.kernel.org/r/20250131-gpio-set-array-helper-v1-0-991c8ccb4d6e@baylibre.com
>
> ---
> David Lechner (13):
>       gpiolib: add gpiod_multi_set_value_cansleep()
>       auxdisplay: seg-led-gpio: use gpiod_multi_set_value_cansleep
>       bus: ts-nbus: validate ts,data-gpios array size
>       bus: ts-nbus: use gpiod_multi_set_value_cansleep
>       gpio: max3191x: use gpiod_multi_set_value_cansleep
>       iio: adc: ad7606: use gpiod_multi_set_value_cansleep
>       iio: amplifiers: hmc425a: use gpiod_multi_set_value_cansleep
>       iio: resolver: ad2s1210: use gpiod_multi_set_value_cansleep
>       mmc: pwrseq_simple: use gpiod_multi_set_value_cansleep
>       mux: gpio: use gpiod_multi_set_value_cansleep
>       net: mdio: mux-gpio: use gpiod_multi_set_value_cansleep
>       phy: mapphone-mdm6600: use gpiod_multi_set_value_cansleep
>       ASoC: adau1701: use gpiod_multi_set_value_cansleep
>
>  drivers/auxdisplay/seg-led-gpio.c           |  3 +--
>  drivers/bus/ts-nbus.c                       | 10 ++++++----
>  drivers/gpio/gpio-max3191x.c                | 18 +++++++-----------
>  drivers/iio/adc/ad7606.c                    |  3 +--
>  drivers/iio/adc/ad7606_spi.c                |  3 +--
>  drivers/iio/amplifiers/hmc425a.c            |  3 +--
>  drivers/iio/resolver/ad2s1210.c             |  8 ++------
>  drivers/mmc/core/pwrseq_simple.c            |  3 +--
>  drivers/mux/gpio.c                          |  4 +---
>  drivers/net/mdio/mdio-mux-gpio.c            |  3 +--
>  drivers/phy/motorola/phy-mapphone-mdm6600.c |  4 +---
>  include/linux/gpio/consumer.h               |  7 +++++++
>  sound/soc/codecs/adau1701.c                 |  4 +---
>  13 files changed, 31 insertions(+), 42 deletions(-)
> ---
> base-commit: df4b2bbff898227db0c14264ac7edd634e79f755
> change-id: 20250131-gpio-set-array-helper-bd4a328370d3
>
> Best regards,
> --
> David Lechner <dlechner@baylibre.com>
>

I can provide an immutable branch for the entire series for everyone
to pull or I can apply patch one, provide an immutable branch and
every subsystem can take their respective patches. What do you prefer?

Bartosz

Re: [PATCH v2 02/13] auxdisplay: seg-led-gpio: use gpiod_multi_set_value_cansleep

[Geert Uytterhoeven]

On Thu, 6 Feb 2025 at 23:48, David Lechner <dlechner@baylibre.com> wrote:
> Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
> gpiod_set_array_value_cansleep().
>
> Acked-by: Andy Shevchenko <andy@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 01/13] gpiolib: add gpiod_multi_set_value_cansleep()

[Geert Uytterhoeven]

Hi David,

On Thu, 6 Feb 2025 at 23:48, David Lechner <dlechner@baylibre.com> wrote:
> Add a new gpiod_multi_set_value_cansleep() helper function with fewer
> parameters than gpiod_set_array_value_cansleep().
>
> Calling gpiod_set_array_value_cansleep() can get quite verbose. In many
> cases, the first arguments all come from the same struct gpio_descs, so
> having a separate function where we can just pass that cuts down on the
> boilerplate.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Thanks for your patch!

> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -655,4 +655,11 @@ static inline void gpiod_unexport(struct gpio_desc *desc)
>
>  #endif /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
>
> +static inline int gpiod_multi_set_value_cansleep(struct gpio_descs *descs,
> +                                                unsigned long *value_bitmap)
> +{
> +       return gpiod_set_array_value_cansleep(descs->ndescs, descs->desc,
> +                                             descs->info, value_bitmap);

I am wondering whether this needs a check for !IS_ERR_OR_NULL(descs),
to handle the !CONFIG_GPIOLIB and gpiod_get_array_optional() cases?

Slightly related: shouldn't gpiod_put_array() (both the implementation
and the !CONFIG_GPIOLIB dummy) allow the caller to pass NULL, to
streamline the gpiod_get_array_optional() case?

> +}
> +
>  #endif

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 00/13] gpiolib: add gpiod_multi_set_value_cansleep

[Ulf Hansson]

On Fri, 7 Feb 2025 at 08:49, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Feb 6, 2025 at 11:48 PM David Lechner <dlechner@baylibre.com> wrote:
> >
> > This series was inspired by some minor annoyance I have experienced a
> > few times in recent reviews.
> >
> > Calling gpiod_set_array_value_cansleep() can be quite verbose due to
> > having so many parameters. In most cases, we already have a struct
> > gpio_descs that contains the first 3 parameters so we end up with 3 (or
> > often even 6) pointer indirections at each call site. Also, people have
> > a tendency to want to hard-code the first argument instead of using
> > struct gpio_descs.ndescs, often without checking that ndescs >= the
> > hard-coded value.
> >
> > So I'm proposing that we add a gpiod_multi_set_value_cansleep()
> > function that is a wrapper around gpiod_set_array_value_cansleep()
> > that has struct gpio_descs as the first parameter to make it a bit
> > easier to read the code and avoid the hard-coding temptation.
> >
> > I've just done gpiod_multi_set_value_cansleep() for now since there
> > were over 10 callers of this one. There aren't as many callers of
> > the get and atomic variants, but we can add those too if this seems
> > like a useful thing to do.
> >
> > Maintainers, if you prefer to have this go through the gpio tree, please
> > give your Acked-by:, otherwise I will resend what is left after the next
> > kernel release.
> >
> > ---
> > Changes in v2:
> > - Renamed new function from gpiods_multi_set_value_cansleep() to
> >   gpiod_multi_set_value_cansleep()
> > - Fixed typo in name of replaced function in all commit messages.
> > - Picked up trailers.
> > - Link to v1: https://lore.kernel.org/r/20250131-gpio-set-array-helper-v1-0-991c8ccb4d6e@baylibre.com
> >
> > ---
> > David Lechner (13):
> >       gpiolib: add gpiod_multi_set_value_cansleep()
> >       auxdisplay: seg-led-gpio: use gpiod_multi_set_value_cansleep
> >       bus: ts-nbus: validate ts,data-gpios array size
> >       bus: ts-nbus: use gpiod_multi_set_value_cansleep
> >       gpio: max3191x: use gpiod_multi_set_value_cansleep
> >       iio: adc: ad7606: use gpiod_multi_set_value_cansleep
> >       iio: amplifiers: hmc425a: use gpiod_multi_set_value_cansleep
> >       iio: resolver: ad2s1210: use gpiod_multi_set_value_cansleep
> >       mmc: pwrseq_simple: use gpiod_multi_set_value_cansleep
> >       mux: gpio: use gpiod_multi_set_value_cansleep
> >       net: mdio: mux-gpio: use gpiod_multi_set_value_cansleep
> >       phy: mapphone-mdm6600: use gpiod_multi_set_value_cansleep
> >       ASoC: adau1701: use gpiod_multi_set_value_cansleep
> >
> >  drivers/auxdisplay/seg-led-gpio.c           |  3 +--
> >  drivers/bus/ts-nbus.c                       | 10 ++++++----
> >  drivers/gpio/gpio-max3191x.c                | 18 +++++++-----------
> >  drivers/iio/adc/ad7606.c                    |  3 +--
> >  drivers/iio/adc/ad7606_spi.c                |  3 +--
> >  drivers/iio/amplifiers/hmc425a.c            |  3 +--
> >  drivers/iio/resolver/ad2s1210.c             |  8 ++------
> >  drivers/mmc/core/pwrseq_simple.c            |  3 +--
> >  drivers/mux/gpio.c                          |  4 +---
> >  drivers/net/mdio/mdio-mux-gpio.c            |  3 +--
> >  drivers/phy/motorola/phy-mapphone-mdm6600.c |  4 +---
> >  include/linux/gpio/consumer.h               |  7 +++++++
> >  sound/soc/codecs/adau1701.c                 |  4 +---
> >  13 files changed, 31 insertions(+), 42 deletions(-)
> > ---
> > base-commit: df4b2bbff898227db0c14264ac7edd634e79f755
> > change-id: 20250131-gpio-set-array-helper-bd4a328370d3
> >
> > Best regards,
> > --
> > David Lechner <dlechner@baylibre.com>
> >
>
> I can provide an immutable branch for the entire series for everyone
> to pull or I can apply patch one, provide an immutable branch and
> every subsystem can take their respective patches. What do you prefer?

The changes look small and trivial to me. I wouldn't mind if you take
them all (at least for mmc). An immutable branch would be good, if it
turns out that we need to pull them.

Kind regards
Uffe

Re: [PATCH v2 05/13] gpio: max3191x: use gpiod_multi_set_value_cansleep

[Andy Shevchenko]

On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
> gpiod_set_array_value_cansleep().

It seems you missed my comment.

> +static void gpiod_set_array_single_value_cansleep(struct gpio_descs *descs,
>                                                   int value)

This is not good namespacing. Can we change this while at it?

max3191x_...

(I would go with max3191x_set_modesel_pins() to make it shorter. I
have no clue why the function repeats so much from gpiod API naming.
Is there anything else which is named in a similar way? Perhaps fix it
by a separate patch?)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 06/13] iio: adc: ad7606: use gpiod_multi_set_value_cansleep

[Andy Shevchenko]

On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
> gpiod_set_array_value().
>
> These are not called in an atomic context, so changing to the cansleep
> variant is fine.

...

>         if (st->gpio_os) {
> -               gpiod_set_array_value(st->gpio_os->ndescs,
> -                                     st->gpio_os->desc, st->gpio_os->info, os);
> +               gpiod_multi_set_value_cansleep(st->gpio_os, os);
>         }

Now I do not see any justification to leave {}.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/13] bus: ts-nbus: use gpiod_multi_set_value_cansleep

[Andy Shevchenko]

On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
> gpiod_set_array_value_cansleep().
>
> ts_nbus->data->ndescs is validated to be 8 during probe, so will have
> the same value as the hard-coded 8 that is removed by this change.

...

>  static void ts_nbus_reset_bus(struct ts_nbus *ts_nbus)

>         values[0] = 0;
> +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);

...

>  static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
>  {
> -       struct gpio_descs *gpios = ts_nbus->data;
>         DECLARE_BITMAP(values, 8);
>
>         values[0] = byte;
>
> -       gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
> +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);

As I said before, this is buggy code on BE64. Needs to be fixed.

>  }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/13] bus: ts-nbus: use gpiod_multi_set_value_cansleep

[Andy Shevchenko]

+Yury.

On Fri, Feb 7, 2025 at 2:15 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:

...

> >  static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
> >  {
> > -       struct gpio_descs *gpios = ts_nbus->data;
> >         DECLARE_BITMAP(values, 8);
> >
> >         values[0] = byte;
> >
> > -       gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
> > +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);
>
> As I said before, this is buggy code on BE64. Needs to be fixed.

Or isn't? Do we have a test case in bitmap for such a case?

> >  }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/13] gpiolib: add gpiod_multi_set_value_cansleep

[Andy Shevchenko]

On Fri, Feb 7, 2025 at 11:48 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Fri, 7 Feb 2025 at 08:49, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Thu, Feb 6, 2025 at 11:48 PM David Lechner <dlechner@baylibre.com> wrote:

...

> > > Maintainers, if you prefer to have this go through the gpio tree, please
> > > give your Acked-by:, otherwise I will resend what is left after the next
> > > kernel release.

> > I can provide an immutable branch for the entire series for everyone
> > to pull or I can apply patch one, provide an immutable branch and
> > every subsystem can take their respective patches. What do you prefer?
>
> The changes look small and trivial to me. I wouldn't mind if you take
> them all (at least for mmc). An immutable branch would be good, if it
> turns out that we need to pull them.

+1 here, the potential user for immutable branch/tag is IIO.
The rest looks trivial and unlikely to have conflicts.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/13] gpio: max3191x: use gpiod_multi_set_value_cansleep

[David Lechner]

On 2/7/25 4:34 AM, Andy Shevchenko wrote:
> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:
>>
>> Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
>> gpiod_set_array_value_cansleep().
> 
> It seems you missed my comment.

Yes, I must have been sleeping on the job. :-/

> 
>> +static void gpiod_set_array_single_value_cansleep(struct gpio_descs *descs,
>>                                                   int value)
> 
> This is not good namespacing. Can we change this while at it?

sure

> 
> max3191x_...
> 
> (I would go with max3191x_set_modesel_pins() to make it shorter. I
> have no clue why the function repeats so much from gpiod API naming.
> Is there anything else which is named in a similar way? Perhaps fix it
> by a separate patch?)
> 

Re: [PATCH v2 04/13] bus: ts-nbus: use gpiod_multi_set_value_cansleep

[David Lechner]

On 2/7/25 6:17 AM, Andy Shevchenko wrote:
> +Yury.
> 
> On Fri, Feb 7, 2025 at 2:15 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:
> 
> ...
> 
>>>  static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
>>>  {
>>> -       struct gpio_descs *gpios = ts_nbus->data;
>>>         DECLARE_BITMAP(values, 8);
>>>
>>>         values[0] = byte;
>>>
>>> -       gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
>>> +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);
>>
>> As I said before, this is buggy code on BE64. Needs to be fixed.
> 
> Or isn't? Do we have a test case in bitmap for such a case?
> 
>>>  }
> 
> 

Maybe not the best style, but I don't think it is buggy. Bitmaps are always
handled in long-sized chunks and not cast to bytes so endianness doesn't affect
it. I didn't see an explicit test, but bitmap_read() and bitmap_write() use
array access like this so indirectly it is being tested.

Re: [PATCH v2 04/13] bus: ts-nbus: use gpiod_multi_set_value_cansleep

[Andy Shevchenko]

On Fri, Feb 07, 2025 at 09:23:56AM -0600, David Lechner wrote:
> On 2/7/25 6:17 AM, Andy Shevchenko wrote:
> > On Fri, Feb 7, 2025 at 2:15 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:

...

> >>>  static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
> >>>  {
> >>> -       struct gpio_descs *gpios = ts_nbus->data;
> >>>         DECLARE_BITMAP(values, 8);
> >>>
> >>>         values[0] = byte;
> >>>
> >>> -       gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
> >>> +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);
> >>
> >> As I said before, this is buggy code on BE64. Needs to be fixed.
> > 
> > Or isn't? Do we have a test case in bitmap for such a case?
> > 
> >>>  }
> 
> Maybe not the best style, but I don't think it is buggy. Bitmaps are always
> handled in long-sized chunks and not cast to bytes so endianness doesn't affect
> it. I didn't see an explicit test, but bitmap_read() and bitmap_write() use
> array access like this so indirectly it is being tested.

Right, the potential (and likely theoretical) issue may come if DEFINE_BITMAP()
out of a sudden will use, let's say, an array of booleans. But it wasn't my
initial complain about the code :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/13] bus: ts-nbus: use gpiod_multi_set_value_cansleep

[Yury Norov]

On Fri, Feb 07, 2025 at 09:23:56AM -0600, David Lechner wrote:
> On 2/7/25 6:17 AM, Andy Shevchenko wrote:
> > +Yury.
> > 
> > On Fri, Feb 7, 2025 at 2:15 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:
> > 
> > ...
> > 
> >>>  static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
> >>>  {
> >>> -       struct gpio_descs *gpios = ts_nbus->data;
> >>>         DECLARE_BITMAP(values, 8);
> >>>
> >>>         values[0] = byte;
> >>>
> >>> -       gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
> >>> +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);
> >>
> >> As I said before, this is buggy code on BE64. Needs to be fixed.
> > 
> > Or isn't? Do we have a test case in bitmap for such a case?
> > 
> >>>  }
> > 
> > 
> 
> Maybe not the best style, but I don't think it is buggy. Bitmaps are always
> handled in long-sized chunks and not cast to bytes so endianness doesn't affect
> it. I didn't see an explicit test, but bitmap_read() and bitmap_write() use
> array access like this so indirectly it is being tested.

Not a bug, but direct addressing to bitmap elements is discouraged.
I'd suggest using bitmap_write(values, byte, 0, 8) instead.

Thanks,
Yury

Re: [PATCH v2 01/13] gpiolib: add gpiod_multi_set_value_cansleep()

[David Lechner]

On 2/7/25 3:10 AM, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Thu, 6 Feb 2025 at 23:48, David Lechner <dlechner@baylibre.com> wrote:
>> Add a new gpiod_multi_set_value_cansleep() helper function with fewer
>> parameters than gpiod_set_array_value_cansleep().
>>
>> Calling gpiod_set_array_value_cansleep() can get quite verbose. In many
>> cases, the first arguments all come from the same struct gpio_descs, so
>> having a separate function where we can just pass that cuts down on the
>> boilerplate.
>>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> 
> Thanks for your patch!
> 
>> --- a/include/linux/gpio/consumer.h
>> +++ b/include/linux/gpio/consumer.h
>> @@ -655,4 +655,11 @@ static inline void gpiod_unexport(struct gpio_desc *desc)
>>
>>  #endif /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
>>
>> +static inline int gpiod_multi_set_value_cansleep(struct gpio_descs *descs,
>> +                                                unsigned long *value_bitmap)
>> +{
>> +       return gpiod_set_array_value_cansleep(descs->ndescs, descs->desc,
>> +                                             descs->info, value_bitmap);
> 
> I am wondering whether this needs a check for !IS_ERR_OR_NULL(descs),
> to handle the !CONFIG_GPIOLIB and gpiod_get_array_optional() cases?

I don't think it is strictly needed, but could be convenient for future use
cases. If we add it, should it be:

	if (IS_ERR_OR_NULL(descs))
		return PTR_ERR(descs);

or:

	if (!descs)
		return -EINVAL;

	if (IS_ERR(descs))
		return PTR_ERR(descs);

?

For comparison, gpiod_set_array_value_cansleep() will return -EINVAL if the
first argument is NULL.

> 
> Slightly related: shouldn't gpiod_put_array() (both the implementation
> and the !CONFIG_GPIOLIB dummy) allow the caller to pass NULL, to
> streamline the gpiod_get_array_optional() case?
> 
>> +}
>> +
>>  #endif
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 01/13] gpiolib: add gpiod_multi_set_value_cansleep()

[Geert Uytterhoeven]

Hi David,

On Fri, 7 Feb 2025 at 17:29, David Lechner <dlechner@baylibre.com> wrote:
> On 2/7/25 3:10 AM, Geert Uytterhoeven wrote:
> > On Thu, 6 Feb 2025 at 23:48, David Lechner <dlechner@baylibre.com> wrote:
> >> Add a new gpiod_multi_set_value_cansleep() helper function with fewer
> >> parameters than gpiod_set_array_value_cansleep().
> >>
> >> Calling gpiod_set_array_value_cansleep() can get quite verbose. In many
> >> cases, the first arguments all come from the same struct gpio_descs, so
> >> having a separate function where we can just pass that cuts down on the
> >> boilerplate.
> >>
> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >
> > Thanks for your patch!
> >
> >> --- a/include/linux/gpio/consumer.h
> >> +++ b/include/linux/gpio/consumer.h
> >> @@ -655,4 +655,11 @@ static inline void gpiod_unexport(struct gpio_desc *desc)
> >>
> >>  #endif /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
> >>
> >> +static inline int gpiod_multi_set_value_cansleep(struct gpio_descs *descs,
> >> +                                                unsigned long *value_bitmap)
> >> +{
> >> +       return gpiod_set_array_value_cansleep(descs->ndescs, descs->desc,
> >> +                                             descs->info, value_bitmap);
> >
> > I am wondering whether this needs a check for !IS_ERR_OR_NULL(descs),
> > to handle the !CONFIG_GPIOLIB and gpiod_get_array_optional() cases?
>
> I don't think it is strictly needed, but could be convenient for future use
> cases. If we add it, should it be:
>
>         if (IS_ERR_OR_NULL(descs))
>                 return PTR_ERR(descs);

return PTR_ERR_OR_ZERO(descs);

(the compiler should optimize away checks common to IS_ERR_OR_NULL()
 and PTR_ERR_OR_ZERO()).

> or:
>
>         if (!descs)
>                 return -EINVAL;
>
>         if (IS_ERR(descs))
>                 return PTR_ERR(descs);
>
> ?

The former.

> For comparison, gpiod_set_array_value_cansleep() will return -EINVAL if the
> first argument is NULL.

That function cannot take an argument returned by a *_optional() call,
if I'm not mistaken.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 00/13] gpiolib: add gpiod_multi_set_value_cansleep

[Jonathan Cameron]

On Fri, 7 Feb 2025 14:20:16 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Feb 7, 2025 at 11:48 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Fri, 7 Feb 2025 at 08:49, Bartosz Golaszewski <brgl@bgdev.pl> wrote:  
> > > On Thu, Feb 6, 2025 at 11:48 PM David Lechner <dlechner@baylibre.com> wrote:  
> 
> ...
> 
> > > > Maintainers, if you prefer to have this go through the gpio tree, please
> > > > give your Acked-by:, otherwise I will resend what is left after the next
> > > > kernel release.  
> 
> > > I can provide an immutable branch for the entire series for everyone
> > > to pull or I can apply patch one, provide an immutable branch and
> > > every subsystem can take their respective patches. What do you prefer?  
> >
> > The changes look small and trivial to me. I wouldn't mind if you take
> > them all (at least for mmc). An immutable branch would be good, if it
> > turns out that we need to pull them.  
> 
> +1 here, the potential user for immutable branch/tag is IIO.
> The rest looks trivial and unlikely to have conflicts.

Whilst I'm not sure if we'll need it, definitely good to have immutable
branch just in case.

There is another change to the ad7606 on list, but it's no where near
this code so shouldn't be a problem however this goes in.
My slight preference would be an immutable with a tag on patch 1.
I'll pull that and apply the IIO ones on top.

If you want to grab the lot though that should be fine as well.

Jonathan

> 

Re: [PATCH v2 07/13] iio: amplifiers: hmc425a: use gpiod_multi_set_value_cansleep

[Jonathan Cameron]

On Thu, 06 Feb 2025 16:48:21 -0600
David Lechner <dlechner@baylibre.com> wrote:

> Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
> gpiod_set_array_value_cansleep().
> 
> Passing NULL as the 3rd argument to gpiod_set_array_value_cansleep()
> only needs to be done if the array was constructed manually, which is
> not the case here. This change effectively replaces that argument with
> st->gpios->array_info. The possible side effect of this change is that
> it could make setting the GPIOs more efficient.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
If it goes through the gpio tree
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/amplifiers/hmc425a.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> index 2ee4c0d70281e24c1c818249b86d89ebe06d4876..d9a359e1388a0f3eb5909bf668ff82102286542b 100644
> --- a/drivers/iio/amplifiers/hmc425a.c
> +++ b/drivers/iio/amplifiers/hmc425a.c
> @@ -161,8 +161,7 @@ static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
>  
>  	values[0] = value;
>  
> -	gpiod_set_array_value_cansleep(st->gpios->ndescs, st->gpios->desc,
> -				       NULL, values);
> +	gpiod_multi_set_value_cansleep(st->gpios, values);
>  	return 0;
>  }
>  
> 

Re: [PATCH v2 08/13] iio: resolver: ad2s1210: use gpiod_multi_set_value_cansleep

[Jonathan Cameron]

On Thu, 06 Feb 2025 16:48:22 -0600
David Lechner <dlechner@baylibre.com> wrote:

> Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
> gpiod_set_array_value().
> 
> These are not called in an atomic context, so changing to the cansleep
> variant is fine.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
If this goes through GPIO
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/resolver/ad2s1210.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
> index b681129a99b6cf399668bf01a1f5a15fbc4f95b8..7f18df790157f1e411fb70de193a49f0677c999f 100644
> --- a/drivers/iio/resolver/ad2s1210.c
> +++ b/drivers/iio/resolver/ad2s1210.c
> @@ -182,8 +182,7 @@ static int ad2s1210_set_mode(struct ad2s1210_state *st, enum ad2s1210_mode mode)
>  
>  	bitmap[0] = mode;
>  
> -	return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
> -				     bitmap);
> +	return gpiod_multi_set_value_cansleep(gpios, bitmap);
>  }
>  
>  /*
> @@ -1473,10 +1472,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
>  
>  		bitmap[0] = st->resolution;
>  
> -		ret = gpiod_set_array_value(resolution_gpios->ndescs,
> -					    resolution_gpios->desc,
> -					    resolution_gpios->info,
> -					    bitmap);
> +		ret = gpiod_multi_set_value_cansleep(resolution_gpios, bitmap);
>  		if (ret < 0)
>  			return dev_err_probe(dev, ret,
>  					     "failed to set resolution gpios\n");
> 

Re: [PATCH v2 06/13] iio: adc: ad7606: use gpiod_multi_set_value_cansleep

[Jonathan Cameron]

On Fri, 7 Feb 2025 14:13:23 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
> > gpiod_set_array_value().
> >
> > These are not called in an atomic context, so changing to the cansleep
> > variant is fine.  
> 
> ...
> 
> >         if (st->gpio_os) {
> > -               gpiod_set_array_value(st->gpio_os->ndescs,
> > -                                     st->gpio_os->desc, st->gpio_os->info, os);
> > +               gpiod_multi_set_value_cansleep(st->gpio_os, os);
> >         }  
> 
> Now I do not see any justification to leave {}.
> 
With that tidied up (good spot!) if this goes through the GPIO tree
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>