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

[Hyungwon Hwang]

Dear Daniel,

On Wed, 18 Mar 2015 09:52:33 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang at 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.

Yes. I agree with you.

> 
> > @@ -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?

I forgot to change the comment in development. Finally it is found that
all exynos mipi dsi's fin is OSC clk which is 24 MHz. So I will remove
the comment, but remain the code as it is.

> 
> > @@ -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.

I think that it can make the code more readable. I agree.

> 
> > @@ -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.

Oh. I forgot. I will change it.

Thanks for your review. I will send it again with the changes you
suggested.

> 
> Cheers,
> Daniel