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
