| Message ID | 20250326122003.122976-17-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add support for RZ/G3E CANFD | expand |
On 26/03/2025 at 21:19, Biju Das wrote: > All existing SoCs support an external clock, but RZ/G3E has only internal > clocks. Add only_internal_clks to struct rcar_canfd_hw_info to handle this > difference. > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> A few nits but: Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > v6->v7: > * No change. > v5->v6: > * No change. > v4->v5: > * Collected tag. > * Improved commit description by "All SoCs supports extenal clock"-> > "All existing SoCs support an external clock". > v3->v4: > * No change. > v2->v3: > * No change > v1->v2: > * No change. > --- > drivers/net/can/rcar/rcar_canfd.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c > index 20e591421cc6..7ad27087a176 100644 > --- a/drivers/net/can/rcar/rcar_canfd.c > +++ b/drivers/net/can/rcar/rcar_canfd.c > @@ -546,6 +546,7 @@ struct rcar_canfd_hw_info { > unsigned multi_channel_irqs:1; /* Has multiple channel irqs */ > unsigned ch_interface_mode:1; /* Has channel interface mode */ > unsigned shared_can_regs:1; /* Has shared classical can registers */ > + unsigned only_internal_clks:1; /* Has only internal clocks */ > }; By default the fields will be set to false if omitted. I think it is a bit more readable if you still set them explicitly to zero. > /* Channel priv data */ > @@ -2045,7 +2046,7 @@ static int rcar_canfd_probe(struct platform_device *pdev) > fcan_freq = clk_get_rate(gpriv->can_clk) / info->postdiv; > } else { > fcan_freq = clk_get_rate(gpriv->can_clk); > - gpriv->extclk = true; > + gpriv->extclk = !gpriv->info->only_internal_clks; Nitpick: if at the end, what matters in the extclk boolean, you may directly name your field: rcar_canfd_hw_info->has_external_clks and no need for the negation above. > } > > addr = devm_platform_ioremap_resource(pdev, 0); Yours sincerely, Vincent Mailhol
Hi Vincent, > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > Sent: 28 March 2025 15:27 > Subject: Re: [PATCH v7 16/18] can: rcar_canfd: Add only_internal_clks variable to struct > rcar_canfd_hw_info > > On 26/03/2025 at 21:19, Biju Das wrote: > > All existing SoCs support an external clock, but RZ/G3E has only > > internal clocks. Add only_internal_clks to struct rcar_canfd_hw_info > > to handle this difference. > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > A few nits but: > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > > --- > > v6->v7: > > * No change. > > v5->v6: > > * No change. > > v4->v5: > > * Collected tag. > > * Improved commit description by "All SoCs supports extenal clock"-> > > "All existing SoCs support an external clock". > > v3->v4: > > * No change. > > v2->v3: > > * No change > > v1->v2: > > * No change. > > --- > > drivers/net/can/rcar/rcar_canfd.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/rcar/rcar_canfd.c > > b/drivers/net/can/rcar/rcar_canfd.c > > index 20e591421cc6..7ad27087a176 100644 > > --- a/drivers/net/can/rcar/rcar_canfd.c > > +++ b/drivers/net/can/rcar/rcar_canfd.c > > @@ -546,6 +546,7 @@ struct rcar_canfd_hw_info { > > unsigned multi_channel_irqs:1; /* Has multiple channel irqs */ > > unsigned ch_interface_mode:1; /* Has channel interface mode */ > > unsigned shared_can_regs:1; /* Has shared classical can registers */ > > + unsigned only_internal_clks:1; /* Has only internal clocks */ > > }; > > By default the fields will be set to false if omitted. I think it is a bit more readable if you still > set them explicitly to zero. OK. Will update for similar patches. > > > /* Channel priv data */ > > @@ -2045,7 +2046,7 @@ static int rcar_canfd_probe(struct platform_device *pdev) > > fcan_freq = clk_get_rate(gpriv->can_clk) / info->postdiv; > > } else { > > fcan_freq = clk_get_rate(gpriv->can_clk); > > - gpriv->extclk = true; > > + gpriv->extclk = !gpriv->info->only_internal_clks; > > Nitpick: if at the end, what matters in the extclk boolean, you may directly name your field: > > rcar_canfd_hw_info->has_external_clks OK. will change the variable as external_clk for consistency with comment 'Has external clock' Cheers, Biju
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index 20e591421cc6..7ad27087a176 100644 --- a/drivers/net/can/rcar/rcar_canfd.c +++ b/drivers/net/can/rcar/rcar_canfd.c @@ -546,6 +546,7 @@ struct rcar_canfd_hw_info { unsigned multi_channel_irqs:1; /* Has multiple channel irqs */ unsigned ch_interface_mode:1; /* Has channel interface mode */ unsigned shared_can_regs:1; /* Has shared classical can registers */ + unsigned only_internal_clks:1; /* Has only internal clocks */ }; /* Channel priv data */ @@ -2045,7 +2046,7 @@ static int rcar_canfd_probe(struct platform_device *pdev) fcan_freq = clk_get_rate(gpriv->can_clk) / info->postdiv; } else { fcan_freq = clk_get_rate(gpriv->can_clk); - gpriv->extclk = true; + gpriv->extclk = !gpriv->info->only_internal_clks; } addr = devm_platform_ioremap_resource(pdev, 0);