Re: [PATCH v4 4/5] pinctrl: airoha: Add support for EN7581 SoC

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Lorenzo,

so these comments:

On Tue, Sep 24, 2024 at 12:12 PM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:

> > > +#include <linux/pinctrl/consumer.h>
> >
> > Why do you need the consumer header?
>
> we need it for pinctrl_gpio_direction_output() and
> pinctrl_gpio_direction_input() for direction_input and direction_output
> callbacks.

I looked it over again and it looks good, I was just confused.

> > > +               arg = airoha_pinctrl_gpio_get_direction(pinctrl, gpio);
> >
> > I don't see why a pin would have to exist in a GPIO range in order to
> > be set as output or input?
> >
> > Can't you just set up the pin as requested and not care whether
> > it has a corresponding GPIO range?
> >
> > Is it over-reuse of the GPIO code? I'd say just set up the pin instead.
>
> Do you mean to get rid of PIN_CONFIG_OUTPUT_ENABLE, PIN_CONFIG_INPUT_ENABLE
> (and even PIN_CONFIG_OUTPUT in airoha_pinconf_set()) here?
> E.g. we need PIN_CONFIG_OUTPUT_ENABLE to enable pwm for pwm-leds:

I was mainly thinking that the
airoha_pinctrl_gpio_get_direction() is limited to pins that are
used for GPIO.

The callback should be usable on any pins, no matter if they
can be muxed to GPIO or not?

> &mfd {
>         ...
>         pio: pinctrl {
>                 ...
>                 pwm_gpio18_idx10_pins: pwm-gpio18-idx10-pins {
>                         function = "pwm";
>                         pins = "gpio18";
>                         output-enable;
>                 };
>         };
> };

Like this one.

Which I think works.

It's the name of the function which confuses me:
airoha_pinctrl_gpio_get_direction() and anything else that
is used directly from the airoha_pinconf_set() function
doesn't really care if the pin is used as GPIO or not does
it?

Can you rename the functions just e.g. airoha_pinctrl_get_direction()
because it has nothing to do with GPIO. It's jus pin control.

Also some defines are confusing this way:

+       /* set output enable */
+       mask = BIT(gpio % AIROHA_GPIO_BANK_SIZE);
+       index = gpio / AIROHA_GPIO_BANK_SIZE;
+       airoha_pinctrl_rmw(pinctrl, pinctrl->gpiochip.out[index],
+                          mask, !input ? mask : 0);

Variables named "gpio" and AIROHA_GPIO_BANK_SIZE despite
it is used for pins that are not (in the Linux sense) GPIO all the time.
This is a big confusion for the mind.

Can you rename the variable from "gpio" to "pin" or so
and the AIROHA_GPIO_BANK_SIZE to AIROHA_PIN_BANK_SIZE
etc so it is clear what is going on?

I understand that the datasheet might be talking about
"GPIO this and GPIO that" but what hardware engineers mean
with GPIO is something else than what Linux mean: for them
it means "it can be muxed so it is kinda-general-purpose-kinda"
but in Linux this has a strict meaning: it can be used by the
gpiolib to control individual lines.

I think this would make it easier for me (and possibly others)
ton understand the driver.

Yours,
Linus Walleij