Re: gpio patches in mmotm

[Guennadi Liakhovetski <g.liakhovetski@pengutronix.de>]

On Tue, 8 Apr 2008, Uwe Kleine-König wrote:

> > I'm storing the GPIO number locally, and if the system doesn't have a 
> > valid GPIO for me, I'm storing an invalid GPIO number. Then at any time if 
> > the GPIO has to be used, I just verify if gpio_is_valid(), and if not, 
> > return an error code for this request, but the driver remains otherwise 
> > functional.
> OK, so in your driver you have:
> 
> 	if (gpio_is_valid(gpio)) {
> 		/* We have a data bus switch. */
> 		ret = gpio_request(gpio, "mt9m001");
> 		if (ret < 0) {
> 			dev_err(&mt9m001->client->dev, "Cannot get GPIO %u\n",
> 				gpio);
> 			return ret;
> 		}
> 		ret = gpio_direction_output(gpio, 0);
> 		if (ret < 0) {
> 			...
> 
> 
> In my eyes the following is better:
> 
> 	/* Do we have a data bus switch? */
> 	ret = gpio_request(gpio, "mt9m001");
> 	if (ret < 0) {
> 		if (ret != -EINVAL) {
> 			dev_err(...);
> 			return ret;
> 		}
> 	} else {
> 		ret = gpio_direction_output(gpio, 0);
> 		if (ret < 0) {
> 			...

Yes, you could do that. But then you have to test either before calling 
gpio_set_value_cansleep() or inside it. And the test you have to perform 
_is_ the validity check, so, you need it anyway.

> Then you don't need to extend the API.  Moreover with your variant the
> check that gpio is valid must be done twice[1].

Actually three times. The one before gpio_free() is not actually needed, 
right, it is anyway checked inside. But gpio_set_value_cansleep() doesn't 
check, so, it would be rude to call it with an invalid value.

> [1] OK, gpio_is_valid and gpio_request might be inline functions, but
> for "my" architecture it is not.

Which arch is it? As I said, you could simplify the two specific camera 
drivers by removing the checks where they are redundant. But on other 
occasions the checks have to be done anyway, so, it is not a question of 
runtime performance (apart from maybe the difference between calling a 
function and executing inline), but just of an extra API member, which you 
can have different opinions about:-)

Thanks
Guennadi
---
Guennadi Liakhovetski