On Wed, Dec 08, 2021 at 04:14:38PM +0200, Andy Shevchenko wrote:

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

quoted

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

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.

Ok.

quoted

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

quoted

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.

Right, I'll evaluate my options and come up with something.

...

quoted

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

Ok, I'll change it.

quoted

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

Thanks.


Best regards,
Jonathan Neuschäfer