linux-kernel - Re: [PATCH v2 bus+gpio 3/4] drivers: gpio: Add support for GPIOs over Moxtet bus

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] [day] [month] [year] [list]

Date:   Fri, 9 Nov 2018 11:12:47 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     marek.behun@....cz
Cc:     ext Tony Lindgren <tony@...mide.com>,
        Shawn Guo <shawnguo@...nel.org>, Rob Herring <robh@...nel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 bus+gpio 3/4] drivers: gpio: Add support for GPIOs over
 Moxtet bus

On Fri, Nov 2, 2018 at 11:35 AM Marek Behún <marek.behun@....cz> wrote:

> This adds support for interpreting the input and output bits of one
> device on Moxtet bus as GPIOs.
> This is needed for example by the SFP cage module of Turris Mox.
>
> Signed-off-by: Marek Behún <marek.behun@....cz>

Overall looks fine, it's OK like this just adding some nitpicking:

> +static const struct moxtet_gpio_desc descs[] = {
> +       [TURRIS_MOX_MODULE_SFP] = {
> +               .in_mask = 0x7,
> +               .out_mask = 0x30,
> +       },
> +};

You can actually use things like:

.in_mask = GENMASK(3,0);

ton indicate exactly which bits you hit, but maybe it is overkill.
(No big deal.)

> +       return (ret & BIT(offset)) ? 1 : 0;

I would usually just clamp like this:

return !!(red & BIT(offset));

> +static int moxtet_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct moxtet_gpio_chip *chip = gpiochip_get_data(gc);
> +
> +       if (chip->desc->in_mask & BIT(offset))
> +               return 1;
> +       else if (chip->desc->out_mask & BIT(offset))
> +               return 0;
> +       else
> +               return -EINVAL;

Maybe insert a comment here that a line is hard wired
as either input or output.

Anyways:
Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

Yours,
Linus Walleij