Re: [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater

[Geert Uytterhoeven]

Hi Eugeniu,

On Sat, Jan 18, 2020 at 2:46 AM Eugeniu Rosca <address@hidden> wrote:
> On Wed, Nov 27, 2019 at 09:42:46AM +0100, Geert Uytterhoeven wrote:
> >   - Create aggregators:
> >
> >     $ echo e6052000.gpio 19,20 \
> >         > /sys/bus/platform/drivers/gpio-aggregator/new_device

> The only unexpected thing is seeing below messages (where gpiochip99 and
> gpiochip22 are inexisting gpiochip names, mistakenly provided on command
> line prior to passing the correct name):
>
> root@rcar-gen3:~# echo gpiochip6 12-13 > 
> /sys/bus/platform/drivers/gpio-aggregator/new_device
> [  915.572905] gpio-aggregator gpio-aggregator.0: cannot find GPIO chip 
> gpiochip99, deferring
> [  915.584224] gpio-aggregator gpio-aggregator.2: cannot find GPIO chip 
> gpiochip99, deferring
> [  915.865281] gpio-aggregator gpio-aggregator.29: cannot find GPIO chip 
> gpiochip22, deferring
>
> Obviously, in the above case, due to a typo in the names, the gpio
> chips will never be found, no matter how long gpio-aggregator defers

Indeed, that is expected behavior: you have created platform devices
referring to resources that are not available.

> their probing. Unfortunately, the driver will continuously emit those
> messages, upon each successfully created/aggregated gpiochip. I built

That is expected behavior, too: every time the driver core manages to
bind a device to a driver, it will retry all previously deferred probes,
in the hope they can be satisfied by the just bound device.

Note that you can destroy these bogus devices, using e.g.

    # echo gpio-aggregator.0 > \
    /sys/bus/platform/drivers/gpio-aggregator/delete_device

> gpio-aggregator as a loadable module, if that's relevant.

Modular or non-modular shouldn't matter w.r.t. this behavior.
Although unloading the module should get rid of the cruft.

> Another comment is that, while the series _does_ allow specifying
> gpio lines in the DTS (this would require a common compatible string
> in gpio_aggregator_dt_ids[] and in the DTS node) and while those lines
> are indeed exposed to userspace, based on my testing, these same gpio
> lines are marked as "used/reserved" by the kernel. This means that
> operating on those gpio pins from userspace will not be possible.
> For instance, gpioget/gpioset return "Device or resource busy":
>
> gpioget: error reading GPIO values: Device or resource busy
> gpioset: error setting the GPIO line values: Device or resource busy
>
> I guess Harish will be unhappy about that, as his expectation was that
> upon merging gpio-aggregator with gpio-inverter, he will be able to
> describe GPIO polarity and names in DTS without "hogging" the pins.
> Perhaps this can be supplemented via an add-on patch later on?

When aggregating GPIO lines, the original GPIO lines are indeed marked
used/reserved, so you cannot use them from userspace.
However, you are expected to use them through the newly created virtual
gpiochip representing the aggregated GPIO lines.

You can try this using the "door" example in
Documentation/admin-guide/gpio/gpio-aggregator.rst, after replacing
gpio2 {19,20} by gpio6 {12,13} to suit your H3ULCB.

> For the whole series (leaving the above findings to your discretion):
>
> Reviewed-by: Eugeniu Rosca <address@hidden>
> Tested-by: Eugeniu Rosca <address@hidden>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- address@hidden

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds