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

[qianggui.song <Qianggui.Song@amlogic.com>]

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?

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

will modify this next patch
> 
>> +{
>> +	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.
> 
Okay
>> +
>> +	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.
> 

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