Re: [PATCH v2 00/16] gpio: Tight IRQ chip integration and banked infrastructure

[Thierry Reding]

On Thu, Sep 28, 2017 at 09:22:17AM -0500, Grygorii Strashko wrote:
> 
> 
> On 09/28/2017 04:56 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> > 
> > Hi Linus,
> > 
> > here's the latest series of patches that implement the tighter IRQ chip
> > integration as well as the banked GPIO infrastructure that we had
> > discussed a couple of weeks/months back.
> > 
> > The first couple of patches are mostly preparatory work in order to
> > consolidate all IRQ chip related fields in a new structure and create
> > the base functionality for adding IRQ chips.
> > 
> > After that, I've added the Tegra186 GPIO support patch that makes use of
> > the new tight integration.
> > 
> > To round things off the new banked GPIO infrastructure is added (along
> > with some more preparatory work), followed by the conversion of the two
> > Tegra GPIO drivers to the new infrastructure.
> 
> Hm. So you've ignored my comments [1].
> 
> Sry, but I do not agree with this series.
> - no prof that it can be re-used by other drivers than tegra
>  (at least I do not see reasons to re-use it for any TI drivers)

I had done some research based on Linus' feedback from an earlier series
and identified the following potential candidates[0] that could move to
this new infrastructure:

	- gpio-intel-mid.c
	- gpio-merrifield.c
	- gpio-pca953x.c
	- gpio-stmpe.c
	- gpio-tc3589x.c
	- gpio-ws16c48.c

Note that this is based on code inspection rather than DT inspection,
because that's fundamentally flawed. If you look at this from a DT
perspective you're going to be tempted to change the DT bindings, but
you can't do that because of backwards compatiblity. This new framework
also doesn't address the issues at that level, but rather tries to be
some common code that is otherwise duplicated in one way or another in
various drivers and therefore hard to maintain. This is what Linus had
originally requested, and that's what the series does.

> - no split

What does this mean? The series is nicely split into separate patches,
so each one individually is easy to review. I've also gone through quite
some trouble to make sure everything builds fine after each patch, so
it's possible to apply individual bits of the series. For example we
could opt to apply everything up to the banked GPIO support if that's
still contentious.

> - all GPIO IRQs mapped statically

This series predates your work on the dynamic IRQ mapping, so I hadn't
picked up those changes. This should be easily solved by the attached
patch, though.

> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
>   which is waste of memory

It's the only way to generically do this. Also I don't think this wastes
that much memory. We have one unsigned int per pin, which even for very
large GPIO controllers is unlikely to exceed one 4 KiB page.

> - DT binding changes not documented and no DT examples

That's because this is completely orthogonal to DT bindings. We can't
make any changes to the bindings because of ABI stability.

> - below is ugly ;)
> +	bank = (spec[0] >> gc->of_gpio_bank_mask) & gc->of_gpio_bank_shift;
> +	pin = (spec[0] >> gc->of_gpio_pin_mask) & gc->of_gpio_pin_shift;

If by ugly you mean wrong, then yes, it's actually the wrong way around.
It should be:

	bank = (spec[0] >> gc->of_gpio_bank_shift) & gc->of_gpio_bank_mask;
	line = (spec[0] >> gc->of_gpio_line_shift) & gc->of_gpio_line_mask;

Thierry

Attachment: signature.asc
Description: PGP signature