[RFC 0/4] OMAP 3 ISP driver improvements

[RFC 0/4] OMAP 3 ISP driver improvements

[Sakari Ailus]

Hi all,

This patchset removes two of the three callbacks in the OMAP 3 ISP driver's
board code.

It is dependent on first and third patch in the "[RFC] On controlling
sensors" patchset:

<URL:http://www.spinics.net/lists/linux-media/msg40861.html>

What will be left is the external clock, which should be moved to use the
generic clock framework in the future. The dependency indeed is the generic
clock framework --- on OMAP 3 the clock is provided by the ISP, so accessing
it through the clock framework isn't possible at the moment. Once we have
that, the ISP should register the xclk to the clock framework and the sensor
driver would use if from there. Then I see no issues in moving to the device
tree.

Also, controlling the CSI-2 receiver in the OMAP 3630 properly is now
possible with this patchset.

Regards,

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

[RFC 1/4] omap3isp: Implement validate_pipeline

[Sakari Ailus]

Validate pipeline of any external entity connected to the ISP driver.
The validation of the pipeline for the part that involves links inside the
domain of another driver must be done by that very driver.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/omap3isp/ispvideo.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index f229057..17bc03c 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -355,6 +355,18 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 		    fmt_source.format.height != fmt_sink.format.height)
 			return -EPIPE;
 
+		if (subdev->host_priv) {
+			/*
+			 * host_priv != NULL: this is a sensor. Issue
+			 * validate_pipeline. We're at our end of the
+			 * pipeline so we quit now.
+			 */
+			ret = v4l2_subdev_call(subdev, pad, validate_pipeline);
+			if (IS_ERR_VALUE(ret))
+				return -EPIPE;
+			break;
+		}
+
 		if (shifter_link) {
 			unsigned int parallel_shift = 0;
 			if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {
-- 
1.7.2.5

Re: [RFC 1/4] omap3isp: Implement validate_pipeline

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Thursday 15 December 2011 10:50:32 Sakari Ailus wrote:
> Validate pipeline of any external entity connected to the ISP driver.
> The validation of the pipeline for the part that involves links inside the
> domain of another driver must be done by that very driver.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/omap3isp/ispvideo.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> b/drivers/media/video/omap3isp/ispvideo.c index f229057..17bc03c 100644
> --- a/drivers/media/video/omap3isp/ispvideo.c
> +++ b/drivers/media/video/omap3isp/ispvideo.c
> @@ -355,6 +355,18 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
>  			return -EPIPE;
> 
> +		if (subdev->host_priv) {
> +			/*
> +			 * host_priv != NULL: this is a sensor. Issue
> +			 * validate_pipeline. We're at our end of the
> +			 * pipeline so we quit now.
> +			 */
> +			ret = v4l2_subdev_call(subdev, pad, validate_pipeline);
> +			if (IS_ERR_VALUE(ret))

Is the validate pipeline operation expected to return a value different than 0 
on success ? If not if (ret < 0) should do.

Although there's another issue. Not all sensors will implement the 
validate_pipeline operation, so you shouldn't return an error if ret == -
ENOIOCTLCMD.

I will comment on the validate_pipeline approach itself in the "On controlling 
sensors" mail thread.

> +				return -EPIPE;
> +			break;
> +		}
> +
>  		if (shifter_link) {
>  			unsigned int parallel_shift = 0;
>  			if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/4] omap3isp: Implement validate_pipeline

[Sakari Ailus]

Hi Laurent,

Thanks for the review!!!

On Thu, Dec 15, 2011 at 11:18:53AM +0100, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.
> 
> On Thursday 15 December 2011 10:50:32 Sakari Ailus wrote:
> > Validate pipeline of any external entity connected to the ISP driver.
> > The validation of the pipeline for the part that involves links inside the
> > domain of another driver must be done by that very driver.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  drivers/media/video/omap3isp/ispvideo.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/omap3isp/ispvideo.c
> > b/drivers/media/video/omap3isp/ispvideo.c index f229057..17bc03c 100644
> > --- a/drivers/media/video/omap3isp/ispvideo.c
> > +++ b/drivers/media/video/omap3isp/ispvideo.c
> > @@ -355,6 +355,18 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
> >  			return -EPIPE;
> > 
> > +		if (subdev->host_priv) {
> > +			/*
> > +			 * host_priv != NULL: this is a sensor. Issue
> > +			 * validate_pipeline. We're at our end of the
> > +			 * pipeline so we quit now.
> > +			 */
> > +			ret = v4l2_subdev_call(subdev, pad, validate_pipeline);
> > +			if (IS_ERR_VALUE(ret))
> 
> Is the validate pipeline operation expected to return a value different than 0 
> on success ? If not if (ret < 0) should do.
> 
> Although there's another issue. Not all sensors will implement the 
> validate_pipeline operation, so you shouldn't return an error if ret == -
> ENOIOCTLCMD.
> 
> I will comment on the validate_pipeline approach itself in the "On controlling 
> sensors" mail thread.

Good point. I'll fix this; same for your comment on the third patch.

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

[RFC 2/4] omap3isp: Add lane configuration to platform data

[Sakari Ailus]

Add lane configuration (order of clock and data lane) to platform data on
both CCP2 and CSI-2.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/omap3isp/ispcsiphy.h |   15 ++-------------
 include/media/omap3isp.h                 |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispcsiphy.h b/drivers/media/video/omap3isp/ispcsiphy.h
index 9596dc6..e93a661 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.h
+++ b/drivers/media/video/omap3isp/ispcsiphy.h
@@ -27,22 +27,11 @@
 #ifndef OMAP3_ISP_CSI_PHY_H
 #define OMAP3_ISP_CSI_PHY_H
 
+#include <media/omap3isp.h>
+
 struct isp_csi2_device;
 struct regulator;
 
-struct csiphy_lane {
-	u8 pos;
-	u8 pol;
-};
-
-#define ISP_CSIPHY2_NUM_DATA_LANES	2
-#define ISP_CSIPHY1_NUM_DATA_LANES	1
-
-struct isp_csiphy_lanes_cfg {
-	struct csiphy_lane data[ISP_CSIPHY2_NUM_DATA_LANES];
-	struct csiphy_lane clk;
-};
-
 struct isp_csiphy_dphy_cfg {
 	u8 ths_term;
 	u8 ths_settle;
diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h
index e917b1d..8fe0bdf 100644
--- a/include/media/omap3isp.h
+++ b/include/media/omap3isp.h
@@ -86,6 +86,19 @@ enum {
 	ISP_CCP2_MODE_CCP2 = 1,
 };
 
+struct csiphy_lane {
+	u8 pos;
+	u8 pol;
+};
+
+#define ISP_CSIPHY2_NUM_DATA_LANES	2
+#define ISP_CSIPHY1_NUM_DATA_LANES	1
+
+struct isp_csiphy_lanes_cfg {
+	struct csiphy_lane data[ISP_CSIPHY2_NUM_DATA_LANES];
+	struct csiphy_lane clk;
+};
+
 /**
  * struct isp_ccp2_platform_data - CCP2 interface platform data
  * @strobe_clk_pol: Strobe/clock polarity
@@ -105,6 +118,7 @@ struct isp_ccp2_platform_data {
 	unsigned int ccp2_mode:1;
 	unsigned int phy_layer:1;
 	unsigned int vpclk_div:2;
+	struct isp_csiphy_lanes_cfg *lanecfg;
 };
 
 /**
@@ -115,6 +129,7 @@ struct isp_ccp2_platform_data {
 struct isp_csi2_platform_data {
 	unsigned crc:1;
 	unsigned vpclk_div:2;
+	struct isp_csiphy_lanes_cfg *lanecfg;
 };
 
 struct isp_subdev_i2c_board_info {
-- 
1.7.2.5

[RFC 3/4] omap3isp: Configure CSI-2 phy based on platform data

[Sakari Ailus]

Configure CSI-2 phy based on platform data in the ISP driver rather than in
platform code.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/omap3isp/isp.h       |    3 -
 drivers/media/video/omap3isp/ispcsiphy.c |   95 ++++++++++++++++++++++++++---
 drivers/media/video/omap3isp/ispcsiphy.h |    4 +
 drivers/media/video/omap3isp/ispvideo.c  |   19 ++++++
 4 files changed, 108 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/omap3isp/isp.h b/drivers/media/video/omap3isp/isp.h
index 705946e..c5935ae 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -126,9 +126,6 @@ struct isp_reg {
 
 struct isp_platform_callback {
 	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
-	int (*csiphy_config)(struct isp_csiphy *phy,
-			     struct isp_csiphy_dphy_cfg *dphy,
-			     struct isp_csiphy_lanes_cfg *lanes);
 	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
 };
 
diff --git a/drivers/media/video/omap3isp/ispcsiphy.c b/drivers/media/video/omap3isp/ispcsiphy.c
index 5be37ce..52af308 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.c
+++ b/drivers/media/video/omap3isp/ispcsiphy.c
@@ -28,6 +28,8 @@
 #include <linux/device.h>
 #include <linux/regulator/consumer.h>
 
+#include "../../../../arch/arm/mach-omap2/control.h"
+
 #include "isp.h"
 #include "ispreg.h"
 #include "ispcsiphy.h"
@@ -138,15 +140,90 @@ static void csiphy_dphy_config(struct isp_csiphy *phy)
 	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
 }
 
-static int csiphy_config(struct isp_csiphy *phy,
-			 struct isp_csiphy_dphy_cfg *dphy,
-			 struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
+ * THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3.
+ */
+#define THS_TERM_D 2000000
+#define THS_TERM(ddrclk_khz)					\
+(								\
+	((25 * (ddrclk_khz)) % THS_TERM_D) ?			\
+		((25 * (ddrclk_khz)) / THS_TERM_D) :		\
+		((25 * (ddrclk_khz)) / THS_TERM_D) - 1		\
+)
+
+#define THS_SETTLE_D 1000000
+#define THS_SETTLE(ddrclk_khz)					\
+(								\
+	((90 * (ddrclk_khz)) % THS_SETTLE_D) ?			\
+		((90 * (ddrclk_khz)) / THS_SETTLE_D) + 4 :	\
+		((90 * (ddrclk_khz)) / THS_SETTLE_D) + 3	\
+)
+
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM	0
+#define TCLK_MISS	1
+#define TCLK_SETTLE	14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+			   struct v4l2_subdev *csi2_subdev,
+			   struct v4l2_subdev *sensor,
+			   struct v4l2_mbus_framefmt *sensor_fmt)
 {
+	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
+	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+	struct isp_csiphy_dphy_cfg csi2phy;
+	int csi2_ddrclk_khz;
+	struct isp_csiphy_lanes_cfg *lanes;
 	unsigned int used_lanes = 0;
 	unsigned int i;
+	u32 cam_phy_ctrl;
+
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
+	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
+		lanes = subdevs->bus.ccp2.lanecfg;
+	else
+		lanes = subdevs->bus.csi2.lanecfg;
+
+	if (!lanes) {
+		dev_err(isp->dev, "no lane configuration\n");
+		return -EINVAL;
+	}
+
+	cam_phy_ctrl = omap_readl(
+		OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
+	/*
+	 * SCM.CONTROL_CAMERA_PHY_CTRL
+	 * - bit[4]    : CSIPHY1 data sent to CSIB
+	 * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2
+	 * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2
+	 */
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1)
+		cam_phy_ctrl |= 1 << 2;
+	else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1)
+		cam_phy_ctrl &= 1 << 2;
+
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
+		cam_phy_ctrl |= 1;
+	else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2)
+		cam_phy_ctrl &= 1;
+
+	omap_writel(cam_phy_ctrl,
+		    OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
+
+	csi2_ddrclk_khz = sensor_fmt->pixel_clock
+		/ (2 * csi2->phy->num_data_lanes)
+		* omap3isp_video_format_info(sensor_fmt->code)->bpp;
+	csi2phy.ths_term = THS_TERM(csi2_ddrclk_khz);
+	csi2phy.ths_settle = THS_SETTLE(csi2_ddrclk_khz);
+	csi2phy.tclk_term = TCLK_TERM;
+	csi2phy.tclk_miss = TCLK_MISS;
+	csi2phy.tclk_settle = TCLK_SETTLE;
 
 	/* Clock and data lanes verification */
-	for (i = 0; i < phy->num_data_lanes; i++) {
+	for (i = 0; i < csi2->phy->num_data_lanes; i++) {
 		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
 			return -EINVAL;
 
@@ -162,10 +239,10 @@ static int csiphy_config(struct isp_csiphy *phy,
 	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
 		return -EINVAL;
 
-	mutex_lock(&phy->mutex);
-	phy->dphy = *dphy;
-	phy->lanes = *lanes;
-	mutex_unlock(&phy->mutex);
+	mutex_lock(&csi2->phy->mutex);
+	csi2->phy->dphy = csi2phy;
+	csi2->phy->lanes = *lanes;
+	mutex_unlock(&csi2->phy->mutex);
 
 	return 0;
 }
@@ -225,8 +302,6 @@ int omap3isp_csiphy_init(struct isp_device *isp)
 	struct isp_csiphy *phy1 = &isp->isp_csiphy1;
 	struct isp_csiphy *phy2 = &isp->isp_csiphy2;
 
-	isp->platform_cb.csiphy_config = csiphy_config;
-
 	phy2->isp = isp;
 	phy2->csi2 = &isp->isp_csi2a;
 	phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES;
diff --git a/drivers/media/video/omap3isp/ispcsiphy.h b/drivers/media/video/omap3isp/ispcsiphy.h
index e93a661..9f93222 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.h
+++ b/drivers/media/video/omap3isp/ispcsiphy.h
@@ -56,6 +56,10 @@ struct isp_csiphy {
 	struct isp_csiphy_dphy_cfg dphy;
 };
 
+int omap3isp_csiphy_config(struct isp_device *isp,
+			   struct v4l2_subdev *csi2_subdev,
+			   struct v4l2_subdev *sensor,
+			   struct v4l2_mbus_framefmt *fmt);
 int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
 void omap3isp_csiphy_release(struct isp_csiphy *phy);
 int omap3isp_csiphy_init(struct isp_device *isp);
diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index 17bc03c..cdcf1d0 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -299,6 +299,8 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 
 	while (1) {
 		unsigned int shifter_link;
+		struct v4l2_subdev *_subdev;
+
 		/* Retrieve the sink format */
 		pad = &subdev->entity.pads[0];
 		if (!(pad->flags & MEDIA_PAD_FL_SINK))
@@ -342,6 +344,7 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 		if (media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			break;
 
+		_subdev = subdev;
 		subdev = media_entity_to_v4l2_subdev(pad->entity);
 
 		fmt_source.pad = pad->index;
@@ -355,6 +358,22 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 		    fmt_source.format.height != fmt_sink.format.height)
 			return -EPIPE;
 
+		/* Configure CSI-2 receiver based on sensor format. */
+		if (_subdev == &isp->isp_csi2a.subdev
+		    || _subdev == &isp->isp_csi2c.subdev) {
+			if (cpu_is_omap3630()) {
+				/*
+				 * FIXME: CSI-2 is supported only on
+				 * the 3630!
+				 */
+				ret = omap3isp_csiphy_config(
+					isp, _subdev, subdev,
+					&fmt_source.format);
+				if (IS_ERR_VALUE(ret))
+					return -EPIPE;
+			}
+		}
+
 		if (subdev->host_priv) {
 			/*
 			 * host_priv != NULL: this is a sensor. Issue
-- 
1.7.2.5

Re: [RFC 3/4] omap3isp: Configure CSI-2 phy based on platform data

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Thursday 15 December 2011 10:50:34 Sakari Ailus wrote:
> Configure CSI-2 phy based on platform data in the ISP driver rather than in
> platform code.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/omap3isp/isp.h       |    3 -
>  drivers/media/video/omap3isp/ispcsiphy.c |   95
> ++++++++++++++++++++++++++--- drivers/media/video/omap3isp/ispcsiphy.h |  
>  4 +
>  drivers/media/video/omap3isp/ispvideo.c  |   19 ++++++
>  4 files changed, 108 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/isp.h
> b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644
> --- a/drivers/media/video/omap3isp/isp.h
> +++ b/drivers/media/video/omap3isp/isp.h
> @@ -126,9 +126,6 @@ struct isp_reg {
> 
>  struct isp_platform_callback {
>  	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
> -	int (*csiphy_config)(struct isp_csiphy *phy,
> -			     struct isp_csiphy_dphy_cfg *dphy,
> -			     struct isp_csiphy_lanes_cfg *lanes);
>  	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
>  };
> 
> diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..52af308 100644
> --- a/drivers/media/video/omap3isp/ispcsiphy.c
> +++ b/drivers/media/video/omap3isp/ispcsiphy.c
> @@ -28,6 +28,8 @@
>  #include <linux/device.h>
>  #include <linux/regulator/consumer.h>
> 
> +#include "../../../../arch/arm/mach-omap2/control.h"
> +
>  #include "isp.h"
>  #include "ispreg.h"
>  #include "ispcsiphy.h"
> @@ -138,15 +140,90 @@ static void csiphy_dphy_config(struct isp_csiphy
> *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
>  }
> 
> -static int csiphy_config(struct isp_csiphy *phy,
> -			 struct isp_csiphy_dphy_cfg *dphy,
> -			 struct isp_csiphy_lanes_cfg *lanes)
> +/*
> + * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
> + * THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3.
> + */
> +#define THS_TERM_D 2000000
> +#define THS_TERM(ddrclk_khz)					\
> +(								\
> +	((25 * (ddrclk_khz)) % THS_TERM_D) ?			\
> +		((25 * (ddrclk_khz)) / THS_TERM_D) :		\
> +		((25 * (ddrclk_khz)) / THS_TERM_D) - 1		\
> +)
> +
> +#define THS_SETTLE_D 1000000
> +#define THS_SETTLE(ddrclk_khz)					\
> +(								\
> +	((90 * (ddrclk_khz)) % THS_SETTLE_D) ?			\
> +		((90 * (ddrclk_khz)) / THS_SETTLE_D) + 4 :	\
> +		((90 * (ddrclk_khz)) / THS_SETTLE_D) + 3	\
> +)

The THS_TERM and THS_SETTLE macros are only used once. I would just put that 
code explictly where it gets used. The macros hinder readability.

> +
> +/*
> + * TCLK values are OK at their reset values
> + */
> +#define TCLK_TERM	0
> +#define TCLK_MISS	1
> +#define TCLK_SETTLE	14
> +
> +int omap3isp_csiphy_config(struct isp_device *isp,
> +			   struct v4l2_subdev *csi2_subdev,
> +			   struct v4l2_subdev *sensor,
> +			   struct v4l2_mbus_framefmt *sensor_fmt)

The number of lanes can depend on the format. Wouldn't it be better to add a 
subdev operation to query the sensor for its bus configuration instead of 
relying on ISP platform data ?

>  {
> +	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
> +	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
> +	struct isp_csiphy_dphy_cfg csi2phy;
> +	int csi2_ddrclk_khz;
> +	struct isp_csiphy_lanes_cfg *lanes;
>  	unsigned int used_lanes = 0;
>  	unsigned int i;
> +	u32 cam_phy_ctrl;
> +
> +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
> +	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> +		lanes = subdevs->bus.ccp2.lanecfg;
> +	else
> +		lanes = subdevs->bus.csi2.lanecfg;
> +
> +	if (!lanes) {
> +		dev_err(isp->dev, "no lane configuration\n");
> +		return -EINVAL;
> +	}
> +
> +	cam_phy_ctrl = omap_readl(
> +		OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> +	/*
> +	 * SCM.CONTROL_CAMERA_PHY_CTRL
> +	 * - bit[4]    : CSIPHY1 data sent to CSIB
> +	 * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2
> +	 * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2
> +	 */
> +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1)
> +		cam_phy_ctrl |= 1 << 2;
> +	else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1)
> +		cam_phy_ctrl &= 1 << 2;
> +
> +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> +		cam_phy_ctrl |= 1;
> +	else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2)
> +		cam_phy_ctrl &= 1;
> +
> +	omap_writel(cam_phy_ctrl,
> +		    OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> +
> +	csi2_ddrclk_khz = sensor_fmt->pixel_clock
> +		/ (2 * csi2->phy->num_data_lanes)
> +		* omap3isp_video_format_info(sensor_fmt->code)->bpp;
> +	csi2phy.ths_term = THS_TERM(csi2_ddrclk_khz);
> +	csi2phy.ths_settle = THS_SETTLE(csi2_ddrclk_khz);
> +	csi2phy.tclk_term = TCLK_TERM;
> +	csi2phy.tclk_miss = TCLK_MISS;
> +	csi2phy.tclk_settle = TCLK_SETTLE;
> 
>  	/* Clock and data lanes verification */
> -	for (i = 0; i < phy->num_data_lanes; i++) {
> +	for (i = 0; i < csi2->phy->num_data_lanes; i++) {
>  		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
>  			return -EINVAL;
> 
> @@ -162,10 +239,10 @@ static int csiphy_config(struct isp_csiphy *phy,
>  	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
>  		return -EINVAL;
> 
> -	mutex_lock(&phy->mutex);
> -	phy->dphy = *dphy;
> -	phy->lanes = *lanes;
> -	mutex_unlock(&phy->mutex);
> +	mutex_lock(&csi2->phy->mutex);
> +	csi2->phy->dphy = csi2phy;
> +	csi2->phy->lanes = *lanes;
> +	mutex_unlock(&csi2->phy->mutex);
> 
>  	return 0;
>  }
> @@ -225,8 +302,6 @@ int omap3isp_csiphy_init(struct isp_device *isp)
>  	struct isp_csiphy *phy1 = &isp->isp_csiphy1;
>  	struct isp_csiphy *phy2 = &isp->isp_csiphy2;
> 
> -	isp->platform_cb.csiphy_config = csiphy_config;
> -
>  	phy2->isp = isp;
>  	phy2->csi2 = &isp->isp_csi2a;
>  	phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES;
> diff --git a/drivers/media/video/omap3isp/ispcsiphy.h
> b/drivers/media/video/omap3isp/ispcsiphy.h index e93a661..9f93222 100644
> --- a/drivers/media/video/omap3isp/ispcsiphy.h
> +++ b/drivers/media/video/omap3isp/ispcsiphy.h
> @@ -56,6 +56,10 @@ struct isp_csiphy {
>  	struct isp_csiphy_dphy_cfg dphy;
>  };
> 
> +int omap3isp_csiphy_config(struct isp_device *isp,
> +			   struct v4l2_subdev *csi2_subdev,
> +			   struct v4l2_subdev *sensor,
> +			   struct v4l2_mbus_framefmt *fmt);
>  int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
>  void omap3isp_csiphy_release(struct isp_csiphy *phy);
>  int omap3isp_csiphy_init(struct isp_device *isp);
> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> b/drivers/media/video/omap3isp/ispvideo.c index 17bc03c..cdcf1d0 100644
> --- a/drivers/media/video/omap3isp/ispvideo.c
> +++ b/drivers/media/video/omap3isp/ispvideo.c
> @@ -299,6 +299,8 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe)
> 
>  	while (1) {
>  		unsigned int shifter_link;
> +		struct v4l2_subdev *_subdev;

What about a more descriptive name ?

> +
>  		/* Retrieve the sink format */
>  		pad = &subdev->entity.pads[0];
>  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> @@ -342,6 +344,7 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) if (media_entity_type(pad->entity) !=
> MEDIA_ENT_T_V4L2_SUBDEV)
>  			break;
> 
> +		_subdev = subdev;
>  		subdev = media_entity_to_v4l2_subdev(pad->entity);
> 
>  		fmt_source.pad = pad->index;
> @@ -355,6 +358,22 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
>  			return -EPIPE;
> 
> +		/* Configure CSI-2 receiver based on sensor format. */
> +		if (_subdev == &isp->isp_csi2a.subdev
> +		    || _subdev == &isp->isp_csi2c.subdev) {
> +			if (cpu_is_omap3630()) {
> +				/*
> +				 * FIXME: CSI-2 is supported only on
> +				 * the 3630!
> +				 */

Is it ? Or do you mean by the driver ? What would it take to support it on 
OMAP34xx and OMAP35xx ?

> +				ret = omap3isp_csiphy_config(
> +					isp, _subdev, subdev,
> +					&fmt_source.format);
> +				if (IS_ERR_VALUE(ret))
> +					return -EPIPE;
> +			}
> +		}

This isn't really pipeline validation, is it ? Should this be performed in 
isp_pipeline_enable() instead ?

> +
>  		if (subdev->host_priv) {
>  			/*
>  			 * host_priv != NULL: this is a sensor. Issue

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/4] omap3isp: Configure CSI-2 phy based on platform data

[Sakari Ailus]

Hi Laurent,

Thanks for the review!

On Thu, Dec 15, 2011 at 11:28:06AM +0100, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.
> 
> On Thursday 15 December 2011 10:50:34 Sakari Ailus wrote:
> > Configure CSI-2 phy based on platform data in the ISP driver rather than in
> > platform code.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  drivers/media/video/omap3isp/isp.h       |    3 -
> >  drivers/media/video/omap3isp/ispcsiphy.c |   95
> > ++++++++++++++++++++++++++--- drivers/media/video/omap3isp/ispcsiphy.h |  
> >  4 +
> >  drivers/media/video/omap3isp/ispvideo.c  |   19 ++++++
> >  4 files changed, 108 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/video/omap3isp/isp.h
> > b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644
> > --- a/drivers/media/video/omap3isp/isp.h
> > +++ b/drivers/media/video/omap3isp/isp.h
> > @@ -126,9 +126,6 @@ struct isp_reg {
> > 
> >  struct isp_platform_callback {
> >  	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
> > -	int (*csiphy_config)(struct isp_csiphy *phy,
> > -			     struct isp_csiphy_dphy_cfg *dphy,
> > -			     struct isp_csiphy_lanes_cfg *lanes);
> >  	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
> >  };
> > 
> > diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> > b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..52af308 100644
> > --- a/drivers/media/video/omap3isp/ispcsiphy.c
> > +++ b/drivers/media/video/omap3isp/ispcsiphy.c
> > @@ -28,6 +28,8 @@
> >  #include <linux/device.h>
> >  #include <linux/regulator/consumer.h>
> > 
> > +#include "../../../../arch/arm/mach-omap2/control.h"
> > +
> >  #include "isp.h"
> >  #include "ispreg.h"
> >  #include "ispcsiphy.h"
> > @@ -138,15 +140,90 @@ static void csiphy_dphy_config(struct isp_csiphy
> > *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> >  }
> > 
> > -static int csiphy_config(struct isp_csiphy *phy,
> > -			 struct isp_csiphy_dphy_cfg *dphy,
> > -			 struct isp_csiphy_lanes_cfg *lanes)
> > +/*
> > + * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
> > + * THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3.
> > + */
> > +#define THS_TERM_D 2000000
> > +#define THS_TERM(ddrclk_khz)					\
> > +(								\
> > +	((25 * (ddrclk_khz)) % THS_TERM_D) ?			\
> > +		((25 * (ddrclk_khz)) / THS_TERM_D) :		\
> > +		((25 * (ddrclk_khz)) / THS_TERM_D) - 1		\
> > +)
> > +
> > +#define THS_SETTLE_D 1000000
> > +#define THS_SETTLE(ddrclk_khz)					\
> > +(								\
> > +	((90 * (ddrclk_khz)) % THS_SETTLE_D) ?			\
> > +		((90 * (ddrclk_khz)) / THS_SETTLE_D) + 4 :	\
> > +		((90 * (ddrclk_khz)) / THS_SETTLE_D) + 3	\
> > +)
> 
> The THS_TERM and THS_SETTLE macros are only used once. I would just put that 
> code explictly where it gets used. The macros hinder readability.

I'll do that.

> > +
> > +/*
> > + * TCLK values are OK at their reset values
> > + */
> > +#define TCLK_TERM	0
> > +#define TCLK_MISS	1
> > +#define TCLK_SETTLE	14
> > +
> > +int omap3isp_csiphy_config(struct isp_device *isp,
> > +			   struct v4l2_subdev *csi2_subdev,
> > +			   struct v4l2_subdev *sensor,
> > +			   struct v4l2_mbus_framefmt *sensor_fmt)
> 
> The number of lanes can depend on the format. Wouldn't it be better to add a 
> subdev operation to query the sensor for its bus configuration instead of 
> relying on ISP platform data ?

In principle, yes. That's an interesting point; how this kind of information
would best be delivered?

On the other hand I don't see any pressing reason to use less lanes than the
maximum, so this could wait IMHO.

Perhaps around the time we standardise how the CSI-2 configuration is being
done? It's not quite as simple as the mbus_config seems to assume. For
example, the lane mapping and then which lanes do you use if you're using
less than the maximum has to be handled in a way or another.

The number of lanes might be something the user would want to touch, but I'm
not entirely sure. You achieve more functionality by providing that
flexibility to the user but I don't see need for configuring that --- still
getting the number of lanes could be interesting.

> >  {
> > +	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
> > +	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
> > +	struct isp_csiphy_dphy_cfg csi2phy;
> > +	int csi2_ddrclk_khz;
> > +	struct isp_csiphy_lanes_cfg *lanes;
> >  	unsigned int used_lanes = 0;
> >  	unsigned int i;
> > +	u32 cam_phy_ctrl;
> > +
> > +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
> > +	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> > +		lanes = subdevs->bus.ccp2.lanecfg;
> > +	else
> > +		lanes = subdevs->bus.csi2.lanecfg;
> > +
> > +	if (!lanes) {
> > +		dev_err(isp->dev, "no lane configuration\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	cam_phy_ctrl = omap_readl(
> > +		OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> > +	/*
> > +	 * SCM.CONTROL_CAMERA_PHY_CTRL
> > +	 * - bit[4]    : CSIPHY1 data sent to CSIB
> > +	 * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2
> > +	 * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2
> > +	 */
> > +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1)
> > +		cam_phy_ctrl |= 1 << 2;
> > +	else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1)
> > +		cam_phy_ctrl &= 1 << 2;
> > +
> > +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> > +		cam_phy_ctrl |= 1;
> > +	else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2)
> > +		cam_phy_ctrl &= 1;
> > +
> > +	omap_writel(cam_phy_ctrl,
> > +		    OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> > +
> > +	csi2_ddrclk_khz = sensor_fmt->pixel_clock
> > +		/ (2 * csi2->phy->num_data_lanes)
> > +		* omap3isp_video_format_info(sensor_fmt->code)->bpp;
> > +	csi2phy.ths_term = THS_TERM(csi2_ddrclk_khz);
> > +	csi2phy.ths_settle = THS_SETTLE(csi2_ddrclk_khz);
> > +	csi2phy.tclk_term = TCLK_TERM;
> > +	csi2phy.tclk_miss = TCLK_MISS;
> > +	csi2phy.tclk_settle = TCLK_SETTLE;
> > 
> >  	/* Clock and data lanes verification */
> > -	for (i = 0; i < phy->num_data_lanes; i++) {
> > +	for (i = 0; i < csi2->phy->num_data_lanes; i++) {
> >  		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
> >  			return -EINVAL;
> > 
> > @@ -162,10 +239,10 @@ static int csiphy_config(struct isp_csiphy *phy,
> >  	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
> >  		return -EINVAL;
> > 
> > -	mutex_lock(&phy->mutex);
> > -	phy->dphy = *dphy;
> > -	phy->lanes = *lanes;
> > -	mutex_unlock(&phy->mutex);
> > +	mutex_lock(&csi2->phy->mutex);
> > +	csi2->phy->dphy = csi2phy;
> > +	csi2->phy->lanes = *lanes;
> > +	mutex_unlock(&csi2->phy->mutex);
> > 
> >  	return 0;
> >  }
> > @@ -225,8 +302,6 @@ int omap3isp_csiphy_init(struct isp_device *isp)
> >  	struct isp_csiphy *phy1 = &isp->isp_csiphy1;
> >  	struct isp_csiphy *phy2 = &isp->isp_csiphy2;
> > 
> > -	isp->platform_cb.csiphy_config = csiphy_config;
> > -
> >  	phy2->isp = isp;
> >  	phy2->csi2 = &isp->isp_csi2a;
> >  	phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES;
> > diff --git a/drivers/media/video/omap3isp/ispcsiphy.h
> > b/drivers/media/video/omap3isp/ispcsiphy.h index e93a661..9f93222 100644
> > --- a/drivers/media/video/omap3isp/ispcsiphy.h
> > +++ b/drivers/media/video/omap3isp/ispcsiphy.h
> > @@ -56,6 +56,10 @@ struct isp_csiphy {
> >  	struct isp_csiphy_dphy_cfg dphy;
> >  };
> > 
> > +int omap3isp_csiphy_config(struct isp_device *isp,
> > +			   struct v4l2_subdev *csi2_subdev,
> > +			   struct v4l2_subdev *sensor,
> > +			   struct v4l2_mbus_framefmt *fmt);
> >  int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
> >  void omap3isp_csiphy_release(struct isp_csiphy *phy);
> >  int omap3isp_csiphy_init(struct isp_device *isp);
> > diff --git a/drivers/media/video/omap3isp/ispvideo.c
> > b/drivers/media/video/omap3isp/ispvideo.c index 17bc03c..cdcf1d0 100644
> > --- a/drivers/media/video/omap3isp/ispvideo.c
> > +++ b/drivers/media/video/omap3isp/ispvideo.c
> > @@ -299,6 +299,8 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe)
> > 
> >  	while (1) {
> >  		unsigned int shifter_link;
> > +		struct v4l2_subdev *_subdev;
> 
> What about a more descriptive name ?

Ack.

> > +
> >  		/* Retrieve the sink format */
> >  		pad = &subdev->entity.pads[0];
> >  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > @@ -342,6 +344,7 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe) if (media_entity_type(pad->entity) !=
> > MEDIA_ENT_T_V4L2_SUBDEV)
> >  			break;
> > 
> > +		_subdev = subdev;
> >  		subdev = media_entity_to_v4l2_subdev(pad->entity);
> > 
> >  		fmt_source.pad = pad->index;
> > @@ -355,6 +358,22 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
> >  			return -EPIPE;
> > 
> > +		/* Configure CSI-2 receiver based on sensor format. */
> > +		if (_subdev == &isp->isp_csi2a.subdev
> > +		    || _subdev == &isp->isp_csi2c.subdev) {
> > +			if (cpu_is_omap3630()) {
> > +				/*
> > +				 * FIXME: CSI-2 is supported only on
> > +				 * the 3630!
> > +				 */
> 
> Is it ? Or do you mean by the driver ? What would it take to support it on 
> OMAP34xx and OMAP35xx ?

I have no way to test it on the OMAP 3430 since I have no CSI-2 sensor
connected to it. As a matter of fact I've never had one, so I don't really
know.

> > +				ret = omap3isp_csiphy_config(
> > +					isp, _subdev, subdev,
> > +					&fmt_source.format);
> > +				if (IS_ERR_VALUE(ret))
> > +					return -EPIPE;
> > +			}
> > +		}
> 
> This isn't really pipeline validation, is it ? Should this be performed in 
> isp_pipeline_enable() instead ?

I'll move it there.

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

Re: [RFC 3/4] omap3isp: Configure CSI-2 phy based on platform data

[Laurent Pinchart]

Hi Sakari,

On Thursday 15 December 2011 12:53:03 Sakari Ailus wrote:
> On Thu, Dec 15, 2011 at 11:28:06AM +0100, Laurent Pinchart wrote:
> > On Thursday 15 December 2011 10:50:34 Sakari Ailus wrote:
> > > Configure CSI-2 phy based on platform data in the ISP driver rather
> > > than in platform code.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

[snip]

> > > diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> > > b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..52af308
> > > 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c
> > > +++ b/drivers/media/video/omap3isp/ispcsiphy.c
> > > @@ -28,6 +28,8 @@

[snip]

> > > +int omap3isp_csiphy_config(struct isp_device *isp,
> > > +			   struct v4l2_subdev *csi2_subdev,
> > > +			   struct v4l2_subdev *sensor,
> > > +			   struct v4l2_mbus_framefmt *sensor_fmt)
> > 
> > The number of lanes can depend on the format. Wouldn't it be better to
> > add a subdev operation to query the sensor for its bus configuration
> > instead of relying on ISP platform data ?
> 
> In principle, yes. That's an interesting point; how this kind of information
> would best be delivered?

There are two separate information that need to be delivered:

- how the lanes are connected on the board
- which lanes are used by the sensor, and for what purpose

The first information must be supplied through platform data, either to the 
sensor driver or the OMAP3 ISP driver (or both). As the second information 
comes from the sensor, my idea was to provide the first to the sensor, and to 
query the sensor in the OMAP3 ISP driver for the full configuration.

> On the other hand I don't see any pressing reason to use less lanes than
> the maximum, so this could wait IMHO.
> 
> Perhaps around the time we standardise how the CSI-2 configuration is being
> done? It's not quite as simple as the mbus_config seems to assume. For
> example, the lane mapping and then which lanes do you use if you're using
> less than the maximum has to be handled in a way or another.

I agree. That's why I don't really like mbus_config, its auto-negotiation 
support approach makes it too limited in my opinion.

> The number of lanes might be something the user would want to touch, but
> I'm not entirely sure. You achieve more functionality by providing that
> flexibility to the user but I don't see need for configuring that ---
> still getting the number of lanes could be interesting.

If we want to expose such configuration I think we should do it on the sensor, 
not the ISP.

> > >  {
> > > 
> > > +	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
> > > +	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
> > > +	struct isp_csiphy_dphy_cfg csi2phy;
> > > +	int csi2_ddrclk_khz;
> > > +	struct isp_csiphy_lanes_cfg *lanes;
> > > 
> > >  	unsigned int used_lanes = 0;
> > >  	unsigned int i;
> > > 
> > > +	u32 cam_phy_ctrl;
> > > +
> > > +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
> > > +	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> > > +		lanes = subdevs->bus.ccp2.lanecfg;
> > > +	else
> > > +		lanes = subdevs->bus.csi2.lanecfg;
> > > +
> > > +	if (!lanes) {
> > > +		dev_err(isp->dev, "no lane configuration\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	cam_phy_ctrl = omap_readl(
> > > +		OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> > > +	/*
> > > +	 * SCM.CONTROL_CAMERA_PHY_CTRL
> > > +	 * - bit[4]    : CSIPHY1 data sent to CSIB
> > > +	 * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2
> > > +	 * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2
> > > +	 */
> > > +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1)
> > > +		cam_phy_ctrl |= 1 << 2;
> > > +	else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1)
> > > +		cam_phy_ctrl &= 1 << 2;
> > > +
> > > +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> > > +		cam_phy_ctrl |= 1;
> > > +	else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2)
> > > +		cam_phy_ctrl &= 1;
> > > +
> > > +	omap_writel(cam_phy_ctrl,
> > > +		    OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> > > +
> > > +	csi2_ddrclk_khz = sensor_fmt->pixel_clock
> > > +		/ (2 * csi2->phy->num_data_lanes)
> > > +		* omap3isp_video_format_info(sensor_fmt->code)->bpp;
> > > +	csi2phy.ths_term = THS_TERM(csi2_ddrclk_khz);
> > > +	csi2phy.ths_settle = THS_SETTLE(csi2_ddrclk_khz);
> > > +	csi2phy.tclk_term = TCLK_TERM;
> > > +	csi2phy.tclk_miss = TCLK_MISS;
> > > +	csi2phy.tclk_settle = TCLK_SETTLE;
> > > 
> > >  	/* Clock and data lanes verification */
> > > 
> > > -	for (i = 0; i < phy->num_data_lanes; i++) {
> > > +	for (i = 0; i < csi2->phy->num_data_lanes; i++) {
> > > 
> > >  		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
> > >  		
> > >  			return -EINVAL;
> > > 
> > > @@ -162,10 +239,10 @@ static int csiphy_config(struct isp_csiphy *phy,
> > > 
> > >  	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
> > >  	
> > >  		return -EINVAL;
> > > 
> > > -	mutex_lock(&phy->mutex);
> > > -	phy->dphy = *dphy;
> > > -	phy->lanes = *lanes;
> > > -	mutex_unlock(&phy->mutex);
> > > +	mutex_lock(&csi2->phy->mutex);
> > > +	csi2->phy->dphy = csi2phy;
> > > +	csi2->phy->lanes = *lanes;
> > > +	mutex_unlock(&csi2->phy->mutex);
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > @@ -225,8 +302,6 @@ int omap3isp_csiphy_init(struct isp_device *isp)
> > > 
> > >  	struct isp_csiphy *phy1 = &isp->isp_csiphy1;
> > >  	struct isp_csiphy *phy2 = &isp->isp_csiphy2;
> > > 
> > > -	isp->platform_cb.csiphy_config = csiphy_config;
> > > -
> > > 
> > >  	phy2->isp = isp;
> > >  	phy2->csi2 = &isp->isp_csi2a;
> > >  	phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES;
> > > 
> > > diff --git a/drivers/media/video/omap3isp/ispcsiphy.h
> > > b/drivers/media/video/omap3isp/ispcsiphy.h index e93a661..9f93222
> > > 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.h
> > > +++ b/drivers/media/video/omap3isp/ispcsiphy.h
> > > @@ -56,6 +56,10 @@ struct isp_csiphy {
> > > 
> > >  	struct isp_csiphy_dphy_cfg dphy;
> > >  
> > >  };
> > > 
> > > +int omap3isp_csiphy_config(struct isp_device *isp,
> > > +			   struct v4l2_subdev *csi2_subdev,
> > > +			   struct v4l2_subdev *sensor,
> > > +			   struct v4l2_mbus_framefmt *fmt);
> > > 
> > >  int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
> > >  void omap3isp_csiphy_release(struct isp_csiphy *phy);
> > >  int omap3isp_csiphy_init(struct isp_device *isp);
> > > 
> > > diff --git a/drivers/media/video/omap3isp/ispvideo.c
> > > b/drivers/media/video/omap3isp/ispvideo.c index 17bc03c..cdcf1d0 100644
> > > --- a/drivers/media/video/omap3isp/ispvideo.c
> > > +++ b/drivers/media/video/omap3isp/ispvideo.c
> > > @@ -299,6 +299,8 @@ static int isp_video_validate_pipeline(struct
> > > isp_pipeline *pipe)
> > > 
> > >  	while (1) {
> > >  	
> > >  		unsigned int shifter_link;
> > > 
> > > +		struct v4l2_subdev *_subdev;
> > 
> > What about a more descriptive name ?
> 
> Ack.
> 
> > > +
> > > 
> > >  		/* Retrieve the sink format */
> > >  		pad = &subdev->entity.pads[0];
> > >  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > > 
> > > @@ -342,6 +344,7 @@ static int isp_video_validate_pipeline(struct
> > > isp_pipeline *pipe) if (media_entity_type(pad->entity) !=
> > > MEDIA_ENT_T_V4L2_SUBDEV)
> > > 
> > >  			break;
> > > 
> > > +		_subdev = subdev;
> > > 
> > >  		subdev = media_entity_to_v4l2_subdev(pad->entity);
> > >  		
> > >  		fmt_source.pad = pad->index;
> > > 
> > > @@ -355,6 +358,22 @@ static int isp_video_validate_pipeline(struct
> > > isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
> > > 
> > >  			return -EPIPE;
> > > 
> > > +		/* Configure CSI-2 receiver based on sensor format. */
> > > +		if (_subdev == &isp->isp_csi2a.subdev
> > > +		    || _subdev == &isp->isp_csi2c.subdev) {
> > > +			if (cpu_is_omap3630()) {
> > > +				/*
> > > +				 * FIXME: CSI-2 is supported only on
> > > +				 * the 3630!
> > > +				 */
> > 
> > Is it ? Or do you mean by the driver ? What would it take to support it
> > on OMAP34xx and OMAP35xx ?
> 
> I have no way to test it on the OMAP 3430 since I have no CSI-2 sensor
> connected to it. As a matter of fact I've never had one, so I don't really
> know.

What about assuming it works on the 34xx and 35xx as well ?

> > > +				ret = omap3isp_csiphy_config(
> > > +					isp, _subdev, subdev,
> > > +					&fmt_source.format);
> > > +				if (IS_ERR_VALUE(ret))
> > > +					return -EPIPE;
> > > +			}
> > > +		}
> > 
> > This isn't really pipeline validation, is it ? Should this be performed
> > in isp_pipeline_enable() instead ?
> 
> I'll move it there.

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/4] omap3isp: Configure CSI-2 phy based on platform data

[Sakari Ailus]

Hi Laurent,

On Thu, Dec 15, 2011 at 01:54:52PM +0100, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thursday 15 December 2011 12:53:03 Sakari Ailus wrote:
> > On Thu, Dec 15, 2011 at 11:28:06AM +0100, Laurent Pinchart wrote:
> > > On Thursday 15 December 2011 10:50:34 Sakari Ailus wrote:
> > > > Configure CSI-2 phy based on platform data in the ISP driver rather
> > > > than in platform code.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> [snip]
> 
> > > > diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..52af308
> > > > 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > +++ b/drivers/media/video/omap3isp/ispcsiphy.c
> > > > @@ -28,6 +28,8 @@
> 
> [snip]
> 
> > > > +int omap3isp_csiphy_config(struct isp_device *isp,
> > > > +			   struct v4l2_subdev *csi2_subdev,
> > > > +			   struct v4l2_subdev *sensor,
> > > > +			   struct v4l2_mbus_framefmt *sensor_fmt)
> > > 
> > > The number of lanes can depend on the format. Wouldn't it be better to
> > > add a subdev operation to query the sensor for its bus configuration
> > > instead of relying on ISP platform data ?
> > 
> > In principle, yes. That's an interesting point; how this kind of information
> > would best be delivered?
> 
> There are two separate information that need to be delivered:
> 
> - how the lanes are connected on the board
> - which lanes are used by the sensor, and for what purpose
> 
> The first information must be supplied through platform data, either to the 
> sensor driver or the OMAP3 ISP driver (or both). As the second information 

Both, and both of them may require configuring it. I don't know sensors that
allow it, but the CSI-2 receiver is flexible in lane mapping.

> comes from the sensor, my idea was to provide the first to the sensor, and to 
> query the sensor in the OMAP3 ISP driver for the full configuration.

In theory, at least, configurations with less lanes need to be specified
separately. There may be limitations on how the lanes can be used, say,
using two out of three lanes may require leaving a aparticular lane unused.

> > The number of lanes might be something the user would want to touch, but
> > I'm not entirely sure. You achieve more functionality by providing that
> > flexibility to the user but I don't see need for configuring that ---
> > still getting the number of lanes could be interesting.
> 
> If we want to expose such configuration I think we should do it on the sensor, 
> not the ISP.

I agree.

...

> > > > @@ -355,6 +358,22 @@ static int isp_video_validate_pipeline(struct
> > > > isp_pipeline *pipe) fmt_source.format.height != fmt_sink.format.height)
> > > > 
> > > >  			return -EPIPE;
> > > > 
> > > > +		/* Configure CSI-2 receiver based on sensor format. */
> > > > +		if (_subdev == &isp->isp_csi2a.subdev
> > > > +		    || _subdev == &isp->isp_csi2c.subdev) {
> > > > +			if (cpu_is_omap3630()) {
> > > > +				/*
> > > > +				 * FIXME: CSI-2 is supported only on
> > > > +				 * the 3630!
> > > > +				 */
> > > 
> > > Is it ? Or do you mean by the driver ? What would it take to support it
> > > on OMAP34xx and OMAP35xx ?
> > 
> > I have no way to test it on the OMAP 3430 since I have no CSI-2 sensor
> > connected to it. As a matter of fact I've never had one, so I don't really
> > know.
> 
> What about assuming it works on the 34xx and 35xx as well ?

I'll change it so.

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

Re: [RFC 3/4] omap3isp: Configure CSI-2 phy based on platform data

[Laurent Pinchart]

On Thursday 15 December 2011 22:50:33 Sakari Ailus wrote:
> On Thu, Dec 15, 2011 at 01:54:52PM +0100, Laurent Pinchart wrote:
> > On Thursday 15 December 2011 12:53:03 Sakari Ailus wrote:
> > > On Thu, Dec 15, 2011 at 11:28:06AM +0100, Laurent Pinchart wrote:
> > > > On Thursday 15 December 2011 10:50:34 Sakari Ailus wrote:
> > > > > Configure CSI-2 phy based on platform data in the ISP driver rather
> > > > > than in platform code.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > [snip]
> > 
> > > > > diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > > b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..52af308
> > > > > 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > > +++ b/drivers/media/video/omap3isp/ispcsiphy.c
> > > > > @@ -28,6 +28,8 @@
> > 
> > [snip]
> > 
> > > > > +int omap3isp_csiphy_config(struct isp_device *isp,
> > > > > +			   struct v4l2_subdev *csi2_subdev,
> > > > > +			   struct v4l2_subdev *sensor,
> > > > > +			   struct v4l2_mbus_framefmt *sensor_fmt)
> > > > 
> > > > The number of lanes can depend on the format. Wouldn't it be better
> > > > to add a subdev operation to query the sensor for its bus
> > > > configuration instead of relying on ISP platform data ?
> > > 
> > > In principle, yes. That's an interesting point; how this kind of
> > > information would best be delivered?
> > 
> > There are two separate information that need to be delivered:
> > 
> > - how the lanes are connected on the board
> > - which lanes are used by the sensor, and for what purpose
> > 
> > The first information must be supplied through platform data, either to
> > the sensor driver or the OMAP3 ISP driver (or both). As the second
> > information
> 
> Both, and both of them may require configuring it. I don't know sensors
> that allow it, but the CSI-2 receiver is flexible in lane mapping.
> 
> > comes from the sensor, my idea was to provide the first to the sensor,
> > and to query the sensor in the OMAP3 ISP driver for the full
> > configuration.
> 
> In theory, at least, configurations with less lanes need to be specified
> separately. There may be limitations on how the lanes can be used, say,
> using two out of three lanes may require leaving a aparticular lane unused.

In theory, I fully agree. This brings additional complexity for use cases that 
might never exist though. Do you think we need to support it from the very 
beginning ?

> > > The number of lanes might be something the user would want to touch,
> > > but I'm not entirely sure. You achieve more functionality by providing
> > > that flexibility to the user but I don't see need for configuring that
> > > --- still getting the number of lanes could be interesting.
> > 
> > If we want to expose such configuration I think we should do it on the
> > sensor, not the ISP.
> 
> I agree.
> 
> ...
> 
> > > > > @@ -355,6 +358,22 @@ static int isp_video_validate_pipeline(struct
> > > > > isp_pipeline *pipe) fmt_source.format.height !=
> > > > > fmt_sink.format.height)
> > > > > 
> > > > >  			return -EPIPE;
> > > > > 
> > > > > +		/* Configure CSI-2 receiver based on sensor format. */
> > > > > +		if (_subdev == &isp->isp_csi2a.subdev
> > > > > +		    || _subdev == &isp->isp_csi2c.subdev) {
> > > > > +			if (cpu_is_omap3630()) {
> > > > > +				/*
> > > > > +				 * FIXME: CSI-2 is supported only on
> > > > > +				 * the 3630!
> > > > > +				 */
> > > > 
> > > > Is it ? Or do you mean by the driver ? What would it take to support
> > > > it on OMAP34xx and OMAP35xx ?
> > > 
> > > I have no way to test it on the OMAP 3430 since I have no CSI-2 sensor
> > > connected to it. As a matter of fact I've never had one, so I don't
> > > really know.
> > 
> > What about assuming it works on the 34xx and 35xx as well ?
> 
> I'll change it so.

Thanks

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/4] omap3isp: Configure CSI-2 phy based on platform data

[Sakari Ailus]

Hi Laurent,

On Mon, Dec 19, 2011 at 01:31:09AM +0100, Laurent Pinchart wrote:
> On Thursday 15 December 2011 22:50:33 Sakari Ailus wrote:
> > On Thu, Dec 15, 2011 at 01:54:52PM +0100, Laurent Pinchart wrote:
> > > On Thursday 15 December 2011 12:53:03 Sakari Ailus wrote:
> > > > On Thu, Dec 15, 2011 at 11:28:06AM +0100, Laurent Pinchart wrote:
> > > > > On Thursday 15 December 2011 10:50:34 Sakari Ailus wrote:
> > > > > > Configure CSI-2 phy based on platform data in the ISP driver rather
> > > > > > than in platform code.
> > > > > > 
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > 
> > > [snip]
> > > 
> > > > > > diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > > > b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..52af308
> > > > > > 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > > > +++ b/drivers/media/video/omap3isp/ispcsiphy.c
> > > > > > @@ -28,6 +28,8 @@
> > > 
> > > [snip]
> > > 
> > > > > > +int omap3isp_csiphy_config(struct isp_device *isp,
> > > > > > +			   struct v4l2_subdev *csi2_subdev,
> > > > > > +			   struct v4l2_subdev *sensor,
> > > > > > +			   struct v4l2_mbus_framefmt *sensor_fmt)
> > > > > 
> > > > > The number of lanes can depend on the format. Wouldn't it be better
> > > > > to add a subdev operation to query the sensor for its bus
> > > > > configuration instead of relying on ISP platform data ?
> > > > 
> > > > In principle, yes. That's an interesting point; how this kind of
> > > > information would best be delivered?
> > > 
> > > There are two separate information that need to be delivered:
> > > 
> > > - how the lanes are connected on the board
> > > - which lanes are used by the sensor, and for what purpose
> > > 
> > > The first information must be supplied through platform data, either to
> > > the sensor driver or the OMAP3 ISP driver (or both). As the second
> > > information
> > 
> > Both, and both of them may require configuring it. I don't know sensors
> > that allow it, but the CSI-2 receiver is flexible in lane mapping.
> > 
> > > comes from the sensor, my idea was to provide the first to the sensor,
> > > and to query the sensor in the OMAP3 ISP driver for the full
> > > configuration.
> > 
> > In theory, at least, configurations with less lanes need to be specified
> > separately. There may be limitations on how the lanes can be used, say,
> > using two out of three lanes may require leaving a aparticular lane unused.
> 
> In theory, I fully agree. This brings additional complexity for use cases that 
> might never exist though. Do you think we need to support it from the very 
> beginning ?

I don't see need for it, at least not for the time being. Still we might
want to show the number of lanes to the user as a read-onlycontrol, but that
could wait as well IMO.

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

query video dev node name using the V4L2 device driver name

[Zhu, Mingcheng]

Hi Laurent and Sakari,

Current media entity contains a few fields to identify a dev node (name, type, group_id). The entity name is the v4l2 dev node name such as "/dev/video0" "/dev/video1". There is no information who is "/dev/video0" and who is /dev/video1". This makes that, after query the media_entity the application still could not figure out who is /dev/video1".

However in V4L2 devices, there is a driver name that the vendor can assign a specific name such "WIFI CAPTURE" or BACK_CAMERA" to the driver name. Is it possible to add the driver name into the media_entity? This makes that, if the userspace application knows the driver name it can use the driver name to find the dev node.


Thanks,

Mingcheng

Re: query video dev node name using the V4L2 device driver name

[Laurent Pinchart]

Hi Mingcheng,

On Monday 19 December 2011 11:21:03 Zhu, Mingcheng wrote:
> Hi Laurent and Sakari,
> 
> Current media entity contains a few fields to identify a dev node (name,
> type, group_id). The entity name is the v4l2 dev node name such as
> "/dev/video0" "/dev/video1". There is no information who is "/dev/video0"
> and who is /dev/video1". This makes that, after query the media_entity the
> application still could not figure out who is /dev/video1".

The media controller framework sets the video devnode entities names to the 
video device name, as provided in the video_device name field. That's 
automatic, so you should just ensure that your video_device name is properly 
set.

> However in V4L2 devices, there is a driver name that the vendor can assign
> a specific name such "WIFI CAPTURE" or BACK_CAMERA" to the driver name. Is
> it possible to add the driver name into the media_entity? This makes that,
> if the userspace application knows the driver name it can use the driver
> name to find the dev node.

Using the driver name isn't optimal, as a driver could create several video 
device nodes for the same hardware device. Those nodes should have different 
names, so you should use the video_device name field as explained above.

-- 
Regards,

Laurent Pinchart

RE: query video dev node name using the V4L2 device driver name

[Zhu, Mingcheng]

Hi Laurent,

I have a problem here. Take following example that we have two video dev nodes as:
/dev/video0: this node is for WIFI capture
/dev/video1: this is the camera driver.

Is it possible for the user space to find out video1 is the camera without open and query each video node's capabilities? 

--Mingcheng


-----Original Message-----
From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] 
Sent: Monday, December 19, 2011 2:31 AM
To: Zhu, Mingcheng
Cc: Sakari Ailus; linux-media@vger.kernel.org
Subject: Re: query video dev node name using the V4L2 device driver name

Hi Mingcheng,

On Monday 19 December 2011 11:21:03 Zhu, Mingcheng wrote:
> Hi Laurent and Sakari,
> 
> Current media entity contains a few fields to identify a dev node (name,
> type, group_id). The entity name is the v4l2 dev node name such as
> "/dev/video0" "/dev/video1". There is no information who is "/dev/video0"
> and who is /dev/video1". This makes that, after query the media_entity the
> application still could not figure out who is /dev/video1".

The media controller framework sets the video devnode entities names to the 
video device name, as provided in the video_device name field. That's 
automatic, so you should just ensure that your video_device name is properly 
set.

> However in V4L2 devices, there is a driver name that the vendor can assign
> a specific name such "WIFI CAPTURE" or BACK_CAMERA" to the driver name. Is
> it possible to add the driver name into the media_entity? This makes that,
> if the userspace application knows the driver name it can use the driver
> name to find the dev node.

Using the driver name isn't optimal, as a driver could create several video 
device nodes for the same hardware device. Those nodes should have different 
names, so you should use the video_device name field as explained above.

-- 
Regards,

Laurent Pinchart

Re: query video dev node name using the V4L2 device driver name

[Laurent Pinchart]

Hi Mingcheng,

On Monday 19 December 2011 19:09:18 Zhu, Mingcheng wrote:
> Hi Laurent,
> 
> I have a problem here. Take following example that we have two video dev
> nodes as:
> /dev/video0: this node is for WIFI capture

WIFI capture ? I'm curious about that, what do you mean exactly ?

> /dev/video1: this is the camera driver.
> 
> Is it possible for the user space to find out video1 is the camera without
> open and query each video node's capabilities?

If the drivers that expose those nodes are media-controller aware, one 
possible solution is to open the media controller device(s) and enumerate the 
entities. I'm not sure that would be faster than opening and querying the 
video nodes though.

-- 
Regards,

Laurent Pinchart

[RFC 4/4] omap3isp: Use pixel clock from sensor media bus frameformat

[Sakari Ailus]

Configure the ISP based on the pixel clock in media bus frame format.
Previously the same was configured from the board code.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/omap3isp/isp.c      |    3 +--
 drivers/media/video/omap3isp/isp.h      |    3 ++-
 drivers/media/video/omap3isp/ispvideo.c |    3 +++
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c
index b818cac..c9bed37 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -344,7 +344,7 @@ void omap3isp_configure_bridge(struct isp_device *isp,
  * Set the average pixel clock required by the sensor. The ISP will use the
  * lowest possible memory bandwidth settings compatible with the clock.
  **/
-static void isp_set_pixel_clock(struct isp_device *isp, unsigned int pixelclk)
+void omap3isp_set_pixel_clock(struct isp_device *isp, unsigned int pixelclk)
 {
 	isp->isp_ccdc.vpcfg.pixelclk = pixelclk;
 }
@@ -2072,7 +2072,6 @@ static int isp_probe(struct platform_device *pdev)
 
 	isp->autoidle = autoidle;
 	isp->platform_cb.set_xclk = isp_set_xclk;
-	isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
 
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);
diff --git a/drivers/media/video/omap3isp/isp.h b/drivers/media/video/omap3isp/isp.h
index c5935ae..dd7b303 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -126,7 +126,6 @@ struct isp_reg {
 
 struct isp_platform_callback {
 	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
-	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
 };
 
 /*
@@ -219,6 +218,8 @@ struct isp_device {
 #define v4l2_dev_to_isp_device(dev) \
 	container_of(dev, struct isp_device, v4l2_dev)
 
+void omap3isp_set_pixel_clock(struct isp_device *isp, unsigned int pixelclk);
+
 void omap3isp_hist_dma_done(struct isp_device *isp);
 
 void omap3isp_flush(struct isp_device *isp);
diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index cdcf1d0..64f29ac 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -372,6 +372,9 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 				if (IS_ERR_VALUE(ret))
 					return -EPIPE;
 			}
+			omap3isp_set_pixel_clock(isp,
+						 fmt_source.format.pixel_clock
+						 * 1000);
 		}
 
 		if (subdev->host_priv) {
-- 
1.7.2.5

Re: [RFC 4/4] omap3isp: Use pixel clock from sensor media bus frameformat

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Thursday 15 December 2011 10:50:35 Sakari Ailus wrote:
> Configure the ISP based on the pixel clock in media bus frame format.
> Previously the same was configured from the board code.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/omap3isp/isp.c      |    3 +--
>  drivers/media/video/omap3isp/isp.h      |    3 ++-
>  drivers/media/video/omap3isp/ispvideo.c |    3 +++
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/isp.c
> b/drivers/media/video/omap3isp/isp.c index b818cac..c9bed37 100644
> --- a/drivers/media/video/omap3isp/isp.c
> +++ b/drivers/media/video/omap3isp/isp.c
> @@ -344,7 +344,7 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>   * Set the average pixel clock required by the sensor. The ISP will use
> the * lowest possible memory bandwidth settings compatible with the clock.
> **/
> -static void isp_set_pixel_clock(struct isp_device *isp, unsigned int
> pixelclk) +void omap3isp_set_pixel_clock(struct isp_device *isp, unsigned
> int pixelclk) {
>  	isp->isp_ccdc.vpcfg.pixelclk = pixelclk;
>  }
> @@ -2072,7 +2072,6 @@ static int isp_probe(struct platform_device *pdev)
> 
>  	isp->autoidle = autoidle;
>  	isp->platform_cb.set_xclk = isp_set_xclk;
> -	isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
> 
>  	mutex_init(&isp->isp_mutex);
>  	spin_lock_init(&isp->stat_lock);
> diff --git a/drivers/media/video/omap3isp/isp.h
> b/drivers/media/video/omap3isp/isp.h index c5935ae..dd7b303 100644
> --- a/drivers/media/video/omap3isp/isp.h
> +++ b/drivers/media/video/omap3isp/isp.h
> @@ -126,7 +126,6 @@ struct isp_reg {
> 
>  struct isp_platform_callback {
>  	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
> -	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
>  };
> 
>  /*
> @@ -219,6 +218,8 @@ struct isp_device {
>  #define v4l2_dev_to_isp_device(dev) \
>  	container_of(dev, struct isp_device, v4l2_dev)
> 
> +void omap3isp_set_pixel_clock(struct isp_device *isp, unsigned int
> pixelclk); +
>  void omap3isp_hist_dma_done(struct isp_device *isp);
> 
>  void omap3isp_flush(struct isp_device *isp);
> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> b/drivers/media/video/omap3isp/ispvideo.c index cdcf1d0..64f29ac 100644
> --- a/drivers/media/video/omap3isp/ispvideo.c
> +++ b/drivers/media/video/omap3isp/ispvideo.c
> @@ -372,6 +372,9 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) if (IS_ERR_VALUE(ret))
>  					return -EPIPE;
>  			}
> +			omap3isp_set_pixel_clock(isp,
> +						 fmt_source.format.pixel_clock
> +						 * 1000);

Similarly to 3/4, I think this belongs to isp_pipeline_enable(), or even 
possibly the subdev s_stream operation (same for 3/4 actually).

>  		}
> 
>  		if (subdev->host_priv) {

-- 
Regards,

Laurent Pinchart