diff mbox series

[net-next,1/5] net: ravb: Get rid of the temporary variable irq

Message ID 20240207120733.1746920-2-claudiu.beznea.uj@bp.renesas.com
State New
Headers
Series net: ravb: Add runtime PM support (part 2) |

Commit Message

claudiu beznea Feb. 7, 2024, 12:07 p.m. UTC 
  From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The 4th argument of ravb_setup_irq() is used to save the IRQ number that
will be further used by the driver code. Not all ravb_setup_irqs() calls
need to save the IRQ number. The previous code used to pass a dummy
variable as the 4th argument in case the IRQ is not needed for further
usage. That is not necessary as the code from ravb_setup_irq() can detect
by itself if the IRQ needs to be saved. Thus, get rid of the code that is
not needed.

Reported-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes since [2]:
- this patch in new

[2] https://lore.kernel.org/all/20240105082339.1468817-1-claudiu.beznea.uj@bp.renesas.com/

 drivers/net/ethernet/renesas/ravb_main.c | 27 +++++++++++++-----------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comments

Sergey Shtylyov Feb. 8, 2024, 8:43 p.m. UTC | #1 
On 2/7/24 3:07 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The 4th argument of ravb_setup_irq() is used to save the IRQ number that
> will be further used by the driver code. Not all ravb_setup_irqs() calls
> need to save the IRQ number. The previous code used to pass a dummy
> variable as the 4th argument in case the IRQ is not needed for further
> usage. That is not necessary as the code from ravb_setup_irq() can detect
> by itself if the IRQ needs to be saved. Thus, get rid of the code that is
> not needed.
> 
> Reported-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9521cd054274..e235342e0827 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2611,17 +2611,20 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
>  		if (!dev_name)
>  			return -ENOMEM;
>  
> -		*irq = platform_get_irq_byname(pdev, irq_name);
> +		error = platform_get_irq_byname(pdev, irq_name);
>  		flags = 0;
>  	} else {
>  		dev_name = ndev->name;
> -		*irq = platform_get_irq(pdev, 0);
> +		error = platform_get_irq(pdev, 0);
>  		flags = IRQF_SHARED;
>  	}
> -	if (*irq < 0)
> -		return *irq;
> +	if (error < 0)
> +		return error;
>  
> -	error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
> +	if (irq)
> +		*irq = error;
> +
> +	error = devm_request_irq(dev, error, handler, flags, dev_name, ndev);
>  	if (error)
>  		netdev_err(ndev, "cannot request IRQ %s\n", dev_name);
>  

   Thanks for addressing my IRC comment! Tho the naming seems awful. :-)
   I'd suggest to add a local variable (named e.g, irq_num) and use it to
store the result of platform_get_irq[_byname]().

[...]

MBR, Sergey
Sergey Shtylyov Feb. 8, 2024, 8:44 p.m. UTC | #2 
On 2/7/24 3:07 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The 4th argument of ravb_setup_irq() is used to save the IRQ number that
> will be further used by the driver code. Not all ravb_setup_irqs() calls
> need to save the IRQ number. The previous code used to pass a dummy
> variable as the 4th argument in case the IRQ is not needed for further
> usage. That is not necessary as the code from ravb_setup_irq() can detect
> by itself if the IRQ needs to be saved. Thus, get rid of the code that is
> not needed.
> 
> Reported-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9521cd054274..e235342e0827 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2611,17 +2611,20 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
>  		if (!dev_name)
>  			return -ENOMEM;
>  
> -		*irq = platform_get_irq_byname(pdev, irq_name);
> +		error = platform_get_irq_byname(pdev, irq_name);
>  		flags = 0;
>  	} else {
>  		dev_name = ndev->name;
> -		*irq = platform_get_irq(pdev, 0);
> +		error = platform_get_irq(pdev, 0);
>  		flags = IRQF_SHARED;
>  	}
> -	if (*irq < 0)
> -		return *irq;
> +	if (error < 0)
> +		return error;
>  
> -	error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
> +	if (irq)
> +		*irq = error;
> +
> +	error = devm_request_irq(dev, error, handler, flags, dev_name, ndev);
>  	if (error)
>  		netdev_err(ndev, "cannot request IRQ %s\n", dev_name);
>  

   Thanks for addressing my IRC comment! Tho the naming seems awful. :-)
   I'd suggest to add a local variable (named e.g. irq_num) and use it to
store the result of platform_get_irq[_byname]().

[...]

MBR, Sergey
claudiu beznea Feb. 9, 2024, 5:48 a.m. UTC | #3 
On 08.02.2024 22:43, Sergey Shtylyov wrote:
> On 2/7/24 3:07 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The 4th argument of ravb_setup_irq() is used to save the IRQ number that
>> will be further used by the driver code. Not all ravb_setup_irqs() calls
>> need to save the IRQ number. The previous code used to pass a dummy
>> variable as the 4th argument in case the IRQ is not needed for further
>> usage. That is not necessary as the code from ravb_setup_irq() can detect
>> by itself if the IRQ needs to be saved. Thus, get rid of the code that is
>> not needed.
>>
>> Reported-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 9521cd054274..e235342e0827 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2611,17 +2611,20 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
>>  		if (!dev_name)
>>  			return -ENOMEM;
>>  
>> -		*irq = platform_get_irq_byname(pdev, irq_name);
>> +		error = platform_get_irq_byname(pdev, irq_name);
>>  		flags = 0;
>>  	} else {
>>  		dev_name = ndev->name;
>> -		*irq = platform_get_irq(pdev, 0);
>> +		error = platform_get_irq(pdev, 0);
>>  		flags = IRQF_SHARED;
>>  	}
>> -	if (*irq < 0)
>> -		return *irq;
>> +	if (error < 0)
>> +		return error;
>>  
>> -	error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
>> +	if (irq)
>> +		*irq = error;
>> +
>> +	error = devm_request_irq(dev, error, handler, flags, dev_name, ndev);
>>  	if (error)
>>  		netdev_err(ndev, "cannot request IRQ %s\n", dev_name);
>>  
> 
>    Thanks for addressing my IRC comment! Tho the naming seems awful. :-)
>    I'd suggest to add a local variable (named e.g, irq_num) and use it to

I tried to avoid that...

> store the result of platform_get_irq[_byname]().
> 
> [...]
> 
> MBR, Sergey
Sergey Shtylyov Feb. 9, 2024, 7:43 p.m. UTC | #4 
On 2/9/24 8:48 AM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> The 4th argument of ravb_setup_irq() is used to save the IRQ number that
>>> will be further used by the driver code. Not all ravb_setup_irqs() calls
>>> need to save the IRQ number. The previous code used to pass a dummy
>>> variable as the 4th argument in case the IRQ is not needed for further
>>> usage. That is not necessary as the code from ravb_setup_irq() can detect
>>> by itself if the IRQ needs to be saved. Thus, get rid of the code that is
>>> not needed.
>>>
>>> Reported-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 9521cd054274..e235342e0827 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2611,17 +2611,20 @@ static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
>>>  		if (!dev_name)
>>>  			return -ENOMEM;
>>>  
>>> -		*irq = platform_get_irq_byname(pdev, irq_name);
>>> +		error = platform_get_irq_byname(pdev, irq_name);
>>>  		flags = 0;
>>>  	} else {
>>>  		dev_name = ndev->name;
>>> -		*irq = platform_get_irq(pdev, 0);
>>> +		error = platform_get_irq(pdev, 0);
>>>  		flags = IRQF_SHARED;
>>>  	}
>>> -	if (*irq < 0)
>>> -		return *irq;
>>> +	if (error < 0)
>>> +		return error;
>>>  
>>> -	error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
>>> +	if (irq)
>>> +		*irq = error;
>>> +
>>> +	error = devm_request_irq(dev, error, handler, flags, dev_name, ndev);
>>>  	if (error)
>>>  		netdev_err(ndev, "cannot request IRQ %s\n", dev_name);
>>>  
>>
>>    Thanks for addressing my IRC comment! Tho the naming seems awful. :-)

	if (error < 0)
		return error;
 
	if (irq)
		*irq = error;

	error = devm_request_irq(dev, error, handler, flags, dev_name, ndev);

   These just don't look right...

>>    I'd suggest to add a local variable (named e.g, irq_num) and use it to
> 
> I tried to avoid that...

   Why? :-)

>> store the result of platform_get_irq[_byname]().
>>
>> [...]

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9521cd054274..e235342e0827 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2611,17 +2611,20 @@  static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
 		if (!dev_name)
 			return -ENOMEM;
 
-		*irq = platform_get_irq_byname(pdev, irq_name);
+		error = platform_get_irq_byname(pdev, irq_name);
 		flags = 0;
 	} else {
 		dev_name = ndev->name;
-		*irq = platform_get_irq(pdev, 0);
+		error = platform_get_irq(pdev, 0);
 		flags = IRQF_SHARED;
 	}
-	if (*irq < 0)
-		return *irq;
+	if (error < 0)
+		return error;
 
-	error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
+	if (irq)
+		*irq = error;
+
+	error = devm_request_irq(dev, error, handler, flags, dev_name, ndev);
 	if (error)
 		netdev_err(ndev, "cannot request IRQ %s\n", dev_name);
 
@@ -2633,7 +2636,7 @@  static int ravb_setup_irqs(struct ravb_private *priv)
 	const struct ravb_hw_info *info = priv->info;
 	struct net_device *ndev = priv->ndev;
 	const char *irq_name, *emac_irq_name;
-	int error, irq;
+	int error;
 
 	if (!info->multi_irqs)
 		return ravb_setup_irq(priv, NULL, NULL, &ndev->irq, ravb_interrupt);
@@ -2656,28 +2659,28 @@  static int ravb_setup_irqs(struct ravb_private *priv)
 		return error;
 
 	if (info->err_mgmt_irqs) {
-		error = ravb_setup_irq(priv, "err_a", "err_a", &irq, ravb_multi_interrupt);
+		error = ravb_setup_irq(priv, "err_a", "err_a", NULL, ravb_multi_interrupt);
 		if (error)
 			return error;
 
-		error = ravb_setup_irq(priv, "mgmt_a", "mgmt_a", &irq, ravb_multi_interrupt);
+		error = ravb_setup_irq(priv, "mgmt_a", "mgmt_a", NULL, ravb_multi_interrupt);
 		if (error)
 			return error;
 	}
 
-	error = ravb_setup_irq(priv, "ch0", "ch0:rx_be", &irq, ravb_be_interrupt);
+	error = ravb_setup_irq(priv, "ch0", "ch0:rx_be", NULL, ravb_be_interrupt);
 	if (error)
 		return error;
 
-	error = ravb_setup_irq(priv, "ch1", "ch1:rx_nc", &irq, ravb_nc_interrupt);
+	error = ravb_setup_irq(priv, "ch1", "ch1:rx_nc", NULL, ravb_nc_interrupt);
 	if (error)
 		return error;
 
-	error = ravb_setup_irq(priv, "ch18", "ch18:tx_be", &irq, ravb_be_interrupt);
+	error = ravb_setup_irq(priv, "ch18", "ch18:tx_be", NULL, ravb_be_interrupt);
 	if (error)
 		return error;
 
-	return ravb_setup_irq(priv, "ch19", "ch19:tx_nc", &irq, ravb_nc_interrupt);
+	return ravb_setup_irq(priv, "ch19", "ch19:tx_nc", NULL, ravb_nc_interrupt);
 }
 
 static int ravb_probe(struct platform_device *pdev)