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