| Message ID | 20250206-gpio-set-array-helper-v2-0-1c5f048f79c3@baylibre.com |
|---|---|
| Headers | show |
| Series | gpiolib: add gpiod_multi_set_value_cansleep | expand |
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
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.
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 >
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,