On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote: > On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding > <thierry.reding@xxxxxxxxx> wrote: > > On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote: > >> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@xxxxxxxxx> wrote: > >> > On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: > >> >> On 07/15/2014 09:36 AM, Alexandre Courbot wrote: > >> >>> > >> >>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote: > >> >>>> > >> >>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote: > >> >>>>> > >> >>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote: > >> >>>>>> > >> >>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote: > >> >>>>> > >> >>>>> > >> >>>>>>> Yes. But now that you say it the gpiod_direction_output() call is > >> >>>>>>> missing > >> >>>>>>> from this patch. > >> >>>>> > >> >>>>> > >> >>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from > >> >>>>>> Documentation/gpio/board.txt > >> >>>>>> and as Linus Walleij explained to me the other day, the lookup is > >> >>>>>> supposed > >> >>>>>> to replace devm_gpio_request_one(), which in turn replaced both the > >> >>>>>> gpio_request and the gpio_direction_output(). Do I need to put the > >> >>>>>> gpiod_direction_output() back or is there another interface for that > >> >>>>>> when > >> >>>>>> registering the board gpios? > >> >>>>> > >> >>>>> > >> >>>>> Indeed. If you *do* need an explicit _output() then that sounds to me > >> >>>>> like we either need a gpiod_get_one() or an extension to the table, > >> >>>>> looking at the code it seems like this is indeed the case. We can set > >> >>>>> if the GPIO is active high/low, or open source/drain but there's no flag > >> >>>>> for the initial state. > >> >>>> > >> >>>> > >> >>>> (adding Alexandre and the gpio list) > >> >>>> > >> >>>> GPIO people: any guidance on how a board file should set a gpio to > >> >>>> output/default-high in a GPIO_LOOKUP() table to replace a > >> >>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()? > >> >>>> Do we need to add an interface extension to do this, e.g. passing > >> >>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH? > >> >>> > >> >>> > >> >>> The way I see it, GPIO mappings (whether they are done using the > >> >>> lookup tables, DT, or ACPI) should only care about details that are > >> >>> relevant to the device layout and that should be abstracted to the > >> >>> driver (e.g. whether the GPIO is active low or open drain) so drivers > >> >>> do not need to check X conditions every time they want to drive the > >> >>> GPIO. > >> >>> > >> >>> Direction and initial value, on the other hand, are clearly properties > >> >>> that ought to be set by the driver itself. Thus my expectation here > >> >>> would be that the driver sets the GPIO direction and initial value as > >> >>> soon as it gets it using gpiod_direction_output(). In other words, > >> >>> there is no replacement for gpio_request_one() with the gpiod > >> >>> interface. Is there any use-case that cannot be covered by calling > >> >>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what > >> >>> gpio_request_one() was doing anyway. > >> >> > >> >> > >> >> I agree with you that this is something that should be done in the driver > >> >> and not in the lookup table. I think that it is still a good idea to have a > >> >> replacement for gpio_request_one with the new GPIO descriptor API. A large > >> >> share of the drivers want to call either gpio_direction_input() or > >> >> gpio_direction_output() right after requesting the GPIO. Combining both the > >> >> requesting and the configuration of the GPIO into one function call makes > >> >> the code a bit shorter and also simplifies the error handling. Even more so > >> >> if e.g. the GPIO is optional. This was one of the main reasons why > >> >> gpio_request_one was introduced, see the commit[1] that added it. > >> > > >> > I am not opposed to it as a convenience function. Note that since the > >> > open-source and open-drain flags are already handled by the lookup > >> > table, the only flags it should handle are those related to direction, > >> > value, and (maybe) sysfs export. > >> > >> Problem is, too much convenience functions seems to ultimately kill convenience. > >> > >> The canonical way to request a GPIO is by providing a (device, > >> function, index) triplet to gpiod_get_index(). Since most functions > >> only need one GPIO, we have gpiod_get(device, function) which is > >> basically an alias to gpiod_get_index(device, function, 0) (note to > >> self: we should probably inline it). > >> > >> On top of these comes another set of convenience functions, > >> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL > >> instead of -ENOENT if the requested GPIO mapping does not exist. This > >> is useful for the common case where a driver can work without a GPIO. > >> > >> Of course these functions all have devm counterparts, so we currently > >> have 8 (devm_)gpiod_get(_index)(_optional) functions. > >> > >> If we are to add functions with an init flags parameter, we will end > >> with 16 functions. That starts to be a bit too much to my taste, and > >> maybe that's where GPIO consumers should sacrifice some convenience to > >> preserve a comprehensible GPIO API. > >> > >> There might be other ways to work around this though. For instance, we > >> could replace the _optional functions by a GPIOF_OPTIONAL flag to be > >> passed to a more generic function that would also accept direction and > >> init value flags. Actually I am not seeing any user of the _optional > >> variant in -next, so maybe we should just do this. Thierry, since you > >> introduced the _optional functions, can we get your thoughts about > >> this? > > > > I personally prefer explicit naming of the functions rather than putting > > a bunch of flags into some parameter. If you're overly concerned about > > the amount of convenience functions, perhaps the _index variants can be > > left out for gpiod_get_one(). I'd argue that if drivers want to deal > > with that level of detail anyway, they may just as well add the index > > explicitly when calling the function. > > > > While we're at it, gpiod_get_one() doesn't sound like a very good name. > > All other variants only request "one" as well. Perhaps something like > > gpiod_get_with_flags() would be a better name. > > > > Then again, maybe rather than add a new set of functions we should bite > > the bullet and change gpiod_get() (and variants) to take an additional > > flags parameter. There aren't all that many users yet (I count 26 > > outside of drivers/gpio), so maybe now would still be a good time to do > > that. > > That sounds reasonable indeed. And preferable to getting an aneurysm > after trying to spell devm_gpiod_get_index_optional_with_flags(). > > This also makes the most sense since most GPIO users will want to set > a direction and value right after obtaining one. So if there is no > objection I will probably start refactoring gpiod_get() this week. Sounds good to me. Thierry
Attachment:
pgpd0DF7Pa9G3.pgp
Description: PGP signature
