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

Fri Jul 12 21:11:52 UTC 2019

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?

>
> 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.

Regards,
Simon