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

Thu Jul 17 00:44:47 PDT 2014

On Thu, Jul 17, 2014 at 01:28:00PM +0900, Alexandre Courbot wrote:
> On Wed, Jul 16, 2014 at 5:50 PM, Rob Jones <rob.jones at codethink.co.uk> 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?
> 
> These are two independant issues, and adapting the get_array() patch
> to a refactored gpiod_get() should be trivial.
> 
> But while we are at it (and sorry for going further off-topic), I also
> think that gpiod_get_array() should not follow the same calling
> convention as gpio_request_array() by taking an array of whatever
> gpiod_get() would require. Instead, I think it should be redefined to
> mean "get all the GPIOs for a given function". For instance, say that
> in the DT you have
> 
> foo-gpios = <&gpio 0 GPIO_ACTIVE_HIGH &gpio 1 GPIO_ACTIVE_HIGH &gpio 2
> GPIO_ACTIVE_HIGH>;
> 
> Then gpiod_get_array(dev, "foo", &num) should return descriptors to
> these 3 GPIOs and assign "3" to num. The same thing can be done with
> the platform lookup tables.
> 
> Actually it would be even better if this API could be designed to
> interact nicely with the multiple GPIO setting patch by Rojhalat:
> http://www.spinics.net/lists/linux-gpio/msg00827.html
> 
> gpiod_get_array() would thus allocate and return an array of GPIO
> descriptors suitable to be used immediatly with gpiod_set_array(). And
> bam, a nice full-circle API for handling multiple GPIOs. My
> expectations have risen all of a sudden. ;)

Should the new gpiod_get_array() function also have a way to specify the
flags similar to gpiod_get()? I agree that making it return all GPIOs of
a given function is a good idea. And given that GPIOs associated with
the same function probably behave very similarly it should be safe to
add flags handling to that as well. That is, I don't think we'd need to
worry about different GPIOs of the same function requiring different
directions or initial values (note that polarity is still handled by the
GPIO specifier).

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140717/f2af4808/attachment-0001.sig>