devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Lars Povlsen <lars.povlsen@microchip.com>,
	Kate Stewart <kstewart@linuxfoundation.org>
Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>
Subject: Re: [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO
Date: Sat, 12 Sep 2020 13:11:02 +0200	[thread overview]
Message-ID: <CACRpkdbUv4r+Cghs1e4OEFCW4Yd1bGGQcWZ5TEb2uDWnVXhPSw@mail.gmail.com> (raw)
In-Reply-To: <20200903133528.8595-3-lars.povlsen@microchip.com>

On Thu, Sep 3, 2020 at 3:35 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.
>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
(...)

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

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.

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

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

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

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

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

> +struct mchp_sgpio_port_addr {

struct microchip_sgpio_port_addr

(Admittedly this is a bit about taste.)

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

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

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

Yours,
Linus Walleij

  reply	other threads:[~2020-09-12 11:11 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-03 13:35 [PATCH v2 0/3] pinctrl: Adding support for Microchip/Microsemi serial GPIO controller Lars Povlsen
2020-09-03 13:35 ` [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver Lars Povlsen
2020-09-12 10:50   ` Linus Walleij
2020-09-13 19:11     ` Lars Povlsen
2020-09-30  9:21       ` Linus Walleij
2020-10-05  8:21         ` Lars Povlsen
2020-09-03 13:35 ` [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO Lars Povlsen
2020-09-12 11:11   ` Linus Walleij [this message]
2020-09-13 19:28     ` Lars Povlsen
2020-09-03 13:35 ` [PATCH v2 3/3] arm64: dts: sparx5: Add SGPIO devices Lars Povlsen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACRpkdbUv4r+Cghs1e4OEFCW4Yd1bGGQcWZ5TEb2uDWnVXhPSw@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=devicetree@vger.kernel.org \
    --cc=kstewart@linuxfoundation.org \
    --cc=lars.povlsen@microchip.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).