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
