Andy Shevchenko writes:

On Wed, Nov 11, 2020 at 2:25 PM Lars Povlsen [off-list ref] wrote:

quoted

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!

...

quoted

+ * Author: [off-list ref]

No First Name Last Name?

I'll add that.

...

quoted

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

quoted

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

...

quoted

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

quoted

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

quoted

+};

...

quoted

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

quoted

+}

...

quoted

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

quoted

+       }

Thanks,

---Lars

--
Lars Povlsen,
Microchip

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel