| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <21039309.e3MgZOjmZl@z50>
Date: Wed, 29 Aug 2018 20:01:01 +0200
From: Janusz Krzysztofik <jmkrzyszt@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Jonathan Corbet <corbet@....net>,
Peter Korsgaard <peter.korsgaard@...co.com>,
Peter Rosin <peda@...ntia.se>,
Ulf Hansson <ulf.hansson@...aro.org>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Dominik Brodowski <linux@...inikbrodowski.net>,
Kishon Vijay Abraham I <kishon@...com>,
Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>, linux-gpio@...r.kernel.org,
Linux Doc Mailing List <linux-doc@...r.kernel.org>,
linux-i2c@...r.kernel.org, linux-mmc@...r.kernel.org,
Network Development <netdev@...r.kernel.org>,
linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
linux-serial@...r.kernel.org,
linux-kernel <linux-kernel@...r.kernel.org>,
Willy Tarreau <w@....eu>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Janusz Krzysztofik <jmkrzyszt@...il.com>
Subject: Re: [RFC RFT PATCH v4 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
On Wednesday, August 29, 2018 2:03:18 PM CEST Miguel Ojeda wrote:
> Hi Janusz,
>
> On Tue, Aug 21, 2018 at 1:43 AM, Janusz Krzysztofik <jmkrzyszt@...il.com> 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@...il.com>
> > ---
> > Documentation/driver-api/gpio/consumer.rst | 22 ++++----
> > drivers/auxdisplay/hd44780.c | 52 +++++++++--------
>
> [CC'ing Willy and Geert for hd44780]
>
> > 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,
Yes, I tried to point out the fact the new API accepts a bitmap of an arbitrary
length, and I tried to use the same code pattern across changes to the API
users.
> but...
>
> > + 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... :-)
This one is an error, should be 1 of course :-), thanks.
> > + 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 >>=?
OK.
Answering you question below:
To make my changes as clear as I could imagine, I decided to use the same indexing as in the original code, i.e., assign high nibble of val to bits 4-7 and two other values - rs and an optional 0 - to bits 8 and 9, respectively.
Unlike in case of array of integers, where for the high nibble part you could just pass a pointer to a sub-array starting at the 5th value (i.e., &values[PIN_DATA4]), it was not possible to do the same for and arbitrary bit of a bitmap, e.g., pass a pointer to the 5th bit of *value_bitmap as an argument pointing to bit 0 of a bitmap to be processed. That's why I shifted the bitmap right by 4 bits.
Then, ...
> >
> > /* 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.
having the rs and optional 0 already assigned to bits 4 and 5 of the bitmap, I just cleared bits 0-3 still containing the high nibble of val and assigned the low nibble of it to those bits, getting a result ready to be passed as an argument to gpiod_set_array_value_cansleep() below.
I hope I didn't miss anything.
Thanks,
Janusz
> >
> > /* 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
>
Powered by blists - more mailing lists