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

Mon Jul 8 05:11:59 UTC 2019

Hello Simon,

gentle ping to my questions only ... hope I did not missed an
answer from you ...

Am 27.06.2019 um 06:12 schrieb Heiko Schocher:
> 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 at 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 at 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 at 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

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