Hi Janusz,

On Wed, Aug 29, 2018 at 10:48 PM Janusz Krzysztofik [off-list ref] 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 <corbet@lwn.net>
Cc: Miguel Ojeda Sandonis <redacted>
Cc: Peter Korsgaard <peter.korsgaard@barco.com>
Cc: Peter Rosin <redacted>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Kishon Vijay Abraham I <redacted>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <redacted>
Cc: Peter Meerwald-Stadler <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <redacted>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Acked-by: Ulf Hansson <redacted>

Thanks for your patch!

        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;

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, &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);

... and:

         /* Low nibble */
        value_bitmap[0] &= ~0x0f;
        value_bitmap[0] |= val & 0x0f;

This comment is not correct, as the low bits will be used.

        /* DATA[4-7], RS, RW */

+       unsigned long value_bitmap[1];
        struct hd44780 *hd = lcd->drvdata;
-       unsigned int i, n;
+       unsigned int n;

        /* Command nibble + RS, RW */
-       for (i = 0; i < 4; i++)
-               values[PIN_DATA4 + i] = !!(cmd & BIT(i));
-       values[PIN_CTRL_RS] = 0;
+       value_bitmap[0] = cmd << PIN_DATA4;
+       __clear_bit(PIN_CTRL_RS, value_bitmap);

Implied by the assignment above.

        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;

Hence:

        /* Command nibble + RS, RW */
        value_bitmap[0] = cmd;
        n = hd->pins[PIN_CTRL_RW] ? 6: 5;

        /* 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);
 }

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