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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 22 Jun 2023 09:02:49 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     nick.hawkins@....com
Cc:     verdun@....com, brgl@...ev.pl, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, jdelvare@...e.com,
        linux@...ck-us.net, andy.shevchenko@...il.com,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
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@....com> wrote:

> From: Nick Hawkins <nick.hawkins@....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@....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