Hi Laurent,
I don't remember when did I see equally nice patch to the omap3isp driver!
:-) Thanks!
A few comments below.
On Thu, Apr 04, 2013 at 01:08:38PM +0200, Laurent Pinchart wrote:
...
> +static int isp_xclk_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct isp_xclk *xclk = to_isp_xclk(hw);
> + unsigned long flags;
> + u32 divider;
> +
> + divider = isp_xclk_calc_divider(&rate, parent_rate);
> +
> + spin_lock_irqsave(&xclk->lock, flags);
> +
> + xclk->divider = divider;
> + if (xclk->enabled)
> + isp_xclk_update(xclk, divider);
> +
> + spin_unlock_irqrestore(&xclk->lock, flags);
> +
> + dev_dbg(xclk->isp->dev, "%s: cam_xclk%c set to %lu Hz (div %u)\n",
> + __func__, xclk->id == ISP_XCLK_A ? 'a' : 'b', rate, divider);
> + return 0;
> +}
> +
> +static const struct clk_ops isp_xclk_ops = {
> + .prepare = isp_xclk_prepare,
> + .unprepare = isp_xclk_unprepare,
> + .enable = isp_xclk_enable,
> + .disable = isp_xclk_disable,
> + .recalc_rate = isp_xclk_recalc_rate,
> + .round_rate = isp_xclk_round_rate,
> + .set_rate = isp_xclk_set_rate,
> +};
> +
> +static const char *isp_xclk_parent_name = "cam_mclk";
> +
> +static const struct clk_init_data isp_xclk_init_data = {
> + .name = "cam_xclk",
> + .ops = &isp_xclk_ops,
> + .parent_names = &isp_xclk_parent_name,
> + .num_parents = 1,
> +};
isp_xclk_init_data is unused.
> +static int isp_xclk_init(struct isp_device *isp)
> +{
> + struct isp_platform_data *pdata = isp->pdata;
> + struct clk_init_data init;
Init can be declared inside the loop.
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
> + struct isp_xclk *xclk = &isp->xclks[i];
> + struct clk *clk;
> +
> + xclk->isp = isp;
> + xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B;
> + xclk->divider = 1;
> + spin_lock_init(&xclk->lock);
> +
> + init.name = i == 0 ? "cam_xclka" : "cam_xclkb";
> + init.ops = &isp_xclk_ops;
> + init.parent_names = &isp_xclk_parent_name;
> + init.num_parents = 1;
> +
> + xclk->hw.init = &init;
> +
> + clk = devm_clk_register(isp->dev, &xclk->hw);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + if (pdata->xclks[i].con_id == NULL &&
> + pdata->xclks[i].dev_id == NULL)
> + continue;
> +
> + xclk->lookup = kzalloc(sizeof(*xclk->lookup), GFP_KERNEL);
How about devm_kzalloc()? You'd save a bit of error handling (which is btw.
missing now, as well as kfree in cleanup).
> + if (xclk->lookup == NULL)
> + return -ENOMEM;
> +
> + xclk->lookup->con_id = pdata->xclks[i].con_id;
> + xclk->lookup->dev_id = pdata->xclks[i].dev_id;
> + xclk->lookup->clk = clk;
> +
> + clkdev_add(xclk->lookup);
> + }
> +
> + return 0;
> +}
> +
> +static void isp_xclk_cleanup(struct isp_device *isp)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
> + struct isp_xclk *xclk = &isp->xclks[i];
> +
> + if (xclk->lookup)
> + clkdev_drop(xclk->lookup);
> + }
> +}
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
