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

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

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

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

[..]

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

[..]

> >> +/**
> >> + * 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?

Regards,
SImon