[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

Wed Jul 16 13:09:28 CEST 2014

On Wed, Jul 16, 2014 at 09:50:02AM +0100, Rob Jones wrote:
> 
> 
> On 16/07/14 08:51, Thierry Reding wrote:
> >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 at gmail.com> 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 at gmail.com> wrote:
> >>>>>On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars at metafoo.de> wrote:
> >>>>>>On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
> >>>>>>>
> >>>>>>>On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd at arndb.de> 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.
> >
> 
> In light of this, should I hold off starting on devm_gpiod_get_array()
> as discussed on here last week?

I'll let Alex or Linus answer this, since I wasn't involved in the
devm_gpiod_get_array() discussions. It's probably going to be tricky to
pass around an array of everything, but I suspect you've already got a
solution to that.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140716/8f862613/attachment.sig>