Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Masahiro,

thanks for your review!

On Thu, Jul 18, 2019 at 1:10 PM Masahiro Yamada
<yamada.masahiro@xxxxxxxxxxxxx> wrote:
> On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:

> > -static void uniphier_gpio_irq_unmask(struct irq_data *data)
> > +static void uniphier_gpio_irq_unmask(struct irq_data *d)
>
> Are you renaming 'data' -> 'd'
> just for your personal preference?

Yes, I am still looking for proof of what kind of terseness gives
the optimal perceptive qualities in written code, so I have only
intuitive ideas about what is the easiest code to read and maintain.

But it's your file, and since you seem not to like it I will
back out this change.

> > -static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> > +static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>
> Again, this seems a noise change.

Contrary to popular belief, coding style changes while writing
new code is not universally seen as "noise", actually the opposite,
sending pure code style changes as singular patches, is seen as
noise. See the following paragraph from
Documentation/process/4.Coding.rst:

"pure coding style fixes are seen as noise by the development community;
they tend to get a chilly reception.  So this type of patch is best
avoided.  It is natural to fix the style of a piece of code while working
on it for other reasons, but coding style changes should not be made for
their own sake."

Given the number of pure coding style patches I get,
one could believe this does not apply ... but I try to be
accepting of it anyway.

> I did not test this patch, but probably it would break my board.

Oh too bad, let's see if we can make it more plausible to
work (I will not apply it while it is in RFT state).

> ->(de)activate hook has offset  UNIPHIER_GPIO_IRQ_OFFSET (=120),
> but you are replacing it with generic  gpiochip_irq_domain_activate,
> which as zero offset.

Ah! Brian gave me the tool to fix this properly, I will try to
iterate and get this right.

> > -       priv->domain = irq_domain_create_hierarchy(
> > -                                       parent_domain, 0,
> > -                                       UNIPHIER_GPIO_IRQ_MAX_NUM,
>
> You are replacing UNIPHIER_GPIO_IRQ_MAX_NUM with gc->ngpio,
> which will much more irqs than needed.
>
> Is it possible to provide more flexibility?

UNIPHIER_GPIO_IRQ_MAX_NUM is 24 and ngpio comes
from the device tree and is compulsory. The current device
trees have:
arch/arm/boot/dts/uniphier-ld4.dtsi:                    ngpios = <136>;
arch/arm/boot/dts/uniphier-pro4.dtsi:                   ngpios = <248>;
arch/arm/boot/dts/uniphier-pro5.dtsi:                   ngpios = <248>;
arch/arm/boot/dts/uniphier-pxs2.dtsi:                   ngpios = <232>;
arch/arm/boot/dts/uniphier-sld8.dtsi:                   ngpios = <136>;

So I suppose that you mean that since only 24 GPIOs can
ever have assigned IRQs, making headroom for say 248 is
a waste of resources.

However irq descriptors are dynamically allocated, so saying
that the irqchip can have 24 descriptors rather than 248
is not going to save any memory.

What you might want is to only allow offset 0..23 to be mapped
to irqs. If I understand correctly this is how the hardware works:
the first 24 GPIOs have IRQs, the rest don't, is that right?

We have a facility for that: struct gpio_irq_chip field
.valid_mask

I will try to come up with a separate patch for that so you can
see if it works.

Yours,
Linus Walleij