Re: [PATCH 5.10.y-cip 15/21] can: rcar_canfd: Add support for RZ/G2L family

[Pavel Machek <pavel@denx.de>]

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

Hi!

> commit 76e9353a80e9e9ff65b362a6dd8545d84427ec30 upstream.
> 
> CANFD block on RZ/G2L SoC is almost identical to one found on
> R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> are split into different sources and the IP doesn't divide (1/2)
> CANFD clock within the IP.
> 
> This patch adds compatible string for RZ/G2L family and splits
> the irq handlers to accommodate both RZ/G2L and R-Car Gen3 SoC's.


> +++ b/drivers/net/can/rcar/rcar_canfd.c
> +
> +static irqreturn_t rcar_canfd_global_err_interrupt(int irq, void *dev_id)
> +{
> +	struct rcar_canfd_global *gpriv = dev_id;
> +	u32 ch;
> +
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
> +		rcar_canfd_handle_global_err(gpriv, ch);
> +
> +	return IRQ_HANDLED;
> +}
> +

You are returning IRQ_HANDLED even when you do no handling. That
probably will not do harm as long as interrupts are not shared. Is
that guaranteed to be true?

> @@ -1577,6 +1653,53 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
>  	priv->can.clock.freq = fcan_freq;
>  	dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
>  
> +	if (gpriv->chip_id == RENESAS_RZG2L) {
> +		char *irq_name;
> +		int err_irq;
> +		int tx_irq;
> +
> +		err_irq = platform_get_irq_byname(pdev, ch == 0 ? "ch0_err" : "ch1_err");
> +		if (err_irq < 0) {
> +			err = err_irq;
> +			goto fail;
> +		}

This leaks ndev; it should free_candev(ndev); before returning, AFAICT.

> @@ -1670,6 +1811,19 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	gpriv->pdev = pdev;
>  	gpriv->channels_mask = channels_mask;
>  	gpriv->fdmode = fdmode;
> +	gpriv->chip_id = chip_id;
> +
> +	if (gpriv->chip_id == RENESAS_RZG2L) {
> +		gpriv->rstc1 = devm_reset_control_get_exclusive(&pdev->dev, "rstp_n");
> +		if (IS_ERR(gpriv->rstc1))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(gpriv->rstc1),
> +					     "failed to get rstp_n\n");
> +
> +		gpriv->rstc2 = devm_reset_control_get_exclusive(&pdev->dev, "rstc_n");
> +		if (IS_ERR(gpriv->rstc2))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(gpriv->rstc2),
> +					     "failed to get rstc_n\n");
> +	}
>  
>  	/* Peripheral clock */
>  	gpriv->clkp = devm_clk_get(&pdev->dev, "fck");

The code is correct but it mixes direct returns with "goto
fail_dev", which is just alias for return. I'd prefer just doing
returns directly, but it really should not mix direct returns with
goto return.

> -	err = devm_request_irq(&pdev->dev, g_irq,
> -			       rcar_canfd_global_interrupt, 0,
> -			       "canfd.gbl", gpriv);

Strange. New code variant no longer allocates canfd.gbl?

> +	err = reset_control_reset(gpriv->rstc1);
> +	if (err)
> +		goto fail_dev;
> +	err = reset_control_reset(gpriv->rstc2);
>  	if (err) {
> -		dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> -			g_irq, err);
> +		reset_control_assert(gpriv->rstc1);
>  		goto fail_dev;
>  	}

I'd introduce new label here, so that you don't need to do
reset_control_assert(gpriv->rstc1); at two different places... like
below...

Best regards,

								Pavel

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 52629c6c19702..ce9b3a0032bf8 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1908,10 +1908,8 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	if (err)
 		goto fail_dev;
 	err = reset_control_reset(gpriv->rstc2);
-	if (err) {
-		reset_control_assert(gpriv->rstc1);
-		goto fail_dev;
-	}
+	if (err)
+		goto fail_reset1;
 
 	/* Enable peripheral clock for register access */
 	err = clk_prepare_enable(gpriv->clkp);
@@ -1976,8 +1974,9 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 fail_clk:
 	clk_disable_unprepare(gpriv->clkp);
 fail_reset:
-	reset_control_assert(gpriv->rstc1);
 	reset_control_assert(gpriv->rstc2);
+fail_reset1:
+	reset_control_assert(gpriv->rstc1);
 fail_dev:
 	return err;
 }




-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]