| Message ID | 20191106154715.155596-4-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: cherryview: Pass irqchip when adding gpiochip | expand |
On Wed, Nov 06, 2019 at 04:47:15PM +0100, Hans de Goede wrote: > We need to convert all old gpio irqchips to pass the irqchip > setup along when adding the gpio_chip. For more info see > drivers/gpio/TODO. > > For chained irqchips this is a pretty straight-forward conversion. > + chip->irq.chip = &pctrl->irqchip; > + if (pctrl->need_valid_mask) > + chip->irq.init_valid_mask = chv_init_irq_valid_mask; I just realize we probably may assign here unconditionally > + chip->irq.init_hw = chv_gpio_irq_init_hw; > + chip->irq.parent_handler = chv_gpio_irq_handler; > + chip->irq.num_parents = 1; > + chip->irq.parents = &pctrl->irq; > + chip->irq.default_type = IRQ_TYPE_NONE; > + chip->irq.handler = handle_bad_irq; > > if (!pctrl->need_valid_mask) { > irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0, > @@ -1640,18 +1651,9 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq) > } > } > > - pctrl->irqchip.name = "chv-gpio"; > - pctrl->irqchip.irq_startup = chv_gpio_irq_startup; > - pctrl->irqchip.irq_ack = chv_gpio_irq_ack; > - pctrl->irqchip.irq_mask = chv_gpio_irq_mask; > - pctrl->irqchip.irq_unmask = chv_gpio_irq_unmask; > - pctrl->irqchip.irq_set_type = chv_gpio_irq_type; > - pctrl->irqchip.flags = IRQCHIP_SKIP_SET_WAKE; > - > - ret = gpiochip_irqchip_add(chip, &pctrl->irqchip, 0, > - handle_bad_irq, IRQ_TYPE_NONE); > + ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl); > if (ret) { > - dev_err(pctrl->dev, "failed to add IRQ chip\n"); > + dev_err(pctrl->dev, "Failed to register gpiochip\n"); > return ret; > } > > @@ -1665,8 +1667,6 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq) > } > } > > - gpiochip_set_chained_irqchip(chip, &pctrl->irqchip, irq, > - chv_gpio_irq_handler); > return 0; > } > > -- > 2.23.0 >
On Wed, Nov 06, 2019 at 06:16:22PM +0200, Andy Shevchenko wrote: > On Wed, Nov 06, 2019 at 04:47:15PM +0100, Hans de Goede wrote: > > We need to convert all old gpio irqchips to pass the irqchip > > setup along when adding the gpio_chip. For more info see > > drivers/gpio/TODO. > > > > For chained irqchips this is a pretty straight-forward conversion. > > > + chip->irq.chip = &pctrl->irqchip; > > > + if (pctrl->need_valid_mask) > > + chip->irq.init_valid_mask = chv_init_irq_valid_mask; > > I just realize we probably may assign here unconditionally Continuing... > > > + chip->irq.init_hw = chv_gpio_irq_init_hw; > > + chip->irq.parent_handler = chv_gpio_irq_handler; > > + chip->irq.num_parents = 1; > > + chip->irq.parents = &pctrl->irq; > > + chip->irq.default_type = IRQ_TYPE_NONE; > > + chip->irq.handler = handle_bad_irq; > > > > if (!pctrl->need_valid_mask) { And here turn it back to NULL and check the pointer against NULL instead of additional variable. What do you think?
Hi, On 06-11-2019 17:17, Andy Shevchenko wrote: > On Wed, Nov 06, 2019 at 06:16:22PM +0200, Andy Shevchenko wrote: >> On Wed, Nov 06, 2019 at 04:47:15PM +0100, Hans de Goede wrote: >>> We need to convert all old gpio irqchips to pass the irqchip >>> setup along when adding the gpio_chip. For more info see >>> drivers/gpio/TODO. >>> >>> For chained irqchips this is a pretty straight-forward conversion. >> >>> + chip->irq.chip = &pctrl->irqchip; >> >>> + if (pctrl->need_valid_mask) >>> + chip->irq.init_valid_mask = chv_init_irq_valid_mask; >> >> I just realize we probably may assign here unconditionally > > Continuing... > >> >>> + chip->irq.init_hw = chv_gpio_irq_init_hw; >>> + chip->irq.parent_handler = chv_gpio_irq_handler; >>> + chip->irq.num_parents = 1; >>> + chip->irq.parents = &pctrl->irq; >>> + chip->irq.default_type = IRQ_TYPE_NONE; >>> + chip->irq.handler = handle_bad_irq; >>> >>> if (!pctrl->need_valid_mask) { > > And here turn it back to NULL and check the pointer against NULL instead of > additional variable. > > What do you think? I think that first setting it and then clearing it again is not very pretty. But ... I do think you are on to something, we can use pctrl->chip.irq.init_valid_mask instead of storing the dmi quirk in the chv_pinctrl struct. Then we can leave the dmi handling as is, and replace later checks for the dmi quirk (in callbacks) with a check for pctrl->chip.irq.init_valid_mask I do believe that that is better then adding a need_validmask member to the chv_pinctrl struct, I will prepare a v3 of the series with this change. Regards, Hans
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index 1ded4bd8d1b4..e7c78acdcfbc 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -150,6 +150,7 @@ struct chv_pin_context { * @irqchip: IRQ chip in this pin controller * @regs: MMIO registers * @need_valid_mask: Use chip.irq.init_valid_mask ? + * @irq: Our parent irq * @intr_lines: Stores mapping between 16 HW interrupt wires and GPIO * offset (in GPIO number space) * @community: Community this pinctrl instance represents @@ -167,6 +168,7 @@ struct chv_pinctrl { struct irq_chip irqchip; void __iomem *regs; bool need_valid_mask; + unsigned int irq; unsigned intr_lines[16]; const struct chv_community *community; u32 saved_intmask; @@ -1620,16 +1622,25 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq) chip->add_pin_ranges = chv_gpio_add_pin_ranges; chip->parent = pctrl->dev; chip->base = -1; - if (pctrl->need_valid_mask) - chip->irq.init_valid_mask = chv_init_irq_valid_mask; - ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl); - if (ret) { - dev_err(pctrl->dev, "Failed to register gpiochip\n"); - return ret; - } + pctrl->irq = irq; + pctrl->irqchip.name = "chv-gpio"; + pctrl->irqchip.irq_startup = chv_gpio_irq_startup; + pctrl->irqchip.irq_ack = chv_gpio_irq_ack; + pctrl->irqchip.irq_mask = chv_gpio_irq_mask; + pctrl->irqchip.irq_unmask = chv_gpio_irq_unmask; + pctrl->irqchip.irq_set_type = chv_gpio_irq_type; + pctrl->irqchip.flags = IRQCHIP_SKIP_SET_WAKE; - chv_gpio_irq_init_hw(chip); + chip->irq.chip = &pctrl->irqchip; + if (pctrl->need_valid_mask) + chip->irq.init_valid_mask = chv_init_irq_valid_mask; + chip->irq.init_hw = chv_gpio_irq_init_hw; + chip->irq.parent_handler = chv_gpio_irq_handler; + chip->irq.num_parents = 1; + chip->irq.parents = &pctrl->irq; + chip->irq.default_type = IRQ_TYPE_NONE; + chip->irq.handler = handle_bad_irq; if (!pctrl->need_valid_mask) { irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0, @@ -1640,18 +1651,9 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq) } } - pctrl->irqchip.name = "chv-gpio"; - pctrl->irqchip.irq_startup = chv_gpio_irq_startup; - pctrl->irqchip.irq_ack = chv_gpio_irq_ack; - pctrl->irqchip.irq_mask = chv_gpio_irq_mask; - pctrl->irqchip.irq_unmask = chv_gpio_irq_unmask; - pctrl->irqchip.irq_set_type = chv_gpio_irq_type; - pctrl->irqchip.flags = IRQCHIP_SKIP_SET_WAKE; - - ret = gpiochip_irqchip_add(chip, &pctrl->irqchip, 0, - handle_bad_irq, IRQ_TYPE_NONE); + ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl); if (ret) { - dev_err(pctrl->dev, "failed to add IRQ chip\n"); + dev_err(pctrl->dev, "Failed to register gpiochip\n"); return ret; } @@ -1665,8 +1667,6 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq) } } - gpiochip_set_chained_irqchip(chip, &pctrl->irqchip, irq, - chv_gpio_irq_handler); return 0; }
We need to convert all old gpio irqchips to pass the irqchip setup along when adding the gpio_chip. For more info see drivers/gpio/TODO. For chained irqchips this is a pretty straight-forward conversion. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Add kerneldoc for chv_pinctrl.irq struct member --- drivers/pinctrl/intel/pinctrl-cherryview.c | 42 +++++++++++----------- 1 file changed, 21 insertions(+), 21 deletions(-)