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

[Dan Carpenter <dan.carpenter@linaro.org>]

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