Messages in this thread | | | Date | Tue, 8 Apr 2008 11:33:05 +0200 | From | Uwe Kleine-König <> | Subject | Re: gpio patches in mmotm |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |