From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 91443C433F5 for ; Thu, 30 Aug 2018 11:11:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1E8522054F for ; Thu, 30 Aug 2018 11:11:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="XTnoGAeR" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1E8522054F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728549AbeH3PM6 (ORCPT ); Thu, 30 Aug 2018 11:12:58 -0400 Received: from mail-qk0-f196.google.com ([209.85.220.196]:40660 "EHLO mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728261AbeH3PM6 (ORCPT ); Thu, 30 Aug 2018 11:12:58 -0400 Received: by mail-qk0-f196.google.com with SMTP id c126-v6so5423456qkd.7; Thu, 30 Aug 2018 04:11:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=QqMP2AgNUNF9Xqp4Y8iFAHR2fQLutX74w+JY89RdMS8=; b=XTnoGAeRi2K79tI1NGY69H2PgZ+gAfOWrlVjHs51H6LH/Btr7XKat7Dd03fvWyxOtt poxuUq+rgM4s2VQ9OKwSAvJI7kGNB4bJKsUcsMOFdSNARthKPY9tmxK/gBe8BmCzAWgm apXwfd9gEem6P/c3PNm/AY3o8iL6yVylG9LYm9rBmDJhzZnIxNtH7RqtrFS3aDYWCy2F Z09YRkQmQTDIa7Ve1w+JoAT+W3Zo9Fpjo7CFWPAu0K8m1QBMjmQtknML7or0+iq0ujug v5d0WT4FVDbcUbhCA0J3Flh17afSygSqt4XUCkhwRBh8fAspeg+A4ZegYfnszqs9s50C 2pTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=QqMP2AgNUNF9Xqp4Y8iFAHR2fQLutX74w+JY89RdMS8=; b=JlNSKHXl1MhMl/I6Ip8tIb+3Yv5cqUGB5WK8ICx7u5cjj+6Ck5zCregzODDsl2DAPS OTj/2HE3Ex/VrKV1tdI67v6NU9V25avZl7nQMlws58JFHa+Dbq4lCFVweurRy9/F/RyG gOBfJyPX3EwDVwRp8/0qlppKiqS8TvYLbN8SWUbCpgdhciQtBtF8ElZEPnrQYjj/7M/C dnAL9Eea2CHz3anpLZ4DB37LmDe17xR8CUyqoFXl48il3UEBWJ96PX2gcSVYharf32bZ awI3qByJvIJcjT9q1cBNzQOi+ayKyUaNca7hMByHyFpSwia10HdJuuovznERHYjHM8QX 9a3w== X-Gm-Message-State: APzg51CvJojHm+fiNxLSonEOYKRmq6GRok3IgRK98A1t/z9jS4IEbK01 3XEph00HT3gBHxZC14LyhqK2WTMBuRBtlBh9T81GQv0j X-Google-Smtp-Source: ANB0VdYhvjmXHn33TdJvXnR0yPuked4L0CwQ3JJDYot5N+dPpAseETvPROzNYxHQcYXefCr2XACYw5+uku+bV6cgmsw= X-Received: by 2002:a37:1204:: with SMTP id c4-v6mr10395060qkh.63.1535627479820; Thu, 30 Aug 2018 04:11:19 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac8:471a:0:0:0:0:0 with HTTP; Thu, 30 Aug 2018 04:10:59 -0700 (PDT) In-Reply-To: <20180829204900.19390-2-jmkrzyszt@gmail.com> References: <20180820234341.5271-1-jmkrzyszt@gmail.com> <20180829204900.19390-1-jmkrzyszt@gmail.com> <20180829204900.19390-2-jmkrzyszt@gmail.com> From: Miguel Ojeda Date: Thu, 30 Aug 2018 13:10:59 +0200 Message-ID: Subject: Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array To: Janusz Krzysztofik Cc: Linus Walleij , Jonathan Corbet , Peter Korsgaard , Peter Rosin , Ulf Hansson , Andrew Lunn , Florian Fainelli , "David S. Miller" , Dominik Brodowski , Greg Kroah-Hartman , Kishon Vijay Abraham I , Lars-Peter Clausen , Michael Hennerich , Jonathan Cameron , Hartmut Knaack , Peter Meerwald-Stadler , Jiri Slaby , Willy Tarreau , Geert Uytterhoeven , Linux Doc Mailing List , linux-i2c@vger.kernel.org, linux-mmc@vger.kernel.org, Network Development , linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, linux-serial@vger.kernel.org, linux-gpio@vger.kernel.org, linux-kernel Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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' values (get) or > - an array of values to assign to the GPIOs (set) > + * value_bitmap - a bitmap to store the GPIOs' values (get) or > + a bitmap of values to assign to the GPIOs (set) > > The descriptor array can be obtained using the gpiod_get_array() function > or one of its variants. If the group of descriptors returned by that function > @@ -366,7 +366,7 @@ the struct gpio_descs returned by gpiod_get_array():: > > struct gpio_descs *my_gpio_descs = gpiod_get_array(...); > gpiod_set_array_value(my_gpio_descs->ndescs, my_gpio_descs->desc, > - my_gpio_values); > + my_gpio_value_bitmap); > > It is also possible to access a completely arbitrary array of descriptors. The > descriptors may be obtained using any combination of gpiod_get() and > diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c > index f1a42f0f1ded..bbbd6a29bf01 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 */ (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. > + 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[1]; Ditto. > + 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] >>= PIN_DATA4; > > /* 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); This is still wrong! What I originally meant in my v4 review is that there is an extra ~ in the second line. 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) - In general, the new code seems harder to read than the original one (but that is my impression). Thanks for your effort! :-) Cheers, Miguel