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

[Sergey Shtylyov <s.shtylyov@omp.ru>]

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".

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

> settings are moved in the open functions. This is a preparatory change to

   I don't see you moving anything into ravb_open() here...

> 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..

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

> +		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...

[...]

MBR, Sergey