Received-SPF: pass (google.com: domain of linux-kernel+bounces-2238-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Sergey Shtylyov <s.shtylyov@omp.ru>
Subject: Re: [PATCH net-next v2 08/21] net: ravb: Move the IRQs get and
 request in the probe function
To: Claudiu <claudiu.beznea@tuxon.dev>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<richardcochran@gmail.com>, <p.zabel@pengutronix.de>,
	<yoshihiro.shimoda.uh@renesas.com>, <wsa+renesas@sang-engineering.com>,
	<geert+renesas@glider.be>
CC: <netdev@vger.kernel.org>, <linux-renesas-soc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Claudiu Beznea
	<claudiu.beznea.uj@bp.renesas.com>
References: <20231214114600.2451162-1-claudiu.beznea.uj@bp.renesas.com>
 <20231214114600.2451162-9-claudiu.beznea.uj@bp.renesas.com>
Organization: Open Mobile Platform
Message-ID: <b3c03bd5-83f2-331e-07c0-eeabca139224@omp.ru>
Date: Sat, 16 Dec 2023 18:53:35 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20231214114600.2451162-9-claudiu.beznea.uj@bp.renesas.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-KSE-Attachment-Filter-Triggered-Filters: Clean
X-KSE-BulkMessagesFiltering-Scan-Result: InTheLimit

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