[PATCH 0/3] i2c/busses: Use platform_get_irq/_optional() variants to fetch IRQ's

[PATCH 0/3] i2c/busses: Use platform_get_irq/_optional() variants to fetch IRQ's

[Lad Prabhakar]

Hi All,

This patch series aims to drop using platform_get_resource() for IRQ types
in preparation for removal of static setup of IRQ resource from DT core
code.

Dropping usage of platform_get_resource() was agreed based on
the discussion [0].

[0] https://patchwork.kernel.org/project/linux-renesas-soc/
patch/20211209001056.29774-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Note: I have just build tested the patches.

Cheers,
Prabhakar

Lad Prabhakar (3):
  i2c: bcm2835: Use platform_get_irq() to get the interrupt
  i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt
  i2c: riic: Use platform_get_irq() to get the interrupt

 drivers/i2c/busses/i2c-bcm2835.c   | 11 +++------
 drivers/i2c/busses/i2c-riic.c      | 10 ++++----
 drivers/i2c/busses/i2c-sh_mobile.c | 39 +++++++++++++++++++++++-------
 3 files changed, 39 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

[Lad Prabhakar]

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>
---
 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;
 
 	ret = request_irq(i2c_dev->irq, bcm2835_i2c_isr, IRQF_SHARED,
 			  dev_name(&pdev->dev), i2c_dev);
-- 
2.17.1

[PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Lad Prabhakar]

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_optional() for DT users only.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 39 +++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 7b8caf172851..d887f351f53c 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
 
 static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
 {
-	struct resource *res;
-	resource_size_t n;
+	struct device_node *np = dev_of_node(&dev->dev);
 	int k = 0, ret;
 
-	while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
-		for (n = res->start; n <= res->end; n++) {
-			ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
-					  0, dev_name(&dev->dev), pd);
+	if (!np) {
+		struct resource *res;
+		resource_size_t n;
+
+		while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
+			for (n = res->start; n <= res->end; n++) {
+				ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
+						       0, dev_name(&dev->dev), pd);
+				if (ret) {
+					dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
+					return ret;
+				}
+			}
+			k++;
+		}
+	} else {
+		int irq;
+
+		do {
+			irq = platform_get_irq_optional(dev, k);
+			if (irq <= 0 && irq != -ENXIO)
+				return irq ? irq : -ENXIO;
+			if (irq == -ENXIO)
+				break;
+			ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
+					       0, dev_name(&dev->dev), pd);
 			if (ret) {
-				dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
+				dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
 				return ret;
 			}
-		}
-		k++;
+			k++;
+		} while (irq);
 	}
 
 	return k > 0 ? 0 : -ENOENT;
-- 
2.17.1

[PATCH 3/3] i2c: riic: Use platform_get_irq() to get the interrupt

[Lad Prabhakar]

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>
---
 drivers/i2c/busses/i2c-riic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 78b84445ee6a..f18b9fe86d4b 100644
--- 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)
+			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;
-- 
2.17.1

Re: [PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

[Florian Fainelli]

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?
-- 
Florian

Re: [PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

[Lad, Prabhakar]

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

Re: [PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

[Lad, 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

Re: [PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

[Florian Fainelli]

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!
-- 
Florian

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

Hi Prabhakar,

> +	if (!np) {

Very minor nit: Maybe 'if (np)' and switch the blocks? Positive logic is
a tad easier to read.

> +		struct resource *res;
> +		resource_size_t n;
> +
> +		while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> +			for (n = res->start; n <= res->end; n++) {
> +				ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> +						       0, dev_name(&dev->dev), pd);
> +				if (ret) {
> +					dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> +					return ret;
> +				}
> +			}
> +			k++;
> +		}

Yeah, it is good to keep the legacy block as is.

> +		do {
> +			irq = platform_get_irq_optional(dev, k);
> +			if (irq <= 0 && irq != -ENXIO)
> +				return irq ? irq : -ENXIO;
> +			if (irq == -ENXIO)
> +				break;
> +			ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> +					       0, dev_name(&dev->dev), pd);
>  			if (ret) {
> -				dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> +				dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
>  				return ret;
>  			}
> -		}
> -		k++;
> +			k++;
> +		} while (irq);

In addition to the 'irq == 0' case from patch 1, I tried to shorten the
block for the np-case. I only came up with this. The assigntment and
comparison of the while-argument is not exactly pretty, but the block
itself is easier to read. I'll let you decide.

		while (irq = platform_get_irq_optional(dev, k) != -ENXIO) {
			if (irq < 0)
				return irq;

			ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
					       0, dev_name(&dev->dev), pd);
			if (ret) {
				dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
				return ret;
			}
			k++;
		}

Only brainstorming, not even build tested.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] i2c: riic: Use platform_get_irq() to get the interrupt

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 122 bytes --]


> +		if (ret <= 0)
> +			return ret ? ret : -ENXIO;

Same comment as patch 1. Otherwise, thanks for doing this cleanup!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

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.

Thanks for your patch!

> In preparation for removal of static setup of IRQ resource from DT core
> code use platform_get_irq_optional() for DT users only.

Why only for DT users?
Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
(non-DT) SoCs already uses platform_get_irq_optional(), so I expect
that to work for both.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
>
>  static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
>  {
> -       struct resource *res;
> -       resource_size_t n;
> +       struct device_node *np = dev_of_node(&dev->dev);
>         int k = 0, ret;
>
> -       while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> -               for (n = res->start; n <= res->end; n++) {
> -                       ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> -                                         0, dev_name(&dev->dev), pd);
> +       if (!np) {
> +               struct resource *res;
> +               resource_size_t n;
> +
> +               while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> +                       for (n = res->start; n <= res->end; n++) {
> +                               ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> +                                                      0, dev_name(&dev->dev), pd);
> +                               if (ret) {
> +                                       dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> +                                       return ret;
> +                               }
> +                       }
> +                       k++;
> +               }
> +       } else {
> +               int irq;
> +
> +               do {
> +                       irq = platform_get_irq_optional(dev, k);

Check for irq == -ENXIO first, to simplify the checks below?

> +                       if (irq <= 0 && irq != -ENXIO)
> +                               return irq ? irq : -ENXIO;

Can irq == 0 really happen?

All SuperH users of the "i2c-sh_mobile" platform device use an
evt2irq() value that is non-zero.

I might have missed something, but it seems the only user of IRQ 0 on
SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
These should have been seeing the "0 is an invalid IRQ number"
warning splat since it was introduced in commit a85a6c86c25be2d2
("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
the rare users may not have upgraded their kernels beyond v5.8 yet...

> +                       if (irq == -ENXIO)
> +                               break;
> +                       ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> +                                              0, dev_name(&dev->dev), pd);
>                         if (ret) {
> -                               dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> +                               dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
>                                 return ret;
>                         }
> -               }
> -               k++;
> +                       k++;
> +               } while (irq);
>         }
>
>         return k > 0 ? 0 : -ENOENT;

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

Re: [PATCH 3/3] i2c: riic: Use platform_get_irq() to get the interrupt

[Geert Uytterhoeven]

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

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 20.12.2021 13:17, Geert Uytterhoeven 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.
> 
> Thanks for your patch!
> 
>> In preparation for removal of static setup of IRQ resource from DT core
>> code use platform_get_irq_optional() for DT users only.
> 
> Why only for DT users?
> Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> that to work for both.
> 
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
>> @@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
>>
>>   static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
>>   {
>> -       struct resource *res;
>> -       resource_size_t n;
>> +       struct device_node *np = dev_of_node(&dev->dev);
>>          int k = 0, ret;
>>
>> -       while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
>> -               for (n = res->start; n <= res->end; n++) {
>> -                       ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
>> -                                         0, dev_name(&dev->dev), pd);
>> +       if (!np) {
>> +               struct resource *res;
>> +               resource_size_t n;
>> +
>> +               while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
>> +                       for (n = res->start; n <= res->end; n++) {
>> +                               ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
>> +                                                      0, dev_name(&dev->dev), pd);
>> +                               if (ret) {
>> +                                       dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
>> +                                       return ret;
>> +                               }
>> +                       }
>> +                       k++;
>> +               }
>> +       } else {
>> +               int irq;
>> +
>> +               do {
>> +                       irq = platform_get_irq_optional(dev, k);
> 
> Check for irq == -ENXIO first, to simplify the checks below?
> 
>> +                       if (irq <= 0 && irq != -ENXIO)
>> +                               return irq ? irq : -ENXIO;
> 
> Can irq == 0 really happen?

    Doesn't matter much in this case -- devm_request_irq() happily takes IRQ0. :-)

> All SuperH users of the "i2c-sh_mobile" platform device use an
> evt2irq() value that is non-zero.
> 
> I might have missed something, but it seems the only user of IRQ 0 on
> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> These should have been seeing the "0 is an invalid IRQ number"
> warning splat since it was introduced in commit a85a6c86c25be2d2
> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:

    Warning or no warning, 0 is still returned. :-/
    My attempt to put an end to this has stuck waiting a review from the IRQ 
people...

> the rare users may not have upgraded their kernels beyond v5.8 yet...
> 
>> +                       if (irq == -ENXIO)
>> +                               break;
>> +                       ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
>> +                                              0, dev_name(&dev->dev), pd);
>>                          if (ret) {
>> -                               dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
>> +                               dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
>>                                  return ret;
>>                          }
>> -               }
>> -               k++;
>> +                       k++;
>> +               } while (irq);
>>          }
>>
>>          return k > 0 ? 0 : -ENOENT;
> 
> 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

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> 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.
>
> Thanks for your patch!
>
> > In preparation for removal of static setup of IRQ resource from DT core
> > code use platform_get_irq_optional() for DT users only.
>
> Why only for DT users?
> Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> that to work for both.
>
For the non DT users the IRQ resource is passed as a range [0] and not
a single interrupt so I went with this approach. Is there a way I'm
missing where we could still use platform_get_irq_xyz() variants for
such cases?

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
> >
> >  static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
> >  {
> > -       struct resource *res;
> > -       resource_size_t n;
> > +       struct device_node *np = dev_of_node(&dev->dev);
> >         int k = 0, ret;
> >
> > -       while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > -               for (n = res->start; n <= res->end; n++) {
> > -                       ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > -                                         0, dev_name(&dev->dev), pd);
> > +       if (!np) {
> > +               struct resource *res;
> > +               resource_size_t n;
> > +
> > +               while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > +                       for (n = res->start; n <= res->end; n++) {
> > +                               ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > +                                                      0, dev_name(&dev->dev), pd);
> > +                               if (ret) {
> > +                                       dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                                       return ret;
> > +                               }
> > +                       }
> > +                       k++;
> > +               }
> > +       } else {
> > +               int irq;
> > +
> > +               do {
> > +                       irq = platform_get_irq_optional(dev, k);
>
> Check for irq == -ENXIO first, to simplify the checks below?
>
OK.

> > +                       if (irq <= 0 && irq != -ENXIO)
> > +                               return irq ? irq : -ENXIO;
>
> Can irq == 0 really happen?
>
> All SuperH users of the "i2c-sh_mobile" platform device use an
> evt2irq() value that is non-zero.
>
> I might have missed something, but it seems the only user of IRQ 0 on
> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
>
I'll keep that in mind if the Ethernet driver falls in the convection
patch changes.

> These should have been seeing the "0 is an invalid IRQ number"
> warning splat since it was introduced in commit a85a6c86c25be2d2
> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
> the rare users may not have upgraded their kernels beyond v5.8 yet...
>
Might be users have not updated their kernels.

[0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454

Cheers,
Prabhakar
> > +                       if (irq == -ENXIO)
> > +                               break;
> > +                       ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> > +                                              0, dev_name(&dev->dev), pd);
> >                         if (ret) {
> > -                               dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                               dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
> >                                 return ret;
> >                         }
> > -               }
> > -               k++;
> > +                       k++;
> > +               } while (irq);
> >         }
> >
> >         return k > 0 ? 0 : -ENOENT;
>
> 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

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Wolfram,

Thank you for the review.

On Mon, Dec 20, 2021 at 10:16 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Prabhakar,
>
> > +     if (!np) {
>
> Very minor nit: Maybe 'if (np)' and switch the blocks? Positive logic is
> a tad easier to read.
>
OK will update it for v2.

> > +             struct resource *res;
> > +             resource_size_t n;
> > +
> > +             while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > +                     for (n = res->start; n <= res->end; n++) {
> > +                             ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > +                                                    0, dev_name(&dev->dev), pd);
> > +                             if (ret) {
> > +                                     dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                                     return ret;
> > +                             }
> > +                     }
> > +                     k++;
> > +             }
>
> Yeah, it is good to keep the legacy block as is.
>
> > +             do {
> > +                     irq = platform_get_irq_optional(dev, k);
> > +                     if (irq <= 0 && irq != -ENXIO)
> > +                             return irq ? irq : -ENXIO;
> > +                     if (irq == -ENXIO)
> > +                             break;
> > +                     ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> > +                                            0, dev_name(&dev->dev), pd);
> >                       if (ret) {
> > -                             dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                             dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
> >                               return ret;
> >                       }
> > -             }
> > -             k++;
> > +                     k++;
> > +             } while (irq);
>
> In addition to the 'irq == 0' case from patch 1, I tried to shorten the
> block for the np-case. I only came up with this. The assigntment and
> comparison of the while-argument is not exactly pretty, but the block
> itself is easier to read. I'll let you decide.
>
>                 while (irq = platform_get_irq_optional(dev, k) != -ENXIO) {
>                         if (irq < 0)
>                                 return irq;
>
>                         ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
>                                                0, dev_name(&dev->dev), pd);
>                         if (ret) {
>                                 dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
>                                 return ret;
>                         }
>                         k++;
>                 }
>
> Only brainstorming, not even build tested.
>
LGTM, I'll give that a shot.

Cheers,
Prabhakar

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Dec 20, 2021 at 12:56 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > 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.
> >
> > Thanks for your patch!
> >
> > > In preparation for removal of static setup of IRQ resource from DT core
> > > code use platform_get_irq_optional() for DT users only.
> >
> > Why only for DT users?
> > Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> > (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> > that to work for both.
> >
> For the non DT users the IRQ resource is passed as a range [0] and not
> a single interrupt so I went with this approach. Is there a way I'm
> missing where we could still use platform_get_irq_xyz() variants for
> such cases?

Oh, I didn't realize it used a single resource with a range.
Is this common, i.e. would it make sense to add support for this to
platform_get_irq_optional()?

> > > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > +++ b/drivers/i2c/busses/i2c-sh_mobile.c

> > > +                       if (irq <= 0 && irq != -ENXIO)
> > > +                               return irq ? irq : -ENXIO;
> >
> > Can irq == 0 really happen?
> >
> > All SuperH users of the "i2c-sh_mobile" platform device use an
> > evt2irq() value that is non-zero.
> >
> > I might have missed something, but it seems the only user of IRQ 0 on
> > SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> > arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> >
> I'll keep that in mind if the Ethernet driver falls in the convection
> patch changes.

The Ethernet driver was converted 6 years ago, cfr. commit
965b2aa78fbcb831 ("net/smsc911x: fix irq resource allocation failure").

> [0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454

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

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Geert,

On Mon, Dec 20, 2021 at 12:54 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Dec 20, 2021 at 12:56 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > 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.
> > >
> > > Thanks for your patch!
> > >
> > > > In preparation for removal of static setup of IRQ resource from DT core
> > > > code use platform_get_irq_optional() for DT users only.
> > >
> > > Why only for DT users?
> > > Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> > > (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> > > that to work for both.
> > >
> > For the non DT users the IRQ resource is passed as a range [0] and not
> > a single interrupt so I went with this approach. Is there a way I'm
> > missing where we could still use platform_get_irq_xyz() variants for
> > such cases?
>
> Oh, I didn't realize it used a single resource with a range.
> Is this common, i.e. would it make sense to add support for this to
> platform_get_irq_optional()?
>
No this isn't common even non dt users should ideally be passing a
single IRQ resource. There are very few such platforms which do this
so I don't see any point in adding this support to
platform_get_irq_optional() unless the IRQ maintainers think otherwise.

> > > > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
>
> > > > +                       if (irq <= 0 && irq != -ENXIO)
> > > > +                               return irq ? irq : -ENXIO;
> > >
> > > Can irq == 0 really happen?
> > >
> > > All SuperH users of the "i2c-sh_mobile" platform device use an
> > > evt2irq() value that is non-zero.
> > >
> > > I might have missed something, but it seems the only user of IRQ 0 on
> > > SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> > > arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> > >
> > I'll keep that in mind if the Ethernet driver falls in the convection
> > patch changes.
>
> The Ethernet driver was converted 6 years ago, cfr. commit
> 965b2aa78fbcb831 ("net/smsc911x: fix irq resource allocation failure").
>
Thanks for the pointer.

Cheers,
Prabhakar

> > [0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454
>
> 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

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Mon, Dec 20, 2021 at 12:53 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 20.12.2021 13:17, Geert Uytterhoeven wrote:
>
> > I might have missed something, but it seems the only user of IRQ 0 on
> > SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> > arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> > These should have been seeing the "0 is an invalid IRQ number"
> > warning splat since it was introduced in commit a85a6c86c25be2d2
> > ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
>
>     Warning or no warning, 0 is still returned. :-/
>     My attempt to put an end to this has stuck waiting a review from the IRQ
> people...

I had another look at this after you asked about it on IRC. I don't
know much SH assembly, but I suspect IRQ 0 has not been delivered
since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
related note, CONFIG_INTC_BALANCING was broken in 2be6bb0c79c7
("sh: intc: Split up the INTC code.") by inadvertently removing the Kconfig
symbol.

        Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/8/22 3:31 PM, Arnd Bergmann wrote:

[...]
>>> I might have missed something, but it seems the only user of IRQ 0 on
>>> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
>>> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
>>> These should have been seeing the "0 is an invalid IRQ number"
>>> warning splat since it was introduced in commit a85a6c86c25be2d2
>>> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
>>
>>     Warning or no warning, 0 is still returned. :-/
>>     My attempt to put an end to this has stuck waiting a review from the IRQ
>> people...
> 
> I had another look at this after you asked about it on IRC. I don't
> know much SH assembly, but I suspect IRQ 0 has not been delivered
> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a

   Mhm... this commit changes the SH3 code while SH778x are SH4A, no?

[...]

> 
>         Arnd

MBR, Sergey

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 4:11 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
>
> On 2/8/22 3:31 PM, Arnd Bergmann wrote:
>
> [...]
> >>> I might have missed something, but it seems the only user of IRQ 0 on
> >>> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> >>> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> >>> These should have been seeing the "0 is an invalid IRQ number"
> >>> warning splat since it was introduced in commit a85a6c86c25be2d2
> >>> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
> >>
> >>     Warning or no warning, 0 is still returned. :-/
> >>     My attempt to put an end to this has stuck waiting a review from the IRQ
> >> people...
> >
> > I had another look at this after you asked about it on IRC. I don't
> > know much SH assembly, but I suspect IRQ 0 has not been delivered
> > since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
>
>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?

This code is shared between both:

arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
../sh3/, entry.o ex.o)

       Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/9/22 6:18 PM, Arnd Bergmann wrote:
> On Wed, Feb 9, 2022 at 4:11 PM Sergei Shtylyov
> <sergei.shtylyov@gmail.com> wrote:
>>
>> On 2/8/22 3:31 PM, Arnd Bergmann wrote:
>>
>> [...]
>>>>> I might have missed something, but it seems the only user of IRQ 0 on
>>>>> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
>>>>> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
>>>>> These should have been seeing the "0 is an invalid IRQ number"
>>>>> warning splat since it was introduced in commit a85a6c86c25be2d2
>>>>> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
>>>>
>>>>     Warning or no warning, 0 is still returned. :-/
>>>>     My attempt to put an end to this has stuck waiting a review from the IRQ
>>>> people...
>>>
>>> I had another look at this after you asked about it on IRC. I don't
>>> know much SH assembly, but I suspect IRQ 0 has not been delivered

   Neither do I, sigh...
   I do know the instuctions are 16-bit and so there are no immediate
opperands... :-)

>>> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
>>
>>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?
> 
> This code is shared between both:
> 
> arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
> ../sh3/, entry.o ex.o)

   Ah, quite convoluted! :-)
   So you mean thet broke the delivery of EVT 0x200 when mucking with NMI?

>        Arnd

MBR, Sergey

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 4:48 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 2/9/22 6:18 PM, Arnd Bergmann wrote:
> >>> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
> >>
> >>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?
> >
> > This code is shared between both:
> >
> > arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
> > ../sh3/, entry.o ex.o)
>
>    Ah, quite convoluted! :-)
>    So you mean thet broke the delivery of EVT 0x200 when mucking with NMI?

Yes, exactly: If I read this right, the added code:

+       shlr2   r4
+       shlr    r4
+       mov     r4, r0          ! save vector->jmp table offset for later
+
+       shlr2   r4              ! vector to IRQ# conversion
+       add     #-0x10, r4
+
+       cmp/pz  r4              ! is it a valid IRQ?
+       bt      10f

gets the vector (0x200 for this device), shifts it five bits to 0x10,
and subtracts 0x10,
then branches to do_IRQ if the interrupt number is non-zero, otherwise it goes
through the exception_handling_table.

         Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/9/22 7:02 PM, Arnd Bergmann wrote:

>>>>> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
>>>>
>>>>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?
>>>
>>> This code is shared between both:
>>>
>>> arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
>>> ../sh3/, entry.o ex.o)
>>
>>    Ah, quite convoluted! :-)
>>    So you mean thet broke the delivery of EVT 0x200 when mucking with NMI?
> 
> Yes, exactly: If I read this right, the added code:
> 
> +       shlr2   r4
> +       shlr    r4
> +       mov     r4, r0          ! save vector->jmp table offset for later
> +
> +       shlr2   r4              ! vector to IRQ# conversion
> +       add     #-0x10, r4
> +
> +       cmp/pz  r4              ! is it a valid IRQ?
> +       bt      10f
> 
> gets the vector (0x200 for this device), shifts it five bits to 0x10,
> and subtracts 0x10,
> then branches to do_IRQ if the interrupt number is non-zero, otherwise it goes
> through the exception_handling_table.

   The SH4 manual I found on my disk (have it from MontaVista times) tells me cmp/pz
sets T if Rn is >= 0, then bt branches if T = 1. So I do think the code is correct.
   One more thing: the board code for those boards was added in 2011, we can assume
it was working back then, right? :-_

>          Arnd

MBR, Sergey

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 5:08 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 2/9/22 7:02 PM, Arnd Bergmann wrote:
> >
> > +       shlr2   r4
> > +       shlr    r4
> > +       mov     r4, r0          ! save vector->jmp table offset for later
> > +
> > +       shlr2   r4              ! vector to IRQ# conversion
> > +       add     #-0x10, r4
> > +
> > +       cmp/pz  r4              ! is it a valid IRQ?
> > +       bt      10f
> >
> > gets the vector (0x200 for this device), shifts it five bits to 0x10,
> > and subtracts 0x10,
> > then branches to do_IRQ if the interrupt number is non-zero, otherwise it goes
> > through the exception_handling_table.
>
>    The SH4 manual I found on my disk (have it from MontaVista times) tells me cmp/pz
> sets T if Rn is >= 0, then bt branches if T = 1. So I do think the code is correct.
>    One more thing: the board code for those boards was added in 2011, we can assume
> it was working back then, right? :-_

Indeed, this does make more sense, I had not realized that the numbers could get
negative here.

         Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Sergei,

On Wed, Feb 9, 2022 at 5:08 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
>    One more thing: the board code for those boards was added in 2011, we can assume
> it was working back then, right? :-_

This assumption may not be true: there is plenty of driver/board
support that was only upstreamed partially.

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

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Sergei,

On Wed, Feb 9, 2022 at 4:48 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 2/9/22 6:18 PM, Arnd Bergmann wrote:
> >>> I had another look at this after you asked about it on IRC. I don't
> >>> know much SH assembly, but I suspect IRQ 0 has not been delivered
>
>    Neither do I, sigh...
>    I do know the instuctions are 16-bit and so there are no immediate
> opperands... :-)

There is byte immediate data (TIL).

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

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/10/22 12:32 PM, Geert Uytterhoeven wrote:

[...]

>>>>> I had another look at this after you asked about it on IRC. I don't
>>>>> know much SH assembly, but I suspect IRQ 0 has not been delivered
>>
>>    Neither do I, sigh...
>>    I do know the instuctions are 16-bit and so there are no immediate
>> opperands... :-)
> 
> There is byte immediate data (TIL).

    Yeah, I figured. :-)

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergey