linux-kernel - Re: [PATCH v3 1/2] GPIO: add single-register GPIO via CREG driver

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: <CACRpkdawC77DMggf1ye1=tXUDbzBqWfw9=OVV5w49Aip6cKGug@mail.gmail.com>
Date:   Wed, 26 Sep 2018 09:18:21 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Eugeniy.Paltsev@...opsys.com
Cc:     "open list:SYNOPSYS ARC ARCHITECTURE" 
        <linux-snps-arc@...ts.infradead.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Vineet Gupta <Vineet.Gupta1@...opsys.com>,
        Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
        Rob Herring <robh+dt@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v3 1/2] GPIO: add single-register GPIO via CREG driver

Hi Eugeniy,

sorry for the delay, the driver is different from what I'm
used to so I needed focuses attention and it took some time.

On Tue, Sep 11, 2018 at 5:09 PM Eugeniy Paltsev
<Eugeniy.Paltsev@...opsys.com> wrote:

> Add single-register MMIO GPIO driver for complex cases where
> only several fields in register belong to GPIO lines and each GPIO
> line owns a field with different length and on/off value.
>
> Such CREG GPIOs are used in Synopsys AXS10x and HSDK boards.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>
> ---
> Changes v2->v3:
>  * Move parameters into a lookup table instead of device tree.
>  * Use the ngpios attribute for instead of snps,ngpios.

This is looking good! Just a few small things I want you to fix
(we can certainly queue this for the next kernel):

> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/of_gpio.h>

Replace this with:
#include <linux/gpio/driver.h>

Drivers should only need to include that file. (Also move away from
mm_gpio_chip as per below.)

> +#include "gpiolib.h"

Why?

> +struct creg_gpio {
> +       struct of_mm_gpio_chip mmchip;

I would prefer to move away from using this. Just create a regular driver
please. Just struct gpio_chip and add a member
void __iomem *base; instead of referring to mmchip.regs.

In fact I want to rewrite all mm_gpio_chips but I just haven't had time.

> +static void creg_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)

Please rename the variable "gpio" to "offset".

This is clearer because "gpio" becomes quite ambigous.

> +static int creg_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +       return 0; /* output */
> +}

I think with the recent code changes in gpiolib you do not need to define
this function at all.
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=devel&id=ae9847f48a4b4bff0335da20be63ac84d94eb54c

> +static int creg_gpio_xlate(struct gpio_chip *gc,
> +                          const struct of_phandle_args *gpiospec, u32 *flags)
> +{
> +       if (gpiospec->args_count != 1) {
> +               dev_err(&gc->gpiodev->dev, "invalid args_count: %d\n",
> +                       gpiospec->args_count);
> +               return -EINVAL;
> +       }
> +
> +       if (gpiospec->args[0] >= gc->ngpio) {
> +               dev_err(&gc->gpiodev->dev, "gpio number is too big: %d\n",
> +                       gpiospec->args[0]);
> +               return -EINVAL;
> +       }
> +
> +       return gpiospec->args[0];
> +}

Another reason to not use mm_gpio_chip: just rely on standard twocell
translation and you can delete this.

> +       /* Check that we suit in 32 bit register */

s/suit/fit

Yours,
Linus Walleij