Re: gpio patches in mmotm

Re: gpio patches in mmotm

[Guennadi Liakhovetski]

Please, do not trim the CC: list. I've also added lkml.

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.

> 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.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: gpio patches in mmotm

[Uwe Kleine-König]

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

Re: gpio patches in mmotm

[Guennadi Liakhovetski]

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

Re: gpio patches in mmotm

[Uwe Kleine-König]

Hello Guennadi,

Guennadi Liakhovetski wrote:
> 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.
Ah, OK.  Before setting the value you must assert that you *requested* the
gpio (and not that it is valid).  In your driver that seems to be
equivalent.

Still I would prefer to store the information that the additional GPIO
is not available explicitly in the driver (e.g. by setting gpio = -1)
because gpio != -1 might be cheaper than gpio_is_valid(gpio).

And I don't like extending an API only to provide a second way to do
something without saving code or performance.

> > 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.
That's wrong.  gpio_free as provided by gpiolib does the check, the
variant of ns9xxx does not.  I think it's not explicit, but as gpio_free
must only be called on a requested gpio, I don't see why this check
should be done by gpio_free.

>                                     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?
arch/arm/mach-ns9xxx.  It's not (yet) fully supported in vanilla, but it
includes support for different SOCs that have a different handling of
their GPIOs.  E.g. the ns9360 has one gpio configuration register per 8
gpios, the ns9215 has one per 4 gpios.  Or another thing: ns9215 has
108 gpios, ns9210 has only 54 where the first 50 gpios are identical to
the first 50 of ns9215, and the last 4 gpios are identical to gpios
105-108 on ns9215.  So gpio_is_valid for ns9xxx has to look like:

	int gpio_is_valid(int gpio)
	{
		...
		if (processor_is_ns9210())
			return gpio >= 0 && gpio < 108 && !(gpio >= 50 && gpio < 105);
		...
	}

(In my eyes that hole is ugly, but with it can calculate the address of
the configuration register without case splitting and can handle ns9215
and ns9210 identically---apart from the is-valid check.)

If you're deeper interested you can compare
- http://ftp1.digi.com/support/documentation/90000675_C.pdf (ns9360);
- http://ftp1.digi.com/support/documentation/90000847_B.pdf (ns9215);
  and
- http://ftp1.digi.com/support/documentation/90000846_B.pdf (ns9210).

>                   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:-)
So you reason that the alternative approach allows only a slight
simplification and so is not worth considering?  But obviously
yes, I have a different opinion. :-)

Best regards
Uwe

-- 
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

Re: gpio patches in mmotm

[Guennadi Liakhovetski]

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

> arch/arm/mach-ns9xxx.  It's not (yet) fully supported in vanilla, but it
> includes support for different SOCs that have a different handling of
> their GPIOs.  E.g. the ns9360 has one gpio configuration register per 8
> gpios, the ns9215 has one per 4 gpios.  Or another thing: ns9215 has
> 108 gpios, ns9210 has only 54 where the first 50 gpios are identical to
> the first 50 of ns9215, and the last 4 gpios are identical to gpios
> 105-108 on ns9215.  So gpio_is_valid for ns9xxx has to look like:
> 
> 	int gpio_is_valid(int gpio)
> 	{
> 		...
> 		if (processor_is_ns9210())
> 			return gpio >= 0 && gpio < 108 && !(gpio >= 50 && gpio < 105);
> 		...
> 	}
> 
> (In my eyes that hole is ugly, but with it can calculate the address of
> the configuration register without case splitting and can handle ns9215
> and ns9210 identically---apart from the is-valid check.)

Ok, I thought it would be something like that. I think, these are two 
different things: GPIO valid and GPIO currently physically existing. 
gpio_is_valid() is a test whether the number being tested at all stands a 
chance to be a GPIO number on this architecture. As you see in 
include/asm-generic/gpio.h it only compares against ARCH_NR_GPIOS, which 
is just the theoretically highest GPIO number. It says nothing about 
whether or not all valid GPIOs are actually present on the system. Think 
about GPIO expanders, there might or might not be one currently available 
on the system. Still gpio_is_valid() will return the same result for any 
given number. PXA CPUs have the same "feature" as ns9xxx - different 
models have differeng GPIOs, and platform add their own GPIO controllers, 
which are often placed at a fixed start number, which means, on some CPUs 
there will be holes too. And gpio_is_valid is not (and should not be) 
checking for those - this is already the task for request_gpio().

> So you reason that the alternative approach allows only a slight
> simplification and so is not worth considering?  But obviously
> yes, I have a different opinion. :-)

No, my reason is that I didn't want to put "intimate knowledge" of GPIO 
interna, like "-1 is not a valid GPIO" in the driver but use an 
abstraction instead. My original proposal was to introduce just one 
NO_GPIO macro to test against, however, David nicely managed to persuade 
me, that the gpio_is_valid approach is better. Unfortunately, I cannot 
argument as nicely as he did, maybe looking through his emails in LKML 
archives will help you:-)

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: gpio patches in mmotm

[Uwe Kleine-König]

Hello Guennadi,

Guennadi Liakhovetski wrote:
>                      My original proposal was to introduce just one 
> NO_GPIO macro to test against, however, David nicely managed to persuade 
> me, that the gpio_is_valid approach is better. Unfortunately, I cannot 
> argument as nicely as he did, maybe looking through his emails in LKML 
> archives will help you:-)
I'm unable to find these mails.  (Using gmane, looking for mails by
David matching gpio_is_valid, the oldest post I can find is
http://article.gmane.org/gmane.linux.kernel/637654.)

Can you please point out the discussion in the archives?

Best regards
Uwe

-- 
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

Re: gpio patches in mmotm

[Guennadi Liakhovetski]

On Wed, 9 Apr 2008, Uwe Kleine-König wrote:

> Hello Guennadi,
> 
> Guennadi Liakhovetski wrote:
> >                      My original proposal was to introduce just one 
> > NO_GPIO macro to test against, however, David nicely managed to persuade 
> > me, that the gpio_is_valid approach is better. Unfortunately, I cannot 
> > argument as nicely as he did, maybe looking through his emails in LKML 
> > archives will help you:-)
> I'm unable to find these mails.  (Using gmane, looking for mails by
> David matching gpio_is_valid, the oldest post I can find is
> http://article.gmane.org/gmane.linux.kernel/637654.)
> 
> Can you please point out the discussion in the archives?

See also

http://marc.info/?t=120179335100005&r=1&w=2
http://marc.info/?t=120267097400002&r=1&w=2

Thanks
Guennadi
---
Guennadi Liakhovetski