Message-ID: <CACRpkdZ_AwiE+HFX6TFBgscaVquKm_tegNSbTT0fhFmpkM7d_Q@mail.gmail.com>
Date: Fri, 20 Dec 2024 13:22:30 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Lee Jones <lee@...nel.org>, Pavel Machek <pavel@....cz>, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-leds@...r.kernel.org, thomas.petazzoni@...tlin.com, 
	DanieleCleri@...on.eu, GaryWang@...on.com.tw
Subject: Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

Hi Thomas,

thanks for your patch!

On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
<thomas.richard@...tlin.com> wrote:

> This enables the pin control support of the onboard FPGA on AAEON UP
> boards.
> Due to the hardware design, the driver shall control its pins in tandem
> with their corresponding Intel SoC GPIOs.
>
> UP boards and UP Squared boards are supported.
>
> Based on the work done by Gary Wang <garywang@...on.com.tw>, largely
> rewritten.
>
> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>

Overall this looks as a good start, some comments below.

> +config PINCTRL_UPBOARD
> +       tristate "AAeon UP board FPGA pin controller"
> +       depends on MFD_UPBOARD_FPGA
> +       select PINMUX
> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS

This implements GPIO so you need:
select GPIOLIB

But I'm not sure because of some oddities, see below.

> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>

Questionable include, see below.

> +static int __upboard_pinctrl_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                                unsigned int offset)

I'm not a fan of functions named with __inner_function() double-underscore
convention. The reason is that double underscore is also used for
compiler intrinsics. Can you just name it

committ_upboard_pinctrl_gpio_request_enable()?

> +static void __upboard_pinctrl_gpio_disable_free(struct pinctrl_dev *pctldev, unsigned int offset)

Dito

> +static int __upboard_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev,
> +                                               unsigned int offset, bool input)

Dito

The pinmux code is very straight forward otherwise, good job!

> +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +       unsigned int pin = pctrl->pctrl_data->pin_header[offset];
> +       int mode;
> +
> +       if (pctrl->gpio[offset])
> +               return gpiod_get_direction(pctrl->gpio[offset]);

See below.

> +       /*
> +        * GPIO was not requested so SoC pin is probably not in GPIO mode.
> +        * When a gpio_chip is registered, the core calls get_direction() for all lines.
> +        * At this time, upboard_gpio_request() was not yet called, so the driver didn't
> +        * request the corresponding SoC pin. So the SoC pin is probably in function (not in
> +        * GPIO mode).
> +        *
> +        * To get the direction of the SoC pin, it shall be requested in GPIO mode.
> +        * Once a SoC pin is set in GPIO mode, there is no way to set it back to its
> +        * function mode.
> +        * Instead of returning the SoC pin direction, the direction of the FPGA pin is
> +        * returned (only for the get_direction() called during the gpio_chip registration).
> +        */
> +       mode = upboard_pinctrl_pin_get_mode(pctrl->pctldev, pin);

Fair enough I guess it's the best we can do here.

> +static int upboard_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +
> +       return gpiod_get_value(pctrl->gpio[offset]);
> +}
> +
> +static void upboard_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +
> +       gpiod_set_value(pctrl->gpio[offset], value);
> +}
> +
> +static int upboard_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +       int ret;
> +
> +       ret = pinctrl_gpio_direction_input(gc, offset);
> +       if (ret)
> +               return ret;
> +
> +       return gpiod_direction_input(pctrl->gpio[offset]);
> +}
> +
> +static int upboard_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +       int ret;
> +
> +       ret = pinctrl_gpio_direction_output(gc, offset);
> +       if (ret)
> +               return ret;
> +
> +       return gpiod_direction_output(pctrl->gpio[offset], value);
> +}

This looks dangerous and I guess also the reason you are including consumer.h.

Explain with a comment in the code what is going on here, like if this
GPIO comes from a completely different hardware unit, it looks like
a recepie for an eternal loop if it would point back to the same GPIO.

All of these have the same "loop out to another hardware" feature
that looks weird to me, but explain what's going on so I understand
it.

To me usually pin control works like this:

linux gpio <-> gpio driver <-> pin control driver

so the pin control driver is a pure "backend" for GPIO,
typically implements in struct pinmux_ops:
.gpio_request_enable()
.gpio_disable_free()
.gpio_set_direction()

that just set up the pin in the corresponding way. If your hardware
cannot mux back a pin from GPIO mode (as a comment says)
I would say that gpio_disable_free() can just return -ENODEV
or something if the pin has been put into gpio mode, maybe
some experimentation is needed there.

The corresponding GPIO driver typically uses GPIO ranges
to access the corresponding pin. It usually call
gpiochip_add_pin_range() to map its pins to the pin control
driver (if e.g. device tree is not used for the ranges).

What you do here is confusing to me, it looks like:

linux gpio <-> this gpio shim <-> pin control <-> other gpio driver

I think it is better to try to keep things separate if you can,
the current design seems to come from an attempt to be
"complete" and protect users from themselves, but we can
never protect users from themselves.

> +static int upboard_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct upboard_pinctrl *pctrl = container_of(gc, struct upboard_pinctrl, chip);
> +
> +       return gpiod_to_irq(pctrl->gpio[offset]);
> +}

If you use the GPIOLIB_IRQCHIP, you do not need to define this function
at all, it is handled by gpiolib.

> +       ret = gpiochip_add_pinlist_range(chip, dev_name(dev), 0, pctrl->pctrl_data->pin_header,
> +                                        pctrl->pctrl_data->ngpio);

I would rather have it that the actual gpio chip (the one that write
something into hardware registers) do this without another gpio chip
inbetween if you see what I mean.

But explain what's going on! I'm curious.

Yours,
Linus Walleij

Powered by blists - more mailing lists