diff mbox series

pinctrl: qcom: spmi-gpio: make the irqchip immutable

Message ID 20220624195112.894916-1-robimarko@gmail.com
State Accepted
Commit 7542766e78fc374d81d8c2db214c4b4308645277
Headers show
Series pinctrl: qcom: spmi-gpio: make the irqchip immutable | expand

Commit Message

Robert Marko June 24, 2022, 7:51 p.m. UTC
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(-)

Comments

Manivannan Sadhasivam July 6, 2022, 2 p.m. UTC | #1
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
>
Linus Walleij July 9, 2022, 11:05 p.m. UTC | #2
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
Marc Zyngier July 12, 2022, 10:42 a.m. UTC | #3
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.
Manivannan Sadhasivam July 12, 2022, 12:44 p.m. UTC | #4
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.
Marc Zyngier July 12, 2022, 3:12 p.m. UTC | #5
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.
Manivannan Sadhasivam July 19, 2022, 7:47 a.m. UTC | #6
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...
Robert Marko Aug. 16, 2022, 12:45 p.m. UTC | #7
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...
>
> --
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam Aug. 19, 2022, 6:34 a.m. UTC | #8
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...
> >
> > --
> > மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

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);