Re: [PATCH 5.10.y-cip 15/21] can: rcar_canfd: Add support for RZ/G2L family
From: Pavel Machek <hidden>
Date: 2021-12-22 23:13:55
Subsystem:
can network drivers, the rest · Maintainers:
Marc Kleine-Budde, Vincent Mailhol, Linus Torvalds
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.
quoted hunk ↗ jump to hunk
+++ 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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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, GermanyAttachments
- signature.asc [application/pgp-signature] 181 bytes