Re: [PATCH v3 10/13] drm/bridge: samsung-dsim: add ability to define clock names for every variant

[Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx>]

On 2025-07-29 07:02, Inki Dae wrote:
> 2025년 7월 7일 (월) 오전 3:28, Kaustabh Chakraborty 
> <kauschluss@xxxxxxxxxxx>님이 작성:
> > -       dsi->clks = devm_kcalloc(dev, dsi->driver_data->num_clks,
> > -                                sizeof(*dsi->clks), GFP_KERNEL);
> > -       if (!dsi->clks)
> > -               return -ENOMEM;
> > -
> > -       for (i = 0; i < dsi->driver_data->num_clks; i++) {
> > -               dsi->clks[i] = devm_clk_get(dev, clk_names[i]);
> > -               if (IS_ERR(dsi->clks[i])) {
> > -                       if (strcmp(clk_names[i], "sclk_mipi") == 0) {
> > -                               dsi->clks[i] = devm_clk_get(dev, 
> > OLD_SCLK_MIPI_CLK_NAME);
> > -                               if (!IS_ERR(dsi->clks[i]))
> > -                                       continue;
> > -                       }
> > -
> > -                       dev_info(dev, "failed to get the clock: %s\n", 
> > clk_names[i]);
> > -                       return PTR_ERR(dsi->clks[i]);
> > -               }
> > +       ret = devm_clk_bulk_get(dev, dsi->driver_data->num_clks,
> > +                               dsi->driver_data->clk_data);
> > +       if (ret) {
> > +               dev_err(dev, "failed to get clocks in bulk (%d)\n", 
> > ret);
> > +               return ret;
>
> Above change modifies the existing behavior.
>
> Previously, when devm_clk_get() failed and the clock name was
> "sclk_mipi", the code included a fallback mechanism to try "pll_clk"
> instead. This fallback logic has been removed in the current patch.
>
> While changing this behavior may raise concerns, the benefits of
> refactoring—specifically, defining clock names per SoC and adopting
> the clk_bulk_* API for improved maintainability—appear to outweigh the
> potential downsides.

I have checked all devices which use this driver.

Here is a mapping of all compatible -> clock names in the driver:
- fsl,imx8mm-mipi-dsim: bus_clk, sclk_mipi
- fsl,imx8mp-mipi-dsim: bus_clk, sclk_mipi
- samsung,exynos3250-mipi-dsi: bus_clk, pll_clk
- samsung,exynos4210-mipi-dsi: bus_clk, sclk_mipi
- samsung,exynos5410-mipi-dsi: bus_clk, pll_clk
- samsung,exynos5422-mipi-dsi: bus_clk, pll_clk
- samsung,exynos5433-mipi-dsi: bus_clk, sclk_mipi,
                               phyclk_mipidphy0_bitclkdiv8,
                               phyclk_mipidphy0_rxclkesc0,
                               sclk_rgb_vclk_to_dsim0

And here is what I found by grep-ing all devicetrees:

arm/boot/dts/nxp/imx/imx7s.dtsi
    compatible = "fsl,imx7d-mipi-dsim", "fsl,imx8mm-mipi-dsim";
    (uses bus_clk, sclk_mipi)

arm/boot/dts/samsung/exynos3250.dtsi
    compatible = "samsung,exynos3250-mipi-dsi";
    (uses bus_clk, pll_clk)

arm/boot/dts/samsung/exynos4.dtsi
    compatible = "samsung,exynos4210-mipi-dsi";
    (uses bus_clk, sclk_mipi)

arm/boot/dts/samsung/exynos5420.dtsi
    compatible = "samsung,exynos5410-mipi-dsi";
    (uses bus_clk, pll_clk)

arm/boot/dts/samsung/exynos5800.dtsi
    compatible = "samsung,exynos5422-mipi-dsi";
    (uses bus_clk, pll_clk - uses node from exynos5420.dtsi)

arm64/boot/dts/exynos/exynos5433.dtsi
    compatible = "samsung,exynos5433-mipi-dsi";
    (uses bus_clk, sclk_mipi, and 3 others as mentioned above)

arm64/boot/dts/freescale/imx8mm.dtsi
    compatible = "fsl,imx8mm-mipi-dsim";
    (uses bus_clk, sclk_mipi)

arm64/boot/dts/freescale/imx8mn.dtsi
    compatible = "fsl,imx8mn-mipi-dsim", "fsl,imx8mm-mipi-dsim";
    (uses bus_clk, sclk_mipi)

arm64/boot/dts/freescale/imx8mp.dtsi
    compatible = "fsl,imx8mp-mipi-dsim";
    (uses bus_clk, sclk_mipi)

So, there shouldn't be any regressions.

> Unless there are objections from other reviewers, I intend to proceed
> with merging this patch.
> If anyone has concerns or sees potential issues with this change,
> please share your thoughts.
>
> Thanks,
> Inki Dae