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
