From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration Date: Wed, 16 Jul 2014 13:09:28 +0200 Message-ID: <20140716110927.GK7978@ulmo> References: <20140714183624.GV6800@sirena.org.uk> <4657735.1VKsbQGuH6@wuerfel> <53C4DF0D.8010208@metafoo.de> <20140716071237.GA7978@ulmo> <20140716075107.GG7978@ulmo> <53C63CBA.20809@codethink.co.uk> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Qo8f1a4rgWw9S/zY" Return-path: Received: from mail-wi0-f177.google.com ([209.85.212.177]:49371 "EHLO mail-wi0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751016AbaGPLJc (ORCPT ); Wed, 16 Jul 2014 07:09:32 -0400 Received: by mail-wi0-f177.google.com with SMTP id ho1so1104293wib.4 for ; Wed, 16 Jul 2014 04:09:31 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53C63CBA.20809@codethink.co.uk> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Rob Jones Cc: Alexandre Courbot , Lars-Peter Clausen , Arnd Bergmann , "alsa-devel@alsa-project.org" , Kukjin Kim , Tomasz Figa , Maurus Cuelenaere , Liam Girdwood , "linux-gpio@vger.kernel.org" , Mark Brown , Linus Walleij , "linux-arm-kernel@lists.infradead.org" --Qo8f1a4rgWw9S/zY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 16, 2014 at 09:50:02AM +0100, Rob Jones wrote: >=20 >=20 > 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 > >> 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 = wrote: > >>>>>On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen wrote: > >>>>>>On 07/15/2014 09:36 AM, Alexandre Courbot wrote: > >>>>>>> > >>>>>>>On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann wro= te: > >>>>>>>> > >>>>>>>>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 tab= le, > >>>>>>>>>looking at the code it seems like this is indeed the case. We c= an 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_g= et()? > >>>>>>>>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 driv= ers > >>>>>>>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 proper= ties > >>>>>>>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 w= hat > >>>>>>>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 t= o 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 directio= n, > >>>>>value, and (maybe) sysfs export. > >>>> > >>>>Problem is, too much convenience functions seems to ultimately kill c= onvenience. > >>>> > >>>>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 putti= ng > >>>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. > > >=20 > 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 --Qo8f1a4rgWw9S/zY Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTxl1nAAoJEN0jrNd/PrOh0RQP/0Xb2qnmeULQBRDu9RZU04YS GxVenJmyNvcNW16VuRwGR8gMdl+fVrZC3qS2vEBTmNwJbq9KTryzAJdcZbe4g6WN 2/eBdeBMYJN/8dxXsxanCAQPUXC6iEwSyItaqBJEFJlzOPqYkV6/wi5rk5WpvOuq dFFRbIbuvmqVPXgOWP7ucpYEKGePeUlz2gjbm2AdSI5qqd6YJukBqMFFmyoNnLa4 ggiRzl4LOfiiybsd3v/sDX38hzDrerqb4Baf10bwVrGyoEaPwaSxqGkV8/N8apxf 3P9ko7NGgv4lRg2nXEmoh3N1YKmel36bTI3SoCZ0QdgdxyVRX+XrRC/NLDmuvmSr 0WONhAdYugjLT9BJbe8LybooT3Ul6GFEPUiAuy9IZY5MOe/NBkR9AuwT3iBYouq3 9+CEdwFzu19uSCpy5IwbchF3hIOjtPJ70iBtu+r/9j2aeX+K4WMsZ53dWUNNmVqZ fovvIOz0atCMGze5wEIDBcBA7ix/deWgziDs+vutHZxUAFxrK2NVqP7ET1ES/WI1 1c+4IHb/dGhtUv3akc1q527/wxBkVU/iutFlnV82fjvW6hwOkHR2mJI3BWZAFsH6 enYeqHX54kYviuTiIIukTXttTNaPnf4KDdZn716vrXlWeQMhO1mMxPfc14sXlnJb AR/s4NXEKM8ByDx54+Z2 =KDRp -----END PGP SIGNATURE----- --Qo8f1a4rgWw9S/zY--