[PATCH 0/2] OMAP3 ISP common clock framework support

[PATCH 0/2] OMAP3 ISP common clock framework support

[Laurent Pinchart]

Hello,

These two patches implement support for the common clock framework in the OMAP3
ISP and MT9P031 drivers. They finally get rid of the last isp_platform_callback
operation, and thus of the isp_platform_callback structure completely, as well
as the only platform callback from the mt9p031 driver.

The patches depend on Mike Turquette's common clock framework reentrancy
patches:

  clk: abstract locking out into helper functions
  clk: allow reentrant calls into the clk framework

v6 of those two patches has been posted to LKML and LAKML.

Mauro, how would you like to proceed with merging these ? Mike, will the above
two patches make it to v3.10 ? If so could you please provide a stable branch
that Mauro could merge ?

Laurent Pinchart (2):
  omap3isp: Use the common clock framework
  mt9p031: Use the common clock framework

 drivers/media/i2c/mt9p031.c           |  22 ++-
 drivers/media/platform/omap3isp/isp.c | 277 +++++++++++++++++++++++++---------
 drivers/media/platform/omap3isp/isp.h |  22 ++-
 include/media/mt9p031.h               |   2 -
 include/media/omap3isp.h              |  10 +-
 5 files changed, 240 insertions(+), 93 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] omap3isp: Use the common clock framework

[Laurent Pinchart]

Expose the two ISP external clocks XCLKA and XCLKB as common clocks for
subdev drivers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/isp.c | 277 +++++++++++++++++++++++++---------
 drivers/media/platform/omap3isp/isp.h |  22 ++-
 include/media/omap3isp.h              |  10 +-
 3 files changed, 225 insertions(+), 84 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 6e5ad8e..1d7dbd5 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -55,6 +55,7 @@
 #include <asm/cacheflush.h>
 
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
@@ -148,6 +149,201 @@ void omap3isp_flush(struct isp_device *isp)
 	isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_REVISION);
 }
 
+/* -----------------------------------------------------------------------------
+ * XCLK
+ */
+
+#define to_isp_xclk(_hw)	container_of(_hw, struct isp_xclk, hw)
+
+static void isp_xclk_update(struct isp_xclk *xclk, u32 divider)
+{
+	switch (xclk->id) {
+	case ISP_XCLK_A:
+		isp_reg_clr_set(xclk->isp, OMAP3_ISP_IOMEM_MAIN, ISP_TCTRL_CTRL,
+				ISPTCTRL_CTRL_DIVA_MASK,
+				divider << ISPTCTRL_CTRL_DIVA_SHIFT);
+		break;
+	case ISP_XCLK_B:
+		isp_reg_clr_set(xclk->isp, OMAP3_ISP_IOMEM_MAIN, ISP_TCTRL_CTRL,
+				ISPTCTRL_CTRL_DIVB_MASK,
+				divider << ISPTCTRL_CTRL_DIVB_SHIFT);
+		break;
+	}
+}
+
+static int isp_xclk_prepare(struct clk_hw *hw)
+{
+	struct isp_xclk *xclk = to_isp_xclk(hw);
+
+	omap3isp_get(xclk->isp);
+
+	return 0;
+}
+
+static void isp_xclk_unprepare(struct clk_hw *hw)
+{
+	struct isp_xclk *xclk = to_isp_xclk(hw);
+
+	omap3isp_put(xclk->isp);
+}
+
+static int isp_xclk_enable(struct clk_hw *hw)
+{
+	struct isp_xclk *xclk = to_isp_xclk(hw);
+	unsigned long flags;
+
+	spin_lock_irqsave(&xclk->lock, flags);
+	isp_xclk_update(xclk, xclk->divider);
+	xclk->enabled = true;
+	spin_unlock_irqrestore(&xclk->lock, flags);
+
+	return 0;
+}
+
+static void isp_xclk_disable(struct clk_hw *hw)
+{
+	struct isp_xclk *xclk = to_isp_xclk(hw);
+	unsigned long flags;
+
+	spin_lock_irqsave(&xclk->lock, flags);
+	isp_xclk_update(xclk, 0);
+	xclk->enabled = false;
+	spin_unlock_irqrestore(&xclk->lock, flags);
+}
+
+static unsigned long isp_xclk_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct isp_xclk *xclk = to_isp_xclk(hw);
+
+	return parent_rate / xclk->divider;
+}
+
+static u32 isp_xclk_calc_divider(unsigned long *rate, unsigned long parent_rate)
+{
+	u32 divider;
+
+	if (*rate >= parent_rate) {
+		*rate = parent_rate;
+		return ISPTCTRL_CTRL_DIV_BYPASS;
+	}
+
+	divider = DIV_ROUND_CLOSEST(parent_rate, *rate);
+	if (divider >= ISPTCTRL_CTRL_DIV_BYPASS)
+		divider = ISPTCTRL_CTRL_DIV_BYPASS - 1;
+
+	*rate = parent_rate / divider;
+	return divider;
+}
+
+static long isp_xclk_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *parent_rate)
+{
+	isp_xclk_calc_divider(&rate, *parent_rate);
+	return rate;
+}
+
+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,
+};
+
+static int isp_xclk_init(struct isp_device *isp)
+{
+	struct isp_platform_data *pdata = isp->pdata;
+	struct clk_init_data init;
+	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);
+		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);
+	}
+}
+
+/* -----------------------------------------------------------------------------
+ * Interrupts
+ */
+
 /*
  * isp_enable_interrupts - Enable ISP interrupts.
  * @isp: OMAP3 ISP device
@@ -180,80 +376,6 @@ static void isp_disable_interrupts(struct isp_device *isp)
 	isp_reg_writel(isp, 0, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0ENABLE);
 }
 
-/**
- * isp_set_xclk - Configures the specified cam_xclk to the desired frequency.
- * @isp: OMAP3 ISP device
- * @xclk: Desired frequency of the clock in Hz. 0 = stable low, 1 is stable high
- * @xclksel: XCLK to configure (0 = A, 1 = B).
- *
- * Configures the specified MCLK divisor in the ISP timing control register
- * (TCTRL_CTRL) to generate the desired xclk clock value.
- *
- * Divisor = cam_mclk_hz / xclk
- *
- * Returns the final frequency that is actually being generated
- **/
-static u32 isp_set_xclk(struct isp_device *isp, u32 xclk, u8 xclksel)
-{
-	u32 divisor;
-	u32 currentxclk;
-	unsigned long mclk_hz;
-
-	if (!omap3isp_get(isp))
-		return 0;
-
-	mclk_hz = clk_get_rate(isp->clock[ISP_CLK_CAM_MCLK]);
-
-	if (xclk >= mclk_hz) {
-		divisor = ISPTCTRL_CTRL_DIV_BYPASS;
-		currentxclk = mclk_hz;
-	} else if (xclk >= 2) {
-		divisor = mclk_hz / xclk;
-		if (divisor >= ISPTCTRL_CTRL_DIV_BYPASS)
-			divisor = ISPTCTRL_CTRL_DIV_BYPASS - 1;
-		currentxclk = mclk_hz / divisor;
-	} else {
-		divisor = xclk;
-		currentxclk = 0;
-	}
-
-	switch (xclksel) {
-	case ISP_XCLK_A:
-		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_MAIN, ISP_TCTRL_CTRL,
-				ISPTCTRL_CTRL_DIVA_MASK,
-				divisor << ISPTCTRL_CTRL_DIVA_SHIFT);
-		dev_dbg(isp->dev, "isp_set_xclk(): cam_xclka set to %d Hz\n",
-			currentxclk);
-		break;
-	case ISP_XCLK_B:
-		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_MAIN, ISP_TCTRL_CTRL,
-				ISPTCTRL_CTRL_DIVB_MASK,
-				divisor << ISPTCTRL_CTRL_DIVB_SHIFT);
-		dev_dbg(isp->dev, "isp_set_xclk(): cam_xclkb set to %d Hz\n",
-			currentxclk);
-		break;
-	case ISP_XCLK_NONE:
-	default:
-		omap3isp_put(isp);
-		dev_dbg(isp->dev, "ISP_ERR: isp_set_xclk(): Invalid requested "
-			"xclk. Must be 0 (A) or 1 (B).\n");
-		return -EINVAL;
-	}
-
-	/* Do we go from stable whatever to clock? */
-	if (divisor >= 2 && isp->xclk_divisor[xclksel - 1] < 2)
-		omap3isp_get(isp);
-	/* Stopping the clock. */
-	else if (divisor < 2 && isp->xclk_divisor[xclksel - 1] >= 2)
-		omap3isp_put(isp);
-
-	isp->xclk_divisor[xclksel - 1] = divisor;
-
-	omap3isp_put(isp);
-
-	return currentxclk;
-}
-
 /*
  * isp_core_init - ISP core settings
  * @isp: OMAP3 ISP device
@@ -1969,6 +2091,7 @@ static int isp_remove(struct platform_device *pdev)
 
 	isp_unregister_entities(isp);
 	isp_cleanup_modules(isp);
+	isp_xclk_cleanup(isp);
 
 	__omap3isp_get(isp, false);
 	iommu_detach_device(isp->domain, &pdev->dev);
@@ -2042,7 +2165,6 @@ static int isp_probe(struct platform_device *pdev)
 	}
 
 	isp->autoidle = autoidle;
-	isp->platform_cb.set_xclk = isp_set_xclk;
 
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);
@@ -2093,6 +2215,10 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_isp;
 
+	ret = isp_xclk_init(isp);
+	if (ret < 0)
+		goto error_isp;
+
 	/* Memory resources */
 	for (m = 0; m < ARRAY_SIZE(isp_res_maps); m++)
 		if (isp->revision == isp_res_maps[m].isp_rev)
@@ -2162,6 +2288,7 @@ detach_dev:
 free_domain:
 	iommu_domain_free(isp->domain);
 error_isp:
+	isp_xclk_cleanup(isp);
 	omap3isp_put(isp);
 error:
 	platform_set_drvdata(pdev, NULL);
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index c77e1f2..cd3eff4 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -29,6 +29,7 @@
 
 #include <media/omap3isp.h>
 #include <media/v4l2-device.h>
+#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/iommu.h>
@@ -125,8 +126,20 @@ struct isp_reg {
 	u32 val;
 };
 
-struct isp_platform_callback {
-	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
+enum isp_xclk_id {
+	ISP_XCLK_A,
+	ISP_XCLK_B,
+};
+
+struct isp_xclk {
+	struct isp_device *isp;
+	struct clk_hw hw;
+	struct clk_lookup *lookup;
+	enum isp_xclk_id id;
+
+	spinlock_t lock;	/* Protects enabled and divider */
+	bool enabled;
+	unsigned int divider;
 };
 
 /*
@@ -149,6 +162,7 @@ struct isp_platform_callback {
  * @cam_mclk: Pointer to camera functional clock structure.
  * @csi2_fck: Pointer to camera CSI2 complexIO clock structure.
  * @l3_ick: Pointer to OMAP3 L3 bus interface clock.
+ * @xclks: External clocks provided by the ISP
  * @irq: Currently attached ISP ISR callbacks information structure.
  * @isp_af: Pointer to current settings for ISP AutoFocus SCM.
  * @isp_hist: Pointer to current settings for ISP Histogram SCM.
@@ -185,12 +199,12 @@ struct isp_device {
 	int has_context;
 	int ref_count;
 	unsigned int autoidle;
-	u32 xclk_divisor[2];	/* Two clocks, a and b. */
 #define ISP_CLK_CAM_ICK		0
 #define ISP_CLK_CAM_MCLK	1
 #define ISP_CLK_CSI2_FCK	2
 #define ISP_CLK_L3_ICK		3
 	struct clk *clock[4];
+	struct isp_xclk xclks[2];
 
 	/* ISP modules */
 	struct ispstat isp_af;
@@ -209,8 +223,6 @@ struct isp_device {
 	unsigned int subclk_resources;
 
 	struct iommu_domain *domain;
-
-	struct isp_platform_callback platform_cb;
 };
 
 #define v4l2_dev_to_isp_device(dev) \
diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h
index 9584269..c9d06d9 100644
--- a/include/media/omap3isp.h
+++ b/include/media/omap3isp.h
@@ -29,10 +29,6 @@
 struct i2c_board_info;
 struct isp_device;
 
-#define ISP_XCLK_NONE			0
-#define ISP_XCLK_A			1
-#define ISP_XCLK_B			2
-
 enum isp_interface_type {
 	ISP_INTERFACE_PARALLEL,
 	ISP_INTERFACE_CSI2A_PHY2,
@@ -153,7 +149,13 @@ struct isp_v4l2_subdevs_group {
 	} bus; /* gcc < 4.6.0 chokes on anonymous union initializers */
 };
 
+struct isp_platform_xclk {
+	const char *dev_id;
+	const char *con_id;
+};
+
 struct isp_platform_data {
+	struct isp_platform_xclk xclks[2];
 	struct isp_v4l2_subdevs_group *subdevs;
 	void (*set_constraints)(struct isp_device *isp, bool enable);
 };
-- 
1.8.1.5

[PATCH 2/2] mt9p031: Use the common clock framework

[Laurent Pinchart]

Configure the device external clock using the common clock framework
instead of a board code callback function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/mt9p031.c | 22 +++++++++++++++-------
 include/media/mt9p031.h     |  2 --
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index e328332..825cc2d 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -12,6 +12,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/gpio.h>
@@ -121,6 +122,8 @@ struct mt9p031 {
 	struct mutex power_lock; /* lock to protect power_count */
 	int power_count;
 
+	struct clk *clk;
+
 	enum mt9p031_model model;
 	struct aptina_pll pll;
 	int reset;
@@ -195,7 +198,7 @@ static int mt9p031_reset(struct mt9p031 *mt9p031)
 					  0);
 }
 
-static int mt9p031_pll_setup(struct mt9p031 *mt9p031)
+static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
 {
 	static const struct aptina_pll_limits limits = {
 		.ext_clock_min = 6000000,
@@ -216,6 +219,12 @@ static int mt9p031_pll_setup(struct mt9p031 *mt9p031)
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
 	struct mt9p031_platform_data *pdata = mt9p031->pdata;
 
+	mt9p031->clk = devm_clk_get(&client->dev, NULL);
+	if (IS_ERR(mt9p031->clk))
+		return PTR_ERR(mt9p031->clk);
+
+	clk_set_rate(mt9p031->clk, pdata->ext_freq);
+
 	mt9p031->pll.ext_clock = pdata->ext_freq;
 	mt9p031->pll.pix_clock = pdata->target_freq;
 
@@ -265,9 +274,8 @@ static int mt9p031_power_on(struct mt9p031 *mt9p031)
 	}
 
 	/* Emable clock */
-	if (mt9p031->pdata->set_xclk)
-		mt9p031->pdata->set_xclk(&mt9p031->subdev,
-					 mt9p031->pdata->ext_freq);
+	if (mt9p031->clk)
+		clk_prepare_enable(mt9p031->clk);
 
 	/* Now RESET_BAR must be high */
 	if (mt9p031->reset != -1) {
@@ -285,8 +293,8 @@ static void mt9p031_power_off(struct mt9p031 *mt9p031)
 		usleep_range(1000, 2000);
 	}
 
-	if (mt9p031->pdata->set_xclk)
-		mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
+	if (mt9p031->clk)
+		clk_disable_unprepare(mt9p031->clk);
 }
 
 static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
@@ -1009,7 +1017,7 @@ static int mt9p031_probe(struct i2c_client *client,
 		mt9p031->reset = pdata->reset;
 	}
 
-	ret = mt9p031_pll_setup(mt9p031);
+	ret = mt9p031_clk_setup(mt9p031);
 
 done:
 	if (ret < 0) {
diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
index 0c97b19..b1e63f2 100644
--- a/include/media/mt9p031.h
+++ b/include/media/mt9p031.h
@@ -5,13 +5,11 @@ struct v4l2_subdev;
 
 /*
  * struct mt9p031_platform_data - MT9P031 platform data
- * @set_xclk: Clock frequency set callback
  * @reset: Chip reset GPIO (set to -1 if not used)
  * @ext_freq: Input clock frequency
  * @target_freq: Pixel clock frequency
  */
 struct mt9p031_platform_data {
-	int (*set_xclk)(struct v4l2_subdev *subdev, int hz);
 	int reset;
 	int ext_freq;
 	int target_freq;
-- 
1.8.1.5

Re: [PATCH 1/2] omap3isp: Use the common clock framework

[Sakari Ailus]

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@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/2] omap3isp: Use the common clock framework

[Sylwester Nawrocki]

Hi Sakari,

On 04/04/2013 01:20 PM, Sakari Ailus wrote:
> 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:
> ...

>> +		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).

clkdev_drop() will free memory allocated here. So using devm_kzalloc()
wouldn't be correct.

>> +		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);
>> +	}
>> +}

Regards,
Sylwester

Re: [PATCH 1/2] omap3isp: Use the common clock framework

[Laurent Pinchart]

Hi Sakari,

Thanks for the comments.

On Thursday 04 April 2013 14:20:04 Sakari Ailus wrote:
> 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.

Indeed. I wonder how I've missed that, the compiler should have complained. 
I'll fix it for v2.

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

OK.

> > +	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).

As Sylwester pointed out, clkdev_drop() frees the memory. I don't really like 
that clkdev_add/clkdev_drop inconsistency, that might be something worth 
fixing at some point.

> > +		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);
> > +	}
> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] omap3isp: Use the common clock framework

[Sakari Ailus]

Hi Sylwester,

On Thu, Apr 04, 2013 at 01:27:32PM +0200, Sylwester Nawrocki wrote:
> clkdev_drop() will free memory allocated here. So using devm_kzalloc()
> wouldn't be correct.

Thanks for pointing this out. I should educate myself on the common clock
framework. The problem, though, is where to get the time for that. :-P

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 0/2] OMAP3 ISP common clock framework support

[Mauro Carvalho Chehab]

Em Thu,  4 Apr 2013 13:08:37 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hello,
> 
> These two patches implement support for the common clock framework in the OMAP3
> ISP and MT9P031 drivers. They finally get rid of the last isp_platform_callback
> operation, and thus of the isp_platform_callback structure completely, as well
> as the only platform callback from the mt9p031 driver.
> 
> The patches depend on Mike Turquette's common clock framework reentrancy
> patches:
> 
>   clk: abstract locking out into helper functions
>   clk: allow reentrant calls into the clk framework
> 
> v6 of those two patches has been posted to LKML and LAKML.
> 
> Mauro, how would you like to proceed with merging these ? 

As discussed on IRC, just add those two patches on a branch based on
master. When asking me to pull from it, say that it should be a topic
branch that will depend on Mike's patches.

I'll hold them on a separate branch, sending them upstream only after
receiving an email from you or Mike that his patches got merged there
already.

> Mike, will the above
> two patches make it to v3.10 ? If so could you please provide a stable branch
> that Mauro could merge ?

Regards,
Mauro

> 
> Laurent Pinchart (2):
>   omap3isp: Use the common clock framework
>   mt9p031: Use the common clock framework
> 
>  drivers/media/i2c/mt9p031.c           |  22 ++-
>  drivers/media/platform/omap3isp/isp.c | 277 +++++++++++++++++++++++++---------
>  drivers/media/platform/omap3isp/isp.h |  22 ++-
>  include/media/mt9p031.h               |   2 -
>  include/media/omap3isp.h              |  10 +-
>  5 files changed, 240 insertions(+), 93 deletions(-)
> 


-- 

Cheers,
Mauro