On Mon, Nov 9, 2020 at 2:07 PM Lars Povlsen [off-list ref] wrote:

quoted

On Thu, Oct 29, 2020 at 3:40 PM Lars Povlsen [off-list ref] wrote:

...

quoted

quoted

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

...

quoted

quoted

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

quoted

quoted

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


...

quoted

quoted

+       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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel