Re: [PATCH v4 2/5] gpio: gxp: Add HPE GXP GPIO PL

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Nick,

thanks for your patch!

This is looking pretty good, I have some minor questions.

On Wed, Jun 21, 2023 at 11:35 PM <nick.hawkins@xxxxxxx> wrote:

> From: Nick Hawkins <nick.hawkins@xxxxxxx>
>
> The GXP SoC supports GPIO on multiple interfaces. The interfaces are
> CPLD and Host. The gpio-gxp-pl driver covers the CPLD which takes
> physical I/O from the board and shares it with GXP via a proprietary
> interface that maps the I/O onto a specific register area of the GXP.
> This driver supports interrupts from the CPLD.
>
> Signed-off-by: Nick Hawkins <nick.hawkins@xxxxxxx>

(...)
> +enum pl_gpio_pn {
> +       IOP_LED1 = 0,
> +       IOP_LED2 = 1,
> +       IOP_LED3 = 2,
> +       IOP_LED4 = 3,
(...)

The confusing bit here is that GPIO means
*generic purpose input/output*
and these use cases are hardcoded into the driver and
do not look very generic purpose at all.

But I understand that it is convenient. I would add some
comment saying that if there is a new version with a
different layout of the pins, we need to make this kind
of stuff go away and just use the numbers.

> +static const struct gpio_chip template_chip = {
> +       .label                  = "gxp_gpio_plreg",
> +       .owner                  = THIS_MODULE,
> +       .get                    = gxp_gpio_pl_get,
> +       .set                    = gxp_gpio_pl_set,
> +       .get_direction = gxp_gpio_pl_get_direction,
> +       .direction_input = gxp_gpio_pl_direction_input,
> +       .direction_output = gxp_gpio_pl_direction_output,
> +       .base = -1,
> +};

Neat! Since you so explicitly have assigned a meaning to each
GPIO line, you can go ahead and assign the .names property as
well. Check in the kernel tree for other drivers doing this.

> +       drvdata->chip = template_chip;
> +       drvdata->chip.ngpio = 80;

If you're always assigning 80 to this you can just put that in the
template as well.

Other than that I think it looks good!

Yours,
Linus Walleij