> 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.
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:
signature.asc
Description: PGP signature
