Hi, On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang@xxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html