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

[Lars Povlsen <lars.povlsen@microchip.com>]

Andy Shevchenko writes:

> On Thu, Oct 29, 2020 at 3:40 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.
>
> First Q is can you use gpio-regmap?
> Second one, why this driver is a pin control? I haven't found any
> evidence it can be plain GPIO.

I think I responded in <87blgp9hhv.fsf@soft-dev15.microsemi.net>, did
you not see that?

>
> Also note, if comment is given about one part of the code, you need to
> check all the rest which are similar and address accordingly.
>
> ...
>
>> +config PINCTRL_MICROCHIP_SGPIO
>> +       bool "Pinctrl driver for Microsemi/Microchip Serial GPIO"
>
>> +       depends on OF
>
> I think this is not needed, see below.

I will remove the OF dependency, I think you are right it can be
done. I just did not see that as a goal in itself.

>
>> +       depends on HAS_IOMEM
>> +       select GPIOLIB
>> +       select GENERIC_PINCONF
>> +       select GENERIC_PINCTRL_GROUPS
>> +       select GENERIC_PINMUX_FUNCTIONS
>
>> +       select OF_GPIO
>
> ...neither this...

OK.

>
>> +       help
>> +         Support for the serial GPIO interface used on Microsemi and
>> +         Microchip SoC's. By using a serial interface, the SIO
>> +         controller significantly extends the number of available
>> +         GPIOs with a minimum number of additional pins on the
>> +         device. The primary purpose of the SIO controller is to
>> +         connect control signals from SFP modules and to act as an
>> +         LED controller.
>
> ...
>
> Missed header here, like bits.h.

I'll add that.

>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>
> I think this driver is OF independent, if you convert the OF leftovers
> to device_/fwnode_ API.

As stated, I'll be removing the OF parts.

>
> Then you need to drop these headers (most of them actually are
> redundant even now) and add property.h. Also you missed
> mod_devicetable.h.
>
>> +#include <linux/clk.h>
>> +#include <linux/pinctrl/pinctrl.h>
>> +#include <linux/pinctrl/pinmux.h>
>> +#include <linux/pinctrl/pinconf.h>
>> +#include <linux/pinctrl/pinconf-generic.h>
>> +#include <linux/platform_device.h>
>
> Perhaps ordered and linux/pinctrl/ be grouped after generic ones?

Sure, I can do that. There's some that *are* needed, but your're right
that some are redundant.

>
> ...
>
>> +#define __shf(x)               (__builtin_ffs(x) - 1)
>> +#define __BF_PREP(bf, x)       (bf & ((x) << __shf(bf)))
>> +#define __BF_GET(bf, x)                (((x & bf) >> __shf(bf)))
>
> Isn't it home grown reimplementation of bitfield.h?
>

This was answered in the aforementioned mail.

> ...
>
>> +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, "%s: Request port %d for pin %d is not activated\n",
>> +                        __func__, addr.port, offset);
>
> Don't use __func__ in messages, it's rarely needed and here I believe
> is not the case.
>

Ok, I will drop that.

>> +       }
>> +
>> +       return 0;
>> +}
>
> ...
>
>> +       /* Note that the SGIO pin is defined by *2* numbers, a port
>> +        * number between 0 and 31, and a bit index, 0 to 3.
>> +        */
>
> /*
>  * Fix multi-line comment
>  * style. Like in this example.
>  */

Sure. A drag network style insist to be different, but thats another
sory...

>
> ...
>
>> +static int microchip_sgpio_get_ports(struct sgpio_priv *priv)
>> +{
>> +       struct device *dev = priv->dev;
>> +       struct device_node *np = dev->of_node;
>> +       int i, ret;
>> +       u32 range_params[64];
>
> Better to use reversed xmas tree order.
>

Ack.

>> +       /* Calculate port mask */
>> +       ret = of_property_read_variable_u32_array(np,
>> +                                                 "microchip,sgpio-port-ranges",
>> +                                                 range_params,
>> +                                                 2,
>> +                                                 ARRAY_SIZE(range_params));
>> +       if (ret < 0 || ret % 2) {
>> +               dev_err(dev, "%s port range\n",
>> +                       ret == -EINVAL ? "Missing" : "Invalid");
>
>

?? Did you have a comment?

>
>> +               return ret;
>> +       }
>> +       for (i = 0; i < ret; i += 2) {
>> +               int start, end;
>> +
>> +               start = range_params[i];
>> +               end = range_params[i + 1];
>> +               if (start > end || end >= SGPIO_BITS_PER_WORD) {
>> +                       dev_err(dev, "Ill-formed port-range [%d:%d]\n",
>> +                               start, end);
>> +               }
>> +               priv->ports |= GENMASK(end, start);
>> +       }
>> +
>> +       return 0;
>> +}
>
> Doesn't GPIO / pin control framework have this helper already?
> If no, have you considered to use proper bitmap API here? (For
> example, bitmap_parselist() or so)
>

Past reviews suggested using an array form. And as the binding is
already reviewed, I would like to keep this as is.

> ...
>
>> +       if (fwnode_property_read_u32(fwnode, "ngpios", &ngpios)) {
>> +               dev_info(dev, "failed to get number of gpios for bank%d\n",
>> +                        bankno);
>> +               ngpios = 64;
>> +       }
>
> Don't mix OF APIs with fwnode APIs.
>

OF is gone.

> ...
>
>> +       pins = devm_kzalloc(dev, sizeof(*pins)*ngpios, GFP_KERNEL);
>> +       if (pins) {
>
> Use usual pattern  and drop one level of indentation ('else' is redundant).

Yes, done.

>
>> +               int i;
>> +               char *p, *names;
>
>> +               names = devm_kzalloc(dev, PIN_NAM_SZ*ngpios, GFP_KERNEL);
>> +
>> +               if (!names)
>
> Redundant blank line.
>

Gone.

>> +                       return -ENOMEM;
>
>> +               for (p = names, i = 0; i < ngpios; i++, p += PIN_NAM_SZ) {
>> +                       struct sgpio_port_addr addr;
>> +
>> +                       sgpio_pin_to_addr(priv, i, &addr);
>
>> +                       snprintf(p, PIN_NAM_SZ, "SGPIO_%c_p%db%d",
>> +                                is_input ? 'I' : 'O',
>> +                                addr.port, addr.bit);
>
> Wow, snprintf() with constant size argument in a loop. Are you sure
> you are doing correct?

I changed this to per-string allocation.

>
>> +                       pins[i].number = i;
>> +                       pins[i].name = p;
>> +               }
>> +       } else
>> +               return -ENOMEM;
>
> ...
>
>> +       pctldev = devm_pinctrl_register(dev, pctl_desc, bank);
>> +       if (IS_ERR(pctldev)) {
>> +               dev_err(dev, "Failed to register pinctrl\n");
>> +               return PTR_ERR(pctldev);
>> +       }
>
> return dev_err_probe(...);
>

Yes.

> ...
>
>> +       /* Get clock */
>
> Useless comment.

Ok, removed.

>
>> +       clk = devm_clk_get(dev, NULL);
>> +       if (IS_ERR(clk)) {
>
>> +               dev_err(dev, "Failed to get clock\n");
>> +               return PTR_ERR(clk);
>
> dev_err_probe() as above.
>

Yes.

>> +       }
>
> ...
>
>> +       /* Get register map */
>
> Useless comment.
>

Removed.

> ...
>
>> +       nbanks = device_get_child_node_count(dev);
>> +       if (nbanks != 2) {
>> +               dev_err(dev, "Must have 2 banks (have %d)\n", nbanks);
>> +               return -EINVAL;
>> +       }
>
> Don't mix device_property API with OF one.
>

Removed OF.

>> +       i = 0;
>> +       device_for_each_child_node(dev, fwnode) {
>
> Ditto.
>

Don't sure I understand this comment, but device_for_each_child_node()
is from <linux/property.h> - this should be OK I think.

>> +               ret = microchip_sgpio_register_bank(dev, priv, fwnode, i++);
>> +               if (ret)
>> +                       return ret;
>> +       }

Thank you for your comments. They are much appreciated.

I will refresh the patch later today.

Cheers,

---Lars

-- 
Lars Povlsen,
Microchip