[PATCH v2 0/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[PATCH v2 0/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Hans de Goede]

Hi All,

Here is v2 of my series for Cherry Trail devices to pass the irqchip when
adding the gpiochip instead of registering it separately. Similar to how
this is done for Bay Trail devices in Andy's recent series:
"[RESEND][PATCH v2 0/7] gpiolib: fix GPIO <-> pin mapping registration".

Note this series depends on that series as well as on the cherryview changes
currently queued in pinctrl/intel.git/for-next .

Changes in v2:
- Add kerndoc comments for new chv_pinctrl struct members

Regards,

Hans

[PATCH v2 1/3] pinctrl: cherryview: Split out irq hw-init into a separate helper function

[Hans de Goede]

Split out irq hw-init into a separate chv_gpio_irq_init_hw() function.
This is a preparation patch for passing the irqchip when adding the
gpiochip.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add kerneldoc for chv_pinctrl.need_valid_mask struct member
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 58 +++++++++++++---------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index dff2a81250b6..3ae0d398368d 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -149,6 +149,7 @@ struct chv_pin_context {
  * @chip: GPIO chip in this pin controller
  * @irqchip: IRQ chip in this pin controller
  * @regs: MMIO registers
+ * @need_valid_mask: Use chip.irq.init_valid_mask ?
  * @intr_lines: Stores mapping between 16 HW interrupt wires and GPIO
  *		offset (in GPIO number space)
  * @community: Community this pinctrl instance represents
@@ -165,6 +166,7 @@ struct chv_pinctrl {
 	struct gpio_chip chip;
 	struct irq_chip irqchip;
 	void __iomem *regs;
+	bool need_valid_mask;
 	unsigned intr_lines[16];
 	const struct chv_community *community;
 	u32 saved_intmask;
@@ -1555,13 +1557,40 @@ static void chv_init_irq_valid_mask(struct gpio_chip *chip,
 	}
 }
 
+static int chv_gpio_irq_init_hw(struct gpio_chip *chip)
+{
+	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
+
+	/*
+	 * The same set of machines in chv_no_valid_mask[] have incorrectly
+	 * configured GPIOs that generate spurious interrupts so we use
+	 * this same list to apply another quirk for them.
+	 *
+	 * See also https://bugzilla.kernel.org/show_bug.cgi?id=197953.
+	 */
+	if (!pctrl->need_valid_mask) {
+		/*
+		 * Mask all interrupts the community is able to generate
+		 * but leave the ones that can only generate GPEs unmasked.
+		 */
+		chv_writel(GENMASK(31, pctrl->community->nirqs),
+			   pctrl->regs + CHV_INTMASK);
+	}
+
+	/* Clear all interrupts */
+	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
+
+	return 0;
+}
+
 static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 {
 	const struct chv_gpio_pinrange *range;
 	struct gpio_chip *chip = &pctrl->chip;
-	bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
 	const struct chv_community *community = pctrl->community;
-	int ret, i, irq_base;
+	int ret, i, irq_base = 0;
+
+	pctrl->need_valid_mask = !dmi_check_system(chv_no_valid_mask);
 
 	*chip = chv_gpio_chip;
 
@@ -1569,7 +1598,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	chip->label = dev_name(pctrl->dev);
 	chip->parent = pctrl->dev;
 	chip->base = -1;
-	if (need_valid_mask)
+	if (pctrl->need_valid_mask)
 		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
 
 	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
@@ -1589,26 +1618,9 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		}
 	}
 
-	/*
-	 * The same set of machines in chv_no_valid_mask[] have incorrectly
-	 * configured GPIOs that generate spurious interrupts so we use
-	 * this same list to apply another quirk for them.
-	 *
-	 * See also https://bugzilla.kernel.org/show_bug.cgi?id=197953.
-	 */
-	if (!need_valid_mask) {
-		/*
-		 * Mask all interrupts the community is able to generate
-		 * but leave the ones that can only generate GPEs unmasked.
-		 */
-		chv_writel(GENMASK(31, pctrl->community->nirqs),
-			   pctrl->regs + CHV_INTMASK);
-	}
-
-	/* Clear all interrupts */
-	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
+	chv_gpio_irq_init_hw(chip);
 
-	if (!need_valid_mask) {
+	if (!pctrl->need_valid_mask) {
 		irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
 						community->npins, NUMA_NO_NODE);
 		if (irq_base < 0) {
@@ -1632,7 +1644,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		return ret;
 	}
 
-	if (!need_valid_mask) {
+	if (!pctrl->need_valid_mask) {
 		for (i = 0; i < community->ngpio_ranges; i++) {
 			range = &community->gpio_ranges[i];
 
-- 
2.23.0

[PATCH v2 2/3] pinctrl: cherryview: Add GPIO <-> pin mapping ranges via callback

[Hans de Goede]

When IRQ chip is instantiated via GPIO library flow, the few functions,
in particular the ACPI event registration mechanism, on some of ACPI based
platforms expect that the pin ranges are initialized to that point.

Add GPIO <-> pin mapping ranges via callback in the GPIO library flow.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 33 ++++++++++++++--------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 3ae0d398368d..1ded4bd8d1b4 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1583,6 +1583,27 @@ static int chv_gpio_irq_init_hw(struct gpio_chip *chip)
 	return 0;
 }
 
+static int chv_gpio_add_pin_ranges(struct gpio_chip *chip)
+{
+	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct chv_community *community = pctrl->community;
+	const struct chv_gpio_pinrange *range;
+	int ret, i;
+
+	for (i = 0; i < community->ngpio_ranges; i++) {
+		range = &community->gpio_ranges[i];
+		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
+					     range->base, range->base,
+					     range->npins);
+		if (ret) {
+			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 {
 	const struct chv_gpio_pinrange *range;
@@ -1596,6 +1617,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 
 	chip->ngpio = community->pins[community->npins - 1].number + 1;
 	chip->label = dev_name(pctrl->dev);
+	chip->add_pin_ranges = chv_gpio_add_pin_ranges;
 	chip->parent = pctrl->dev;
 	chip->base = -1;
 	if (pctrl->need_valid_mask)
@@ -1607,17 +1629,6 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		return ret;
 	}
 
-	for (i = 0; i < community->ngpio_ranges; i++) {
-		range = &community->gpio_ranges[i];
-		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
-					     range->base, range->base,
-					     range->npins);
-		if (ret) {
-			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
-			return ret;
-		}
-	}
-
 	chv_gpio_irq_init_hw(chip);
 
 	if (!pctrl->need_valid_mask) {
-- 
2.23.0

[PATCH v2 3/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Hans de Goede]

We need to convert all old gpio irqchips to pass the irqchip
setup along when adding the gpio_chip. For more info see
drivers/gpio/TODO.

For chained irqchips this is a pretty straight-forward conversion.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add kerneldoc for chv_pinctrl.irq struct member
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 42 +++++++++++-----------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 1ded4bd8d1b4..e7c78acdcfbc 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -150,6 +150,7 @@ struct chv_pin_context {
  * @irqchip: IRQ chip in this pin controller
  * @regs: MMIO registers
  * @need_valid_mask: Use chip.irq.init_valid_mask ?
+ * @irq: Our parent irq
  * @intr_lines: Stores mapping between 16 HW interrupt wires and GPIO
  *		offset (in GPIO number space)
  * @community: Community this pinctrl instance represents
@@ -167,6 +168,7 @@ struct chv_pinctrl {
 	struct irq_chip irqchip;
 	void __iomem *regs;
 	bool need_valid_mask;
+	unsigned int irq;
 	unsigned intr_lines[16];
 	const struct chv_community *community;
 	u32 saved_intmask;
@@ -1620,16 +1622,25 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	chip->add_pin_ranges = chv_gpio_add_pin_ranges;
 	chip->parent = pctrl->dev;
 	chip->base = -1;
-	if (pctrl->need_valid_mask)
-		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
 
-	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to register gpiochip\n");
-		return ret;
-	}
+	pctrl->irq = irq;
+	pctrl->irqchip.name = "chv-gpio";
+	pctrl->irqchip.irq_startup = chv_gpio_irq_startup;
+	pctrl->irqchip.irq_ack = chv_gpio_irq_ack;
+	pctrl->irqchip.irq_mask = chv_gpio_irq_mask;
+	pctrl->irqchip.irq_unmask = chv_gpio_irq_unmask;
+	pctrl->irqchip.irq_set_type = chv_gpio_irq_type;
+	pctrl->irqchip.flags = IRQCHIP_SKIP_SET_WAKE;
 
-	chv_gpio_irq_init_hw(chip);
+	chip->irq.chip = &pctrl->irqchip;
+	if (pctrl->need_valid_mask)
+		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
+	chip->irq.init_hw = chv_gpio_irq_init_hw;
+	chip->irq.parent_handler = chv_gpio_irq_handler;
+	chip->irq.num_parents = 1;
+	chip->irq.parents = &pctrl->irq;
+	chip->irq.default_type = IRQ_TYPE_NONE;
+	chip->irq.handler = handle_bad_irq;
 
 	if (!pctrl->need_valid_mask) {
 		irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
@@ -1640,18 +1651,9 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		}
 	}
 
-	pctrl->irqchip.name = "chv-gpio";
-	pctrl->irqchip.irq_startup = chv_gpio_irq_startup;
-	pctrl->irqchip.irq_ack = chv_gpio_irq_ack;
-	pctrl->irqchip.irq_mask = chv_gpio_irq_mask;
-	pctrl->irqchip.irq_unmask = chv_gpio_irq_unmask;
-	pctrl->irqchip.irq_set_type = chv_gpio_irq_type;
-	pctrl->irqchip.flags = IRQCHIP_SKIP_SET_WAKE;
-
-	ret = gpiochip_irqchip_add(chip, &pctrl->irqchip, 0,
-				   handle_bad_irq, IRQ_TYPE_NONE);
+	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
 	if (ret) {
-		dev_err(pctrl->dev, "failed to add IRQ chip\n");
+		dev_err(pctrl->dev, "Failed to register gpiochip\n");
 		return ret;
 	}
 
@@ -1665,8 +1667,6 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		}
 	}
 
-	gpiochip_set_chained_irqchip(chip, &pctrl->irqchip, irq,
-				     chv_gpio_irq_handler);
 	return 0;
 }
 
-- 
2.23.0

Re: [PATCH v2 3/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Wed, Nov 06, 2019 at 04:47:15PM +0100, Hans de Goede wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip. For more info see
> drivers/gpio/TODO.
> 
> For chained irqchips this is a pretty straight-forward conversion.

> +	chip->irq.chip = &pctrl->irqchip;

> +	if (pctrl->need_valid_mask)
> +		chip->irq.init_valid_mask = chv_init_irq_valid_mask;

I just realize we probably may assign here unconditionally

> +	chip->irq.init_hw = chv_gpio_irq_init_hw;
> +	chip->irq.parent_handler = chv_gpio_irq_handler;
> +	chip->irq.num_parents = 1;
> +	chip->irq.parents = &pctrl->irq;
> +	chip->irq.default_type = IRQ_TYPE_NONE;
> +	chip->irq.handler = handle_bad_irq;
>  
>  	if (!pctrl->need_valid_mask) {
>  		irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
> @@ -1640,18 +1651,9 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>  		}
>  	}
>  
> -	pctrl->irqchip.name = "chv-gpio";
> -	pctrl->irqchip.irq_startup = chv_gpio_irq_startup;
> -	pctrl->irqchip.irq_ack = chv_gpio_irq_ack;
> -	pctrl->irqchip.irq_mask = chv_gpio_irq_mask;
> -	pctrl->irqchip.irq_unmask = chv_gpio_irq_unmask;
> -	pctrl->irqchip.irq_set_type = chv_gpio_irq_type;
> -	pctrl->irqchip.flags = IRQCHIP_SKIP_SET_WAKE;
> -
> -	ret = gpiochip_irqchip_add(chip, &pctrl->irqchip, 0,
> -				   handle_bad_irq, IRQ_TYPE_NONE);
> +	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
>  	if (ret) {
> -		dev_err(pctrl->dev, "failed to add IRQ chip\n");
> +		dev_err(pctrl->dev, "Failed to register gpiochip\n");
>  		return ret;
>  	}
>  
> @@ -1665,8 +1667,6 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>  		}
>  	}
>  
> -	gpiochip_set_chained_irqchip(chip, &pctrl->irqchip, irq,
> -				     chv_gpio_irq_handler);
>  	return 0;
>  }
>  
> -- 
> 2.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Wed, Nov 06, 2019 at 06:16:22PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 06, 2019 at 04:47:15PM +0100, Hans de Goede wrote:
> > We need to convert all old gpio irqchips to pass the irqchip
> > setup along when adding the gpio_chip. For more info see
> > drivers/gpio/TODO.
> > 
> > For chained irqchips this is a pretty straight-forward conversion.
> 
> > +	chip->irq.chip = &pctrl->irqchip;
> 
> > +	if (pctrl->need_valid_mask)
> > +		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
> 
> I just realize we probably may assign here unconditionally

Continuing...

> 
> > +	chip->irq.init_hw = chv_gpio_irq_init_hw;
> > +	chip->irq.parent_handler = chv_gpio_irq_handler;
> > +	chip->irq.num_parents = 1;
> > +	chip->irq.parents = &pctrl->irq;
> > +	chip->irq.default_type = IRQ_TYPE_NONE;
> > +	chip->irq.handler = handle_bad_irq;
> >  
> >  	if (!pctrl->need_valid_mask) {

And here turn it back to NULL and check the pointer against NULL instead of
additional variable.

What do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Hans de Goede]

Hi,

On 06-11-2019 17:17, Andy Shevchenko wrote:
> On Wed, Nov 06, 2019 at 06:16:22PM +0200, Andy Shevchenko wrote:
>> On Wed, Nov 06, 2019 at 04:47:15PM +0100, Hans de Goede wrote:
>>> We need to convert all old gpio irqchips to pass the irqchip
>>> setup along when adding the gpio_chip. For more info see
>>> drivers/gpio/TODO.
>>>
>>> For chained irqchips this is a pretty straight-forward conversion.
>>
>>> +	chip->irq.chip = &pctrl->irqchip;
>>
>>> +	if (pctrl->need_valid_mask)
>>> +		chip->irq.init_valid_mask = chv_init_irq_valid_mask;
>>
>> I just realize we probably may assign here unconditionally
> 
> Continuing...
> 
>>
>>> +	chip->irq.init_hw = chv_gpio_irq_init_hw;
>>> +	chip->irq.parent_handler = chv_gpio_irq_handler;
>>> +	chip->irq.num_parents = 1;
>>> +	chip->irq.parents = &pctrl->irq;
>>> +	chip->irq.default_type = IRQ_TYPE_NONE;
>>> +	chip->irq.handler = handle_bad_irq;
>>>   
>>>   	if (!pctrl->need_valid_mask) {
> 
> And here turn it back to NULL and check the pointer against NULL instead of
> additional variable.
> 
> What do you think?

I think that first setting it and then clearing it again is not
very pretty. But ...

I do think you are on to something, we can use pctrl->chip.irq.init_valid_mask
instead of storing the dmi quirk in the chv_pinctrl struct.

Then we can leave the dmi handling as is, and replace later checks for
the dmi quirk (in callbacks) with a check for pctrl->chip.irq.init_valid_mask
I do believe that that is better then adding a need_validmask member to
the chv_pinctrl struct, I will prepare a v3 of the series with this change.

Regards,

Hans

Re: [PATCH v2 0/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Wed, Nov 06, 2019 at 04:47:12PM +0100, Hans de Goede wrote:
> Hi All,
> 
> Here is v2 of my series for Cherry Trail devices to pass the irqchip when
> adding the gpiochip instead of registering it separately. Similar to how
> this is done for Bay Trail devices in Andy's recent series:
> "[RESEND][PATCH v2 0/7] gpiolib: fix GPIO <-> pin mapping registration".
> 
> Note this series depends on that series as well as on the cherryview changes
> currently queued in pinctrl/intel.git/for-next .

LGTM independently on the discussion in patch 3

> 
> Changes in v2:
> - Add kerndoc comments for new chv_pinctrl struct members
> 
> Regards,
> 
> Hans
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Mika Westerberg]

On Wed, Nov 06, 2019 at 04:47:12PM +0100, Hans de Goede wrote:
> Hi All,
> 
> Here is v2 of my series for Cherry Trail devices to pass the irqchip when
> adding the gpiochip instead of registering it separately. Similar to how
> this is done for Bay Trail devices in Andy's recent series:
> "[RESEND][PATCH v2 0/7] gpiolib: fix GPIO <-> pin mapping registration".
> 
> Note this series depends on that series as well as on the cherryview changes
> currently queued in pinctrl/intel.git/for-next .
> 
> Changes in v2:
> - Add kerndoc comments for new chv_pinctrl struct members

The series looks good to me now. Thanks!

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 0/3] pinctrl: cherryview: Pass irqchip when adding gpiochip

[Andy Shevchenko]

On Wed, Nov 06, 2019 at 07:47:59PM +0200, Mika Westerberg wrote:
> On Wed, Nov 06, 2019 at 04:47:12PM +0100, Hans de Goede wrote:
> > Hi All,
> > 
> > Here is v2 of my series for Cherry Trail devices to pass the irqchip when
> > adding the gpiochip instead of registering it separately. Similar to how
> > this is done for Bay Trail devices in Andy's recent series:
> > "[RESEND][PATCH v2 0/7] gpiolib: fix GPIO <-> pin mapping registration".
> > 
> > Note this series depends on that series as well as on the cherryview changes
> > currently queued in pinctrl/intel.git/for-next .
> > 
> > Changes in v2:
> > - Add kerndoc comments for new chv_pinctrl struct members
> 
> The series looks good to me now. Thanks!
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review-andy branch for better testing coverage, though it's not an
immediate candidate for v5.5 due to dependencies.

-- 
With Best Regards,
Andy Shevchenko