| Message ID | 20220108084218.31877-3-qianggui.song@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | irqchip/meson-gpio: Add support for Meson-S4 SoC | expand |
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. > 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. > 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? > + 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. M.
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. >
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; 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; 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; + 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; } @@ -507,7 +522,7 @@ static int meson_gpio_irq_of_init(struct device_node *node, struct device_node * } pr_info("%d to %d gpio interrupt mux initialized\n", - ctl->params->nr_hwirq, NUM_CHANNEL); + ctl->params->nr_hwirq, ctl->channel_num); return 0;
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(-)