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

[Andy Shevchenko]

On Mon, Nov 9, 2020 at 2:07 PM Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> wrote:
> > On Thu, Oct 29, 2020 at 3:40 PM Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> wrote:

...

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

Perhaps it makes sense to add functions like field_get(), field_prep()
to that header?

...

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

OF vs device property API I think.

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

Yes, but you are using something like a,b,c,d which corresponds to
[a..b], [c..d] if I'm not mistaken. And I believe that there are
plenty of drivers using this approach for some ranges (not
specifically for GPIO). And it should be an API available which does
all these checks and other stuff under the hood. I will be surprised
if there is no such. In the latter case, add one and use, many will
benefit from it.


...

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

Yes, either leave it (as you have done), or replace it with OF centric one.

-- 
With Best Regards,
Andy Shevchenko