Hi Vincent,
> -----Original Message-----
> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> 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@xxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> A few nits but:
>
> Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
>
> > ---
> > 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
