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

[Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>]

Andy Shevchenko writes:

> 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.

Well I am certainly glad to hear that!

>
> ...
>
>> + * Author: <lars.povlsen@xxxxxxxxxxxxx>
>
> No First Name Last Name?
>

I'll add that.


> ...
>
>> +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.

Fine.

>
>> +       }
>> +       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?

No, I see other drivers also setting this up explicitly.

>
>> +};
>
> ...
>
>> +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.
>

This checks whether the requested signal is actually enabled in the
bitstream. If it is not, it will trigger a warning message. I recon it
should also signal this with the error code, so I'll add that.

Generic code does not have knowledge about the bit stream configuration
(priv->ports), so it can't check for that.

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

We can do that.

>
>> +       }

Thanks,

---Lars

--
Lars Povlsen,
Microchip