Messages in this thread | | | From | Linus Walleij <> | Date | Thu, 22 Jun 2023 09:02:49 +0200 | Subject | Re: [PATCH v4 2/5] gpio: gxp: Add HPE GXP GPIO PL |
| |
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@hpe.com> wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com> > > 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@hpe.com>
(...) > +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
| |