[PATCH v2] staging: gpib: simplify and fix get_data_lines

[PATCH v2] staging: gpib: simplify and fix get_data_lines

[Osama Abdelkader]

The function `get_data_lines()` in gpib_bitbang.c currently reads 8
GPIO descriptors individually and combines them into a byte.
This has two issues:

  * `gpiod_get_value()` returns an `int` which may be negative on
    error. Assigning it directly into a `u8` may propagate unexpected
    values. Masking ensures only the LSB is used.
  * The code is repetitive and harder to extend.

Fix this by introducing a local array of GPIO descriptors and looping
over them, while masking the return value to its least significant bit.

This reduces duplication, makes the code more maintainable, and avoids
possible data corruption from negative `gpiod_get_value()` returns.

Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
---
v2:
Just print the gpio pin error and leave the bit as zero
---
 drivers/staging/gpib/gpio/gpib_bitbang.c | 28 ++++++++++++++----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
index 17884810fd69..f4ca59c007dd 100644
--- a/drivers/staging/gpib/gpio/gpib_bitbang.c
+++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
@@ -1403,17 +1403,23 @@ static void set_data_lines(u8 byte)
 
 static u8 get_data_lines(void)
 {
-	u8 ret;
-
-	ret = gpiod_get_value(D01);
-	ret |= gpiod_get_value(D02) << 1;
-	ret |= gpiod_get_value(D03) << 2;
-	ret |= gpiod_get_value(D04) << 3;
-	ret |= gpiod_get_value(D05) << 4;
-	ret |= gpiod_get_value(D06) << 5;
-	ret |= gpiod_get_value(D07) << 6;
-	ret |= gpiod_get_value(D08) << 7;
-	return ~ret;
+	struct gpio_desc *lines[8] = {
+		D01, D02, D03, D04, D05, D06, D07, D08
+	};
+
+	u8 val = 0;
+	int ret, i;
+
+	for (i = 0; i < 8; i++) {
+		ret = gpiod_get_value(lines[i]);
+		if (ret < 0) {
+			pr_err("get GPIO pin %d error: %d\n", i, ret);
+			continue;
+		}
+		val |= (ret & 1) << i;
+	}
+
+	return ~val;
 }
 
 static void set_data_lines_input(void)
-- 
2.43.0

Re: [PATCH v2] staging: gpib: simplify and fix get_data_lines

[Dan Carpenter]

On Wed, Aug 27, 2025 at 01:38:57PM +0200, Osama Abdelkader wrote:
> The function `get_data_lines()` in gpib_bitbang.c currently reads 8
> GPIO descriptors individually and combines them into a byte.
> This has two issues:
> 
>   * `gpiod_get_value()` returns an `int` which may be negative on
>     error. Assigning it directly into a `u8` may propagate unexpected
>     values. Masking ensures only the LSB is used.

This part isn't really true any more.

>   * The code is repetitive and harder to extend.
> 
> Fix this by introducing a local array of GPIO descriptors and looping
> over them, while masking the return value to its least significant bit.

There really isn't any need to mask now that we're checking for
negatives.

> 
> This reduces duplication, makes the code more maintainable, and avoids
> possible data corruption from negative `gpiod_get_value()` returns.
> 
> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> ---
> v2:
> Just print the gpio pin error and leave the bit as zero
> ---
>  drivers/staging/gpib/gpio/gpib_bitbang.c | 28 ++++++++++++++----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
> index 17884810fd69..f4ca59c007dd 100644
> --- a/drivers/staging/gpib/gpio/gpib_bitbang.c
> +++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
> @@ -1403,17 +1403,23 @@ static void set_data_lines(u8 byte)
>  
>  static u8 get_data_lines(void)
>  {
> -	u8 ret;
> -
> -	ret = gpiod_get_value(D01);
> -	ret |= gpiod_get_value(D02) << 1;
> -	ret |= gpiod_get_value(D03) << 2;
> -	ret |= gpiod_get_value(D04) << 3;
> -	ret |= gpiod_get_value(D05) << 4;
> -	ret |= gpiod_get_value(D06) << 5;
> -	ret |= gpiod_get_value(D07) << 6;
> -	ret |= gpiod_get_value(D08) << 7;
> -	return ~ret;
> +	struct gpio_desc *lines[8] = {
> +		D01, D02, D03, D04, D05, D06, D07, D08
> +	};
> +

Delete this blank line.

> +	u8 val = 0;
> +	int ret, i;
> +
> +	for (i = 0; i < 8; i++) {
> +		ret = gpiod_get_value(lines[i]);
> +		if (ret < 0) {
> +			pr_err("get GPIO pin %d error: %d\n", i, ret);
> +			continue;
> +		}
> +		val |= (ret & 1) << i;

Delete the mask.

(I wavered on whether I should comment on the nit picky things I've
said in this email, but in the end it was the out of date commit
message which pushed me over the edge.  I would have ignored the
other things otherwise).

regards,
dan carpenter

Re: [PATCH v2] staging: gpib: simplify and fix get_data_lines

[Dave Penkler]

On Wed, Aug 27, 2025 at 03:16:28PM +0300, Dan Carpenter wrote:
> On Wed, Aug 27, 2025 at 01:38:57PM +0200, Osama Abdelkader wrote:
> > The function `get_data_lines()` in gpib_bitbang.c currently reads 8
> > GPIO descriptors individually and combines them into a byte.
> > This has two issues:
> > 
> >   * `gpiod_get_value()` returns an `int` which may be negative on
> >     error. Assigning it directly into a `u8` may propagate unexpected
> >     values. Masking ensures only the LSB is used.
> 
> This part isn't really true any more.
> 
> >   * The code is repetitive and harder to extend.
> > 
> > Fix this by introducing a local array of GPIO descriptors and looping
> > over them, while masking the return value to its least significant bit.
> 
> There really isn't any need to mask now that we're checking for
> negatives.
> 
> > 
> > This reduces duplication, makes the code more maintainable, and avoids
> > possible data corruption from negative `gpiod_get_value()` returns.
> > 
> > Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> > ---
> > v2:
> > Just print the gpio pin error and leave the bit as zero
> > ---
> >  drivers/staging/gpib/gpio/gpib_bitbang.c | 28 ++++++++++++++----------
> >  1 file changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
> > index 17884810fd69..f4ca59c007dd 100644
> > --- a/drivers/staging/gpib/gpio/gpib_bitbang.c
> > +++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
> > @@ -1403,17 +1403,23 @@ static void set_data_lines(u8 byte)
> >  
> >  static u8 get_data_lines(void)
> >  {
> > -	u8 ret;
> > -
> > -	ret = gpiod_get_value(D01);
> > -	ret |= gpiod_get_value(D02) << 1;
> > -	ret |= gpiod_get_value(D03) << 2;
> > -	ret |= gpiod_get_value(D04) << 3;
> > -	ret |= gpiod_get_value(D05) << 4;
> > -	ret |= gpiod_get_value(D06) << 5;
> > -	ret |= gpiod_get_value(D07) << 6;
> > -	ret |= gpiod_get_value(D08) << 7;
> > -	return ~ret;
> > +	struct gpio_desc *lines[8] = {
> > +		D01, D02, D03, D04, D05, D06, D07, D08
> > +	};
> > +
> 
> Delete this blank line.
> 
> > +	u8 val = 0;
> > +	int ret, i;
> > +
> > +	for (i = 0; i < 8; i++) {
> > +		ret = gpiod_get_value(lines[i]);
> > +		if (ret < 0) {
> > +			pr_err("get GPIO pin %d error: %d\n", i, ret);
> > +			continue;
> > +		}
> > +		val |= (ret & 1) << i;
> 
> Delete the mask.
> 
> (I wavered on whether I should comment on the nit picky things I've
> said in this email, but in the end it was the out of date commit
> message which pushed me over the edge.  I would have ignored the
> other things otherwise).
> 
> regards,
> dan carpenter
> 
> 
This patch seems unnecessary.
The code will never be extended.
In the unlikely case of errors it will produce a huge streams of console spam.
It negatively impacts performance:  114209 bytes/sec vs 118274 bytes/sec.
regards,
-Dave

Re: [PATCH v2] staging: gpib: simplify and fix get_data_lines

[Osama Abdelkader]

On 8/27/25 4:11 PM, Dave Penkler wrote:
> On Wed, Aug 27, 2025 at 03:16:28PM +0300, Dan Carpenter wrote:
>> On Wed, Aug 27, 2025 at 01:38:57PM +0200, Osama Abdelkader wrote:
>>> The function `get_data_lines()` in gpib_bitbang.c currently reads 8
>>> GPIO descriptors individually and combines them into a byte.
>>> This has two issues:
>>>
>>>   * `gpiod_get_value()` returns an `int` which may be negative on
>>>     error. Assigning it directly into a `u8` may propagate unexpected
>>>     values. Masking ensures only the LSB is used.
>> This part isn't really true any more.
>>
>>>   * The code is repetitive and harder to extend.
>>>
>>> Fix this by introducing a local array of GPIO descriptors and looping
>>> over them, while masking the return value to its least significant bit.
>> There really isn't any need to mask now that we're checking for
>> negatives.
>>
>>> This reduces duplication, makes the code more maintainable, and avoids
>>> possible data corruption from negative `gpiod_get_value()` returns.
>>>
>>> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
>>> ---
>>> v2:
>>> Just print the gpio pin error and leave the bit as zero
>>> ---
>>>  drivers/staging/gpib/gpio/gpib_bitbang.c | 28 ++++++++++++++----------
>>>  1 file changed, 17 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
>>> index 17884810fd69..f4ca59c007dd 100644
>>> --- a/drivers/staging/gpib/gpio/gpib_bitbang.c
>>> +++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
>>> @@ -1403,17 +1403,23 @@ static void set_data_lines(u8 byte)
>>>  
>>>  static u8 get_data_lines(void)
>>>  {
>>> -	u8 ret;
>>> -
>>> -	ret = gpiod_get_value(D01);
>>> -	ret |= gpiod_get_value(D02) << 1;
>>> -	ret |= gpiod_get_value(D03) << 2;
>>> -	ret |= gpiod_get_value(D04) << 3;
>>> -	ret |= gpiod_get_value(D05) << 4;
>>> -	ret |= gpiod_get_value(D06) << 5;
>>> -	ret |= gpiod_get_value(D07) << 6;
>>> -	ret |= gpiod_get_value(D08) << 7;
>>> -	return ~ret;
>>> +	struct gpio_desc *lines[8] = {
>>> +		D01, D02, D03, D04, D05, D06, D07, D08
>>> +	};
>>> +
>> Delete this blank line.
>>
>>> +	u8 val = 0;
>>> +	int ret, i;
>>> +
>>> +	for (i = 0; i < 8; i++) {
>>> +		ret = gpiod_get_value(lines[i]);
>>> +		if (ret < 0) {
>>> +			pr_err("get GPIO pin %d error: %d\n", i, ret);
>>> +			continue;
>>> +		}
>>> +		val |= (ret & 1) << i;
>> Delete the mask.
>>
>> (I wavered on whether I should comment on the nit picky things I've
>> said in this email, but in the end it was the out of date commit
>> message which pushed me over the edge.  I would have ignored the
>> other things otherwise).
>>
>> regards,
>> dan carpenter
>>
>>
> This patch seems unnecessary.
> The code will never be extended.

But using for loop is more readable than writing 8 similar lines, or?

> In the unlikely case of errors it will produce a huge streams of console spam.
> It negatively impacts performance:  114209 bytes/sec vs 118274 bytes/sec.

We can remove that error message to not impact the performance, but storing errors even unlikely cases
as gpio data is a bug, or?

> regards,
> -Dave

Re: [PATCH v2] staging: gpib: simplify and fix get_data_lines

[Dave Penkler]

On Fri, Aug 29, 2025 at 04:34:05PM +0200, Osama Abdelkader wrote:
> 
> On 8/27/25 4:11 PM, Dave Penkler wrote:
> > On Wed, Aug 27, 2025 at 03:16:28PM +0300, Dan Carpenter wrote:
> >> On Wed, Aug 27, 2025 at 01:38:57PM +0200, Osama Abdelkader wrote:
> >>> The function `get_data_lines()` in gpib_bitbang.c currently reads 8
> >>> GPIO descriptors individually and combines them into a byte.
> >>> This has two issues:
> >>>
> >>>   * `gpiod_get_value()` returns an `int` which may be negative on
> >>>     error. Assigning it directly into a `u8` may propagate unexpected
> >>>     values. Masking ensures only the LSB is used.
> >> This part isn't really true any more.
> >>
> >>>   * The code is repetitive and harder to extend.
> >>>
> >>> Fix this by introducing a local array of GPIO descriptors and looping
> >>> over them, while masking the return value to its least significant bit.
> >> There really isn't any need to mask now that we're checking for
> >> negatives.
> >>
> >>> This reduces duplication, makes the code more maintainable, and avoids
> >>> possible data corruption from negative `gpiod_get_value()` returns.
> >>>
> >>> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> >>> ---
> >>> v2:
> >>> Just print the gpio pin error and leave the bit as zero
> >>> ---
> >>>  drivers/staging/gpib/gpio/gpib_bitbang.c | 28 ++++++++++++++----------
> >>>  1 file changed, 17 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
> >>> index 17884810fd69..f4ca59c007dd 100644
> >>> --- a/drivers/staging/gpib/gpio/gpib_bitbang.c
> >>> +++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
> >>> @@ -1403,17 +1403,23 @@ static void set_data_lines(u8 byte)
> >>>  
> >>>  static u8 get_data_lines(void)
> >>>  {
> >>> -	u8 ret;
> >>> -
> >>> -	ret = gpiod_get_value(D01);
> >>> -	ret |= gpiod_get_value(D02) << 1;
> >>> -	ret |= gpiod_get_value(D03) << 2;
> >>> -	ret |= gpiod_get_value(D04) << 3;
> >>> -	ret |= gpiod_get_value(D05) << 4;
> >>> -	ret |= gpiod_get_value(D06) << 5;
> >>> -	ret |= gpiod_get_value(D07) << 6;
> >>> -	ret |= gpiod_get_value(D08) << 7;
> >>> -	return ~ret;
> >>> +	struct gpio_desc *lines[8] = {
> >>> +		D01, D02, D03, D04, D05, D06, D07, D08
> >>> +	};
> >>> +
> >> Delete this blank line.
> >>
> >>> +	u8 val = 0;
> >>> +	int ret, i;
> >>> +
> >>> +	for (i = 0; i < 8; i++) {
> >>> +		ret = gpiod_get_value(lines[i]);
> >>> +		if (ret < 0) {
> >>> +			pr_err("get GPIO pin %d error: %d\n", i, ret);
> >>> +			continue;
> >>> +		}
> >>> +		val |= (ret & 1) << i;
> >> Delete the mask.
> >>
> >> (I wavered on whether I should comment on the nit picky things I've
> >> said in this email, but in the end it was the out of date commit
> >> message which pushed me over the edge.  I would have ignored the
> >> other things otherwise).
> >>
> >> regards,
> >> dan carpenter
> >>
> >>
> > This patch seems unnecessary.
> > The code will never be extended.
> 
> But using for loop is more readable than writing 8 similar lines, or?
> 
> > In the unlikely case of errors it will produce a huge streams of console spam.
> > It negatively impacts performance:  114209 bytes/sec vs 118274 bytes/sec.
> 
> We can remove that error message to not impact the performance, but storing errors even unlikely cases
> as gpio data is a bug, or?

Hi again,
Even with the following code, eliminating the error test, the
performance is still negatively impacted: 114865 vs 118274 bytes/sec.

static u8 get_data_lines(void)
{
  struct gpio_desc *lines[8] = {D01, D02, D03, D04, D05, D06, D07, D08}; 
  u8 val = 0;
  int i;

  for (i = 0; i < 8; i++)
             val |= gpiod_get_value(lines[i]) << i;
   return ~val;
}

Variable shifts are just slower than hardcoded shifts. Most of the
delay for GPIB reads and writes comes from the relatively long
interrupt latency on the pi's (> 2 usecs). There are 2 interrupts per
byte read. Even so the loop implementation causes a noticeable
degradation in performance which we want to avoid.

Regarding testing for error returns:
We won't get ENODEV since on the raspberry pi the gpios are
implemented on the SoC so cannot "disappear" once allocated.

In the case of a direction bug (which we don't have) the gpiod subsystem
will emit a warning.

Further it is not worth checking for error returns on the
gpiod_get/set_value calls with the bcma_gpio_get/set_value
implementations since the latter do not return negative values.