Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 11 Nov 2020 16:38:29 +0200 | Subject | Re: [PATCH v9 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO |
| |
On Wed, Nov 11, 2020 at 2: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. > > 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@microchip.com>
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
| |