Re: [PATCH 4/4] irqchip/meson-gpio: Add support for meson s4 SoCs

From: Marc Zyngier <maz@kernel.org>
To: qianggui.song <Qianggui.Song@amlogic.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Kevin Hilman <khilman@baylibre.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-amlogic@lists.infradead.org>
Subject: Re: [PATCH 4/4] irqchip/meson-gpio: Add support for meson s4 SoCs
Date: Mon, 10 Jan 2022 14:54:36 +0000	[thread overview]
Message-ID: <87lezntzdf.wl-maz@kernel.org> (raw)
In-Reply-To: <835a3f28-ec6a-9cd2-d574-5ef87b630fb3@amlogic.com>

On Mon, 10 Jan 2022 12:32:23 +0000,
qianggui.song <Qianggui.Song@amlogic.com> wrote:
> 
> 
> 
> On 1/8/22 7:06 PM, Marc Zyngier wrote:
> > 
> > On Sat, 08 Jan 2022 08:42:18 +0000,
> > Qianggui Song <qianggui.song@amlogic.com> wrote:
> >> 
> >> The meson s4 SoCs support 12 gpio irq lines compared with previous
> >> serial chips and have something different, details are as below.
> >> 
> >> IRQ Number:
> >> - 80:68 13 pins on bank Z
> >> - 67:48 20 pins on bank X
> >> - 47:36 12 pins on bank H
> >> - 35:24 12 pins on bank D
> >> - 23:22 2  pins on bank E
> >> - 21:14 8  pins on bank C
> >> - 13:0  13 pins on bank B
> >> 
> >> - PADCTRL_GPIO_IRQ_CTRL0
> >>    bit[31]:    enable/disable the whole irq lines
> > 
> > s/the whole/all the/
> Okay
> > 
> >>    bit[12-23]: single edge trigger
> >>    bit[0-11]:  poll trigger
> >> 
> >> - PADCTRL_GPIO_IRQ_CTRL[X]
> >> - bit[0-16]: 7 bits to chooge gpio source for irq line 2*[X] - 2
> > 
> > choose?
> yes it is choose
> > 
> >> - bit[16-22]:7 bits to chooge gpio source for irq line 2*[X] - 1
> >>    where X = 1-6
> >> 
> >> - PADCTRL_GPIO_IRQ_CTRL[7]
> >>    bit[0-11]: both edge trigger
> > 
> > This information would fit better in the code than in the commit
> > message.
> > 
> will copy this to code.
> >> 
> >> Signed-off-by: Qianggui Song <qianggui.song@amlogic.com>
> >> ---
> >>   drivers/irqchip/irq-meson-gpio.c | 51 ++++++++++++++++++++++++++++++++
> >>   1 file changed, 51 insertions(+)
> >> 
> >> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c
> >> index 98419428fcbd..c5d20a866c37 100644
> >> --- a/drivers/irqchip/irq-meson-gpio.c
> >> +++ b/drivers/irqchip/irq-meson-gpio.c
> >> @@ -42,6 +42,11 @@
> >>   #define REG_PIN_SEL_SHIFT(x)	(((x) % 4) * 8)
> >>   #define REG_FILTER_SEL_SHIFT(x)	((x) * 4)
> >>   +/* use for s4 chips */
> > 
> > s/use/Used/
> okay
> > 
> >> +#define REG_EDGE_POL_S4	0x1c
> >> +#define REG_EDGE_POL_MASK_S4(x)				\
> >> +	({typeof(x) _x = (x); BIT(_x) | BIT(12 + (_x)); })
> > 
> > Why on Earth should this macro handle multiple types?
> If I use  #define REG_EDGE_POL_MASK_S4(x) (BIT(x) | BIT(12 + (x)), when
> run git format-patch -1 --stdout | ./script/checkpath.pl --strict -,
> will get a message
> CHECK: Macro argument reuse 'x' - possible side-effects?

Well, either expand the macro directly in the code (after all, there
is only one use for it) or make this a C function instead of a macro.

	M.

-- 
Without deviation from the norm, progress is not possible.

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