On Sun, Oct 13, 2024 at 12:08 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
> Introduce pinctrl driver for EN7581 SoC. Current EN7581 pinctrl driver
> supports the following functionalities:
> - pin multiplexing
> - pin pull-up, pull-down, open-drain, current strength,
> {input,output}_enable, output_{low,high}
> - gpio controller
> - irq controller
>
> Tested-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> Co-developed-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> Signed-off-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
Nitpicks follow:
I would have changed the below:
+ pinctrl->gpiochip.data = gpio_data_regs;
+ pinctrl->gpiochip.dir = gpio_dir_regs;
+ pinctrl->gpiochip.out = gpio_out_regs;
+ pinctrl->gpiochip.status = irq_status_regs;
+ pinctrl->gpiochip.level = irq_level_regs;
+ pinctrl->gpiochip.edge = irq_edge_regs;
Can't you just use e.g.
chip->data = ... etc in the top section?
+ chip->parent = dev;
+ chip->label = dev_name(dev);
+ chip->request = gpiochip_generic_request;
+ chip->free = gpiochip_generic_free;
+ chip->direction_input = pinctrl_gpio_direction_input;
+ chip->direction_output = airoha_gpio_direction_output;
+ chip->set = airoha_gpio_set;
+ chip->get = airoha_gpio_get;
+ chip->base = -1;
+ chip->ngpio = AIROHA_NUM_PINS;
I always call that varible "gc" rather than chip, but no big deal.
+ chip->irq.default_type = IRQ_TYPE_NONE;
+ chip->irq.handler = handle_simple_irq;
+ gpio_irq_chip_set_chip(&chip->irq, &airoha_gpio_irq_chip);
I usually declare a local variable
struct gpio_irq_chip *girq;
girq = &chip->irq;
girq->default_type =...
Yours,
Linus Walleij
