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] [day] [month] [year] [list]

Message-ID: <87v9e94o5d.fsf@microchip.com>
Date:   Fri, 13 Nov 2020 10:11:26 +0100
From:   Lars Povlsen <lars.povlsen@...rochip.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Lars Povlsen <lars.povlsen@...rochip.com>,
        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


Andy Shevchenko writes:

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

Well I am certainly glad to hear that!

>
> ...
>
>> + * Author: <lars.povlsen@...rochip.com>
>
> No First Name Last Name?
>

I'll add that.


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

Fine.

>
>> +       }
>> +       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?

No, I see other drivers also setting this up explicitly.

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

This checks whether the requested signal is actually enabled in the
bitstream. If it is not, it will trigger a warning message. I recon it
should also signal this with the error code, so I'll add that.

Generic code does not have knowledge about the bit stream configuration
(priv->ports), so it can't check for that.

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

We can do that.

>
>> +       }

Thanks,

---Lars

--
Lars Povlsen,
Microchip