Re: [PATCH v6 5/6] pinctrl: airoha: Add support for EN7581 SoC

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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