Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs

[Marc Zyngier <maz@kernel.org>]

On 2019-12-10 02:08, Qianggui Song wrote:
> Hi, Marc
>      Thank you for your review
>
> On 2019/12/6 21:13, Marc Zyngier wrote:
>> On 2019-12-06 12:17, Qianggui Song wrote:
>>> Since Meson-A1 Socs register layout of gpio interrupt controller 
>>> have
>>> difference with previous chips, registers to decide irq line and
>>> offset
>>> of trigger method are all changed, the current driver should be
>>> modified.
>>>
>>> Signed-off-by: Qianggui Song <qianggui.song@amlogic.com>
>>> ---
>>>  drivers/irqchip/irq-meson-gpio.c | 79
>>> ++++++++++++++++++++++++--------
>>>  1 file changed, 60 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-meson-gpio.c
>>> b/drivers/irqchip/irq-meson-gpio.c
>>> index 829084b568fa..1824ffc30de2 100644
>>> --- a/drivers/irqchip/irq-meson-gpio.c
>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>> @@ -30,44 +30,74 @@
>>>   * stuck at 0. Bits 8 to 15 are responsive and have the expected
>>>   * effect.
>>>   */
>>> -#define REG_EDGE_POL_EDGE(x)	BIT(x)
>>> -#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
>>> -#define REG_BOTH_EDGE(x)	BIT(8 + (x))
>>> -#define REG_EDGE_POL_MASK(x)    (	\
>>> -		REG_EDGE_POL_EDGE(x) |	\
>>> -		REG_EDGE_POL_LOW(x)  |	\
>>> -		REG_BOTH_EDGE(x))
>>> +#define REG_EDGE_POL_EDGE(params,
>>> x)	BIT((params)->edge_single_offset + (x))
>>> +#define REG_EDGE_POL_LOW(params, x)	BIT((params)->pol_low_offset +
>>> (x))
>>> +#define REG_BOTH_EDGE(params, x)	BIT((params)->edge_both_offset +
>>> (x))
>>> +#define REG_EDGE_POL_MASK(params, x)    (	\
>>> +		REG_EDGE_POL_EDGE(params, x) |	\
>>> +		REG_EDGE_POL_LOW(params, x)  |	\
>>> +		REG_BOTH_EDGE(params, x))
>>>  #define REG_PIN_SEL_SHIFT(x)	(((x) % 4) * 8)
>>>  #define REG_FILTER_SEL_SHIFT(x)	((x) * 4)
>>>
>>> +#define INIT_MESON8_COMMON_DATA					\
>>> +	.edge_single_offset = 0,				\
>>> +	.pol_low_offset = 16,					\
>>> +	.pin_sel_mask = 0xff,					\
>>> +	.ops = {						\
>>> +		.gpio_irq_sel_pin = meson8_gpio_irq_sel_pin,	\
>>> +	},
>>
>> Please place the #defines that operate on the various data 
>> structures
>> *after* the definition of the structures. It would greatly help
>> reading the changes.
>>
> OK, will place it below the definition of struct 
> meson_gpio_irq_params
> in the next patch.
>>> +
>>> +struct meson_gpio_irq_controller;
>>> +static void meson8_gpio_irq_sel_pin(struct 
>>> meson_gpio_irq_controller
>>> *ctl,
>>> +				    unsigned int channel, unsigned long hwirq);
>>> +struct irq_ctl_ops {
>>> +	void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl,
>>> +					 unsigned int channel,
>>> +					 unsigned long hwirq);
>>> +	void (*gpio_irq_init)(struct meson_gpio_irq_controller *ctl);
>>> +};
>>> +
>>>  struct meson_gpio_irq_params {
>>>  	unsigned int nr_hwirq;
>>>  	bool support_edge_both;
>>> +	unsigned int edge_both_offset;
>>> +	unsigned int edge_single_offset;
>>> +	unsigned int pol_low_offset;
>>> +	unsigned int pin_sel_mask;
>>> +	struct irq_ctl_ops ops;
>>>  };
>>>
>>>  static const struct meson_gpio_irq_params meson8_params = {
>>>  	.nr_hwirq = 134,
>>> +	INIT_MESON8_COMMON_DATA
>>>  };
>>>
>>>  static const struct meson_gpio_irq_params meson8b_params = {
>>>  	.nr_hwirq = 119,
>>> +	INIT_MESON8_COMMON_DATA
>>>  };
>>>
>>>  static const struct meson_gpio_irq_params gxbb_params = {
>>>  	.nr_hwirq = 133,
>>> +	INIT_MESON8_COMMON_DATA
>>>  };
>>>
>>>  static const struct meson_gpio_irq_params gxl_params = {
>>>  	.nr_hwirq = 110,
>>> +	INIT_MESON8_COMMON_DATA
>>>  };
>>>
>>>  static const struct meson_gpio_irq_params axg_params = {
>>>  	.nr_hwirq = 100,
>>> +	INIT_MESON8_COMMON_DATA
>>>  };
>>>
>>>  static const struct meson_gpio_irq_params sm1_params = {
>>>  	.nr_hwirq = 100,
>>>  	.support_edge_both = true,
>>> +	.edge_both_offset = 8,
>>> +	INIT_MESON8_COMMON_DATA
>>>  };
>>
>> OK, this isn't great. The least you could do is to make
>> your initializer parametric, so that it takes the nr_hwirq as
>> a parameter.
>>
>> Then, any additional member that overrides common behaviour
>> should come after the main initializer.
>>
>> Also, do you need 'support_edge_both'? Isn't a non-zero
>> 'edge_both_offset' enough to detect the feature?
>>
>
> Sorry, but I am not very clear that "make your initializer 
> parametric,
> so that it takes the nr_hwirq as a parameter". Is that
> initializer(initial function in .ops ? ) as a parameter of struct
> meson_gpio_irq_params ? If nr_hwirq as a parameter of init function 
> of
> .ops then will make lot of init function for each platform.
>
> How about move .ops from  macro like below:
> #define INIT_MESON8_COMMON_DATA					\
> 	.edge_single_offset = 0,				\
> 	.pol_low_offset = 16,					\
> 	.pin_sel_mask = 0xff,
>
> static const struct meson_gpio_irq_params sm1_params = {
>  	.nr_hwirq = 100,//main initializer
> 	.ops = {
>               .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin,
>                /*in below to assign support_edge_both
>                 * edge_both_offset
>                 * call after main initializer to additional
>                 * member
>                 */
>               .gpio_irq_init = meson_sm1_irq_init,
> 	},
>   	INIT_MESON8_COMMON_DATA// m8 to sm1 are the same.
> };

No, what I'm suggesting is something like this:

diff --git a/drivers/irqchip/irq-meson-gpio.c 
b/drivers/irqchip/irq-meson-gpio.c
index 8478100706a6..27a3207a944d 100644
--- a/drivers/irqchip/irq-meson-gpio.c
+++ b/drivers/irqchip/irq-meson-gpio.c
@@ -43,24 +43,27 @@
  /* Below is used for Meson-A1 series like chips*/
  #define REG_PIN_A1_SEL	0x04

-#define INIT_MESON8_COMMON_DATA					\
-	.edge_single_offset = 0,				\
-	.pol_low_offset = 16,					\
-	.pin_sel_mask = 0xff,					\
-	.ops = {						\
-		.gpio_irq_sel_pin = meson8_gpio_irq_sel_pin,	\
-	},
-
-#define INIT_MESON_A1_COMMON_DATA				\
-	.support_edge_both = true,				\
-	.edge_both_offset = 16,					\
-	.edge_single_offset = 8,				\
-	.pol_low_offset = 0,					\
-	.pin_sel_mask = 0x7f,					\
-	.ops = {						\
-		.gpio_irq_sel_pin = meson_a1_gpio_irq_sel_pin,	\
-		.gpio_irq_init = meson_a1_gpio_irq_init,	\
-	},
+#define INIT_MESON_COMMON(irqs, init, sel)		\
+	.nr_hwirq = irqs,				\
+	.ops = {					\
+		.gpio_irq_sel_pin = sel,		\
+		.gpio_irq_init = init,			\
+	}
+
+#define INIT_MESON8_COMMON_DATA(irqs)			\
+	INIT_MESON_COMMON(irqs, NULL,			\
+			  meson8_gpio_irq_sel_pin),	\
+	.pol_low_offset = 16,				\
+	.pin_sel_mask = 0xff,
+
+#define INIT_MESON_A1_COMMON_DATA(irqs)			\
+	INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init,	\
+			  meson_a1_gpio_irq_sel_pin),	\
+	.support_edge_both = true,			\
+	.edge_both_offset = 16,				\
+	.edge_single_offset = 8,			\
+	.pol_low_offset = 0,				\
+	.pin_sel_mask = 0x7f,

  struct meson_gpio_irq_controller;
  static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller 
*ctl,
@@ -89,40 +92,33 @@ struct meson_gpio_irq_params {
  };

  static const struct meson_gpio_irq_params meson8_params = {
-	.nr_hwirq = 134,
-	INIT_MESON8_COMMON_DATA
+	INIT_MESON8_COMMON_DATA(134),
  };

  static const struct meson_gpio_irq_params meson8b_params = {
-	.nr_hwirq = 119,
-	INIT_MESON8_COMMON_DATA
+	INIT_MESON8_COMMON_DATA(119),
  };

  static const struct meson_gpio_irq_params gxbb_params = {
-	.nr_hwirq = 133,
-	INIT_MESON8_COMMON_DATA
+	INIT_MESON8_COMMON_DATA(133),
  };

  static const struct meson_gpio_irq_params gxl_params = {
-	.nr_hwirq = 110,
-	INIT_MESON8_COMMON_DATA
+	INIT_MESON8_COMMON_DATA(110),
  };

  static const struct meson_gpio_irq_params axg_params = {
-	.nr_hwirq = 100,
-	INIT_MESON8_COMMON_DATA
+	INIT_MESON8_COMMON_DATA(100),
  };

  static const struct meson_gpio_irq_params sm1_params = {
-	.nr_hwirq = 100,
+	INIT_MESON8_COMMON_DATA(100),
  	.support_edge_both = true,
  	.edge_both_offset = 8,
-	INIT_MESON8_COMMON_DATA
  };

  static const struct meson_gpio_irq_params a1_params = {
-	.nr_hwirq = 62,
-	INIT_MESON_A1_COMMON_DATA
+	INIT_MESON_A1_COMMON_DATA(62),
  };

  static const struct of_device_id meson_irq_gpio_matches[] = {


Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...