linux-kernel - Re: [PATCH v7 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VeN4jKjOA=WO0mgkSAbWZUMUfkrX3yV83y0iYnh1rp84Q@mail.gmail.com>
Date:   Mon, 9 Nov 2020 16:00:16 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Lars Povlsen <lars.povlsen@...rochip.com>
Cc:     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 v7 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl
 driver for Microsemi Serial GPIO

On Mon, Nov 9, 2020 at 2:07 PM Lars Povlsen <lars.povlsen@...rochip.com> wrote:
> > On Thu, Oct 29, 2020 at 3:40 PM Lars Povlsen <lars.povlsen@...rochip.com> 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