Linus Walleij writes:

On Thu, Sep 3, 2020 at 3:35 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.

Signed-off-by: Lars Povlsen <redacted>

(...)

quoted

diff --git a/drivers/pinctrl/pinctrl-mchp-sgpio.c b/drivers/pinctrl/pinctrl-mchp-sgpio.c

Can we just spell it out
pinctrl-microchip-sgpio.c ?

The abbreviation seems arbitrary and unnecessary.

Well, not completely arbitrary, but maybe unnecessary... I'll change
that. I'll also change that for any symbols/defines off course.

I do see that this chip is using the pinctrl abstractions (very nicely)
and should be under drivers/pinctrl/*.

Sadly it doesn't mean the bindings need to be in pinctrl ... unless you
plan to add pinctrl bindings later.

quoted

+config PINCTRL_MCHP_SGPIO
+       bool "Pinctrl driver for Microsemi/Microchip Serial GPIO"
+       depends on OF
+       depends on HAS_IOMEM
+       select GPIOLIB
+       select GENERIC_PINCONF
+       select GENERIC_PINCTRL_GROUPS
+       select GENERIC_PINMUX_FUNCTIONS

Nice use of these abstractions!

Thanks!

quoted

+// SPDX-License-Identifier: (GPL-2.0 OR MIT)

What's up with this OR MIT? I'd like Kate to OK this.

quoted

+#define MCHP_SGPIOS_PER_BANK   32
+#define MCHP_SGPIO_BANK_DEPTH  4
+
+#define PIN_NAM_SZ     (sizeof("SGPIO_D_pXXbY")+1)
+
+enum {
+       REG_INPUT_DATA,
+       REG_PORT_CONFIG,
+       REG_PORT_ENABLE,
+       REG_SIO_CONFIG,
+       REG_SIO_CLOCK,
+       MAXREG
+};
+
+struct mchp_sgpio_props {

Just call it struct microchip_gpio_variant it is easier to read and
does not abbreviate randomly, also it is a per-variant set of properties
so calling it variant is more to the point.

Fine.

quoted

+struct mchp_sgpio_priv {

I would just spell it out struct microchip_sgpio, it is implicit that
the struct is private since it is defined in a c file.

Fine.

quoted

+struct mchp_sgpio_port_addr {

struct microchip_sgpio_port_addr

(Admittedly this is a bit about taste.)

quoted

+static inline void sgpio_writel(struct mchp_sgpio_priv *priv,
+                               u32 val, u32 rno, u32 off)
+{
+       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
+
+       writel(val, reg);
+}
+
+static inline void sgpio_clrsetbits(struct mchp_sgpio_priv *priv,
+                                   u32 rno, u32 off, u32 clear, u32 set)
+{
+       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
+       u32 val = readl(reg);
+
+       val &= ~clear;
+       val |= set;
+
+       writel(val, reg);
+}

This looks like a reimplementation of regmap_update_bits for a bit,
have you considered just using regmap? It's pretty simple.

Well, the registers are not in a regmap, so I did not consider that. The
utility function also serves to abstract the variant register address.

quoted

+static int mchp_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+
+       /* Fixed-position function */
+       return sgpio_is_input(priv, gpio) ? 0 : -EINVAL;
+}
+
+static int mchp_sgpio_direction_output(struct gpio_chip *gc,
+                                      unsigned int gpio, int value)
+{
+       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+       struct mchp_sgpio_port_addr addr;
+
+       sgpio_pin_to_addr(priv, gpio, &addr);
+
+       /* Fixed-position function */
+       if (addr.input)
+               return -EINVAL;
+
+       sgpio_output_set(priv, &addr, value);
+
+       return 0;
+}

This looks like the right way to handle this!

I'm glad you think so...

quoted

+static int mchp_sgpio_of_xlate(struct gpio_chip *gc,
+                              const struct of_phandle_args *gpiospec,
+                              u32 *flags)
+{
+       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+       int pin, base;
+
+       if (gpiospec->args[0] > MCHP_SGPIOS_PER_BANK ||
+           gpiospec->args[1] > priv->bitcount)
+               return -EINVAL;
+       base = priv->bitcount * gpiospec->args[0];
+       pin = base + gpiospec->args[1];
+       /* Add to 2nd half of output range if output */
+       if (gpiospec->args[2] == PIN_OUTPUT)
+               pin += (priv->ngpios / 2);
+
+       if (pin > gc->ngpio)
+               return -EINVAL;
+
+       if (flags)
+               *flags = gpiospec->args[3];
+
+       return pin;
+}

I don't like this. I would certainly prefer the driver to just use standard
GPIO bindings. I do not understand why this is necessary.

If for nothing else, there should be a big comment explaining this.

The only real problem I have with the driver is this extra flag tagged onto
all the GPIOs, this seems unnecessary, and something the hardware
driver should already know from the compatible string.

I hope my previous comments have cleared this up.

Yours,
Linus Walleij

Thank you for your time and comments!

---Lars

-- 
Lars Povlsen,
Microchip

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