Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Daniel Stone <daniel@fooishbar.org>]

Hi,

On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang@samsung.com> wrote:
> +#define REG(dsi, reg)  ((dsi)->reg_base + dsi->driver_data->regs[(reg)])

This seems like a good change in general, but please split it up: it
makes bisection much easier if you have one patch which adds no
functionality and should have exactly the same behaviour, and then
another patch which introduces your changes.

> @@ -431,15 +579,11 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
>         u16 m;
>         u32 reg;
>
> -       clk_set_rate(dsi->pll_clk, dsi->pll_clk_rate);
> -
> -       fin = clk_get_rate(dsi->pll_clk);
> -       if (!fin) {
> -               dev_err(dsi->dev, "failed to get PLL clock frequency\n");
> -               return 0;
> -       }
> -
> -       dev_dbg(dsi->dev, "PLL input frequency: %lu\n", fin);
> +       /*
> +        * The input PLL clock for MIPI DSI in Exynos5433 seems to be fixed
> +        * by OSC CLK.
> +        */
> +       fin = 24 * MHZ;

Er, is this always true on other platforms as well? Shouldn't this be
a part of the DeviceTree description?

> @@ -509,7 +656,7 @@ static int exynos_dsi_enable_clock(struct exynos_dsi *dsi)
>         dev_dbg(dsi->dev, "hs_clk = %lu, byte_clk = %lu, esc_clk = %lu\n",
>                 hs_clk, byte_clk, esc_clk);
>
> -       reg = readl(dsi->reg_base + DSIM_CLKCTRL_REG);
> +       reg = readl(REG(dsi, DSIM_CLKCTRL_REG));

Instead of this readl(REG()) pattern you have everywhere, maybe it
would be easier to introduce a dsi_read_reg(dsi, reg_enum_value)
helper, and the same for write_reg.

> @@ -1720,18 +1873,16 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>                 return -EPROBE_DEFER;
>         }
>
> -       dsi->pll_clk = devm_clk_get(dev, "pll_clk");
> -       if (IS_ERR(dsi->pll_clk)) {
> -               dev_info(dev, "failed to get dsi pll input clock\n");
> -               ret = PTR_ERR(dsi->pll_clk);
> -               goto err_del_component;
> -       }
> -
> -       dsi->bus_clk = devm_clk_get(dev, "bus_clk");
> -       if (IS_ERR(dsi->bus_clk)) {
> -               dev_info(dev, "failed to get dsi bus clock\n");
> -               ret = PTR_ERR(dsi->bus_clk);
> -               goto err_del_component;
> +       dsi->clks = devm_kzalloc(dev,
> +                               sizeof(*dsi->clks) * dsi->driver_data->num_clks,
> +                               GFP_KERNEL);
> +       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])) {
> +                       dev_info(dev, "failed to get dsi pll input clock\n");

This error message seems wrong; it should contain the name of the
actual failing clock.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel