lkml.org 
[lkml]   [2020]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2020-11-11 15:39    [W:0.065 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site