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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Nov 9, 2020 at 3:27 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.

Please, elaborate what you said previously, because now it has no
justification to be a pin control driver.

...

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

I answered the old thread that it probably makes sense to make
something like field_get() / field_prep() available for everybody.

...

> +       unsigned int bit = 3 * addr->bit;

> +       unsigned int bit = 3 * addr->bit;

Magic number. Perhaps a defined constant?

...

> +               return -EOPNOTSUPP;

Are you sure? IIRC internally we are using ENOTSUPP.

Couple of drivers seem to be wrongly using the other one.

...

> +                       err = -EOPNOTSUPP;

Ditto.

...

> +       if (input != bank->is_input) {

> +               dev_err(pctldev->dev, "Pin %d direction as %s is not possible\n",
> +                       pin, input ? "input" : "output");

Do we need this noise? Isn't user space getting a proper error code as
per doc and can handle this?

> +               return -EINVAL;
> +       }

...

> +       if ((priv->ports & BIT(addr.port)) == 0) {

'!' is equivalent to ' == 0', but it's up to you.

> +               dev_warn(priv->dev, "Request port %d for pin %d is not activated\n",
> +                        addr.port, offset);
> +       }

...

> +       return bank->is_input ?
> +               sgpio_input_get(priv, &addr) : sgpio_output_get(priv, &addr);

I would leave it on one line.

...

> +       pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount);

Redundant parentheses.

...

> +static int microchip_sgpio_get_ports(struct sgpio_priv *priv)
> +{
> +       struct device *dev = priv->dev;
> +       struct device_node *np = dev->of_node;
> +       u32 range_params[64];
> +       int i, ret;
> +
> +       /* 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;
> +}

As per previous version comment, i.e. perhaps find an existing API for
this kind of parser or introduce a generic one.

...

> +       bool is_input = (bankno == 0);

Now I'm not sure why you need this variable here and in the structure.
Can't you use the above check directly?
If you want to have flexible settings (i.e. not all generations of hw
have it like this) I would rather recommend to do it via DT.

...

> +       for (i = 0; i < ngpios; i++) {
> +               char name[sizeof("SGPIO_D_pXXbY\0")];
> +               struct sgpio_port_addr addr;
> +
> +               sgpio_pin_to_addr(priv, i, &addr);
> +               snprintf(name, sizeof(name), "SGPIO_%c_p%db%d",
> +                        is_input ? 'I' : 'O',
> +                        addr.port, addr.bit);
> +
> +               pins[i].number = i;
> +               pins[i].name = devm_kstrdup(dev, name, GFP_KERNEL);

Above with this is a NIH of devm_kasprintf().

> +               if (!pins[i].name)
> +                       return -ENOMEM;
> +       }

...

> +       /* Get clock */

Useless comment.

...

> +       div_clock = clk_get_rate(clk);
> +       if (of_property_read_u32(dev->of_node, "bus-frequency", &priv->clock))

Again, choose one API, no need to spread among several.

As I have told you already: if any comment is given against one
location in the code, check the entire contribution for similar places
and address accordingly.

> +               priv->clock = 12500000;
> +       if (priv->clock <= 0 || priv->clock > (div_clock / 2)) {

How can an unsigned value be possible less than 0?

> +               dev_err(dev, "Invalid frequency %d\n", priv->clock);
> +               return -EINVAL;

> +       }

...

> +       /* Get register map */

Useless.

...

> +       /* Get rest of device properties */

This one though may be left.

-- 
With Best Regards,
Andy Shevchenko