On Wed, Dec 8, 2021 at 3:58 PM Jonathan Neuschäfer
[off-list ref] wrote:

On Wed, Dec 08, 2021 at 01:24:18PM +0200, Andy Shevchenko wrote:

quoted

On Tuesday, December 7, 2021, Jonathan Neuschäfer [off-list ref]
wrote:

...

quoted

quoted

+       ours = ((1UL << gpio->num_irqs) - 1) << gpio->first_irq_bit;

BIT()

I'll use it, but in this case, I think it doesn't simplify much the
whole expression all that much.

It is still better to use in my opinion.

Is there perhaps a macro that
constructs a continuous bitmask of N bits, perhaps additionally
left-shifted by M bits?

Maybe somewhere in the bitmap_* API...

Maybe, I dunno since I haven't clearly got this code anyway, so up to
you to check and see what to do about it.

...

quoted

quoted

+       struct fwnode_handle *np;

 Either be fully OF, or don’t name ‘np' here. We usually use fwnode or
‘child’ in this case.

Ah, I thought "np" (= node pointer) was still appropriate because I'm
dealing with firmware _nodes_. My intention was indeed to switch fully
to the fwnode API.

Just a convention "de facto".

...

quoted

quoted

+               ret = gpiochip_add_pin_range(&gpio->gc,
dev_name(pctrl->dev),
+                                            0, bank->base, bank->length);
+               if (ret) {
+                       dev_err(pctrl->dev, "Failed to add pin range for
GPIO bank %u\n", reg);
+                       return ret;
+               }

Please move it to the corresponding callback.

What's the corresponding callback?

https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/driver.h#L400

-- 
With Best Regards,
Andy Shevchenko