Re: [PATCH 2/4] irqchip/meson-gpio: support more than 8 channels gpio irq line

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

On 1/8/22 6:37 PM, Marc Zyngier wrote:
> 
> On Sat, 08 Jan 2022 08:42:16 +0000,
> Qianggui Song <qianggui.song@amlogic.com> wrote:
>>
>> Current meson gpio irqchip driver only support 8 channels for gpio irq
>> line, later chips may have more then 8 channels, so need to modify code
>> to support more.
>>
>> Signed-off-by: Qianggui Song <qianggui.song@amlogic.com>
>> ---
>>   drivers/irqchip/irq-meson-gpio.c | 33 +++++++++++++++++++++++---------
>>   1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c
>> index d90ff0b92480..6a7b4fb13452 100644
>> --- a/drivers/irqchip/irq-meson-gpio.c
>> +++ b/drivers/irqchip/irq-meson-gpio.c
>> @@ -16,7 +16,6 @@
>>   #include <linux/of.h>
>>   #include <linux/of_address.h>
>>   
>> -#define NUM_CHANNEL 8
>>   #define MAX_INPUT_MUX 256
>>   
>>   #define REG_EDGE_POL	0x00
>> @@ -60,6 +59,7 @@ struct irq_ctl_ops {
>>   
>>   struct meson_gpio_irq_params {
>>   	unsigned int nr_hwirq;
>> +	unsigned int channel_num;
> 
> For consistency, please name this nr_channels.
Okay
> 
>>   	bool support_edge_both;
>>   	unsigned int edge_both_offset;
>>   	unsigned int edge_single_offset;
>> @@ -81,6 +81,7 @@ struct meson_gpio_irq_params {
>>   	.edge_single_offset = 0,				\
>>   	.pol_low_offset = 16,					\
>>   	.pin_sel_mask = 0xff,					\
>> +	.channel_num = 8,					\
>>   
>>   #define INIT_MESON_A1_COMMON_DATA(irqs)				\
>>   	INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init,		\
>> @@ -90,6 +91,7 @@ struct meson_gpio_irq_params {
>>   	.edge_single_offset = 8,				\
>>   	.pol_low_offset = 0,					\
>>   	.pin_sel_mask = 0x7f,					\
>> +	.channel_num = 8,					\
>>   
>>   static const struct meson_gpio_irq_params meson8_params = {
>>   	INIT_MESON8_COMMON_DATA(134)
>> @@ -136,8 +138,9 @@ static const struct of_device_id meson_irq_gpio_matches[] = {
>>   struct meson_gpio_irq_controller {
>>   	const struct meson_gpio_irq_params *params;
>>   	void __iomem *base;
>> -	u32 channel_irqs[NUM_CHANNEL];
>> -	DECLARE_BITMAP(channel_map, NUM_CHANNEL);
>> +	u32 *channel_irqs;
>> +	unsigned long *channel_map;
>> +	u8 channel_num;
> 
> Same thing. Though this is completely superfluous, see below.
I will remove this variable
> 
>>   	spinlock_t lock;
>>   };
>>   
>> @@ -207,8 +210,8 @@ meson_gpio_irq_request_channel(struct meson_gpio_irq_controller *ctl,
>>   	spin_lock_irqsave(&ctl->lock, flags);
>>   
>>   	/* Find a free channel */
>> -	idx = find_first_zero_bit(ctl->channel_map, NUM_CHANNEL);
>> -	if (idx >= NUM_CHANNEL) {
>> +	idx = find_first_zero_bit(ctl->channel_map, ctl->channel_num);
>> +	if (idx >= ctl->channel_num) {
>>   		spin_unlock_irqrestore(&ctl->lock, flags);
>>   		pr_err("No channel available\n");
>>   		return -ENOSPC;
>> @@ -447,13 +450,25 @@ static int meson_gpio_irq_parse_dt(struct device_node *node, struct meson_gpio_i
>>   
>>   	ctl->params = match->data;
>>   
>> +	ctl->channel_num = ctl->params->channel_num;
> 
> Since you already have a pointer to params, why do you need to
> duplicate this information?
will remove it next patch
> 
>> +	ctl->channel_irqs = kcalloc(ctl->channel_num,
>> +				    sizeof(*ctl->channel_irqs), GFP_KERNEL);
>> +	if (!ctl->channel_irqs)
>> +		return -ENOMEM;
>> +
>> +	ctl->channel_map = bitmap_zalloc(ctl->params->channel_num, GFP_KERNEL);
>> +	if (!ctl->channel_map) {
>> +		kfree(ctl->channel_irqs);
>> +		return -ENOMEM;
>> +	}
>> +
>>   	ret = of_property_read_variable_u32_array(node,
>>   						  "amlogic,channel-interrupts",
>>   						  ctl->channel_irqs,
>> -						  NUM_CHANNEL,
>> -						  NUM_CHANNEL);
>> +						  ctl->channel_num,
>> +						  ctl->channel_num);
>>   	if (ret < 0) {
>> -		pr_err("can't get %d channel interrupts\n", NUM_CHANNEL);
>> +		pr_err("can't get %d channel interrupts\n", ctl->channel_num);
>>   		return ret;
> 
> You are now leaking the bitmap and channel_map allocations.
will fix it next patch
> 
> 	M.
> 

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