From: Lars Povlsen <lars.povlsen@microchip.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Lars Povlsen <lars.povlsen@microchip.com>,
Kate Stewart <kstewart@linuxfoundation.org>,
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: Sun, 13 Sep 2020 21:28:05 +0200 [thread overview]
Message-ID: <87pn6pwk6y.fsf@soft-dev15.microsemi.net> (raw)
In-Reply-To: <CACRpkdbUv4r+Cghs1e4OEFCW4Yd1bGGQcWZ5TEb2uDWnVXhPSw@mail.gmail.com>
Linus Walleij writes:
> 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.
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.
>
>> +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!
>
>> +// 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.
>
Fine.
>> +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.
>> +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.
>
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.
>> +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...
>
>> +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
next prev parent reply other threads:[~2020-09-13 19:28 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
2020-09-13 19:28 ` Lars Povlsen [this message]
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=87pn6pwk6y.fsf@soft-dev15.microsemi.net \
--to=lars.povlsen@microchip.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=alexandre.belloni@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=kstewart@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).