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

Dear Daniel,

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

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.

Yes. That also looks good to me.

...

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

Yes. That's resonable.

...

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

OK.

Best regards, Hyungwon Hwang

Cheers, Daniel