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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3332 bytes --]

> Hi Lorenzo,
> 
> so these comments:
> 
> On Tue, Sep 24, 2024 at 12:12 PM Lorenzo Bianconi <lorenzo@kernel.org> 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.

ack, no worries.

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

ack, I will do in v6

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

ack, I will do in v6

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

ack.

Regards,
Lorenzo

> 
> Yours,
> Linus Walleij

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]