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

From: Andy Shevchenko
Date: Fri Oct 16 2020 - 10:33:28 EST


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