Re: gpio patches in mmotm

From: Uwe Kleine-König
Date: Tue Apr 08 2008 - 05:33:24 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/