[U-Boot] [PATCH v4] gpio: add gpio-hog support
Heiko Schocher
hs at denx.de
Mon Jul 15 06:15:59 UTC 2019
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 at 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 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?
>
> 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
More information about the U-Boot
mailing list