[PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Robert Marko]

Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
immutable") added a warning to indicate if the gpiolib is altering the
internals of irqchips.

Following this change the following warning is now observed for the SPMI
PMIC pinctrl driver:
gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!

Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index c3255b0bece4..406ee0933d0b 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -171,7 +171,6 @@ struct pmic_gpio_state {
 	struct regmap	*map;
 	struct pinctrl_dev *ctrl;
 	struct gpio_chip chip;
-	struct irq_chip irq;
 	u8 usid;
 	u8 pid_base;
 };
@@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
 	return fwspec;
 }
 
+static const struct irq_chip spmi_gpio_irq_chip = {
+	.name		= "spmi-gpio",
+	.irq_ack	= irq_chip_ack_parent,
+	.irq_mask	= irq_chip_mask_parent,
+	.irq_unmask	= irq_chip_unmask_parent,
+	.irq_set_type	= irq_chip_set_type_parent,
+	.irq_set_wake	= irq_chip_set_wake_parent,
+	.flags		= IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
 static int pmic_gpio_probe(struct platform_device *pdev)
 {
 	struct irq_domain *parent_domain;
@@ -1081,16 +1091,8 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	if (!parent_domain)
 		return -ENXIO;
 
-	state->irq.name = "spmi-gpio",
-	state->irq.irq_ack = irq_chip_ack_parent,
-	state->irq.irq_mask = irq_chip_mask_parent,
-	state->irq.irq_unmask = irq_chip_unmask_parent,
-	state->irq.irq_set_type = irq_chip_set_type_parent,
-	state->irq.irq_set_wake = irq_chip_set_wake_parent,
-	state->irq.flags = IRQCHIP_MASK_ON_SUSPEND,
-
 	girq = &state->chip.irq;
-	girq->chip = &state->irq;
+	gpio_irq_chip_set_chip(girq, &spmi_gpio_irq_chip);
 	girq->default_type = IRQ_TYPE_NONE;
 	girq->handler = handle_level_irq;
 	girq->fwnode = of_node_to_fwnode(state->dev->of_node);
-- 
2.36.1

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Manivannan Sadhasivam]

On Fri, Jun 24, 2022 at 09:51:12PM +0200, Robert Marko wrote:
> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> immutable") added a warning to indicate if the gpiolib is altering the
> internals of irqchips.
> 
> Following this change the following warning is now observed for the SPMI
> PMIC pinctrl driver:
> gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> 
> Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

I've also tested this patch on Lenovo X13s, so

Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index c3255b0bece4..406ee0933d0b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -171,7 +171,6 @@ struct pmic_gpio_state {
>  	struct regmap	*map;
>  	struct pinctrl_dev *ctrl;
>  	struct gpio_chip chip;
> -	struct irq_chip irq;
>  	u8 usid;
>  	u8 pid_base;
>  };
> @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
>  	return fwspec;
>  }
>  
> +static const struct irq_chip spmi_gpio_irq_chip = {
> +	.name		= "spmi-gpio",
> +	.irq_ack	= irq_chip_ack_parent,
> +	.irq_mask	= irq_chip_mask_parent,
> +	.irq_unmask	= irq_chip_unmask_parent,
> +	.irq_set_type	= irq_chip_set_type_parent,
> +	.irq_set_wake	= irq_chip_set_wake_parent,
> +	.flags		= IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};
> +
>  static int pmic_gpio_probe(struct platform_device *pdev)
>  {
>  	struct irq_domain *parent_domain;
> @@ -1081,16 +1091,8 @@ static int pmic_gpio_probe(struct platform_device *pdev)
>  	if (!parent_domain)
>  		return -ENXIO;
>  
> -	state->irq.name = "spmi-gpio",
> -	state->irq.irq_ack = irq_chip_ack_parent,
> -	state->irq.irq_mask = irq_chip_mask_parent,
> -	state->irq.irq_unmask = irq_chip_unmask_parent,
> -	state->irq.irq_set_type = irq_chip_set_type_parent,
> -	state->irq.irq_set_wake = irq_chip_set_wake_parent,
> -	state->irq.flags = IRQCHIP_MASK_ON_SUSPEND,
> -
>  	girq = &state->chip.irq;
> -	girq->chip = &state->irq;
> +	gpio_irq_chip_set_chip(girq, &spmi_gpio_irq_chip);
>  	girq->default_type = IRQ_TYPE_NONE;
>  	girq->handler = handle_level_irq;
>  	girq->fwnode = of_node_to_fwnode(state->dev->of_node);
> -- 
> 2.36.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Linus Walleij]

On Fri, Jun 24, 2022 at 9:51 PM Robert Marko <robimarko@gmail.com> wrote:

> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> immutable") added a warning to indicate if the gpiolib is altering the
> internals of irqchips.
>
> Following this change the following warning is now observed for the SPMI
> PMIC pinctrl driver:
> gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
>
> Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>

Patch applied!

Yours,
Linus Walleij

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Marc Zyngier]

On Fri, 24 Jun 2022 20:51:12 +0100,
Robert Marko <robimarko@gmail.com> wrote:
> 
> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> immutable") added a warning to indicate if the gpiolib is altering the
> internals of irqchips.
> 
> Following this change the following warning is now observed for the SPMI
> PMIC pinctrl driver:
> gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> 
> Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index c3255b0bece4..406ee0933d0b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -171,7 +171,6 @@ struct pmic_gpio_state {
>  	struct regmap	*map;
>  	struct pinctrl_dev *ctrl;
>  	struct gpio_chip chip;
> -	struct irq_chip irq;
>  	u8 usid;
>  	u8 pid_base;
>  };
> @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
>  	return fwspec;
>  }
>  
> +static const struct irq_chip spmi_gpio_irq_chip = {
> +	.name		= "spmi-gpio",
> +	.irq_ack	= irq_chip_ack_parent,
> +	.irq_mask	= irq_chip_mask_parent,
> +	.irq_unmask	= irq_chip_unmask_parent,

No, this is wrong. Please look at the documentation to see how you
must now directly call into the gpiolib helpers for these two
callbacks.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Manivannan Sadhasivam]

On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
> On Fri, 24 Jun 2022 20:51:12 +0100,
> Robert Marko <robimarko@gmail.com> wrote:
> > 
> > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > immutable") added a warning to indicate if the gpiolib is altering the
> > internals of irqchips.
> > 
> > Following this change the following warning is now observed for the SPMI
> > PMIC pinctrl driver:
> > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> > 
> > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> > 
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > index c3255b0bece4..406ee0933d0b 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
> >  	struct regmap	*map;
> >  	struct pinctrl_dev *ctrl;
> >  	struct gpio_chip chip;
> > -	struct irq_chip irq;
> >  	u8 usid;
> >  	u8 pid_base;
> >  };
> > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> >  	return fwspec;
> >  }
> >  
> > +static const struct irq_chip spmi_gpio_irq_chip = {
> > +	.name		= "spmi-gpio",
> > +	.irq_ack	= irq_chip_ack_parent,
> > +	.irq_mask	= irq_chip_mask_parent,
> > +	.irq_unmask	= irq_chip_unmask_parent,
> 
> No, this is wrong. Please look at the documentation to see how you
> must now directly call into the gpiolib helpers for these two
> callbacks.
> 

IIUC, you are referring to gpiochip_disable_irq() and
gpiochip_enable_irq() APIs. These APIs are supposed to let the gpiolib
know about that the IRQ usage of these GPIOs. But for the case of hierarchial
IRQ domain, isn't the parent is going to do that?

Please correct me if I'm wrong.

Thanks,
Mani

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Marc Zyngier]

On Tue, 12 Jul 2022 13:44:45 +0100,
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> 
> On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
> > On Fri, 24 Jun 2022 20:51:12 +0100,
> > Robert Marko <robimarko@gmail.com> wrote:
> > > 
> > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > > immutable") added a warning to indicate if the gpiolib is altering the
> > > internals of irqchips.
> > > 
> > > Following this change the following warning is now observed for the SPMI
> > > PMIC pinctrl driver:
> > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> > > 
> > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> > > 
> > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > ---
> > >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
> > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > index c3255b0bece4..406ee0933d0b 100644
> > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
> > >  	struct regmap	*map;
> > >  	struct pinctrl_dev *ctrl;
> > >  	struct gpio_chip chip;
> > > -	struct irq_chip irq;
> > >  	u8 usid;
> > >  	u8 pid_base;
> > >  };
> > > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> > >  	return fwspec;
> > >  }
> > >  
> > > +static const struct irq_chip spmi_gpio_irq_chip = {
> > > +	.name		= "spmi-gpio",
> > > +	.irq_ack	= irq_chip_ack_parent,
> > > +	.irq_mask	= irq_chip_mask_parent,
> > > +	.irq_unmask	= irq_chip_unmask_parent,
> > 
> > No, this is wrong. Please look at the documentation to see how you
> > must now directly call into the gpiolib helpers for these two
> > callbacks.
> > 
> 
> IIUC, you are referring to gpiochip_disable_irq() and
> gpiochip_enable_irq() APIs.

I am indeed.

> These APIs are supposed to let the gpiolib know about that the IRQ
> usage of these GPIOs. But for the case of hierarchial IRQ domain,
> isn't the parent is going to do that?

Why would it? The parent has no clue about what sits above it. In a
hierarchical configuration, each level is responsible for its own
level, and the GPIO layer should be responsible for its own
management.

> Please correct me if I'm wrong.

I'm afraid you are, and this patch is a fairly obvious change in
behaviour, as the callbacks you mention above are not called anymore,
while they were before.

If they are not necessary (for reasons I can't fathom), then this
should be clearly explained.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Robert Marko]

On Tue, 12 Jul 2022 at 17:12, Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 12 Jul 2022 13:44:45 +0100,
> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
> > > On Fri, 24 Jun 2022 20:51:12 +0100,
> > > Robert Marko <robimarko@gmail.com> wrote:
> > > >
> > > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > > > immutable") added a warning to indicate if the gpiolib is altering the
> > > > internals of irqchips.
> > > >
> > > > Following this change the following warning is now observed for the SPMI
> > > > PMIC pinctrl driver:
> > > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> > > >
> > > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> > > >
> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > ---
> > > >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > index c3255b0bece4..406ee0933d0b 100644
> > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
> > > >   struct regmap   *map;
> > > >   struct pinctrl_dev *ctrl;
> > > >   struct gpio_chip chip;
> > > > - struct irq_chip irq;
> > > >   u8 usid;
> > > >   u8 pid_base;
> > > >  };
> > > > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> > > >   return fwspec;
> > > >  }
> > > >
> > > > +static const struct irq_chip spmi_gpio_irq_chip = {
> > > > + .name           = "spmi-gpio",
> > > > + .irq_ack        = irq_chip_ack_parent,
> > > > + .irq_mask       = irq_chip_mask_parent,
> > > > + .irq_unmask     = irq_chip_unmask_parent,
> > >
> > > No, this is wrong. Please look at the documentation to see how you
> > > must now directly call into the gpiolib helpers for these two
> > > callbacks.
> > >
> >
> > IIUC, you are referring to gpiochip_disable_irq() and
> > gpiochip_enable_irq() APIs.
>
> I am indeed.
>
> > These APIs are supposed to let the gpiolib know about that the IRQ
> > usage of these GPIOs. But for the case of hierarchial IRQ domain,
> > isn't the parent is going to do that?
>
> Why would it? The parent has no clue about what sits above it. In a
> hierarchical configuration, each level is responsible for its own
> level, and the GPIO layer should be responsible for its own
> management.
>
> > Please correct me if I'm wrong.
>
> I'm afraid you are, and this patch is a fairly obvious change in
> behaviour, as the callbacks you mention above are not called anymore,
> while they were before.
>
> If they are not necessary (for reasons I can't fathom), then this
> should be clearly explained.

Hi Marc,
I will look at IRQ GPIO docs, but in this case, then we have more
conversions that
are not correct.

Regards,
Robert
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Marc Zyngier]

On 2022-07-13 12:08, Robert Marko wrote:
> On Tue, 12 Jul 2022 at 17:12, Marc Zyngier <maz@kernel.org> wrote:
>> 
>> On Tue, 12 Jul 2022 13:44:45 +0100,
>> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>> >
>> > On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
>> > > On Fri, 24 Jun 2022 20:51:12 +0100,
>> > > Robert Marko <robimarko@gmail.com> wrote:
>> > > >
>> > > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
>> > > > immutable") added a warning to indicate if the gpiolib is altering the
>> > > > internals of irqchips.
>> > > >
>> > > > Following this change the following warning is now observed for the SPMI
>> > > > PMIC pinctrl driver:
>> > > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
>> > > >
>> > > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
>> > > >
>> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
>> > > > ---
>> > > >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
>> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
>> > > >
>> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> > > > index c3255b0bece4..406ee0933d0b 100644
>> > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> > > > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
>> > > >   struct regmap   *map;
>> > > >   struct pinctrl_dev *ctrl;
>> > > >   struct gpio_chip chip;
>> > > > - struct irq_chip irq;
>> > > >   u8 usid;
>> > > >   u8 pid_base;
>> > > >  };
>> > > > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
>> > > >   return fwspec;
>> > > >  }
>> > > >
>> > > > +static const struct irq_chip spmi_gpio_irq_chip = {
>> > > > + .name           = "spmi-gpio",
>> > > > + .irq_ack        = irq_chip_ack_parent,
>> > > > + .irq_mask       = irq_chip_mask_parent,
>> > > > + .irq_unmask     = irq_chip_unmask_parent,
>> > >
>> > > No, this is wrong. Please look at the documentation to see how you
>> > > must now directly call into the gpiolib helpers for these two
>> > > callbacks.
>> > >
>> >
>> > IIUC, you are referring to gpiochip_disable_irq() and
>> > gpiochip_enable_irq() APIs.
>> 
>> I am indeed.
>> 
>> > These APIs are supposed to let the gpiolib know about that the IRQ
>> > usage of these GPIOs. But for the case of hierarchial IRQ domain,
>> > isn't the parent is going to do that?
>> 
>> Why would it? The parent has no clue about what sits above it. In a
>> hierarchical configuration, each level is responsible for its own
>> level, and the GPIO layer should be responsible for its own
>> management.
>> 
>> > Please correct me if I'm wrong.
>> 
>> I'm afraid you are, and this patch is a fairly obvious change in
>> behaviour, as the callbacks you mention above are not called anymore,
>> while they were before.
>> 
>> If they are not necessary (for reasons I can't fathom), then this
>> should be clearly explained.
> 
> Hi Marc,
> I will look at IRQ GPIO docs, but in this case, then we have more
> conversions that
> are not correct.

Then please point them out.

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

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Robert Marko]

On Wed, 13 Jul 2022 at 13:47, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2022-07-13 12:08, Robert Marko wrote:
> > On Tue, 12 Jul 2022 at 17:12, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On Tue, 12 Jul 2022 13:44:45 +0100,
> >> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> >> >
> >> > On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
> >> > > On Fri, 24 Jun 2022 20:51:12 +0100,
> >> > > Robert Marko <robimarko@gmail.com> wrote:
> >> > > >
> >> > > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> >> > > > immutable") added a warning to indicate if the gpiolib is altering the
> >> > > > internals of irqchips.
> >> > > >
> >> > > > Following this change the following warning is now observed for the SPMI
> >> > > > PMIC pinctrl driver:
> >> > > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> >> > > >
> >> > > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> >> > > >
> >> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> >> > > > ---
> >> > > >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
> >> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
> >> > > >
> >> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> >> > > > index c3255b0bece4..406ee0933d0b 100644
> >> > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> >> > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> >> > > > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
> >> > > >   struct regmap   *map;
> >> > > >   struct pinctrl_dev *ctrl;
> >> > > >   struct gpio_chip chip;
> >> > > > - struct irq_chip irq;
> >> > > >   u8 usid;
> >> > > >   u8 pid_base;
> >> > > >  };
> >> > > > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> >> > > >   return fwspec;
> >> > > >  }
> >> > > >
> >> > > > +static const struct irq_chip spmi_gpio_irq_chip = {
> >> > > > + .name           = "spmi-gpio",
> >> > > > + .irq_ack        = irq_chip_ack_parent,
> >> > > > + .irq_mask       = irq_chip_mask_parent,
> >> > > > + .irq_unmask     = irq_chip_unmask_parent,
> >> > >
> >> > > No, this is wrong. Please look at the documentation to see how you
> >> > > must now directly call into the gpiolib helpers for these two
> >> > > callbacks.
> >> > >
> >> >
> >> > IIUC, you are referring to gpiochip_disable_irq() and
> >> > gpiochip_enable_irq() APIs.
> >>
> >> I am indeed.
> >>
> >> > These APIs are supposed to let the gpiolib know about that the IRQ
> >> > usage of these GPIOs. But for the case of hierarchial IRQ domain,
> >> > isn't the parent is going to do that?
> >>
> >> Why would it? The parent has no clue about what sits above it. In a
> >> hierarchical configuration, each level is responsible for its own
> >> level, and the GPIO layer should be responsible for its own
> >> management.
> >>
> >> > Please correct me if I'm wrong.
> >>
> >> I'm afraid you are, and this patch is a fairly obvious change in
> >> behaviour, as the callbacks you mention above are not called anymore,
> >> while they were before.
> >>
> >> If they are not necessary (for reasons I can't fathom), then this
> >> should be clearly explained.
> >
> > Hi Marc,
> > I will look at IRQ GPIO docs, but in this case, then we have more
> > conversions that
> > are not correct.
>
> Then please point them out.

Oh, now I get the issue, I was misunderstanding it completely.
gpiochip_enable_irq and gpiochip_disable_irq are not being called
at all.

However, I dont see them being called before the conversion as well.
I am not really familiar with the PMIC IRQ-s, looked like an easy conversion
to get rid of the warning.

Manivannan can you shed some light on this?

Regards,
Robert





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

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Marc Zyngier]

On Wed, 13 Jul 2022 13:33:32 +0100,
Robert Marko <robimarko@gmail.com> wrote:
> 
> On Wed, 13 Jul 2022 at 13:47, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On 2022-07-13 12:08, Robert Marko wrote:
> > > I will look at IRQ GPIO docs, but in this case, then we have more
> > > conversions that
> > > are not correct.
> >
> > Then please point them out.
> 
> Oh, now I get the issue, I was misunderstanding it completely.
> gpiochip_enable_irq and gpiochip_disable_irq are not being called
> at all.
> 
> However, I dont see them being called before the conversion as well.
> I am not really familiar with the PMIC IRQ-s, looked like an easy conversion
> to get rid of the warning.

They definitely were. Look at how gpiochip_add_data() eventually ends
up calling gpiochip_set_irq_hooks((), which hijacks the irq_chip
function pointers to insert the calls to these helpers.

This is what this conversion process is all about, and you absolutely
need to understand that code before blindly changing drivers to squash
a run-time warning.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Robert Marko]

On Wed, 13 Jul 2022 at 15:10, Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 13 Jul 2022 13:33:32 +0100,
> Robert Marko <robimarko@gmail.com> wrote:
> >
> > On Wed, 13 Jul 2022 at 13:47, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On 2022-07-13 12:08, Robert Marko wrote:
> > > > I will look at IRQ GPIO docs, but in this case, then we have more
> > > > conversions that
> > > > are not correct.
> > >
> > > Then please point them out.
> >
> > Oh, now I get the issue, I was misunderstanding it completely.
> > gpiochip_enable_irq and gpiochip_disable_irq are not being called
> > at all.
> >
> > However, I dont see them being called before the conversion as well.
> > I am not really familiar with the PMIC IRQ-s, looked like an easy conversion
> > to get rid of the warning.
>
> They definitely were. Look at how gpiochip_add_data() eventually ends
> up calling gpiochip_set_irq_hooks((), which hijacks the irq_chip
> function pointers to insert the calls to these helpers.

Well, that is the thing, since irqchip->irq_enable and
irqchip->irq_disable were never
populated in the SPMI GPIO driver, gpiochip_set_irq_hooks then does
not insert them.
During runtime, gpiochip_irq_enable and gpiochip_irq_disable are never
used even before
the conversion, that is what I am trying to convey.

>
> This is what this conversion process is all about, and you absolutely
> need to understand that code before blindly changing drivers to squash
> a run-time warning.

Yeah, I agree, hopefully, I am not annoying.

Regards,
Robert
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Marc Zyngier]

On Wed, 13 Jul 2022 14:34:46 +0100,
Robert Marko <robimarko@gmail.com> wrote:
> 
> On Wed, 13 Jul 2022 at 15:10, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 13 Jul 2022 13:33:32 +0100,
> > Robert Marko <robimarko@gmail.com> wrote:
> > >
> > > On Wed, 13 Jul 2022 at 13:47, Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On 2022-07-13 12:08, Robert Marko wrote:
> > > > > I will look at IRQ GPIO docs, but in this case, then we have more
> > > > > conversions that
> > > > > are not correct.
> > > >
> > > > Then please point them out.
> > >
> > > Oh, now I get the issue, I was misunderstanding it completely.
> > > gpiochip_enable_irq and gpiochip_disable_irq are not being called
> > > at all.
> > >
> > > However, I dont see them being called before the conversion as well.
> > > I am not really familiar with the PMIC IRQ-s, looked like an easy conversion
> > > to get rid of the warning.
> >
> > They definitely were. Look at how gpiochip_add_data() eventually ends
> > up calling gpiochip_set_irq_hooks((), which hijacks the irq_chip
> > function pointers to insert the calls to these helpers.
> 
> Well, that is the thing, since irqchip->irq_enable and
> irqchip->irq_disable were never populated in the SPMI GPIO driver,
> gpiochip_set_irq_hooks then does not insert them.
> During runtime, gpiochip_irq_enable and gpiochip_irq_disable are
> never used even before the conversion, that is what I am trying to
> convey.

It is the mask/unmask versions that would be used, with similar
effects.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Linus Walleij]

On Sun, Jul 10, 2022 at 1:05 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jun 24, 2022 at 9:51 PM Robert Marko <robimarko@gmail.com> wrote:
>
> > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > immutable") added a warning to indicate if the gpiolib is altering the
> > internals of irqchips.
> >
> > Following this change the following warning is now observed for the SPMI
> > PMIC pinctrl driver:
> > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> >
> > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
>
> Patch applied!

Jumping the gun too quick.

Reverted the patch following Marc's feedback.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Robert Marko]

On Mon, 18 Jul 2022 at 11:59, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sun, Jul 10, 2022 at 1:05 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Jun 24, 2022 at 9:51 PM Robert Marko <robimarko@gmail.com> wrote:
> >
> > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > > immutable") added a warning to indicate if the gpiolib is altering the
> > > internals of irqchips.
> > >
> > > Following this change the following warning is now observed for the SPMI
> > > PMIC pinctrl driver:
> > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> > >
> > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> > >
> > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> >
> > Patch applied!
>
> Jumping the gun too quick.
>
> Reverted the patch following Marc's feedback.

Thanks for doing this, I was planning to send a revert patch.

Regards,
Robert
>
> Yours,
> Linus Walleij

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Manivannan Sadhasivam]

On Wed, Jul 13, 2022 at 02:33:32PM +0200, Robert Marko wrote:
> On Wed, 13 Jul 2022 at 13:47, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On 2022-07-13 12:08, Robert Marko wrote:
> > > On Tue, 12 Jul 2022 at 17:12, Marc Zyngier <maz@kernel.org> wrote:
> > >>
> > >> On Tue, 12 Jul 2022 13:44:45 +0100,
> > >> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> > >> >
> > >> > On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
> > >> > > On Fri, 24 Jun 2022 20:51:12 +0100,
> > >> > > Robert Marko <robimarko@gmail.com> wrote:
> > >> > > >
> > >> > > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > >> > > > immutable") added a warning to indicate if the gpiolib is altering the
> > >> > > > internals of irqchips.
> > >> > > >
> > >> > > > Following this change the following warning is now observed for the SPMI
> > >> > > > PMIC pinctrl driver:
> > >> > > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> > >> > > >
> > >> > > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> > >> > > >
> > >> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > >> > > > ---
> > >> > > >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
> > >> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > >> > > >
> > >> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > >> > > > index c3255b0bece4..406ee0933d0b 100644
> > >> > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > >> > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > >> > > > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
> > >> > > >   struct regmap   *map;
> > >> > > >   struct pinctrl_dev *ctrl;
> > >> > > >   struct gpio_chip chip;
> > >> > > > - struct irq_chip irq;
> > >> > > >   u8 usid;
> > >> > > >   u8 pid_base;
> > >> > > >  };
> > >> > > > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> > >> > > >   return fwspec;
> > >> > > >  }
> > >> > > >
> > >> > > > +static const struct irq_chip spmi_gpio_irq_chip = {
> > >> > > > + .name           = "spmi-gpio",
> > >> > > > + .irq_ack        = irq_chip_ack_parent,
> > >> > > > + .irq_mask       = irq_chip_mask_parent,
> > >> > > > + .irq_unmask     = irq_chip_unmask_parent,
> > >> > >
> > >> > > No, this is wrong. Please look at the documentation to see how you
> > >> > > must now directly call into the gpiolib helpers for these two
> > >> > > callbacks.
> > >> > >
> > >> >
> > >> > IIUC, you are referring to gpiochip_disable_irq() and
> > >> > gpiochip_enable_irq() APIs.
> > >>
> > >> I am indeed.
> > >>
> > >> > These APIs are supposed to let the gpiolib know about that the IRQ
> > >> > usage of these GPIOs. But for the case of hierarchial IRQ domain,
> > >> > isn't the parent is going to do that?
> > >>
> > >> Why would it? The parent has no clue about what sits above it. In a
> > >> hierarchical configuration, each level is responsible for its own
> > >> level, and the GPIO layer should be responsible for its own
> > >> management.
> > >>
> > >> > Please correct me if I'm wrong.
> > >>
> > >> I'm afraid you are, and this patch is a fairly obvious change in
> > >> behaviour, as the callbacks you mention above are not called anymore,
> > >> while they were before.
> > >>
> > >> If they are not necessary (for reasons I can't fathom), then this
> > >> should be clearly explained.
> > >
> > > Hi Marc,
> > > I will look at IRQ GPIO docs, but in this case, then we have more
> > > conversions that
> > > are not correct.
> >
> > Then please point them out.
> 
> Oh, now I get the issue, I was misunderstanding it completely.
> gpiochip_enable_irq and gpiochip_disable_irq are not being called
> at all.
> 
> However, I dont see them being called before the conversion as well.
> I am not really familiar with the PMIC IRQ-s, looked like an easy conversion
> to get rid of the warning.
> 
> Manivannan can you shed some light on this?
> 

I hope you got the answer by now. When I looked into the conversion I saw that
there were missing calls to gpiochip_{enable/disable}_irq APIs. But at that
time I blindly assumed (yeah very bad of myself) that the parent irqchip will
handle that :(

Anyway, you should call these helpers from the mask/unmask callbacks as a part
of the conversion patch. Let me know if you are onto it or not!

Thanks,
Mani

> Regards,
> Robert
> 
> 
> 
> 
> 
> >
> >          M.
> > --
> > Jazz is not dead. It just smells funny...

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Robert Marko]

On Tue, 19 Jul 2022 at 09:47, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jul 13, 2022 at 02:33:32PM +0200, Robert Marko wrote:
> > On Wed, 13 Jul 2022 at 13:47, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On 2022-07-13 12:08, Robert Marko wrote:
> > > > On Tue, 12 Jul 2022 at 17:12, Marc Zyngier <maz@kernel.org> wrote:
> > > >>
> > > >> On Tue, 12 Jul 2022 13:44:45 +0100,
> > > >> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> > > >> >
> > > >> > On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
> > > >> > > On Fri, 24 Jun 2022 20:51:12 +0100,
> > > >> > > Robert Marko <robimarko@gmail.com> wrote:
> > > >> > > >
> > > >> > > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > > >> > > > immutable") added a warning to indicate if the gpiolib is altering the
> > > >> > > > internals of irqchips.
> > > >> > > >
> > > >> > > > Following this change the following warning is now observed for the SPMI
> > > >> > > > PMIC pinctrl driver:
> > > >> > > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> > > >> > > >
> > > >> > > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> > > >> > > >
> > > >> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > >> > > > ---
> > > >> > > >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
> > > >> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > > >> > > >
> > > >> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > >> > > > index c3255b0bece4..406ee0933d0b 100644
> > > >> > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > >> > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > >> > > > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
> > > >> > > >   struct regmap   *map;
> > > >> > > >   struct pinctrl_dev *ctrl;
> > > >> > > >   struct gpio_chip chip;
> > > >> > > > - struct irq_chip irq;
> > > >> > > >   u8 usid;
> > > >> > > >   u8 pid_base;
> > > >> > > >  };
> > > >> > > > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> > > >> > > >   return fwspec;
> > > >> > > >  }
> > > >> > > >
> > > >> > > > +static const struct irq_chip spmi_gpio_irq_chip = {
> > > >> > > > + .name           = "spmi-gpio",
> > > >> > > > + .irq_ack        = irq_chip_ack_parent,
> > > >> > > > + .irq_mask       = irq_chip_mask_parent,
> > > >> > > > + .irq_unmask     = irq_chip_unmask_parent,
> > > >> > >
> > > >> > > No, this is wrong. Please look at the documentation to see how you
> > > >> > > must now directly call into the gpiolib helpers for these two
> > > >> > > callbacks.
> > > >> > >
> > > >> >
> > > >> > IIUC, you are referring to gpiochip_disable_irq() and
> > > >> > gpiochip_enable_irq() APIs.
> > > >>
> > > >> I am indeed.
> > > >>
> > > >> > These APIs are supposed to let the gpiolib know about that the IRQ
> > > >> > usage of these GPIOs. But for the case of hierarchial IRQ domain,
> > > >> > isn't the parent is going to do that?
> > > >>
> > > >> Why would it? The parent has no clue about what sits above it. In a
> > > >> hierarchical configuration, each level is responsible for its own
> > > >> level, and the GPIO layer should be responsible for its own
> > > >> management.
> > > >>
> > > >> > Please correct me if I'm wrong.
> > > >>
> > > >> I'm afraid you are, and this patch is a fairly obvious change in
> > > >> behaviour, as the callbacks you mention above are not called anymore,
> > > >> while they were before.
> > > >>
> > > >> If they are not necessary (for reasons I can't fathom), then this
> > > >> should be clearly explained.
> > > >
> > > > Hi Marc,
> > > > I will look at IRQ GPIO docs, but in this case, then we have more
> > > > conversions that
> > > > are not correct.
> > >
> > > Then please point them out.
> >
> > Oh, now I get the issue, I was misunderstanding it completely.
> > gpiochip_enable_irq and gpiochip_disable_irq are not being called
> > at all.
> >
> > However, I dont see them being called before the conversion as well.
> > I am not really familiar with the PMIC IRQ-s, looked like an easy conversion
> > to get rid of the warning.
> >
> > Manivannan can you shed some light on this?
> >
>
> I hope you got the answer by now. When I looked into the conversion I saw that
> there were missing calls to gpiochip_{enable/disable}_irq APIs. But at that
> time I blindly assumed (yeah very bad of myself) that the parent irqchip will
> handle that :(
>
> Anyway, you should call these helpers from the mask/unmask callbacks as a part
> of the conversion patch. Let me know if you are onto it or not!

Hi, I completely missed your reply.
Currently, I am pretty swamped with other work so I dont know when
will I be able
to look into this again.

Regards,
Robert
>
> Thanks,
> Mani
>
> > Regards,
> > Robert
> >
> >
> >
> >
> >
> > >
> > >          M.
> > > --
> > > Jazz is not dead. It just smells funny...
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] pinctrl: qcom: spmi-gpio: make the irqchip immutable

[Manivannan Sadhasivam]

On Tue, Aug 16, 2022 at 02:45:07PM +0200, Robert Marko wrote:
> On Tue, 19 Jul 2022 at 09:47, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Wed, Jul 13, 2022 at 02:33:32PM +0200, Robert Marko wrote:
> > > On Wed, 13 Jul 2022 at 13:47, Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On 2022-07-13 12:08, Robert Marko wrote:
> > > > > On Tue, 12 Jul 2022 at 17:12, Marc Zyngier <maz@kernel.org> wrote:
> > > > >>
> > > > >> On Tue, 12 Jul 2022 13:44:45 +0100,
> > > > >> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> > > > >> >
> > > > >> > On Tue, Jul 12, 2022 at 11:42:32AM +0100, Marc Zyngier wrote:
> > > > >> > > On Fri, 24 Jun 2022 20:51:12 +0100,
> > > > >> > > Robert Marko <robimarko@gmail.com> wrote:
> > > > >> > > >
> > > > >> > > > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> > > > >> > > > immutable") added a warning to indicate if the gpiolib is altering the
> > > > >> > > > internals of irqchips.
> > > > >> > > >
> > > > >> > > > Following this change the following warning is now observed for the SPMI
> > > > >> > > > PMIC pinctrl driver:
> > > > >> > > > gpio gpiochip1: (200f000.spmi:pmic@0:gpio@c000): not an immutable chip, please consider fixing it!
> > > > >> > > >
> > > > >> > > > Fix this by making the irqchip in the SPMI PMIC pinctrl driver immutable.
> > > > >> > > >
> > > > >> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > >> > > > ---
> > > > >> > > >  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 22 ++++++++++++----------
> > > > >> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > > > >> > > >
> > > > >> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > >> > > > index c3255b0bece4..406ee0933d0b 100644
> > > > >> > > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > >> > > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > >> > > > @@ -171,7 +171,6 @@ struct pmic_gpio_state {
> > > > >> > > >   struct regmap   *map;
> > > > >> > > >   struct pinctrl_dev *ctrl;
> > > > >> > > >   struct gpio_chip chip;
> > > > >> > > > - struct irq_chip irq;
> > > > >> > > >   u8 usid;
> > > > >> > > >   u8 pid_base;
> > > > >> > > >  };
> > > > >> > > > @@ -988,6 +987,17 @@ static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
> > > > >> > > >   return fwspec;
> > > > >> > > >  }
> > > > >> > > >
> > > > >> > > > +static const struct irq_chip spmi_gpio_irq_chip = {
> > > > >> > > > + .name           = "spmi-gpio",
> > > > >> > > > + .irq_ack        = irq_chip_ack_parent,
> > > > >> > > > + .irq_mask       = irq_chip_mask_parent,
> > > > >> > > > + .irq_unmask     = irq_chip_unmask_parent,
> > > > >> > >
> > > > >> > > No, this is wrong. Please look at the documentation to see how you
> > > > >> > > must now directly call into the gpiolib helpers for these two
> > > > >> > > callbacks.
> > > > >> > >
> > > > >> >
> > > > >> > IIUC, you are referring to gpiochip_disable_irq() and
> > > > >> > gpiochip_enable_irq() APIs.
> > > > >>
> > > > >> I am indeed.
> > > > >>
> > > > >> > These APIs are supposed to let the gpiolib know about that the IRQ
> > > > >> > usage of these GPIOs. But for the case of hierarchial IRQ domain,
> > > > >> > isn't the parent is going to do that?
> > > > >>
> > > > >> Why would it? The parent has no clue about what sits above it. In a
> > > > >> hierarchical configuration, each level is responsible for its own
> > > > >> level, and the GPIO layer should be responsible for its own
> > > > >> management.
> > > > >>
> > > > >> > Please correct me if I'm wrong.
> > > > >>
> > > > >> I'm afraid you are, and this patch is a fairly obvious change in
> > > > >> behaviour, as the callbacks you mention above are not called anymore,
> > > > >> while they were before.
> > > > >>
> > > > >> If they are not necessary (for reasons I can't fathom), then this
> > > > >> should be clearly explained.
> > > > >
> > > > > Hi Marc,
> > > > > I will look at IRQ GPIO docs, but in this case, then we have more
> > > > > conversions that
> > > > > are not correct.
> > > >
> > > > Then please point them out.
> > >
> > > Oh, now I get the issue, I was misunderstanding it completely.
> > > gpiochip_enable_irq and gpiochip_disable_irq are not being called
> > > at all.
> > >
> > > However, I dont see them being called before the conversion as well.
> > > I am not really familiar with the PMIC IRQ-s, looked like an easy conversion
> > > to get rid of the warning.
> > >
> > > Manivannan can you shed some light on this?
> > >
> >
> > I hope you got the answer by now. When I looked into the conversion I saw that
> > there were missing calls to gpiochip_{enable/disable}_irq APIs. But at that
> > time I blindly assumed (yeah very bad of myself) that the parent irqchip will
> > handle that :(
> >
> > Anyway, you should call these helpers from the mask/unmask callbacks as a part
> > of the conversion patch. Let me know if you are onto it or not!
> 
> Hi, I completely missed your reply.
> Currently, I am pretty swamped with other work so I dont know when
> will I be able
> to look into this again.
> 

No worries! I will handle it.

Thanks,
Mani

> Regards,
> Robert
> >
> > Thanks,
> > Mani
> >
> > > Regards,
> > > Robert
> > >
> > >
> > >
> > >
> > >
> > > >
> > > >          M.
> > > > --
> > > > Jazz is not dead. It just smells funny...
> >
> > --
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்