linux-kernel - Re: [PATCH v6 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO

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, 16 Oct 2020 17:34:15 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Lars Povlsen <lars.povlsen@...rochip.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [PATCH v6 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl
 driver for Microsemi Serial GPIO

On Wed, Oct 14, 2020 at 6:25 PM Lars Povlsen <lars.povlsen@...rochip.com> wrote:
>
> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO
> (SGPIO) device used in various SoC's.

...

> +#define PIN_NAM_SZ     (sizeof("SGPIO_D_pXXbY")+1)

+1 for what?

...

> +#define __shf(x)               (__builtin_ffsll(x) - 1)
> +#define __BF_PREP(bf, x)       (bf & ((x) << __shf(bf)))
> +#define __BF_GET(bf, x)                (((x & bf) >> __shf(bf)))

This smells like bitfield.h.

...

> +static int sgpio_input_get(struct sgpio_priv *priv,
> +                          struct sgpio_port_addr *addr)
> +{

> +       int ret;
> +
> +       ret = !!(sgpio_readl(priv, REG_INPUT_DATA, addr->bit) &
> +                BIT(addr->port));
> +
> +       return ret;

Sounds like one line.

> +}

> +static int sgpio_get_functions_count(struct pinctrl_dev *pctldev)
> +{

> +       return 1;

I didn't get why it's not a pure GPIO driver?
It has only one function (no pinmux).
I didn't find any pin control features either.

What did I miss?

...

> +static int microchip_sgpio_get_value(struct gpio_chip *gc, unsigned int gpio)
> +{
> +       struct sgpio_bank *bank = gpiochip_get_data(gc);
> +       struct sgpio_priv *priv = bank->priv;
> +       struct sgpio_port_addr addr;

> +       int ret;

No need.

> +
> +       sgpio_pin_to_addr(priv, gpio, &addr);
> +
> +       if (bank->is_input)
> +               ret = sgpio_input_get(priv, &addr);
> +       else
> +               ret = sgpio_output_get(priv, &addr);
> +
> +       return ret;
> +}


...


> +       ret = devm_gpiochip_add_data(dev, gc, bank);
> +       if (ret == 0)

> +               dev_info(dev, "Registered %d GPIOs\n", ngpios);

No noise.

> +       else
> +               dev_err(dev, "Failed to register: ret %d\n", ret);
> +

...

> +       /* Get register map */
> +       regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv->regs = devm_ioremap_resource(dev, regs);

devm_platform_ioremap_resource();

> +       if (IS_ERR(priv->regs))
> +               return PTR_ERR(priv->regs);

> +       priv->properties = of_device_get_match_data(dev);

It's interesting you have a mix between OF APIs and device property
APIs. Choose one. If you stick with OF, use of_property_ and so,
otherwise replace of_*() by corresponding device_*() or generic calls.

Can you use gpio-regmap APIs?

-- 
With Best Regards,
Andy Shevchenko