linux-kernel - Re: [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC

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:   Fri, 9 Jun 2017 13:37:26 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Jianhong Chen <chenjh@...k-chips.com>,
        Heiko Stübner <heiko@...ech.de>
Cc:     Alexandre Courbot <gnurou@...il.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Tao Huang <huangtao@...k-chips.com>, tony.xie@...k-chips.com,
        zhangqing@...k-chips.com,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        w.egorov@...tec.de, Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        Linux Input <linux-input@...r.kernel.org>
Subject: Re: [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC

Heiko, can you please look at this patch.

On Thu, Jun 8, 2017 at 9:30 AM, Jianhong Chen <chenjh@...k-chips.com> wrote:

> From: chenjh <chenjh@...k-chips.com>

Full name please.

> RK805 has two configurable GPIOs that can be used for several
> purposes. These are output only.
>
> This driver is generic for other Rockchip PMICs to be added.
>
> Signed-off-by: chenjh <chenjh@...k-chips.com>

Dito.

Your commit message says they are output-only, yet you implement
.direction_input(). So what is is going to be?

> +#include <linux/i2c.h>
> +#include <linux/gpio.h>

Only use:
#include <linux/gpio/driver.h>

> +/*
> + * @mode: supported modes for this gpio, i.e. OUTPUT_MODE, OUTPUT_MODE...

Are you saying this should be an enum or a set of flags?

> +static int rk805_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret, val;
> +       struct rk805_gpio *gpio = gpiochip_get_data(chip);
> +
> +       ret = regmap_read(gpio->rk808->regmap, gpio->pins[offset].reg, &val);
> +       if (ret) {
> +               dev_err(gpio->dev, "gpio%d not support output mode\n", offset);
> +               return ret;
> +       }
> +
> +       return (val & gpio->pins[offset].val_msk) ? 1 : 0;

Do this:

return !!(val & gpio->pins[offset].val_msk)

> +static int rk805_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret;
> +       struct rk805_gpio *gpio = gpiochip_get_data(chip);
> +
> +       /* switch to gpio mode */
> +       if (gpio->pins[offset].func_mask) {
> +               ret = regmap_update_bits(gpio->rk808->regmap,
> +                                        gpio->pins[offset].reg,
> +                                        gpio->pins[offset].func_mask,
> +                                        gpio->pins[offset].func_mask);
> +               if (ret) {
> +                       dev_err(gpio->dev, "set gpio%d func failed\n", offset);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}

This is pin control. Why don't you implement a proper pin control
driver for this chip?

If you don't, this will just come back and haunt you.

Why not merge the driver into drivers/pinctrl/* and name it
pinctrl-rk805.c to begin with?

> +static const struct gpio_chip rk805_chip = {
> +       .label                  = "rk805-gpio",
> +       .owner                  = THIS_MODULE,
> +       .direction_input        = rk805_gpio_direction_input,
> +       .direction_output       = rk805_gpio_direction_output,

Please implement .get_direction()

> +       .get                    = rk805_gpio_get,
> +       .set                    = rk805_gpio_set,
> +       .request                = rk805_gpio_request,
> +       .base                   = -1,
> +       .ngpio                  = 2,
> +       .can_sleep              = true,

Consider assigning the .names[] array some pin names.

Yours,
Linus Walleij