Date:   Sat, 08 Jan 2022 11:06:42 +0000
Message-ID: <87r19itrjx.wl-maz@kernel.org>
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
In-Reply-To: <20220108084218.31877-5-qianggui.song@amlogic.com>
References: <20220108084218.31877-1-qianggui.song@amlogic.com>
        <20220108084218.31877-5-qianggui.song@amlogic.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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/

>   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?

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

> 
> 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/

> +#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?

> +
>  struct meson_gpio_irq_controller;
>  static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl,
>  				    unsigned int channel, unsigned long hwirq);
> @@ -50,6 +55,9 @@ static void meson_a1_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl,
>  				      unsigned int channel,
>  				      unsigned long hwirq);
>  static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl);
> +static unsigned int
> +meson_s4_gpio_irq_sel_type(struct meson_gpio_irq_controller *ctl,
> +			   unsigned int idx, u32 val);
>  
>  struct irq_ctl_ops {
>  	void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl,
> @@ -96,6 +104,17 @@ struct meson_gpio_irq_params {
>  	.pin_sel_mask = 0x7f,					\
>  	.channel_num = 8,					\
>  
> +#define INIT_MESON_S4_COMMON_DATA(irqs)                         \
> +	INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init,         \
> +			  meson_a1_gpio_irq_sel_pin,            \
> +			  meson_s4_gpio_irq_sel_type)           \
> +	.support_edge_both = true,				\
> +	.edge_both_offset = 0,					\
> +	.edge_single_offset = 12,				\
> +	.pol_low_offset = 0,					\
> +	.pin_sel_mask = 0xff,					\
> +	.channel_num = 12,					\
> +
>  static const struct meson_gpio_irq_params meson8_params = {
>  	INIT_MESON8_COMMON_DATA(134)
>  };
> @@ -126,6 +145,10 @@ static const struct meson_gpio_irq_params a1_params = {
>  	INIT_MESON_A1_COMMON_DATA(62)
>  };
>  
> +static const struct meson_gpio_irq_params s4_params = {
> +	INIT_MESON_S4_COMMON_DATA(82)
> +};
> +
>  static const struct of_device_id meson_irq_gpio_matches[] = {
>  	{ .compatible = "amlogic,meson8-gpio-intc", .data = &meson8_params },
>  	{ .compatible = "amlogic,meson8b-gpio-intc", .data = &meson8b_params },
> @@ -135,6 +158,7 @@ static const struct of_device_id meson_irq_gpio_matches[] = {
>  	{ .compatible = "amlogic,meson-g12a-gpio-intc", .data = &axg_params },
>  	{ .compatible = "amlogic,meson-sm1-gpio-intc", .data = &sm1_params },
>  	{ .compatible = "amlogic,meson-a1-gpio-intc", .data = &a1_params },
> +	{ .compatible = "amlogic,meson-s4-gpio-intc", .data = &s4_params },
>  	{ }
>  };
>  
> @@ -202,6 +226,33 @@ static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl)
>  	meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, BIT(31), BIT(31));
>  }
>  
> +static unsigned int
> +meson_s4_gpio_irq_sel_type(struct meson_gpio_irq_controller *ctl,
> +			   unsigned int idx, unsigned int type)

An 'unsigned int' return type, directly returned by a caller that
has 'int' as its return type. What could possibly go wrong?

> +{
> +	unsigned int val = 0;
> +
> +	meson_gpio_irq_update_bits(ctl, REG_EDGE_POL_S4, BIT(0 + (idx)), 0);

Drop the 0 + as well as the useless bracketing all over this function.

> +
> +	if (type == IRQ_TYPE_EDGE_BOTH) {
> +		val |= BIT(ctl->params->edge_both_offset + (idx));
> +		meson_gpio_irq_update_bits(ctl, REG_EDGE_POL_S4,
> +					   BIT(ctl->params->edge_both_offset + (idx)), val);
> +		return 0;
> +	}
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> +		val |= BIT(ctl->params->pol_low_offset + (idx));
> +
> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> +		val |= BIT(ctl->params->edge_single_offset  + (idx));
> +
> +	meson_gpio_irq_update_bits(ctl, REG_EDGE_POL,
> +				   REG_EDGE_POL_MASK_S4(idx), val);
> +
> +	return 0;
> +};
> +
>  static int
>  meson_gpio_irq_request_channel(struct meson_gpio_irq_controller *ctl,
>  			       unsigned long  hwirq,

	M.

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