diff mbox series

[v2,04/13] bus: ts-nbus: use gpiod_multi_set_value_cansleep

Message ID 20250206-gpio-set-array-helper-v2-4-1c5f048f79c3@baylibre.com
State New
Headers show
Series gpiolib: add gpiod_multi_set_value_cansleep | expand

Commit Message

David Lechner Feb. 6, 2025, 10:48 p.m. UTC 
Reduce verbosity by using gpiod_multi_set_value_cansleep() instead of
gpiod_set_array_value_cansleep().

ts_nbus->data->ndescs is validated to be 8 during probe, so will have
the same value as the hard-coded 8 that is removed by this change.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/bus/ts-nbus.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Andy Shevchenko Feb. 7, 2025, 12:17 p.m. UTC | #1 
+Yury.

On Fri, Feb 7, 2025 at 2:15 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:

...

> >  static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
> >  {
> > -       struct gpio_descs *gpios = ts_nbus->data;
> >         DECLARE_BITMAP(values, 8);
> >
> >         values[0] = byte;
> >
> > -       gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
> > +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);
>
> As I said before, this is buggy code on BE64. Needs to be fixed.

Or isn't? Do we have a test case in bitmap for such a case?

> >  }
Andy Shevchenko Feb. 7, 2025, 3:34 p.m. UTC | #2 
On Fri, Feb 07, 2025 at 09:23:56AM -0600, David Lechner wrote:
> On 2/7/25 6:17 AM, Andy Shevchenko wrote:
> > On Fri, Feb 7, 2025 at 2:15 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Fri, Feb 7, 2025 at 12:48 AM David Lechner <dlechner@baylibre.com> wrote:

...

> >>>  static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
> >>>  {
> >>> -       struct gpio_descs *gpios = ts_nbus->data;
> >>>         DECLARE_BITMAP(values, 8);
> >>>
> >>>         values[0] = byte;
> >>>
> >>> -       gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
> >>> +       gpiod_multi_set_value_cansleep(ts_nbus->data, values);
> >>
> >> As I said before, this is buggy code on BE64. Needs to be fixed.
> > 
> > Or isn't? Do we have a test case in bitmap for such a case?
> > 
> >>>  }
> 
> Maybe not the best style, but I don't think it is buggy. Bitmaps are always
> handled in long-sized chunks and not cast to bytes so endianness doesn't affect
> it. I didn't see an explicit test, but bitmap_read() and bitmap_write() use
> array access like this so indirectly it is being tested.

Right, the potential (and likely theoretical) issue may come if DEFINE_BITMAP()
out of a sudden will use, let's say, an array of booleans. But it wasn't my
initial complain about the code :-)
diff mbox series

Patch

diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
index d3ee102a13893c83c50e41f7298821f4d7ae3487..b4c9308caf0647a3261071d9527fffce77784af2 100644
--- a/drivers/bus/ts-nbus.c
+++ b/drivers/bus/ts-nbus.c
@@ -109,8 +109,7 @@  static void ts_nbus_reset_bus(struct ts_nbus *ts_nbus)
 
 	values[0] = 0;
 
-	gpiod_set_array_value_cansleep(8, ts_nbus->data->desc,
-				       ts_nbus->data->info, values);
+	gpiod_multi_set_value_cansleep(ts_nbus->data, values);
 	gpiod_set_value_cansleep(ts_nbus->csn, 0);
 	gpiod_set_value_cansleep(ts_nbus->strobe, 0);
 	gpiod_set_value_cansleep(ts_nbus->ale, 0);
@@ -150,12 +149,11 @@  static int ts_nbus_read_byte(struct ts_nbus *ts_nbus, u8 *val)
  */
 static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
 {
-	struct gpio_descs *gpios = ts_nbus->data;
 	DECLARE_BITMAP(values, 8);
 
 	values[0] = byte;
 
-	gpiod_set_array_value_cansleep(8, gpios->desc, gpios->info, values);
+	gpiod_multi_set_value_cansleep(ts_nbus->data, values);
 }
 
 /*