| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87pn6pwk6y.fsf@soft-dev15.microsemi.net>
Date: Sun, 13 Sep 2020 21:28:05 +0200
From: Lars Povlsen <lars.povlsen@...rochip.com>
To: Linus Walleij <linus.walleij@...aro.org>
CC: Lars Povlsen <lars.povlsen@...rochip.com>,
Kate Stewart <kstewart@...uxfoundation.org>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO
Linus Walleij writes:
> On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@...rochip.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@...rochip.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
Powered by blists - more mailing lists