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

[Osama Abdelkader <osama.abdelkader@gmail.com>]

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