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

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

On Wed, Aug 27, 2025 at 11:28:36AM +0200, Osama Abdelkader wrote:
> We can change the return type to int and propagate the error, so:
> 
> static int get_data_lines(u8 *out)
> 
> {
> 
> 	int val, i;
> 
> 	u8 ret = 0;
> 
> 	struct gpio_desc *lines[8] = { D01, D02, D03, D04, D05, D06, D07, D08 };
> 	for (i = 0; i < 8; i++) { 		val = gpiod_get_value(lines[i]);
> 
> 		if (val < 0)
> 
> 			return val; // propagate error
> 
> 		ret |= (val & 1) << i;
> 
> 	}
> 
> 	*out = ~ret; 	return 0;
> 
> }
> 
> Then in the caller:
> 
> u8 data;
> if (!get_data_lines(&data))
> 
> 	priv->rbuf[priv->count++] = data;
> 
> or we print the error here, What do you think?
> 

Printing the error closer to where the error occured is better.

How would we handle the error correctly?  Sometimes it's easy
because it's if we continue then we will crash and almost anything is
better than crashing.  But here if we return a 1 or 0, what's the
worst that can happen?  We can't know without testing.  Adding new
error checking often breaks stuff.  I've done it before where you
have code like:

	ret = frob();
	if (ret)
		return ret;

	frob();  // <-- here
	if (ret)
		return ret;

And obviously the original author left off the "ret = " part on the
second call.  So I don't feel bad about that I added that, but in
practice the second frob() always fails and I can't test it so now
people's driver doesn't load.

So adding error handling is a bit risky unless you have a way to test
this code.  Just printing an error and continuing as best we can is
safer.  People will let us know if the error ever happens.

Let's not over complicate it for an error which will likely never
happen.  We can just print an error and leave the bit as zero.

regards,
dan carpenter