| Message ID | 20220624195112.894916-1-robimarko@gmail.com |
|---|---|
| State | Accepted |
| Commit | 7542766e78fc374d81d8c2db214c4b4308645277 |
| Headers | show |
| Series | pinctrl: qcom: spmi-gpio: make the irqchip immutable | expand |
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 >
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
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.
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.
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.
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.
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.
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...
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.
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.
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.
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
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...
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... > > -- > மணிவண்ணன் சதாசிவம்
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 --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);
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(-)