[U-Boot] [PATCH v4] gpio: add gpio-hog support

[Heiko Schocher <hs@denx.de>]

Hello Simon,

Am 26.06.2019 um 17:07 schrieb Simon Glass:
> Hi Heiko,
> 
> On Mon, 24 Jun 2019 at 00:16, Heiko Schocher <hs@denx.de> wrote:
>>
>> Hello Simon,
>>
>> Am 22.06.2019 um 21:09 schrieb Simon Glass:
>>> Hi Heiko,
>>>
>>> On Wed, 12 Jun 2019 at 05:12, Heiko Schocher <hs@denx.de> wrote:
>>>>
>>>> add gpio-hog support. GPIO hogging is a mechanism
>>>> providing automatic GPIO request and configuration
>>>> as part of the gpio-controller's driver probe function.
>>>>
>>>> for more infos see:
>>>> doc/device-tree-bindings/gpio/gpio.txt
>>>>
>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>>
>>>> ---
>>>> clean travis build, see result:
>>>> https://travis-ci.org/hsdenx/u-boot-test/builds/544040419
>>>>
>>>> Changes in v4:
>>>> - rework as suggested by Patrick
>>>>     based on patch:
>>>>     http://patchwork.ozlabs.org/patch/1098913/
>>>>     use new UCLASS_NOP
>>>>     hog detection + DT parsing in gpio_post_bind() .... info saved in platdata
>>>>     gpio configuration for hog (request and set_dir) => probe function
>>>
>>>
> 
> [..]
> 
>>>> +config DM_GPIO_HOG
>>>
>>> Can we drop the DM_ prefix? At some point DM_GPIO will go away and we
>>> don't want to rename these sorts of options.
>>
>> Hmm... we also have the DM_GPIO symbol, as the gpio hog only works with
>> DM_GPIO enabled, I thought DM_GPIO_HOG is the better choice, but of course
>> I can change this.
> 
> Yes I understand that, but in general we hope one day to rename
> DM_GPIO to GPIO, once everything uses DM. That's why I am trying to
> keep the DM_ suffix only for things that need it.

Yep, already dropped, see temp version (travisbuild clean):
https://github.com/hsdenx/u-boot-test/commit/509bad758b8abc94dbbebc2a387b9e97c30b6543

>>
>>>> +       bool "Enable GPIO hog support"
>>>> +       depends on DM_GPIO
>>>> +       default n
>>>> +       help
>>>> +         Enable gpio hog support
>>>> +         The GPIO chip may contain GPIO hog definitions. GPIO hogging
>>>> +         is a mechanism providing automatic GPIO request and config-
>>>> +         uration as part of the gpio-controller's driver probe function.
>>>> +
>>>>    config ALTERA_PIO
>>>>           bool "Altera PIO driver"
>>>>           depends on DM_GPIO
>>>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
>>>> index da5e9ba6e5..308d0863ad 100644
>>>> --- a/drivers/gpio/gpio-uclass.c
>>>> +++ b/drivers/gpio/gpio-uclass.c
>>>> @@ -5,6 +5,9 @@
>>>>
>>>>    #include <common.h>
>>>>    #include <dm.h>
>>>> +#include <dm/device-internal.h>
>>>> +#include <dm/lists.h>
>>>> +#include <dm/uclass-internal.h>
>>>>    #include <dt-bindings/gpio/gpio.h>
>>>>    #include <errno.h>
>>>>    #include <fdtdec.h>
>>>> @@ -141,6 +144,118 @@ static int gpio_find_and_xlate(struct gpio_desc *desc,
>>>>                   return gpio_xlate_offs_flags(desc->dev, desc, args);
>>>>    }
>>>>
>>>> +#if defined(CONFIG_DM_GPIO_HOG)
>>>> +
>>>> +struct gpio_hog_priv {
>>>> +       struct gpio_desc gpiod;
>>>
>>> As above I don't think you need this.
>>
>> Hmm.. when probing I need to request the gpio with gpio_dev_request_index()
>> for which I need to pass a pointer to struct gpio_desc ...
> 
> The caller can declare this in a local variable.

The caller is always the device probe function, as gpio_hog_probe_all()
loops over all gpio hog devices and probes them with device_probe()
which cannot return a gpio_desc pointer, nor can such a pointer passed to.

Why isn't it valid to store in each gpio hog device the gpio desc it
belongs too in device private data?

>> And later I want to have the possibility to access the gpio for example
>> with dm_gpio_set_value() and need therefore the gpio descriptor, so I must
>> store it somewhere. Or do I overlook something obvious?
> 
> In that case the caller should store it, perhaps in a local variable,
> or perhaps in a private data structure if needed for a long time.
> 
> [..]

Of course I can introduce a list for each gpio hog definition filled
when calling gpio_hog_probe_all() ... but than I have a second list
which uses memory, may has to keep in sync the list if device are
removed ... I want to avoid such an approach ... as we have already a
list of gpio hog devices and the priv storage pointer for device private
data.

What do you think?

>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +int gpio_hog_probe_all(void)
>>>> +{
>>>> +       struct udevice *dev;
>>>> +       int ret;
>>>> +
>>>> +       for (uclass_first_device(UCLASS_NOP, &dev);
>>>
>>> Perhaps you should create a new uclass for this?
>>
>> Hmm.. yes, I had an approach with a new uclass, but I thought, the new [1]
>> UCLASS_NOP is valid for this here to?
>>
>> [1] http://patchwork.ozlabs.org/patch/1098913/
> 
> I see your point and I can see arguments for each side. But the fact
> that you have to check the driver suggests to me that a new uclass is
> better.
> 
> [..]

Ok with this approach the gpio_hog additions to gpio-uclass can be
moved to a gpio-hog-uclass.c file. Also the problem where to store
the gpio descriptor moves from device private data to uclass private
data?

Is it worth?

>>>> +/**
>>>> + * gpio_hog_probe_all() - probe all gpio devices with
>>>> + * gpio-hog subnodes.
>>>> + *
>>>> + * @return:    Returns return value from device_probe()
>>>
>>> What if one fails and the others succeed?
>>
>> Currently I return with error code from device_probe(). So the
>> rest of the gpio-hogs are not probed.
>>
>> Hmm.. may it is better to emmit a debug message and continue?
> 
> Sounds reasonable. But it seems to me that if there is an error there
> should be some visible report, so this could perhaps return the error
> code? Then the caller could print a messag?

Ok, so I add a printf message. I like more to add it in
gpio_hog_probe_all(), because more than one gpio_hog can fail.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de