linux-kernel - Re: [PATCH 2/2] pinctrl: sprd: Add Spreadtrum pin control 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: <CACRpkdaBcGNTeUDCcVgK9JKZUPKD-qfO8PVHURR2F_pgvEgViQ@mail.gmail.com>
Date:   Mon, 29 May 2017 18:28:49 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Baolin Wang <baolin.wang@...eadtrum.com>
Cc:     Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        Baolin Wang <baolin.wang@...aro.org>
Subject: Re: [PATCH 2/2] pinctrl: sprd: Add Spreadtrum pin control driver

On Sat, May 27, 2017 at 7:56 AM, Baolin Wang <baolin.wang@...eadtrum.com> wrote:

> This patch adds the pin control driver for Spreadtrum SC9860 platform.
>
> Signed-off-by: Baolin Wang <baolin.wang@...eadtrum.com>

Overall I see that you want to store functions and groups in the device tree
using the current helpers from Tony. That is OK if you want to take that
approach, though I prefer the pins/groups/function encoding in plaintext.

But when it comes to pin config:

> +static int sprd_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin_id,
> +                           unsigned long *config)
> +{
> +       struct sprd_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       struct sprd_pin *pin = sprd_pinctrl_get_pin_by_id(pctl, pin_id);
> +
> +       if (!pin)
> +               return -EINVAL;
> +
> +       if (pin->type == GLOBAL_CTRL_PIN) {
> +               *config = (readl((void __iomem *)pin->reg) >>
> +                          pin->bit_offset) & PINCTRL_BIT_MASK(pin->bit_width);
> +       } else {
> +               *config = readl((void __iomem *)pin->reg);
> +       }
> +
> +       return 0;
> +}
> +
> +static int sprd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin_id,
> +                           unsigned long *configs, unsigned int num_configs)
> +{
> +       struct sprd_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       struct sprd_pin *pin = sprd_pinctrl_get_pin_by_id(pctl, pin_id);
> +       unsigned long reg;
> +       int i;
> +
> +       if (!pin)
> +               return -EINVAL;
> +
> +       for (i = 0; i < num_configs; i++) {
> +               if (pin->type == GLOBAL_CTRL_PIN) {
> +                       reg = readl((void __iomem *)pin->reg);
> +                       reg &= ~(PINCTRL_BIT_MASK(pin->bit_width)
> +                               << pin->bit_offset);
> +                       reg |= (configs[i] & PINCTRL_BIT_MASK(pin->bit_width))
> +                               << pin->bit_offset;
> +                       writel(reg, (void __iomem *)pin->reg);
> +               } else {
> +                       writel(configs[i], (void __iomem *)pin->reg);
> +               }
> +               pin->config = configs[i];
> +       }
> +
> +       return 0;
> +}

This is just hammering the register values, you have effectively defined your
register layout as your device tree ABI.

Please don't do this, please use genric pin config and use the approach
other drivers take with explicit strings encoding the pin configuration.

Even pinctrl-single that maintain quite a bit of muxing data in the device
tree still use the generic pin config API, see:
drivers/pinctrl/pinctrl-single.c
pcs_pinconf_get(), pcs_pinconf_set()

Same for group config.

Yours,
Linus Walleij