Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
Hi Miguel,
On Thursday, August 30, 2018 1:10:59 PM CEST Miguel Ojeda wrote:
> Hi Janusz,
>
> On Wed, Aug 29, 2018 at 10:48 PM, Janusz Krzysztofik
> wrote:
> > ...
> > /* High nibble + RS, RW */
> > - for (i = 4; i < 8; i++)
> > - values[PIN_DATA0 + i] = !!(val & BIT(i));
> > - values[PIN_CTRL_RS] = rs;
> > + value_bitmap[0] = val;
> > + __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> > n = 5;
> > if (hd->pins[PIN_CTRL_RW]) {
> > - values[PIN_CTRL_RW] = 0;
> > + __clear_bit(PIN_CTRL_RW, value_bitmap);
> > n++;
> > }
> > + value_bitmap[0] >>= PIN_DATA4;
> >
> > /* Present the data to the port */
> > - gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4],
> > - [PIN_DATA4]);
> > + gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4],
value_bitmap);
> >
> > hd44780_strobe_gpio(hd);
> >
> > /* Low nibble */
> > - for (i = 0; i < 4; i++)
> > - values[PIN_DATA4 + i] = !!(val & BIT(i));
> > + value_bitmap[0] &= ~((1 << PIN_DATA4) - 1);
> > + value_bitmap[0] |= val & ~((1 << PIN_DATA4) - 1);
>
> This is still wrong! What I originally meant in my v4 review is that
> there is an extra ~ in the second line.
Indeed, that's wrong, I missed your original point, sorry.
> Also, a couple of general comments:
>
> - Please review the list of CCs (I was not CC'd originally, so maybe
> there are other maintainers that aren't, either)
That's probably because early versions of the series, prior to v4, were not
touching existing GPIO API so there were no changes to users of gpiod_get/
set_array_value() and their variants. From v4 on, you are in the loop so don't
worry, you haven't missed anything.
But anyway, thanks for your suggestion to review the Cc; list, I've done that
for v7 and added still a few people who contributed most to the code being
changed.
> - In general, the new code seems harder to read than the original one
> (but that is my impression).
I hope we are slowly approaching acceptable readability in recent iterations.
Thanks,
Janusz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
On Wed, Aug 29, 2018 at 10:48 PM Janusz Krzysztofik wrote:
So it's no secret that I strongly fancy this patch set.
What would be great at this point is to have some people test
that the drivers still work as expected, even better if they can do
some benchmarking.
> - values[PIN_DATA0 + i] = !!(val & BIT(i));
> - values[PIN_CTRL_RS] = rs;
> + value_bitmap[0] = val;
> + __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> n = 9;
> if (hd->pins[PIN_CTRL_RW]) {
> - values[PIN_CTRL_RW] = 0;
> + __clear_bit(PIN_CTRL_RW, value_bitmap);
This seems fine to me, but I understand the comment that the
code becomes harder to read.
I think part of it is those __assign_bit() and __clear_bit() with
the double-underscore of unclear meaning. The meaning is
"non atomic" IIRC, so maybe I should move forward
with my plan to send a sed script to Torvalds just renaming all
of those to something sane in the next merge window.
Like __assign_bit() -> assign_bit_nonatomic()
Yours,
Linus Walleij
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
From: Miguel Ojeda > Sent: 30 August 2018 12:11 ... > > + unsigned long value_bitmap[1]; /* for DATA[0-7], RS, RW */ > > (I read your comments in the other email) > > I still find this odd, but if everyone is going to have this change > done like this, consistency is better. Maybe there ought to be a define so you can do: DEFINE_BITMAP(value_bitmap, 32); While it might just generate an unsigned long [] there is probably scope for stronger typing. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
Hi Janusz, On Wed, Aug 29, 2018 at 10:48 PM, Janusz Krzysztofik wrote: > Most users of get/set array functions iterate consecutive bits of data, > usually a single integer, while processing array of results obtained > from, or building an array of values to be passed to those functions. > Save time wasted on those iterations by changing the functions' API to > accept bitmaps. > > All current users are updated as well. > > More benefits from the change are expected as soon as planned support > for accepting/passing those bitmaps directly from/to respective GPIO > chip callbacks if applicable is implemented. > > Cc: Jonathan Corbet > Cc: Miguel Ojeda Sandonis > Cc: Peter Korsgaard > Cc: Peter Rosin > Cc: Andrew Lunn > Cc: Florian Fainelli > Cc: "David S. Miller" > Cc: Dominik Brodowski > Cc: Kishon Vijay Abraham I > Cc: Lars-Peter Clausen > Cc: Michael Hennerich > Cc: Jonathan Cameron > Cc: Hartmut Knaack > Cc: Peter Meerwald-Stadler > Cc: Greg Kroah-Hartman > Cc: Jiri Slaby > Signed-off-by: Janusz Krzysztofik > Acked-by: Ulf Hansson > --- > Documentation/driver-api/gpio/consumer.rst | 22 > drivers/auxdisplay/hd44780.c| 52 + > drivers/bus/ts-nbus.c | 19 ++- > drivers/gpio/gpio-max3191x.c| 17 +++--- > drivers/gpio/gpiolib.c | 86 > +++-- > drivers/gpio/gpiolib.h | 4 +- > drivers/i2c/muxes/i2c-mux-gpio.c| 8 +-- > drivers/mmc/core/pwrseq_simple.c| 13 ++--- > drivers/mux/gpio.c | 9 +-- > drivers/net/phy/mdio-mux-gpio.c | 3 +- > drivers/pcmcia/soc_common.c | 11 ++-- > drivers/phy/motorola/phy-mapphone-mdm6600.c | 17 +++--- > drivers/staging/iio/adc/ad7606.c| 9 +-- > drivers/tty/serial/serial_mctrl_gpio.c | 7 ++- > include/linux/gpio/consumer.h | 18 +++--- > 15 files changed, 140 insertions(+), 155 deletions(-) > > diff --git a/Documentation/driver-api/gpio/consumer.rst > b/Documentation/driver-api/gpio/consumer.rst > index aa03f389d41d..ed68042ddccf 100644 > --- a/Documentation/driver-api/gpio/consumer.rst > +++ b/Documentation/driver-api/gpio/consumer.rst > @@ -323,29 +323,29 @@ The following functions get or set the values of an > array of GPIOs:: > > int gpiod_get_array_value(unsigned int array_size, > struct gpio_desc **desc_array, > - int *value_array); > + unsigned long *value_bitmap); > int gpiod_get_raw_array_value(unsigned int array_size, > struct gpio_desc **desc_array, > - int *value_array); > + unsigned long *value_bitmap); > int gpiod_get_array_value_cansleep(unsigned int array_size, >struct gpio_desc **desc_array, > - int *value_array); > + unsigned long *value_bitmap); > int gpiod_get_raw_array_value_cansleep(unsigned int array_size, >struct gpio_desc **desc_array, > - int *value_array); > + unsigned long *value_bitmap); > > void gpiod_set_array_value(unsigned int array_size, >struct gpio_desc **desc_array, > - int *value_array) > + unsigned long *value_bitmap) > void gpiod_set_raw_array_value(unsigned int array_size, >struct gpio_desc **desc_array, > - int *value_array) > + unsigned long *value_bitmap) > void gpiod_set_array_value_cansleep(unsigned int array_size, > struct gpio_desc **desc_array, > - int *value_array) > + unsigned long *value_bitmap) > void gpiod_set_raw_array_value_cansleep(unsigned int array_size, > struct gpio_desc **desc_array, > - int *value_array) > + unsigned long *value_bitmap) > > The array can be an arbitrary set of GPIOs. The functions will try to access > GPIOs belonging to the same bank or chip simultaneously if supported by the > @@ -356,8 +356,8 @@ accessed sequentially. > The functions take three arguments: > * array_size- the number of array elements > * desc_array- an array of GPIO descriptors > - * value_array - an array to store the GPIOs'
Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
Hi Janusz,
On Wed, Aug 29, 2018 at 10:48 PM Janusz Krzysztofik wrote:
> Most users of get/set array functions iterate consecutive bits of data,
> usually a single integer, while processing array of results obtained
> from, or building an array of values to be passed to those functions.
> Save time wasted on those iterations by changing the functions' API to
> accept bitmaps.
>
> All current users are updated as well.
>
> More benefits from the change are expected as soon as planned support
> for accepting/passing those bitmaps directly from/to respective GPIO
> chip callbacks if applicable is implemented.
>
> Cc: Jonathan Corbet
> Cc: Miguel Ojeda Sandonis
> Cc: Peter Korsgaard
> Cc: Peter Rosin
> Cc: Andrew Lunn
> Cc: Florian Fainelli
> Cc: "David S. Miller"
> Cc: Dominik Brodowski
> Cc: Kishon Vijay Abraham I
> Cc: Lars-Peter Clausen
> Cc: Michael Hennerich
> Cc: Jonathan Cameron
> Cc: Hartmut Knaack
> Cc: Peter Meerwald-Stadler
> Cc: Greg Kroah-Hartman
> Cc: Jiri Slaby
> Signed-off-by: Janusz Krzysztofik
> Acked-by: Ulf Hansson
Thanks for your patch!
> --- a/drivers/auxdisplay/hd44780.c
> +++ b/drivers/auxdisplay/hd44780.c
> @@ -62,20 +62,19 @@ static void hd44780_strobe_gpio(struct hd44780 *hd)
> /* write to an LCD panel register in 8 bit GPIO mode */
> static void hd44780_write_gpio8(struct hd44780 *hd, u8 val, unsigned int rs)
> {
> - int values[10]; /* for DATA[0-7], RS, RW */
> - unsigned int i, n;
> + unsigned long value_bitmap[1]; /* for DATA[0-7], RS, RW */
> + unsigned int n;
>
> - for (i = 0; i < 8; i++)
> - values[PIN_DATA0 + i] = !!(val & BIT(i));
> - values[PIN_CTRL_RS] = rs;
> + value_bitmap[0] = val;
> + __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> n = 9;
> if (hd->pins[PIN_CTRL_RW]) {
> - values[PIN_CTRL_RW] = 0;
> + __clear_bit(PIN_CTRL_RW, value_bitmap);
The clearing is not needed, as this has been done by 'value_bitmap[0] = val;'
> n++;
> }
So the above block can be simplified to:
n = hd->pins[PIN_CTRL_RW] ? 10 : 9;
>
> /* Present the data to the port */
> - gpiod_set_array_value_cansleep(n, >pins[PIN_DATA0], values);
> + gpiod_set_array_value_cansleep(n, >pins[PIN_DATA0], value_bitmap);
>
> hd44780_strobe_gpio(hd);
> }
> @@ -83,32 +82,31 @@ static void hd44780_write_gpio8(struct hd44780 *hd, u8
> val, unsigned int rs)
> /* write to an LCD panel register in 4 bit GPIO mode */
> static void hd44780_write_gpio4(struct hd44780 *hd, u8 val, unsigned int rs)
> {
> - int values[10]; /* for DATA[0-7], RS, RW, but DATA[0-3] is unused */
> - unsigned int i, n;
> + /* for DATA[0-7], RS, RW, but DATA[0-3] is unused */
This comment is not correct, as the low bits will be used.
/* DATA[4-7], RS, RW */
> + unsigned long value_bitmap[1];
> + unsigned int n;
>
> /* High nibble + RS, RW */
> - for (i = 4; i < 8; i++)
> - values[PIN_DATA0 + i] = !!(val & BIT(i));
> - values[PIN_CTRL_RS] = rs;
> + value_bitmap[0] = val;
> + __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> n = 5;
> if (hd->pins[PIN_CTRL_RW]) {
> - values[PIN_CTRL_RW] = 0;
> + __clear_bit(PIN_CTRL_RW, value_bitmap);
Not needed.
> n++;
> }
Hence:
n = hd->pins[PIN_CTRL_RW] ? 6: 5;
> + value_bitmap[0] >>= PIN_DATA4;
Yuck?!?
Isn't it more readable to just do:
/* High nibble + RS, RW */
value_bitmap[0] = val >> 4;
__assign_bit(4, value_bitmap, rs);
n = hd->pins[PIN_CTRL_RW] ? 6: 5;
>
> /* Present the data to the port */
> - gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4],
> - [PIN_DATA4]);
> + gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4], value_bitmap);
>
> hd44780_strobe_gpio(hd);
>
> /* Low nibble */
> - for (i = 0; i < 4; i++)
> - values[PIN_DATA4 + i] = !!(val & BIT(i));
> + value_bitmap[0] &= ~((1 << PIN_DATA4) - 1);
> + value_bitmap[0] |= val & ~((1 << PIN_DATA4) - 1);
... and:
/* Low nibble */
value_bitmap[0] &= ~0x0f;
value_bitmap[0] |= val & 0x0f;
>
> /* Present the data to the port */
> - gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4],
> - [PIN_DATA4]);
> + gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4], value_bitmap);
>
> hd44780_strobe_gpio(hd);
> }
> @@ -155,23 +153,23 @@ static void hd44780_write_cmd_gpio4(struct charlcd
> *lcd, int cmd)
> /* Send 4-bits of a command to the LCD panel in raw 4 bit GPIO mode */
> static void hd44780_write_cmd_raw_gpio4(struct charlcd *lcd, int cmd)
> {
> - int values[10]; /* for DATA[0-7], RS, RW, but DATA[0-3] is unused */
> + /* for DATA[0-7], RS, RW, but
Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
On 2018-08-29 22:48, Janusz Krzysztofik wrote:
> Most users of get/set array functions iterate consecutive bits of data,
> usually a single integer, while processing array of results obtained
> from, or building an array of values to be passed to those functions.
> Save time wasted on those iterations by changing the functions' API to
> accept bitmaps.
>
> All current users are updated as well.
>
> More benefits from the change are expected as soon as planned support
> for accepting/passing those bitmaps directly from/to respective GPIO
> chip callbacks if applicable is implemented.
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c
> b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 401308e3d036..4e36e0eac7a3 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -22,18 +22,16 @@ struct gpiomux {
> struct i2c_mux_gpio_platform_data data;
> unsigned gpio_base;
> struct gpio_desc **gpios;
> - int *values;
> + int *values; /* FIXME: no longer needed */
That's a half-measure...
> };
>
> static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
> {
> + unsigned long value_bitmap[1] = { val, };
> int i;
...and i is no longer needed. Hmm, I'd expect a warning about that?
>
> - for (i = 0; i < mux->data.n_gpios; i++)
> - mux->values[i] = (val >> i) & 1;
> -
> gpiod_set_array_value_cansleep(mux->data.n_gpios,
> -mux->gpios, mux->values);
> +mux->gpios, value_bitmap);
> }
>
> static int i2c_mux_gpio_select(struct i2c_mux_core *muxc, u32 chan)
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index 6fdd9316db8b..734e1b43aed6 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -17,20 +17,17 @@
>
> struct mux_gpio {
> struct gpio_descs *gpios;
> - int *val;
> + int *val; /* FIXME: no longer needed */
> };
>
> static int mux_gpio_set(struct mux_control *mux, int state)
> {
> struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> + unsigned long value_bitmap[1] = { state, };
> int i;
>
> - for (i = 0; i < mux_gpio->gpios->ndescs; i++)
> - mux_gpio->val[i] = (state >> i) & 1;
> -
> gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> -mux_gpio->gpios->desc,
> -mux_gpio->val);
> +mux_gpio->gpios->desc, value_bitmap);
>
> return 0;
> }
Dito for this driver. Just squash the following and be done with
it, no attribution needed...
With that (for the changes in i2c-mux-gpio.c and mux/gpio.c)
Reviewed-by: Peter Rosin
Linus, I expect this will will end up in some immutable branch for me
to pick up, in case I need to? Not that I expect further work to clash
in these two drivers, but...
Cheers,
Peter
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 4e36e0eac7a3..06a89a29250a 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -22,13 +22,11 @@ struct gpiomux {
struct i2c_mux_gpio_platform_data data;
unsigned gpio_base;
struct gpio_desc **gpios;
- int *values; /* FIXME: no longer needed */
};
static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
{
unsigned long value_bitmap[1] = { val, };
- int i;
gpiod_set_array_value_cansleep(mux->data.n_gpios,
mux->gpios, value_bitmap);
@@ -180,15 +178,13 @@ static int i2c_mux_gpio_probe(struct platform_device
*pdev)
return -EPROBE_DEFER;
muxc = i2c_mux_alloc(parent, >dev, mux->data.n_values,
-mux->data.n_gpios * sizeof(*mux->gpios) +
-mux->data.n_gpios * sizeof(*mux->values), 0,
+mux->data.n_gpios * sizeof(*mux->gpios), 0,
i2c_mux_gpio_select, NULL);
if (!muxc) {
ret = -ENOMEM;
goto alloc_failed;
}
mux->gpios = muxc->priv;
- mux->values = (int *)(mux->gpios + mux->data.n_gpios);
muxc->priv = mux;
platform_set_drvdata(pdev, muxc);
diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
index 734e1b43aed6..eb1798677dfd 100644
--- a/drivers/mux/gpio.c
+++ b/drivers/mux/gpio.c
@@ -17,14 +17,12 @@
struct mux_gpio {
struct gpio_descs *gpios;
- int *val; /* FIXME: no longer needed */
};
static int mux_gpio_set(struct mux_control *mux, int state)
{
struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
unsigned long value_bitmap[1] = { state, };
- int i;
gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
mux_gpio->gpios->desc, value_bitmap);
@@ -55,13 +53,11 @@ static int mux_gpio_probe(struct platform_device *pdev)
