> 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