Re: [RFC RFT PATCH v4 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
From: Miguel Ojeda <hidden>
Date: 2018-08-29 12:03:43
Also in:
linux-doc, linux-gpio, linux-i2c, linux-mmc, linux-serial, lkml
Hi Janusz, On Tue, Aug 21, 2018 at 1:43 AM, Janusz Krzysztofik [off-list ref] wrote:
Most users of get/set array functions iterate consecutive bits of data, usually a single integer, while or 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. Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- Documentation/driver-api/gpio/consumer.rst | 22 ++++---- drivers/auxdisplay/hd44780.c | 52 +++++++++--------
[CC'ing Willy and Geert for hd44780]
quoted hunk ↗ jump to hunk
diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c index f1a42f0f1ded..d340473aa142 100644 --- 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 */
Why [1]? I understand it is because in other cases it may be more than one, but...
quoted hunk ↗ jump to hunk
+ 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); n++; } /* Present the data to the port */ - gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA0], values); + gpiod_set_array_value_cansleep(n, &hd->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 */ + unsigned long value_bitmap[0];
This one is even more strange... :-)
+ 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);
n++;
}
+ value_bitmap[0] = value_bitmap[0] >> PIN_DATA4;Maybe >>=?
/* Present the data to the port */
- gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA4],
- &values[PIN_DATA4]);
+ gpiod_set_array_value_cansleep(n, &hd->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);Are you sure this is correct? You are basically doing an or of value_bitmap and val and clearing the low-nibble.
/* Present the data to the port */
- gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA4],
- &values[PIN_DATA4]);
+ gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA4], value_bitmap);
hd44780_strobe_gpio(hd);
}Cheers, Miguel