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

[Heiko Schocher <hs@denx.de>]

Hello Simon,

Am 12.07.2019 um 23:11 schrieb Simon Glass:
> Hi Heiko,
> 
> On Wed, 26 Jun 2019 at 22:12, Heiko Schocher <hs@denx.de> wrote:
>>
>> 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?
> 
> I don't see any point. Your caller needs to store the pointer anyway,
> or if not, could just as easily have a local variable with the info.
> 
> Also, passing a pointer to the struct is how we consistently do it so far.
> 
> What is the benefit of storing it in the callee rather than the caller?
> 
>>
>>>> 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.
> 
> I understand what you are saying about devices being removed, but
> actually removable is fine, it's only unbinding that is dangerous. And
> we already have that problem. We would need to add refcounts to fix
> it, and given that bootloaders don't normally need to unbind, I have
> not worried about it too much.
> 
> The approach I suggest would not use any more memory I think.
> 
>>
>> What do you think?
> 
> Well I still think it is better to have the caller own the struct.
> 
>>
>>>>>> +
>>>>>> +       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?
> 
> I don't think you have explained why you actually need to store the
> GPIO descriptor. Where do you use it later? Is it just the
> gpio_hog_probe_all() function?

You can use it later! See example this patch adds in
doc/device-tree-bindings/gpio/gpio.txt:

+       struct gpio_desc *desc;
+       int ret;
+
+       ret = gpio_hog_lookup_name("boot_rescue", &desc);
+       if (ret)
+               return;
+       if (dm_gpio_get_value(desc) == 1)
+               printf("\nBooting into Rescue System\n");
+       else if (dm_gpio_get_value(desc) == 0)
+               printf("\nBoot normal\n");

So in this example, gpio "board_rescue" is defined through DTS and code
which uses this gpio is generic ... makes it nice to handle board
differences (as I have on the aristainetos board, which I rework
for DM support).

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