linux-kernel - Re: [PATCH v4 1/2] gpio: Add driver for PC Engines APU boards

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]

Message-ID: <CACRpkdZx-GgfgfGmB6KmTjzBEMnKyq3pUsHsgaZ1uBooN_sj9g@mail.gmail.com>
Date:   Mon, 19 Nov 2018 14:46:06 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     fe@....tdt.de
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>, piotr.krol@...eb.com,
        Eckert.Florian@...glemail.com, Darren Hart <dvhart@...radead.org>,
        platform-driver-x86 <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] gpio: Add driver for PC Engines APU boards

On Thu, Nov 15, 2018 at 2:32 PM Florian Eckert <fe@....tdt.de> wrote:

> Add a new device driver "gpio-apu" which will handle the GPIOs on APU2
> and APU3 devices from PC Engines.
>
> APU2 (https://pcengines.ch/schema/apu2c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
>
> APU3 (https://pcengines.ch/schema/apu3c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
> - G33 is "simswap" connected to SIM switch IC to swap the SIM between
>   mPCIe2 and mPCIe3 slot
>
> Signed-off-by: Florian Eckert <fe@....tdt.de>

This is looking better and better! Thanks to everyone helping out
and thanks for your perseverance Florian!

> +config GPIO_APU
> +       tristate "PC Engines APU2/APU3 GPIO support"
> +       depends on X86
> +       select GPIO_GENERIC

Excellent idea.

But it seems you are not using this. You would be using
it if you used bgpio_init() but if I understand correctly this
driver cannot use that because this GPIO is something like
one register per pin, correct?

Let me suggest:

> +struct apu_gpio_pdata {
> +       struct platform_device *pdev;
> +       struct gpio_chip *chip;

Make that a real member of this struct and not a pointer.
I.e. just remove the "*".

> +static struct apu_gpio_pdata *apu_gpio;

Why a static local? It seems you can just pass around
the pointer.

> +static int gpio_apu_get_dir(struct gpio_chip *chip, unsigned int offset)
> +{
> +       u32 val;
> +
> +       spin_lock(&apu_gpio->lock);
> +
> +       val = ~ioread32(apu_gpio->addr[offset]);
> +       val = (val >> APU_GPIO_BIT_DIR) & 1;

I would just:

#include <linux/bits.h>

val = ~ioread32(apu_gpio->addr[offset]);
spin_unlock();

return !!(val & BIT(APU_GPIO_BIT_DIR));

This clamps the value to [0,1] in a nice way.

> +static int gpio_apu_get_data(struct gpio_chip *chip, unsigned int offset)
> +{
> +       u32 val;
> +
> +       spin_lock(&apu_gpio->lock);
> +
> +       val = ioread32(apu_gpio->addr[offset]);
> +
> +       spin_unlock(&apu_gpio->lock);
> +
> +       return (val >> APU_GPIO_BIT_RD) & 1;

return !!(val & BIT(APU_GPIO_BIT_RD));

> +       return devm_gpiochip_add_data(&pdev->dev, apu_gpio->chip, NULL);

Instead of passing NULL pas apu_gpio as the last argument and in
all callbacks you can use:

struct apu_gpio_pdata *apu_gpio = gpiochip_get_data(gc);

To get a pointer to the per-instance state container.

Yours,
Linus Walleij