| Message ID | 20211218165258.16716-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | i2c/busses: Use platform_get_irq/_optional() variants to fetch IRQ's | expand |
On 12/18/2021 8:52 AM, Lad Prabhakar wrote: > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static > allocation of IRQ resources in DT core code, this causes an issue > when using hierarchical interrupt domains using "interrupts" property > in the node as this bypasses the hierarchical setup and messes up the > irq chaining. > > In preparation for removal of static setup of IRQ resource from DT core > code use platform_get_irq(). > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Just one nit below: > --- > drivers/i2c/busses/i2c-bcm2835.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > index 37443edbf754..d63dec5f3cb1 100644 > --- a/drivers/i2c/busses/i2c-bcm2835.c > +++ b/drivers/i2c/busses/i2c-bcm2835.c > @@ -402,7 +402,7 @@ static const struct i2c_adapter_quirks bcm2835_i2c_quirks = { > static int bcm2835_i2c_probe(struct platform_device *pdev) > { > struct bcm2835_i2c_dev *i2c_dev; > - struct resource *mem, *irq; > + struct resource *mem; > int ret; > struct i2c_adapter *adap; > struct clk *mclk; > @@ -452,12 +452,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > return ret; > } > > - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > - if (!irq) { > - dev_err(&pdev->dev, "No IRQ resource\n"); > - return -ENODEV; > - } > - i2c_dev->irq = irq->start; > + i2c_dev->irq = platform_get_irq(pdev, 0); > + if (i2c_dev->irq <= 0) > + return i2c_dev->irq ? i2c_dev->irq : -ENXIO; Why not just check for a negative return code and propagate it as is?
Hi Florian, Thank you for the review. On Sat, Dec 18, 2021 at 9:17 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 12/18/2021 8:52 AM, Lad Prabhakar wrote: > > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static > > allocation of IRQ resources in DT core code, this causes an issue > > when using hierarchical interrupt domains using "interrupts" property > > in the node as this bypasses the hierarchical setup and messes up the > > irq chaining. > > > > In preparation for removal of static setup of IRQ resource from DT core > > code use platform_get_irq(). > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Just one nit below: > > --- > > drivers/i2c/busses/i2c-bcm2835.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > > index 37443edbf754..d63dec5f3cb1 100644 > > --- a/drivers/i2c/busses/i2c-bcm2835.c > > +++ b/drivers/i2c/busses/i2c-bcm2835.c > > @@ -402,7 +402,7 @@ static const struct i2c_adapter_quirks bcm2835_i2c_quirks = { > > static int bcm2835_i2c_probe(struct platform_device *pdev) > > { > > struct bcm2835_i2c_dev *i2c_dev; > > - struct resource *mem, *irq; > > + struct resource *mem; > > int ret; > > struct i2c_adapter *adap; > > struct clk *mclk; > > @@ -452,12 +452,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > > return ret; > > } > > > > - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > - if (!irq) { > > - dev_err(&pdev->dev, "No IRQ resource\n"); > > - return -ENODEV; > > - } > > - i2c_dev->irq = irq->start; > > + i2c_dev->irq = platform_get_irq(pdev, 0); > > + if (i2c_dev->irq <= 0) > > + return i2c_dev->irq ? i2c_dev->irq : -ENXIO; > > Why not just check for a negative return code and propagate it as is? > platform_get_irq() may return 0 said that we do get a splat in this case and further request_irq() will fail so instead check it here. Cheers, Prabhakar
Hi Florian, On Sat, Dec 18, 2021 at 10:44 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > Hi Florian, > > Thank you for the review. > > On Sat, Dec 18, 2021 at 9:17 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > > > > > On 12/18/2021 8:52 AM, Lad Prabhakar wrote: > > > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static > > > allocation of IRQ resources in DT core code, this causes an issue > > > when using hierarchical interrupt domains using "interrupts" property > > > in the node as this bypasses the hierarchical setup and messes up the > > > irq chaining. > > > > > > In preparation for removal of static setup of IRQ resource from DT core > > > code use platform_get_irq(). > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Just one nit below: > > > --- > > > drivers/i2c/busses/i2c-bcm2835.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > > > index 37443edbf754..d63dec5f3cb1 100644 > > > --- a/drivers/i2c/busses/i2c-bcm2835.c > > > +++ b/drivers/i2c/busses/i2c-bcm2835.c > > > @@ -402,7 +402,7 @@ static const struct i2c_adapter_quirks bcm2835_i2c_quirks = { > > > static int bcm2835_i2c_probe(struct platform_device *pdev) > > > { > > > struct bcm2835_i2c_dev *i2c_dev; > > > - struct resource *mem, *irq; > > > + struct resource *mem; > > > int ret; > > > struct i2c_adapter *adap; > > > struct clk *mclk; > > > @@ -452,12 +452,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > > > return ret; > > > } > > > > > > - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > > - if (!irq) { > > > - dev_err(&pdev->dev, "No IRQ resource\n"); > > > - return -ENODEV; > > > - } > > > - i2c_dev->irq = irq->start; > > > + i2c_dev->irq = platform_get_irq(pdev, 0); > > > + if (i2c_dev->irq <= 0) > > > + return i2c_dev->irq ? i2c_dev->irq : -ENXIO; > > > > Why not just check for a negative return code and propagate it as is? > > > platform_get_irq() may return 0 said that we do get a splat in this > case and further request_irq() will fail so instead check it here. > My bad, just the negative check should suffice. Cheers, Prabhakar
On 12/19/2021 1:52 AM, Lad, Prabhakar wrote: > Hi Florian, > > On Sat, Dec 18, 2021 at 10:44 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: >> >> Hi Florian, >> >> Thank you for the review. >> >> On Sat, Dec 18, 2021 at 9:17 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >>> >>> >>> >>> On 12/18/2021 8:52 AM, Lad Prabhakar wrote: >>>> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static >>>> allocation of IRQ resources in DT core code, this causes an issue >>>> when using hierarchical interrupt domains using "interrupts" property >>>> in the node as this bypasses the hierarchical setup and messes up the >>>> irq chaining. >>>> >>>> In preparation for removal of static setup of IRQ resource from DT core >>>> code use platform_get_irq(). >>>> >>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>> >>> Just one nit below: >>>> --- >>>> drivers/i2c/busses/i2c-bcm2835.c | 11 ++++------- >>>> 1 file changed, 4 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c >>>> index 37443edbf754..d63dec5f3cb1 100644 >>>> --- a/drivers/i2c/busses/i2c-bcm2835.c >>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c >>>> @@ -402,7 +402,7 @@ static const struct i2c_adapter_quirks bcm2835_i2c_quirks = { >>>> static int bcm2835_i2c_probe(struct platform_device *pdev) >>>> { >>>> struct bcm2835_i2c_dev *i2c_dev; >>>> - struct resource *mem, *irq; >>>> + struct resource *mem; >>>> int ret; >>>> struct i2c_adapter *adap; >>>> struct clk *mclk; >>>> @@ -452,12 +452,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) >>>> return ret; >>>> } >>>> >>>> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >>>> - if (!irq) { >>>> - dev_err(&pdev->dev, "No IRQ resource\n"); >>>> - return -ENODEV; >>>> - } >>>> - i2c_dev->irq = irq->start; >>>> + i2c_dev->irq = platform_get_irq(pdev, 0); >>>> + if (i2c_dev->irq <= 0) >>>> + return i2c_dev->irq ? i2c_dev->irq : -ENXIO; >>> >>> Why not just check for a negative return code and propagate it as is? >>> >> platform_get_irq() may return 0 said that we do get a splat in this >> case and further request_irq() will fail so instead check it here. >> > My bad, just the negative check should suffice. Think so too, looking forward to v2, thanks!
> + if (ret <= 0) > + return ret ? ret : -ENXIO; Same comment as patch 1. Otherwise, thanks for doing this cleanup!
Hi Prabhakar, On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static > allocation of IRQ resources in DT core code, this causes an issue > when using hierarchical interrupt domains using "interrupts" property > in the node as this bypasses the hierarchical setup and messes up the > irq chaining. > > In preparation for removal of static setup of IRQ resource from DT core > code use platform_get_irq(). > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -433,12 +433,12 @@ static int riic_i2c_probe(struct platform_device *pdev) > } > > for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) { > - res = platform_get_resource(pdev, IORESOURCE_IRQ, riic_irqs[i].res_num); > - if (!res) > - return -ENODEV; > + ret = platform_get_irq(pdev, riic_irqs[i].res_num); > + if (ret <= 0) This can be "ret < 0". > + return ret ? ret : -ENXIO; > > - ret = devm_request_irq(&pdev->dev, res->start, riic_irqs[i].isr, > - 0, riic_irqs[i].name, riic); > + ret = devm_request_irq(&pdev->dev, ret, riic_irqs[i].isr, > + 0, riic_irqs[i].name, riic); > if (ret) { > dev_err(&pdev->dev, "failed to request irq %s\n", riic_irqs[i].name); > return ret; With the above fixed: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds