linux-kernel - Re: [PATCH v9 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Nov 2020 16:38:29 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Lars Povlsen <lars.povlsen@...rochip.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [PATCH v9 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl
 driver for Microsemi Serial GPIO

On Wed, Nov 11, 2020 at 2:25 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.
>
> 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.

...

> + * Author: <lars.povlsen@...rochip.com>

No First Name Last Name?

...

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

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

...

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

> +       .pin_config_config_dbg_show = pinconf_generic_dump_config,

Do you need this? I mean isn't it default by pin core?

> +};

...

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

> +}

...

> +       if (priv->in.gpio.ngpio != priv->out.gpio.ngpio) {
> +               dev_err(dev, "Banks must have same GPIO count\n");
> +               return -EINVAL;

-ERANGE?

> +       }

-- 
With Best Regards,
Andy Shevchenko