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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Oct 29, 2020 at 3:40 PM Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> 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.

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.

> +       depends on HAS_IOMEM
> +       select GPIOLIB
> +       select GENERIC_PINCONF
> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS

> +       select OF_GPIO

...neither this...

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

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

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?

...

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

...

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

> +       }
> +
> +       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.
 */

...

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

> +       /* 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");



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

...

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

...

> +       pins = devm_kzalloc(dev, sizeof(*pins)*ngpios, GFP_KERNEL);
> +       if (pins) {

Use usual pattern  and drop one level of indentation ('else' is redundant).

> +               int i;
> +               char *p, *names;

> +               names = devm_kzalloc(dev, PIN_NAM_SZ*ngpios, GFP_KERNEL);
> +
> +               if (!names)

Redundant blank line.

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

> +                       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(...);

...

> +       /* Get clock */

Useless comment.

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

> +       }

...

> +       /* Get register map */

Useless comment.

...

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

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

Ditto.

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

-- 
With Best Regards,
Andy Shevchenko