On Fri, Jul 11, 2014 at 03:45:03PM +0200, Arnd Bergmann wrote:

Here are a few patches from my randconfig and multiplatform series. The smartq patch is a re-send of the one I screwed up the other day. the dependencies and dma data removal should be trivial, the dmaengine one probably needs some closer inspection.

Hrm, can you check what's going on with the series - I've got two copies of patch 2 and an extra patch [PATCH] ASoC: s3c64xx/smartq: use dynamic registration which is threaded before this cover letter and looks like a resend of the prior copy with the platform_data header missing that got included by mistake? The patches look OK but I just want to make sure I'm applying the right things.

On Sat, Jul 12, 2014 at 05:27:59PM +0200, Lars-Peter Clausen wrote:

On 07/11/2014 03:45 PM, Arnd Bergmann wrote:

...

As a prerequisite for moving s3c64xx into multiplatform configurations, we need to change the smartq audio driver to stop using hardcoded gpio numbers from the header file, and instead pass the gpio data through platform_data.

This should be using the gpiod API. The gpiod API allows you to pass the GPIOs without having to add a platform_data struct.

OTOH that's a more invasive change that's harder to do mechanically - I'm not sure it's sensible to insist on someone doing it for generic cleanups (rather than actively working with the particular platform).

On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:

On 07/14/2014 05:46 PM, Arnd Bergmann wrote: [...]

...

...

...

+static struct gpiod_lookup_table smartq_audio_gpios = {

• .dev_id = "smartq-audio",
• .table = {

•       GPIO_LOOKUP("GPL", 12, "headphone detect", 0),
  

•       GPIO_LOOKUP("GPK", 12, "amplifiers shutdown", GPIO_ACTIVE_HIGH),
  

There is no such thing as GPIO_ACTIVE_HIGH, just 0 for flags.

The original driver does gpio_direction_output(..., 1);

For some reason I earlier concluded that this was what the '1' would need to get converted to. Are you sure '0' is correct then?

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?

Arnd

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.

On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann arnd@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.

On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen lars@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@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.

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@gmail.com wrote:

...

On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen lars@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@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.

Thierry

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@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@gmail.com wrote:

...

On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen lars@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@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.

Thierry

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@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@gmail.com wrote:

...

On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen lars@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@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

On Wed, Jul 16, 2014 at 5:50 PM, Rob Jones rob.jones@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@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@gmail.com wrote:

...

On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen lars@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@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. ;)

On Thu, Jul 17, 2014 at 4:44 PM, Thierry Reding thierry.reding@gmail.com wrote:

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@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@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@gmail.com > wrote: >> >> On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen lars@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@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()?

Probably an optional array of flags could not hurt, to follow the example of 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).

Right. It may very well be that a single flag specifier (as opposed to an array) will be enough for this case. If you need to request some GPIOs as input and some other as output then they are clearly different functions and requesting them together would be an abuse of the API.

Initial values of output GPIOs might be a reason why we want an array though, and I leave it to Rob to decide what is best here since he has a better idea of how this is going to be used.

On Thu, Jul 17, 2014 at 11:17:23AM +0100, Mark Brown wrote:

On Thu, Jul 17, 2014 at 05:55:36PM +0900, Alexandre Courbot wrote:

...

Right. It may very well be that a single flag specifier (as opposed to an array) will be enough for this case. If you need to request some GPIOs as input and some other as output then they are clearly different functions and requesting them together would be an abuse of the API.

Not so sure about that - what about requesting GPIOs for a bidirectional bus? Thinking about SPI bitbanging here.

Wouldn't you want to use a different means that the gpiod_array_*() API to handle those cases? gpiod_array_*() is probably most useful to handle bulk operations on a set of GPIOs that do essentially the same thing. If you get and then need to index into that array to handle them all differently then you don't gain very much.

Thierry

On Thu, Jul 17, 2014 at 12:41:23PM +0200, Thierry Reding wrote:

On Thu, Jul 17, 2014 at 11:17:23AM +0100, Mark Brown wrote:

...

Not so sure about that - what about requesting GPIOs for a bidirectional bus? Thinking about SPI bitbanging here.

Wouldn't you want to use a different means that the gpiod_array_*() API to handle those cases? gpiod_array_*() is probably most useful to handle bulk operations on a set of GPIOs that do essentially the same thing. If you get and then need to index into that array to handle them all differently then you don't gain very much.

For set and get, sure - but it's still useful to be able to do bulk requests for GPIOs especially since that's the only bit of the interface that requires error handling.

On Mon, Jul 21, 2014 at 12:36:43PM +0900, Alexandre Courbot wrote:

On Thu, Jul 17, 2014 at 8:05 PM, Mark Brown broonie@kernel.org wrote:

...

For set and get, sure - but it's still useful to be able to do bulk requests for GPIOs especially since that's the only bit of the interface that requires error handling.

I foresee many problems if people start using gpiod_array_get() as a way to spare a few lines of error-checking code. First all the GPIOs would end into an array instead of members with meaningful names - unless they are moved later on, but doing so would add extra code and somewhat kill the purpose. It also becomes more difficult to maintain as you are dealing with array indexes to update all over the code.

You just need a few defines for the names, it's not a big deal.

Finally, it will make it more difficult to use gpiod_array_*() the way it is intended to be used, as you would have to discriminate between GPIOs of the same function and the rest by yourself.

Yes, you probably shouldn't mix and match here but that's fine.

Also, if such a convenience function is legitimate for GPIO, shouldn't it also apply to other sub-systems? E.g. regulator_array_get()?

It's certainly a totally reasonable and expected way of using regulator_bulk_get().

Maybe I am missing your point, but I still think some error-handling code really doesn't hurt here, and the few drivers that would actually benefit from a more automated GPIO request error handling can easily implement it themselves. Let's keep gpiod_array_*() single-purposed and to the point.

I'm not sure I see the massive complication TBH - it's not so much about complexity as it is about reducing the amount of boilerplate that people need to get right.

On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:

On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding

...

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.

Yes, please!

On Tue, Jul 15, 2014 at 04:36:20PM +0900, Alexandre Courbot wrote:

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,

Well, it's a bit of a mix really - from a hardware point of view they normally are actually often fixed.

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.

Right, the original goal was to provide a better interface for requesting a GPIO - it saves a lot of error handling code throughout the kernel and avoids the possibility of bugs due to users forgetting to mark the directio for the GPIO. Having a direct replacement also makes conversions to gpiod easier.

On Fri, Jul 11, 2014 at 03:45:07PM +0200, Arnd Bergmann wrote:

Commit ae602456e83c92 ("ASoC: samsung: drop support for legacy S3C24XX DMA API") removed the old code for the samsung specific DMA interfaces, now that everybody can use dmaengine.

Applied, thanks.

On Fri, Jul 11, 2014 at 03:45:08PM +0200, Arnd Bergmann wrote:

The s3c_dma_client structures and the 'ch' and 'ops' members in s3c_dma_params were only used by the legacy DMA driver and serve no function any more. This removes any reference to them.

Applied, thanks.