Re: [PATCH net-next v2 08/21] net: ravb: Move the IRQs get and request in the probe function

[claudiu beznea <claudiu.beznea@tuxon.dev>]

On 16.12.2023 17:53, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Move the IRQs get and request in the driver's probe function. As some IP
>> variants switches to reset operation mode as a result of setting module
> 
>    s/switches/switch/.
>    Also, the manuals call this "operating mode", not to mix with one of
> the modes -- "operation mode".

ok

> 
>> standby through clock enable/disable APIs, to implement runtime PM the
>> resource parsing and requests are moved in the probe function and IP
> 
>    Requesting.
>    Could you explain in more detail why you need to do this?

Ok, I'll update it in the next version.

> 
>> settings are moved in the open functions. This is a preparatory change to
> 
>    I don't see you moving anything into ravb_open() here...

Indeed, this is the general explanation. I'll adapt it to explain it what
has been done in the commit (as it should have been).

> 
>> add runtime PM support for all IP variants.
>>
>> 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 83691a0f0cc2..d7f6e8ea8e79 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1731,7 +1731,7 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
>>  	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
> 
>    Ugh, I didn't realize we had the managed device API call in a function
> called from ravb_open()... :-/
> 
> [...]
>> @@ -2616,6 +2536,127 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
>>  	}
>>  }
>>  
>> +static int ravb_get_irqs(struct ravb_private *priv)
>> +{
>> +	const char *err_a_irq_name = NULL, *mgmt_a_irq_name = NULL;
> 
>    You don't seem to use these as the pointers. Could be bool instead?
> But even that doesn't seem necessary..

Indeed, I've messed it a bit. I'll update it in the next version.

> 
>> +	const struct ravb_hw_info *info = priv->info;
>> +	struct platform_device *pdev = priv->pdev;
>> +	struct net_device *ndev = priv->ndev;
>> +	const char *irq_name, *emac_irq_name;
>> +	int i, irq;
>> +
>> +	if (!info->multi_irqs) {
>> +		irq = platform_get_irq(pdev, 0);
>> +		if (irq < 0)
>> +			return irq;
>> +
>> +		ndev->irq = irq;
>> +		return 0;
>> +	}
>> +
>> +	if (info->err_mgmt_irqs) {
>> +		irq_name = "dia";
>> +		emac_irq_name = "line3";
>> +		err_a_irq_name = "err_a";
>> +		mgmt_a_irq_name = "mgmt_a";
>> +	} else {
>> +		irq_name = "ch22";
>> +		emac_irq_name = "ch24";
>> +	}
>> +
>> +	irq = platform_get_irq_byname(pdev, irq_name);
>> +	if (irq < 0)
>> +		return irq;
>> +	ndev->irq = irq;
>> +
>> +	irq = platform_get_irq_byname(pdev, emac_irq_name);
>> +	if (irq < 0)
>> +		return irq;
>> +	priv->emac_irq = irq;
>> +
>> +	if (err_a_irq_name) {
> 
>    Why not just ctest info->err_mgmt_irqs here, as it was before
> this patch?

I can't tell ATM what I've wanted to achieve here. Indeed, just checking
info->err_mgmt_irqs should be better.

> 
>> +		irq = platform_get_irq_byname(pdev, "err_a");
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->erra_irq = irq;
>> +	}
>> +
>> +	if (mgmt_a_irq_name) {
>> +		irq = platform_get_irq_byname(pdev, "mgmt_a");
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->mgmta_irq = irq;
>> +	}
>> +
>> +	for (i = 0; i < NUM_RX_QUEUE; i++) {
>> +		irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->rx_irqs[i] = irq;
>> +	}
>> +	for (i = 0; i < NUM_TX_QUEUE; i++) {
>> +		irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->tx_irqs[i] = irq;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ravb_request_irqs(struct ravb_private *priv)
> 
>    I'm not sure separating getting and requesting IRQs is a good idea.
> As you're switching to using the managed device API anyway, you could
> save on some IRQ-related fields in the *struct* ravb_private, I think...

I'll have a look. By keeping them separated I tried to have the code doing
the similar things grouped together, tried to keep code similar to what was
previously and tried to avoid huge functions (having parse and request in a
single function will lead, AFAICT, at a function with more lines of code
(difficult to browse in my opinion)).

Thank you for your review,
Claudiu Beznea

> 
> [...]
> 
> MBR, Sergey