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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Nov 11, 2020 at 2: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.
>
> The driver is added as a pinctrl driver, albeit only having just GPIO
> support currently. The hardware supports other functions that will be
> added following.

Thanks for an update!
Seems closer to the final. My comments below.

...

> + * Author: <lars.povlsen@xxxxxxxxxxxxx>

No First Name Last Name?

...

> +static int sgpio_output_get(struct sgpio_priv *priv,
> +                           struct sgpio_port_addr *addr)
> +{
> +       u32 val, portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port);
> +       unsigned int bit = SGPIO_SRC_BITS * addr->bit;
> +
> +       switch (priv->properties->arch) {
> +       case SGPIO_ARCH_LUTON:
> +               val = FIELD_GET(SGPIO_LUTON_BIT_SOURCE, portval);
> +               break;
> +       case SGPIO_ARCH_OCELOT:
> +               val = FIELD_GET(SGPIO_OCELOT_BIT_SOURCE, portval);
> +               break;
> +       case SGPIO_ARCH_SPARX5:
> +               val = FIELD_GET(SGPIO_SPARX5_BIT_SOURCE, portval);
> +               break;
> +       default:
> +               val = 0;

Missed break; statement.

> +       }
> +       return !!(val & BIT(bit));
> +}

...

> +static const struct pinconf_ops sgpio_confops = {
> +       .is_generic = true,
> +       .pin_config_get = sgpio_pinconf_get,
> +       .pin_config_set = sgpio_pinconf_set,

> +       .pin_config_config_dbg_show = pinconf_generic_dump_config,

Do you need this? I mean isn't it default by pin core?

> +};

...

> +static int sgpio_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                    struct pinctrl_gpio_range *range,
> +                                    unsigned int offset)
> +{
> +       struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev);
> +       struct sgpio_priv *priv = bank->priv;
> +       struct sgpio_port_addr addr;
> +
> +       sgpio_pin_to_addr(priv, offset, &addr);
> +
> +       if ((priv->ports & BIT(addr.port)) == 0) {
> +               dev_warn(priv->dev, "Request port %d.%d: Port is not enabled\n",
> +                        addr.port, addr.bit);
> +       }
> +
> +       return 0;

I believe this function also does some sanity checks. Perhaps you need
to call a generic one.
Hence check what should be done in the tear down case.

> +}

...

> +       if (priv->in.gpio.ngpio != priv->out.gpio.ngpio) {
> +               dev_err(dev, "Banks must have same GPIO count\n");
> +               return -EINVAL;

-ERANGE?

> +       }

-- 
With Best Regards,
Andy Shevchenko