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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Lars Povlsen <lars.povlsen@microchip.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	devicetree <devicetree@vger.kernel.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>
Subject: Re: [PATCH v6 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO
Date: Fri, 16 Oct 2020 17:34:15 +0300	[thread overview]
Message-ID: <CAHp75Vdd6ECJaWytYVz+5GYZrwybzZmviUOt3H=t-4LH=_idKg@mail.gmail.com> (raw)
In-Reply-To: <20201014100707.2728637-3-lars.povlsen@microchip.com>

On Wed, Oct 14, 2020 at 6:25 PM Lars Povlsen <lars.povlsen@microchip.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