Re: gpio patches in mmotm

["Uwe Kleine-König" <Uwe.Kleine-Koenig@digi.com>]

Hello Guennadi,


Guennadi Liakhovetski wrote:
> Please, do not trim the CC: list. I've also added lkml.
Oh, thanks.  I thought I'm used to hitting reply-to-all 8-(.
I also added Andrew back (even though adding lkml might be just as good.
:-))

> On Tue, 18 Mar 2008, Uwe Kleine-KЖnig wrote:
> > Guennadi Liakhovetski wrote:
> > > On Mon, 17 Mar 2008, Uwe Kleine-KЖnig wrote:
> > > 
> > > > I'm nure sure if I like gpio_is_valid().  When do you think it should be
> > > > used?  (i.e. in which situations gpio_request doesn't do the right
> > > > thing?)
> > > 
> > > For example, in situations similar to what I have in mt9m001 and mt9v022 
> > > camera drivers. Those cameras can be built with an i2c gpio extender, 
> > > which can be used to switch between 8 and 10 bit data bus widths. But that 
> > > extender is not always available. So, those drivers request a gpio, and if 
> > > it is not available on the system, the gpio_is_valid() test fails.
> > I found your patch, but no tree where it applies.  Can you point me to a
> > tree where it applies?
> 
> These drivers are currently in the v4l-dvb tree 
> http://git.kernel.org/?p=linux/kernel/git/mchehab/v4l-dvb.git;a=summary in 
> the devel branch.
OK, when I searched your driver I found the tree, but only looked in the
master (=HEAD) branch.
 
> > Why isn't it enough that gpio_request fails in such a situation?
> 
> 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) {
			...

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

For me gpio_is_valid would only make sense if there might be situations
where you want to know if a certain GPIO exists but even if it does you
won't gpio_request it.

Best regards
Uwe

[1] OK, gpio_is_valid and gpio_request might be inline functions, but
for "my" architecture it is not.
-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962