2025년 7월 7일 (월) 오전 3:28, Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx>님이 작성:
>
> Presently, all devices refer to clock names from a single array. The
> only controlling parameter is the number of clocks (num_clks field of
> samsung_dsim_driver_data) which uses the first n clocks of that array.
> As new devices are added, this approach turns out to be cumbersome.
>
> Separate the clock names in individual arrays required by each variant,
> in a struct clk_bulk_data. Add a pointer field to the driver data struct
> which points to their respective clock names, and rework the clock usage
> code to use the clk_bulk_* API instead.
>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 88 +++++++++++++++++------------------
> include/drm/bridge/samsung-dsim.h | 2 +-
> 2 files changed, 44 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 4b49707730db76aa8fd3ab973b02507436750889..b6b3bbcbd0f438e5e1d3faf18f8c2d532a4ecc93 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -219,23 +219,31 @@
> #define DSI_XFER_TIMEOUT_MS 100
> #define DSI_RX_FIFO_EMPTY 0x30800002
>
> -#define OLD_SCLK_MIPI_CLK_NAME "pll_clk"
> -
> #define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
>
> -static const char *const clk_names[5] = {
> - "bus_clk",
> - "sclk_mipi",
> - "phyclk_mipidphy0_bitclkdiv8",
> - "phyclk_mipidphy0_rxclkesc0",
> - "sclk_rgb_vclk_to_dsim0"
> -};
> -
> enum samsung_dsim_transfer_type {
> EXYNOS_DSI_TX,
> EXYNOS_DSI_RX,
> };
>
> +static struct clk_bulk_data exynos3_clk_bulk_data[] = {
> + { .id = "bus_clk" },
> + { .id = "pll_clk" },
> +};
> +
> +static struct clk_bulk_data exynos4_clk_bulk_data[] = {
> + { .id = "bus_clk" },
> + { .id = "sclk_mipi" },
> +};
> +
> +static struct clk_bulk_data exynos5433_clk_bulk_data[] = {
> + { .id = "bus_clk" },
> + { .id = "sclk_mipi" },
> + { .id = "phyclk_mipidphy0_bitclkdiv8" },
> + { .id = "phyclk_mipidphy0_rxclkesc0" },
> + { .id = "sclk_rgb_vclk_to_dsim0" },
> +};
> +
> enum reg_idx {
> DSIM_STATUS_REG, /* Status register (legacy) */
> DSIM_LINK_STATUS_REG, /* Link status register */
> @@ -408,7 +416,8 @@ static const struct samsung_dsim_driver_data exynos3_dsi_driver_data = {
> .has_legacy_status_reg = 1,
> .has_freqband = 1,
> .has_clklane_stop = 1,
> - .num_clks = 2,
> + .clk_data = exynos3_clk_bulk_data,
> + .num_clks = ARRAY_SIZE(exynos3_clk_bulk_data),
> .max_freq = 1000,
> .wait_for_hdr_fifo = 1,
> .wait_for_reset = 1,
> @@ -439,7 +448,8 @@ static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
> .has_legacy_status_reg = 1,
> .has_freqband = 1,
> .has_clklane_stop = 1,
> - .num_clks = 2,
> + .clk_data = exynos4_clk_bulk_data,
> + .num_clks = ARRAY_SIZE(exynos4_clk_bulk_data),
> .max_freq = 1000,
> .wait_for_hdr_fifo = 1,
> .wait_for_reset = 1,
> @@ -468,7 +478,8 @@ static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
> .reg_ofs = exynos_reg_ofs,
> .plltmr_reg = 0x58,
> .has_legacy_status_reg = 1,
> - .num_clks = 2,
> + .clk_data = exynos3_clk_bulk_data,
> + .num_clks = ARRAY_SIZE(exynos3_clk_bulk_data),
> .max_freq = 1000,
> .wait_for_hdr_fifo = 1,
> .wait_for_reset = 1,
> @@ -497,7 +508,8 @@ static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
> .plltmr_reg = 0xa0,
> .has_legacy_status_reg = 1,
> .has_clklane_stop = 1,
> - .num_clks = 5,
> + .clk_data = exynos5433_clk_bulk_data,
> + .num_clks = ARRAY_SIZE(exynos5433_clk_bulk_data),
> .max_freq = 1500,
> .wait_for_hdr_fifo = 1,
> .wait_for_reset = 0,
> @@ -526,7 +538,8 @@ static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
> .plltmr_reg = 0xa0,
> .has_legacy_status_reg = 1,
> .has_clklane_stop = 1,
> - .num_clks = 2,
> + .clk_data = exynos3_clk_bulk_data,
> + .num_clks = ARRAY_SIZE(exynos3_clk_bulk_data),
> .max_freq = 1500,
> .wait_for_hdr_fifo = 1,
> .wait_for_reset = 1,
> @@ -555,7 +568,8 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
> .plltmr_reg = 0xa0,
> .has_legacy_status_reg = 1,
> .has_clklane_stop = 1,
> - .num_clks = 2,
> + .clk_data = exynos4_clk_bulk_data,
> + .num_clks = ARRAY_SIZE(exynos4_clk_bulk_data),
> .max_freq = 2100,
> .wait_for_hdr_fifo = 1,
> .wait_for_reset = 0,
> @@ -2021,7 +2035,7 @@ int samsung_dsim_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct samsung_dsim *dsi;
> - int ret, i;
> + int ret;
>
> dsi = devm_drm_bridge_alloc(dev, struct samsung_dsim, bridge, &samsung_dsim_bridge_funcs);
> if (IS_ERR(dsi))
> @@ -2045,23 +2059,11 @@ int samsung_dsim_probe(struct platform_device *pdev)
> if (ret)
> return dev_err_probe(dev, ret, "failed to get regulators\n");
>
> - 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.
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
> }
>
> dsi->reg_base = devm_platform_ioremap_resource(pdev, 0);
> @@ -2134,7 +2136,7 @@ static int samsung_dsim_suspend(struct device *dev)
> {
> struct samsung_dsim *dsi = dev_get_drvdata(dev);
> const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> - int ret, i;
> + int ret;
>
> usleep_range(10000, 20000);
>
> @@ -2150,8 +2152,7 @@ static int samsung_dsim_suspend(struct device *dev)
>
> phy_power_off(dsi->phy);
>
> - for (i = driver_data->num_clks - 1; i > -1; i--)
> - clk_disable_unprepare(dsi->clks[i]);
> + clk_bulk_disable_unprepare(driver_data->num_clks, driver_data->clk_data);
>
> ret = regulator_bulk_disable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
> if (ret < 0)
> @@ -2164,7 +2165,7 @@ static int samsung_dsim_resume(struct device *dev)
> {
> struct samsung_dsim *dsi = dev_get_drvdata(dev);
> const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> - int ret, i;
> + int ret;
>
> ret = regulator_bulk_enable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
> if (ret < 0) {
> @@ -2172,11 +2173,9 @@ static int samsung_dsim_resume(struct device *dev)
> return ret;
> }
>
> - for (i = 0; i < driver_data->num_clks; i++) {
> - ret = clk_prepare_enable(dsi->clks[i]);
> - if (ret < 0)
> - goto err_clk;
> - }
> + ret = clk_bulk_prepare_enable(driver_data->num_clks, driver_data->clk_data);
> + if (ret < 0)
> + goto err_clk;
>
> ret = phy_power_on(dsi->phy);
> if (ret < 0) {
> @@ -2187,8 +2186,7 @@ static int samsung_dsim_resume(struct device *dev)
> return 0;
>
> err_clk:
> - while (--i > -1)
> - clk_disable_unprepare(dsi->clks[i]);
> + clk_bulk_disable_unprepare(driver_data->num_clks, driver_data->clk_data);
> regulator_bulk_disable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
>
> return ret;
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index 04ed11787bbd22503f7221cad1a491a4e5e66781..eb9fdbab1b34074923daa0aa0443c33c5b99ae42 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -58,6 +58,7 @@ struct samsung_dsim_driver_data {
> unsigned int has_clklane_stop:1;
> unsigned int has_broken_fifoctrl_emptyhdr:1;
> unsigned int has_sfrctrl:1;
> + struct clk_bulk_data *clk_data;
> unsigned int num_clks;
> unsigned int min_freq;
> unsigned int max_freq;
> @@ -104,7 +105,6 @@ struct samsung_dsim {
>
> void __iomem *reg_base;
> struct phy *phy;
> - struct clk **clks;
> struct clk *pll_clk;
> struct regulator_bulk_data supplies[2];
> int irq;
>
> --
> 2.49.0
>
>
