Hi Geert,
> BTW, the diff looks much better with the --histogram option of
> git diff/show.
Thanks, I tend to forget this option.
> > + if (IS_ERR(clk))
> > + return clk;
>
> Missing "kfree(csn)".
Ouch, yes!
> > + return clk_register_divider_table(NULL, name, parent_name, 0, sdnckcr,
> > + 0, 2, 0, cpg_sd_div_table, &cpg_lock);
>
> So the SDn clock can no longer be disabled, as CPG_SD_STP_CK
> handling is gone?
Yes. I thought we can do it since we had 7f2c2f38c1c0 ("clk: renesas:
rcar-gen3: Remove stp_ck handling for SDHI") anyhow.
> > + if (ref_clk == priv->clkh)
>
> "if (priv->clkh)", for consistency with above?
Can do. I even had this originally. Then, I thought the comparison makes
it easier to understand. But it seems, it is understandable enough
without the comparison.
> > + /* Fallback for old DTs */
> > + if (of_device_is_compatible(pdev->dev.of_node, "renesas,rcar-gen3-sdhi"))
>
> I think it would be cleaner to check a flag in struct
> renesas_sdhi_of_data instead.
Because new SoCs with the fallback compatible might show up?
> > * Some controllers provide a 2nd clock just to run the internal card
> > * detection logic. Unfortunately, the existing driver architecture does
>
> The core looks good to me, but I have to admit I'm no expert on the
> SDHn/SDn clock relations and the various SDHI transfer modes.
I am really glad you like the changes in general. And you point to the
reason for this change. All the clock relations of the SDHI transfer
modes should go into the SDHI driver. Now, we can control SDnH and SDn
seperately, so the SDHI driver can do the proper things depending on the
mode and the quirks of the SDHI instance. I really think the clock
driver part should be as simple as it is with this series.
Thanks for the review, I will fix the other minor issues soon as well.
Happy hacking,
Wolfram
Attachment:
signature.asc
Description: PGP signature
