[PATCH v2] V4L: add media bus configuration subdev operations

[PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

Add media bus configuration types and two subdev operations to get
supported mediabus configurations and to set a specific configuration.
Subdevs can support several configurations, e.g., they can send video data
on 1 or several lanes, can be configured to use a specific CSI-2 channel,
in such cases subdevice drivers return bitmasks with all respective bits
set. When a set-configuration operation is called, it has to specify a
non-ambiguous configuration.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v2:

1. Removed parallel bus width flags. As Laurent correctly pointed out, bus 
width can be configured based on the mediabus format.

2. Removed the clock parameter for now. Passing timing information between 
the subdevices and the host / bridge driver is indeed necessary, but it is 
not yet quite clear, what is the best way to do this. This requires more 
thinking and can be added as an extra field to struct v4l2_mbus_config 
later. The argument, that "struct clk" is still platform specific is 
correct, but I am too tempted by the possibilities, the clkdev offers us 
to give up this idea immediatrely. Maybe drivers, that need such a clock, 
could use a platform callback to create a clock instance for them, or get 
a clock object from the platform with platform data. However, there are 
also opinions, that the clkdev API is completely unsuitable for this 
purpose. I'd commit this without any timing first, and consider 
possibilities as a second step.

 include/media/v4l2-mediabus.h |   89 +++++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-subdev.h   |    6 +++
 2 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 971c7fa..e0ffba0 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -13,6 +13,95 @@
 
 #include <linux/v4l2-mediabus.h>
 
+/* Parallel flags */
+/* Can the client run in master or in slave mode */
+#define V4L2_MBUS_MASTER			(1 << 0)
+#define V4L2_MBUS_SLAVE				(1 << 1)
+/* Which signal polarities it supports */
+#define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
+#define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
+#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
+#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 5)
+#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 6)
+#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
+#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
+#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
+
+/* Serial flags */
+/* How many lanes the client can use */
+#define V4L2_MBUS_CSI2_1_LANE			(1 << 0)
+#define V4L2_MBUS_CSI2_2_LANE			(1 << 1)
+#define V4L2_MBUS_CSI2_3_LANE			(1 << 2)
+#define V4L2_MBUS_CSI2_4_LANE			(1 << 3)
+/* On which channels it can send video data */
+#define V4L2_MBUS_CSI2_CHANNEL_0		(1 << 4)
+#define V4L2_MBUS_CSI2_CHANNEL_1		(1 << 5)
+#define V4L2_MBUS_CSI2_CHANNEL_2		(1 << 6)
+#define V4L2_MBUS_CSI2_CHANNEL_3		(1 << 7)
+/* Does it support only continuous or also non-continuous clock mode */
+#define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK		(1 << 8)
+#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	(1 << 9)
+
+#define V4L2_MBUS_CSI2_LANES		(V4L2_MBUS_CSI2_1_LANE | V4L2_MBUS_CSI2_2_LANE | \
+					 V4L2_MBUS_CSI2_3_LANE | V4L2_MBUS_CSI2_4_LANE)
+#define V4L2_MBUS_CSI2_CHANNELS		(V4L2_MBUS_CSI2_CHANNEL_0 | V4L2_MBUS_CSI2_CHANNEL_1 | \
+					 V4L2_MBUS_CSI2_CHANNEL_2 | V4L2_MBUS_CSI2_CHANNEL_3)
+
+/**
+ * v4l2_mbus_type - media bus type
+ * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
+ * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation
+ * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
+ */
+enum v4l2_mbus_type {
+	V4L2_MBUS_PARALLEL,
+	V4L2_MBUS_BT656,
+	V4L2_MBUS_CSI2,
+};
+
+/**
+ * v4l2_mbus_config - media bus configuration
+ * @type:	in: interface type
+ * @flags:	in / out: configuration flags, depending on @type
+ */
+struct v4l2_mbus_config {
+	enum v4l2_mbus_type type;
+	unsigned long flags;
+};
+
+static inline unsigned long v4l2_mbus_config_compatible(struct v4l2_mbus_config *cfg,
+							unsigned long flags)
+{
+	unsigned long common_flags, hsync, vsync, pclk, data, mode;
+	unsigned long mipi_lanes, mipi_clock;
+
+	common_flags = cfg->flags & flags;
+
+	switch (cfg->type) {
+	case V4L2_MBUS_PARALLEL:
+		hsync = common_flags & (V4L2_MBUS_HSYNC_ACTIVE_HIGH |
+					V4L2_MBUS_HSYNC_ACTIVE_LOW);
+		vsync = common_flags & (V4L2_MBUS_VSYNC_ACTIVE_HIGH |
+					V4L2_MBUS_VSYNC_ACTIVE_LOW);
+		pclk = common_flags & (V4L2_MBUS_PCLK_SAMPLE_RISING |
+				       V4L2_MBUS_PCLK_SAMPLE_FALLING);
+		data = common_flags & (V4L2_MBUS_DATA_ACTIVE_HIGH |
+				       V4L2_MBUS_DATA_ACTIVE_LOW);
+		mode = common_flags & (V4L2_MBUS_MASTER | V4L2_MBUS_SLAVE);
+		return (!hsync || !vsync || !pclk || !data || !mode) ?
+			0 : common_flags;
+	case V4L2_MBUS_CSI2:
+		mipi_lanes = common_flags & V4L2_MBUS_CSI2_LANES;
+		mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK |
+					     V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
+		return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
+	case V4L2_MBUS_BT656:
+		/* TODO: implement me */
+		return 0;
+	}
+	return 0;
+}
+
 static inline void v4l2_fill_pix_format(struct v4l2_pix_format *pix_fmt,
 				const struct v4l2_mbus_framefmt *mbus_fmt)
 {
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1562c4f..75919ef 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
    try_mbus_fmt: try to set a pixel format on a video data source
 
    s_mbus_fmt: set a pixel format on a video data source
+
+   g_mbus_config: get supported mediabus configurations
+
+   s_mbus_config: set a certain mediabus configuration
  */
 struct v4l2_subdev_video_ops {
 	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
@@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
 			    struct v4l2_mbus_framefmt *fmt);
 	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
 			  struct v4l2_mbus_framefmt *fmt);
+	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
+	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
 };
 
 /*
-- 
1.7.2.5

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Hans Verkuil]

On Wednesday, June 22, 2011 23:26:29 Guennadi Liakhovetski wrote:
> Add media bus configuration types and two subdev operations to get
> supported mediabus configurations and to set a specific configuration.
> Subdevs can support several configurations, e.g., they can send video data
> on 1 or several lanes, can be configured to use a specific CSI-2 channel,
> in such cases subdevice drivers return bitmasks with all respective bits
> set. When a set-configuration operation is called, it has to specify a
> non-ambiguous configuration.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> v2:
> 
> 1. Removed parallel bus width flags. As Laurent correctly pointed out, bus 
> width can be configured based on the mediabus format.
> 
> 2. Removed the clock parameter for now. Passing timing information between 
> the subdevices and the host / bridge driver is indeed necessary, but it is 
> not yet quite clear, what is the best way to do this. This requires more 
> thinking and can be added as an extra field to struct v4l2_mbus_config 
> later. The argument, that "struct clk" is still platform specific is 
> correct, but I am too tempted by the possibilities, the clkdev offers us 
> to give up this idea immediatrely. Maybe drivers, that need such a clock, 
> could use a platform callback to create a clock instance for them, or get 
> a clock object from the platform with platform data. However, there are 
> also opinions, that the clkdev API is completely unsuitable for this 
> purpose. I'd commit this without any timing first, and consider 
> possibilities as a second step.
> 
>  include/media/v4l2-mediabus.h |   89 +++++++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h   |    6 +++
>  2 files changed, 95 insertions(+), 0 deletions(-)

Some small stuff:
 
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 971c7fa..e0ffba0 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -13,6 +13,95 @@
>  
>  #include <linux/v4l2-mediabus.h>
>  
> +/* Parallel flags */
> +/* Can the client run in master or in slave mode */
> +#define V4L2_MBUS_MASTER			(1 << 0)
> +#define V4L2_MBUS_SLAVE				(1 << 1)
> +/* Which signal polarities it supports */
> +#define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
> +#define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 5)
> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 6)
> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
> +
> +/* Serial flags */
> +/* How many lanes the client can use */
> +#define V4L2_MBUS_CSI2_1_LANE			(1 << 0)
> +#define V4L2_MBUS_CSI2_2_LANE			(1 << 1)
> +#define V4L2_MBUS_CSI2_3_LANE			(1 << 2)
> +#define V4L2_MBUS_CSI2_4_LANE			(1 << 3)
> +/* On which channels it can send video data */
> +#define V4L2_MBUS_CSI2_CHANNEL_0		(1 << 4)
> +#define V4L2_MBUS_CSI2_CHANNEL_1		(1 << 5)
> +#define V4L2_MBUS_CSI2_CHANNEL_2		(1 << 6)
> +#define V4L2_MBUS_CSI2_CHANNEL_3		(1 << 7)
> +/* Does it support only continuous or also non-continuous clock mode */
> +#define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK		(1 << 8)
> +#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	(1 << 9)
> +
> +#define V4L2_MBUS_CSI2_LANES		(V4L2_MBUS_CSI2_1_LANE | V4L2_MBUS_CSI2_2_LANE | \
> +					 V4L2_MBUS_CSI2_3_LANE | V4L2_MBUS_CSI2_4_LANE)
> +#define V4L2_MBUS_CSI2_CHANNELS		(V4L2_MBUS_CSI2_CHANNEL_0 | V4L2_MBUS_CSI2_CHANNEL_1 | \
> +					 V4L2_MBUS_CSI2_CHANNEL_2 | V4L2_MBUS_CSI2_CHANNEL_3)
> +
> +/**
> + * v4l2_mbus_type - media bus type
> + * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
> + * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation
> + * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
> + */
> +enum v4l2_mbus_type {
> +	V4L2_MBUS_PARALLEL,
> +	V4L2_MBUS_BT656,
> +	V4L2_MBUS_CSI2,
> +};
> +
> +/**
> + * v4l2_mbus_config - media bus configuration
> + * @type:	in: interface type
> + * @flags:	in / out: configuration flags, depending on @type
> + */
> +struct v4l2_mbus_config {
> +	enum v4l2_mbus_type type;
> +	unsigned long flags;

Flags should be unsigned int. unsigned long is 64 bits on a 64-bit system,
which is unnecessary.

> +};
> +
> +static inline unsigned long v4l2_mbus_config_compatible(struct v4l2_mbus_config *cfg,
> +							unsigned long flags)

This function is too big to be a static inline. I would also go for a bool return type.
And cfg should be a const pointer.

> +{
> +	unsigned long common_flags,

> hsync, vsync, pclk, data, mode;

These can be bools.

> +	unsigned long mipi_lanes, mipi_clock;

Ditto.

> +
> +	common_flags = cfg->flags & flags;
> +
> +	switch (cfg->type) {
> +	case V4L2_MBUS_PARALLEL:
> +		hsync = common_flags & (V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> +					V4L2_MBUS_HSYNC_ACTIVE_LOW);
> +		vsync = common_flags & (V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> +					V4L2_MBUS_VSYNC_ACTIVE_LOW);
> +		pclk = common_flags & (V4L2_MBUS_PCLK_SAMPLE_RISING |
> +				       V4L2_MBUS_PCLK_SAMPLE_FALLING);
> +		data = common_flags & (V4L2_MBUS_DATA_ACTIVE_HIGH |
> +				       V4L2_MBUS_DATA_ACTIVE_LOW);
> +		mode = common_flags & (V4L2_MBUS_MASTER | V4L2_MBUS_SLAVE);
> +		return (!hsync || !vsync || !pclk || !data || !mode) ?
> +			0 : common_flags;
> +	case V4L2_MBUS_CSI2:
> +		mipi_lanes = common_flags & V4L2_MBUS_CSI2_LANES;
> +		mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK |
> +					     V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
> +		return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> +	case V4L2_MBUS_BT656:
> +		/* TODO: implement me */

Isn't this identical to MBUS_PARALLEL, except that it can ignore the hsync/vsync
signals? So this case can go in between the 'vsync =' and 'pclk =' lines above.
(hsync and vsync should be initialized to true of course).

> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  static inline void v4l2_fill_pix_format(struct v4l2_pix_format *pix_fmt,
>  				const struct v4l2_mbus_framefmt *mbus_fmt)
>  {
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1562c4f..75919ef 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
>     try_mbus_fmt: try to set a pixel format on a video data source
>  
>     s_mbus_fmt: set a pixel format on a video data source
> +
> +   g_mbus_config: get supported mediabus configurations
> +
> +   s_mbus_config: set a certain mediabus configuration
>   */
>  struct v4l2_subdev_video_ops {
>  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
>  			    struct v4l2_mbus_framefmt *fmt);
>  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
>  			  struct v4l2_mbus_framefmt *fmt);
> +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);

cfg can be a const pointer.

>  };
>  
>  /*
> 

Regards,

	Hans

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

Hi Hans

Thanks for the review, agree to all, except one:

On Wed, 22 Jun 2011, Hans Verkuil wrote:

> On Wednesday, June 22, 2011 23:26:29 Guennadi Liakhovetski wrote:

[snip]

> > +static inline unsigned long v4l2_mbus_config_compatible(struct v4l2_mbus_config *cfg,
> > +							unsigned long flags)
> 
> This function is too big to be a static inline. I would also go for a bool return type.
> And cfg should be a const pointer.

return is not just a bool, it's a mask of common flags.

> > +	switch (cfg->type) {
> > +	case V4L2_MBUS_PARALLEL:
> > +		hsync = common_flags & (V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> > +					V4L2_MBUS_HSYNC_ACTIVE_LOW);
> > +		vsync = common_flags & (V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> > +					V4L2_MBUS_VSYNC_ACTIVE_LOW);
> > +		pclk = common_flags & (V4L2_MBUS_PCLK_SAMPLE_RISING |
> > +				       V4L2_MBUS_PCLK_SAMPLE_FALLING);
> > +		data = common_flags & (V4L2_MBUS_DATA_ACTIVE_HIGH |
> > +				       V4L2_MBUS_DATA_ACTIVE_LOW);
> > +		mode = common_flags & (V4L2_MBUS_MASTER | V4L2_MBUS_SLAVE);
> > +		return (!hsync || !vsync || !pclk || !data || !mode) ?
> > +			0 : common_flags;
> > +	case V4L2_MBUS_CSI2:
> > +		mipi_lanes = common_flags & V4L2_MBUS_CSI2_LANES;
> > +		mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK |
> > +					     V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
> > +		return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> > +	case V4L2_MBUS_BT656:
> > +		/* TODO: implement me */
> 
> Isn't this identical to MBUS_PARALLEL, except that it can ignore the hsync/vsync
> signals? So this case can go in between the 'vsync =' and 'pclk =' lines above.
> (hsync and vsync should be initialized to true of course).

Well, maybe. We could do that or leave it unimplemented until someone 
really uses it.

> > @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
> >  			    struct v4l2_mbus_framefmt *fmt);
> >  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
> >  			  struct v4l2_mbus_framefmt *fmt);
> > +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> 
> cfg can be a const pointer.

In s_... you mean, not in g_...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Sakari Ailus]

Hi Guennadi,

Thanks for the patch. I have a few comments below.

On Wed, Jun 22, 2011 at 11:26:29PM +0200, Guennadi Liakhovetski wrote:
> Add media bus configuration types and two subdev operations to get
> supported mediabus configurations and to set a specific configuration.
> Subdevs can support several configurations, e.g., they can send video data
> on 1 or several lanes, can be configured to use a specific CSI-2 channel,
> in such cases subdevice drivers return bitmasks with all respective bits
> set. When a set-configuration operation is called, it has to specify a
> non-ambiguous configuration.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> v2:
> 
> 1. Removed parallel bus width flags. As Laurent correctly pointed out, bus 
> width can be configured based on the mediabus format.
> 
> 2. Removed the clock parameter for now. Passing timing information between 
> the subdevices and the host / bridge driver is indeed necessary, but it is 
> not yet quite clear, what is the best way to do this. This requires more 
> thinking and can be added as an extra field to struct v4l2_mbus_config 
> later. The argument, that "struct clk" is still platform specific is 
> correct, but I am too tempted by the possibilities, the clkdev offers us 
> to give up this idea immediatrely. Maybe drivers, that need such a clock, 
> could use a platform callback to create a clock instance for them, or get 
> a clock object from the platform with platform data. However, there are 
> also opinions, that the clkdev API is completely unsuitable for this 
> purpose. I'd commit this without any timing first, and consider 
> possibilities as a second step.
> 
>  include/media/v4l2-mediabus.h |   89 +++++++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h   |    6 +++
>  2 files changed, 95 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 971c7fa..e0ffba0 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -13,6 +13,95 @@
>  
>  #include <linux/v4l2-mediabus.h>
>  
> +/* Parallel flags */
> +/* Can the client run in master or in slave mode */
> +#define V4L2_MBUS_MASTER			(1 << 0)
> +#define V4L2_MBUS_SLAVE				(1 << 1)

What are master and slave in this case? Something other than transmitter and
receiver?

> +/* Which signal polarities it supports */
> +#define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
> +#define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 5)
> +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 6)
> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
> +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
> +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
> +
> +/* Serial flags */
> +/* How many lanes the client can use */
> +#define V4L2_MBUS_CSI2_1_LANE			(1 << 0)
> +#define V4L2_MBUS_CSI2_2_LANE			(1 << 1)
> +#define V4L2_MBUS_CSI2_3_LANE			(1 << 2)
> +#define V4L2_MBUS_CSI2_4_LANE			(1 << 3)
> +/* On which channels it can send video data */
> +#define V4L2_MBUS_CSI2_CHANNEL_0		(1 << 4)
> +#define V4L2_MBUS_CSI2_CHANNEL_1		(1 << 5)
> +#define V4L2_MBUS_CSI2_CHANNEL_2		(1 << 6)
> +#define V4L2_MBUS_CSI2_CHANNEL_3		(1 << 7)

It might not be just video data but e.g. metadata.

Is four the maximum number of channels in CSI2?

> +/* Does it support only continuous or also non-continuous clock mode */
> +#define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK		(1 << 8)
> +#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	(1 << 9)

What's noncontinuous clock for CSI2? Does it mean in some situations the
clock may be stopped based on certain criteria?

> +#define V4L2_MBUS_CSI2_LANES		(V4L2_MBUS_CSI2_1_LANE | V4L2_MBUS_CSI2_2_LANE | \
> +					 V4L2_MBUS_CSI2_3_LANE | V4L2_MBUS_CSI2_4_LANE)
> +#define V4L2_MBUS_CSI2_CHANNELS		(V4L2_MBUS_CSI2_CHANNEL_0 | V4L2_MBUS_CSI2_CHANNEL_1 | \
> +					 V4L2_MBUS_CSI2_CHANNEL_2 | V4L2_MBUS_CSI2_CHANNEL_3)
> +
> +/**
> + * v4l2_mbus_type - media bus type
> + * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
> + * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation
> + * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
> + */
> +enum v4l2_mbus_type {
> +	V4L2_MBUS_PARALLEL,
> +	V4L2_MBUS_BT656,
> +	V4L2_MBUS_CSI2,
> +};
> +
> +/**
> + * v4l2_mbus_config - media bus configuration
> + * @type:	in: interface type
> + * @flags:	in / out: configuration flags, depending on @type
> + */
> +struct v4l2_mbus_config {
> +	enum v4l2_mbus_type type;
> +	unsigned long flags;
> +};
> +
> +static inline unsigned long v4l2_mbus_config_compatible(struct v4l2_mbus_config *cfg,
> +							unsigned long flags)
> +{
> +	unsigned long common_flags, hsync, vsync, pclk, data, mode;
> +	unsigned long mipi_lanes, mipi_clock;
> +
> +	common_flags = cfg->flags & flags;
> +
> +	switch (cfg->type) {
> +	case V4L2_MBUS_PARALLEL:
> +		hsync = common_flags & (V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> +					V4L2_MBUS_HSYNC_ACTIVE_LOW);
> +		vsync = common_flags & (V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> +					V4L2_MBUS_VSYNC_ACTIVE_LOW);
> +		pclk = common_flags & (V4L2_MBUS_PCLK_SAMPLE_RISING |
> +				       V4L2_MBUS_PCLK_SAMPLE_FALLING);
> +		data = common_flags & (V4L2_MBUS_DATA_ACTIVE_HIGH |
> +				       V4L2_MBUS_DATA_ACTIVE_LOW);
> +		mode = common_flags & (V4L2_MBUS_MASTER | V4L2_MBUS_SLAVE);
> +		return (!hsync || !vsync || !pclk || !data || !mode) ?
> +			0 : common_flags;
> +	case V4L2_MBUS_CSI2:
> +		mipi_lanes = common_flags & V4L2_MBUS_CSI2_LANES;
> +		mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK |
> +					     V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
> +		return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> +	case V4L2_MBUS_BT656:
> +		/* TODO: implement me */
> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  static inline void v4l2_fill_pix_format(struct v4l2_pix_format *pix_fmt,
>  				const struct v4l2_mbus_framefmt *mbus_fmt)
>  {
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1562c4f..75919ef 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
>     try_mbus_fmt: try to set a pixel format on a video data source
>  
>     s_mbus_fmt: set a pixel format on a video data source
> +
> +   g_mbus_config: get supported mediabus configurations
> +
> +   s_mbus_config: set a certain mediabus configuration
>   */
>  struct v4l2_subdev_video_ops {
>  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
>  			    struct v4l2_mbus_framefmt *fmt);
>  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
>  			  struct v4l2_mbus_framefmt *fmt);
> +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);

How would the ops be used and by whom?

How complete configuration for CSI2 bus is the above intend to be? Complete,
I suppose, and so I think we'll also need to specify how e.g. the CSI2 clock
and data lanes have been connected between transmitter and receiver. This is
less trivial to guess than clock polarity and requires further information
from the board and lane mapping configuration capabilities of both.
Shouldn't this information be also added to CSI2 configuration?

Do you think a single bitmask would suffice for this in the long run?

I can see some use for the information in the set operation in lane
configuration, for example, as you mentioned. My guess would be that the
number of lanes _might_ be something that the user space would want to know
or possibly even configure --- but we'll first need to discuss low-level
sensor control interface.

But otherwise the configuration should likely be rather static and board
specific. Wouldn't the subdevs get this as part of the platform data, or
how?

I would just keep the bus configuration static board dependent information
until we have that part working and used by drivers and extend it later on.

Just my 0,05 euros.

>  };
>  
>  /*
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

Hi Sakari

On Fri, 24 Jun 2011, Sakari Ailus wrote:

> Hi Guennadi,
> 
> Thanks for the patch. I have a few comments below.
> 
> On Wed, Jun 22, 2011 at 11:26:29PM +0200, Guennadi Liakhovetski wrote:
> > Add media bus configuration types and two subdev operations to get
> > supported mediabus configurations and to set a specific configuration.
> > Subdevs can support several configurations, e.g., they can send video data
> > on 1 or several lanes, can be configured to use a specific CSI-2 channel,
> > in such cases subdevice drivers return bitmasks with all respective bits
> > set. When a set-configuration operation is called, it has to specify a
> > non-ambiguous configuration.
> > 
> > Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > v2:
> > 
> > 1. Removed parallel bus width flags. As Laurent correctly pointed out, bus 
> > width can be configured based on the mediabus format.
> > 
> > 2. Removed the clock parameter for now. Passing timing information between 
> > the subdevices and the host / bridge driver is indeed necessary, but it is 
> > not yet quite clear, what is the best way to do this. This requires more 
> > thinking and can be added as an extra field to struct v4l2_mbus_config 
> > later. The argument, that "struct clk" is still platform specific is 
> > correct, but I am too tempted by the possibilities, the clkdev offers us 
> > to give up this idea immediatrely. Maybe drivers, that need such a clock, 
> > could use a platform callback to create a clock instance for them, or get 
> > a clock object from the platform with platform data. However, there are 
> > also opinions, that the clkdev API is completely unsuitable for this 
> > purpose. I'd commit this without any timing first, and consider 
> > possibilities as a second step.
> > 
> >  include/media/v4l2-mediabus.h |   89 +++++++++++++++++++++++++++++++++++++++++
> >  include/media/v4l2-subdev.h   |    6 +++
> >  2 files changed, 95 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 971c7fa..e0ffba0 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -13,6 +13,95 @@
> >  
> >  #include <linux/v4l2-mediabus.h>
> >  
> > +/* Parallel flags */
> > +/* Can the client run in master or in slave mode */
> > +#define V4L2_MBUS_MASTER			(1 << 0)
> > +#define V4L2_MBUS_SLAVE				(1 << 1)
> 
> What are master and slave in this case? Something other than transmitter and
> receiver?

No, these are master and slave modes of a parallel camera connection. In 
the master mode the client (sensor) is providing the pixel clock, in the 
slave mode the bridge is "clocking" data out of the client. In my 
experience, the slave mode is rarely used, but many camera sensors and 
some SoC host interfaces still support it.

> 
> > +/* Which signal polarities it supports */
> > +#define V4L2_MBUS_HSYNC_ACTIVE_HIGH		(1 << 2)
> > +#define V4L2_MBUS_HSYNC_ACTIVE_LOW		(1 << 3)
> > +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH		(1 << 4)
> > +#define V4L2_MBUS_VSYNC_ACTIVE_LOW		(1 << 5)
> > +#define V4L2_MBUS_PCLK_SAMPLE_RISING		(1 << 6)
> > +#define V4L2_MBUS_PCLK_SAMPLE_FALLING		(1 << 7)
> > +#define V4L2_MBUS_DATA_ACTIVE_HIGH		(1 << 8)
> > +#define V4L2_MBUS_DATA_ACTIVE_LOW		(1 << 9)
> > +
> > +/* Serial flags */
> > +/* How many lanes the client can use */
> > +#define V4L2_MBUS_CSI2_1_LANE			(1 << 0)
> > +#define V4L2_MBUS_CSI2_2_LANE			(1 << 1)
> > +#define V4L2_MBUS_CSI2_3_LANE			(1 << 2)
> > +#define V4L2_MBUS_CSI2_4_LANE			(1 << 3)
> > +/* On which channels it can send video data */
> > +#define V4L2_MBUS_CSI2_CHANNEL_0		(1 << 4)
> > +#define V4L2_MBUS_CSI2_CHANNEL_1		(1 << 5)
> > +#define V4L2_MBUS_CSI2_CHANNEL_2		(1 << 6)
> > +#define V4L2_MBUS_CSI2_CHANNEL_3		(1 << 7)
> 
> It might not be just video data but e.g. metadata.
> 
> Is four the maximum number of channels in CSI2?

Yes.

> > +/* Does it support only continuous or also non-continuous clock mode */
> > +#define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK		(1 << 8)
> > +#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	(1 << 9)
> 
> What's noncontinuous clock for CSI2? Does it mean in some situations the
> clock may be stopped based on certain criteria?

It's when the clock lane is idle in certain intervals. See the 
specification for details. In any case, say, if the sensor supports both 
clock modes, and the bridge only supports the compulsory continuous mode, 
the optional non-continuous mode will not work.

> > +#define V4L2_MBUS_CSI2_LANES		(V4L2_MBUS_CSI2_1_LANE | V4L2_MBUS_CSI2_2_LANE | \
> > +					 V4L2_MBUS_CSI2_3_LANE | V4L2_MBUS_CSI2_4_LANE)
> > +#define V4L2_MBUS_CSI2_CHANNELS		(V4L2_MBUS_CSI2_CHANNEL_0 | V4L2_MBUS_CSI2_CHANNEL_1 | \
> > +					 V4L2_MBUS_CSI2_CHANNEL_2 | V4L2_MBUS_CSI2_CHANNEL_3)
> > +
> > +/**
> > + * v4l2_mbus_type - media bus type
> > + * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
> > + * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation
> > + * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
> > + */
> > +enum v4l2_mbus_type {
> > +	V4L2_MBUS_PARALLEL,
> > +	V4L2_MBUS_BT656,
> > +	V4L2_MBUS_CSI2,
> > +};
> > +
> > +/**
> > + * v4l2_mbus_config - media bus configuration
> > + * @type:	in: interface type
> > + * @flags:	in / out: configuration flags, depending on @type
> > + */
> > +struct v4l2_mbus_config {
> > +	enum v4l2_mbus_type type;
> > +	unsigned long flags;
> > +};
> > +
> > +static inline unsigned long v4l2_mbus_config_compatible(struct v4l2_mbus_config *cfg,
> > +							unsigned long flags)
> > +{
> > +	unsigned long common_flags, hsync, vsync, pclk, data, mode;
> > +	unsigned long mipi_lanes, mipi_clock;
> > +
> > +	common_flags = cfg->flags & flags;
> > +
> > +	switch (cfg->type) {
> > +	case V4L2_MBUS_PARALLEL:
> > +		hsync = common_flags & (V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> > +					V4L2_MBUS_HSYNC_ACTIVE_LOW);
> > +		vsync = common_flags & (V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> > +					V4L2_MBUS_VSYNC_ACTIVE_LOW);
> > +		pclk = common_flags & (V4L2_MBUS_PCLK_SAMPLE_RISING |
> > +				       V4L2_MBUS_PCLK_SAMPLE_FALLING);
> > +		data = common_flags & (V4L2_MBUS_DATA_ACTIVE_HIGH |
> > +				       V4L2_MBUS_DATA_ACTIVE_LOW);
> > +		mode = common_flags & (V4L2_MBUS_MASTER | V4L2_MBUS_SLAVE);
> > +		return (!hsync || !vsync || !pclk || !data || !mode) ?
> > +			0 : common_flags;
> > +	case V4L2_MBUS_CSI2:
> > +		mipi_lanes = common_flags & V4L2_MBUS_CSI2_LANES;
> > +		mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK |
> > +					     V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
> > +		return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> > +	case V4L2_MBUS_BT656:
> > +		/* TODO: implement me */
> > +		return 0;
> > +	}
> > +	return 0;
> > +}
> > +
> >  static inline void v4l2_fill_pix_format(struct v4l2_pix_format *pix_fmt,
> >  				const struct v4l2_mbus_framefmt *mbus_fmt)
> >  {
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 1562c4f..75919ef 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
> >     try_mbus_fmt: try to set a pixel format on a video data source
> >  
> >     s_mbus_fmt: set a pixel format on a video data source
> > +
> > +   g_mbus_config: get supported mediabus configurations
> > +
> > +   s_mbus_config: set a certain mediabus configuration
> >   */
> >  struct v4l2_subdev_video_ops {
> >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
> >  			    struct v4l2_mbus_framefmt *fmt);
> >  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
> >  			  struct v4l2_mbus_framefmt *fmt);
> > +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> 
> How would the ops be used and by whom?

In the first line I want to use them to make re-use of soc-camera and 
other subdev drivers possible. Soc-camera drivers would switch to these 
from their specific bus-configuration methods, other drivers are free to 
use or not to use them.

> How complete configuration for CSI2 bus is the above intend to be? Complete,
> I suppose, and so I think we'll also need to specify how e.g. the CSI2 clock
> and data lanes have been connected between transmitter and receiver. This is
> less trivial to guess than clock polarity and requires further information
> from the board and lane mapping configuration capabilities of both.
> Shouldn't this information be also added to CSI2 configuration?
> 
> Do you think a single bitmask would suffice for this in the long run?

If you have certain additions to this API I would be happy to consider 
them. Otherwise, if you just suppose, that we might need more 
configuration options, we can always add them in the future, when we know 
more about them and have specific use-cases, with which we can test them. 
E.g., I am not sure what data- and clock-lane connection possibilities you 
refer to above. Don't you _have_ to connect lane 1 to 1 etc?

> I can see some use for the information in the set operation in lane
> configuration, for example, as you mentioned. My guess would be that the
> number of lanes _might_ be something that the user space would want to know
> or possibly even configure --- but we'll first need to discuss low-level
> sensor control interface.
> 
> But otherwise the configuration should likely be rather static and board
> specific. Wouldn't the subdevs get this as part of the platform data, or
> how?
> 
> I would just keep the bus configuration static board dependent information
> until we have that part working and used by drivers and extend it later on.

As I said, drivers are free to not use these methods and implement a 
platform-provided configuration method. In which case, as, I think, Hans 
pointed out earlier, they can also choose to use this struct in their 
platform data.

> 
> Just my 0,05 euros.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Sakari Ailus]

On Fri, Jun 24, 2011 at 12:35:03AM +0200, Guennadi Liakhovetski wrote:
> Hi Sakari

Hi Guennadi,

> On Fri, 24 Jun 2011, Sakari Ailus wrote:
> 
> > Hi Guennadi,
> > 
> > Thanks for the patch. I have a few comments below.
> > 
> > On Wed, Jun 22, 2011 at 11:26:29PM +0200, Guennadi Liakhovetski wrote:

[clip]

> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 1562c4f..75919ef 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
> > >     try_mbus_fmt: try to set a pixel format on a video data source
> > >  
> > >     s_mbus_fmt: set a pixel format on a video data source
> > > +
> > > +   g_mbus_config: get supported mediabus configurations
> > > +
> > > +   s_mbus_config: set a certain mediabus configuration
> > >   */
> > >  struct v4l2_subdev_video_ops {
> > >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > > @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
> > >  			    struct v4l2_mbus_framefmt *fmt);
> > >  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
> > >  			  struct v4l2_mbus_framefmt *fmt);
> > > +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > 
> > How would the ops be used and by whom?
> 
> In the first line I want to use them to make re-use of soc-camera and 
> other subdev drivers possible. Soc-camera drivers would switch to these 
> from their specific bus-configuration methods, other drivers are free to 
> use or not to use them.
> 
> > How complete configuration for CSI2 bus is the above intend to be? Complete,
> > I suppose, and so I think we'll also need to specify how e.g. the CSI2 clock
> > and data lanes have been connected between transmitter and receiver. This is
> > less trivial to guess than clock polarity and requires further information
> > from the board and lane mapping configuration capabilities of both.
> > Shouldn't this information be also added to CSI2 configuration?
> > 
> > Do you think a single bitmask would suffice for this in the long run?
> 
> If you have certain additions to this API I would be happy to consider 
> them. Otherwise, if you just suppose, that we might need more 
> configuration options, we can always add them in the future, when we know 
> more about them and have specific use-cases, with which we can test them. 
> E.g., I am not sure what data- and clock-lane connection possibilities you 
> refer to above. Don't you _have_ to connect lane 1 to 1 etc?

You don't need to. It actually depends on the hardware: the OMAP 3 CSI-2
receiver, for example, may freely configure the order of the data and clock
lanes. And you'll actually have to do that configuration since this is
purely board dependent.

I'm not sure whether the CSI-2 specification requires this, however. I
wouldn't be surprised if there were sensors which could also do this.

For OMAP 3 ISP driver this configuration is in struct isp_csiphy_lanes_cfg
in drivers/media/video/omap3isp/ispcsiphy.h at the moment.

The OMAP 3 ISP driver currently uses all the lanes it has, as far as I
understand, so you can't disable them right now.

> > I can see some use for the information in the set operation in lane
> > configuration, for example, as you mentioned. My guess would be that the
> > number of lanes _might_ be something that the user space would want to know
> > or possibly even configure --- but we'll first need to discuss low-level
> > sensor control interface.
> > 
> > But otherwise the configuration should likely be rather static and board
> > specific. Wouldn't the subdevs get this as part of the platform data, or
> > how?
> > 
> > I would just keep the bus configuration static board dependent information
> > until we have that part working and used by drivers and extend it later on.
> 
> As I said, drivers are free to not use these methods and implement a 
> platform-provided configuration method. In which case, as, I think, Hans 
> pointed out earlier, they can also choose to use this struct in their 
> platform data.

If the structures are expected to be generic I somehow feel that a field of
flags isn't the best way to describe the configuration of CSI-2 or other
busses. Why not to just use a structure with bus type and an union for
bus-specific configuration parameters? It'd be easier to access and also to
change as needed than flags in an unsigned long field.

Also, this structure is primarily about capabilities and not configuration.
Having settings e.g. for active low vsync and the opposite isn't making this
easier for drivers. There should be just one --- more so if the matching
will be specific to SoC camera only.

What would you think of having separate structures for configuration and
capabilities of the busses? I think this way the mechanism would be more
useful outside the scope of SoC camera. The CSI-2 and parallel busses are
still quite generic.

That said, at this point I'm not exactly sure what configuration should be
board specific and what should be dynamically configurable. The lane setup I
mentioned earlier, for example, is something that would be good to be
dynamically configurable in the future. Even if there are e.g. two lanes the
user still might want to use just one. In this case you need to be able to
tell the hardware has that two lanes but you only use one of them.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

Hi Sakari

On Mon, 27 Jun 2011, Sakari Ailus wrote:

> On Fri, Jun 24, 2011 at 12:35:03AM +0200, Guennadi Liakhovetski wrote:
> > Hi Sakari
> 
> Hi Guennadi,
> 
> > On Fri, 24 Jun 2011, Sakari Ailus wrote:
> > 
> > > Hi Guennadi,
> > > 
> > > Thanks for the patch. I have a few comments below.
> > > 
> > > On Wed, Jun 22, 2011 at 11:26:29PM +0200, Guennadi Liakhovetski wrote:
> 
> [clip]
> 
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index 1562c4f..75919ef 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
> > > >     try_mbus_fmt: try to set a pixel format on a video data source
> > > >  
> > > >     s_mbus_fmt: set a pixel format on a video data source
> > > > +
> > > > +   g_mbus_config: get supported mediabus configurations
> > > > +
> > > > +   s_mbus_config: set a certain mediabus configuration
> > > >   */
> > > >  struct v4l2_subdev_video_ops {
> > > >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > > > @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
> > > >  			    struct v4l2_mbus_framefmt *fmt);
> > > >  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
> > > >  			  struct v4l2_mbus_framefmt *fmt);
> > > > +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > > +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > 
> > > How would the ops be used and by whom?
> > 
> > In the first line I want to use them to make re-use of soc-camera and 
> > other subdev drivers possible. Soc-camera drivers would switch to these 
> > from their specific bus-configuration methods, other drivers are free to 
> > use or not to use them.
> > 
> > > How complete configuration for CSI2 bus is the above intend to be? Complete,
> > > I suppose, and so I think we'll also need to specify how e.g. the CSI2 clock
> > > and data lanes have been connected between transmitter and receiver. This is
> > > less trivial to guess than clock polarity and requires further information
> > > from the board and lane mapping configuration capabilities of both.
> > > Shouldn't this information be also added to CSI2 configuration?
> > > 
> > > Do you think a single bitmask would suffice for this in the long run?
> > 
> > If you have certain additions to this API I would be happy to consider 
> > them. Otherwise, if you just suppose, that we might need more 
> > configuration options, we can always add them in the future, when we know 
> > more about them and have specific use-cases, with which we can test them. 
> > E.g., I am not sure what data- and clock-lane connection possibilities you 
> > refer to above. Don't you _have_ to connect lane 1 to 1 etc?
> 
> You don't need to. It actually depends on the hardware: the OMAP 3 CSI-2
> receiver, for example, may freely configure the order of the data and clock
> lanes. And you'll actually have to do that configuration since this is
> purely board dependent.

Nice

> I'm not sure whether the CSI-2 specification requires this, however. I
> wouldn't be surprised if there were sensors which could also do this.

Ok, to be flexible and consistent, we could do the same as with other 
parameters. We could design a query like "to which pin can you route data 
lane 1?" with possible replies "I can only route lane 1 to pin 1," or "I 
can route lane 1 to pins 1, 2, 3, 4, or 5" (for 4 data-lanes and one clock 
lane. Then the configuration request could do "please, route your data 
lane 1 to pin 1." Or we can hard-code this routing in the platform data 
for now.

I, probably, already mentioned this before, but this is also how the 
present soc-camera API evolved in the past: in the beginning it also was 
completely static, then I noticed, that instead of hard coding the 
configuration, I can let drivers negotiate parameters automatically. Then 
gradually the number of auto-negotiated parameters grew, as more flexible 
hardware had to be handled.

We could do the same here: first hard-code this, then see, if it is 
becoming a burden, having to hard-code these. Notice, that 
auto-configuring these should not be a problem. This is not like 
configuring a signal polarity, of which one can work better, than the 
other.

> For OMAP 3 ISP driver this configuration is in struct isp_csiphy_lanes_cfg
> in drivers/media/video/omap3isp/ispcsiphy.h at the moment.
> 
> The OMAP 3 ISP driver currently uses all the lanes it has, as far as I
> understand, so you can't disable them right now.
> 
> > > I can see some use for the information in the set operation in lane
> > > configuration, for example, as you mentioned. My guess would be that the
> > > number of lanes _might_ be something that the user space would want to know
> > > or possibly even configure --- but we'll first need to discuss low-level
> > > sensor control interface.
> > > 
> > > But otherwise the configuration should likely be rather static and board
> > > specific. Wouldn't the subdevs get this as part of the platform data, or
> > > how?
> > > 
> > > I would just keep the bus configuration static board dependent information
> > > until we have that part working and used by drivers and extend it later on.
> > 
> > As I said, drivers are free to not use these methods and implement a 
> > platform-provided configuration method. In which case, as, I think, Hans 
> > pointed out earlier, they can also choose to use this struct in their 
> > platform data.
> 
> If the structures are expected to be generic I somehow feel that a field of
> flags isn't the best way to describe the configuration of CSI-2 or other
> busses. Why not to just use a structure with bus type and an union for
> bus-specific configuration parameters? It'd be easier to access and also to
> change as needed than flags in an unsigned long field.

Well, yes, a union can be a good idea, thanks.

> Also, this structure is primarily about capabilities and not configuration.
> Having settings e.g. for active low vsync and the opposite isn't making this
> easier for drivers. There should be just one --- more so if the matching
> will be specific to SoC camera only.
> 
> What would you think of having separate structures for configuration and
> capabilities of the busses? I think this way the mechanism would be more
> useful outside the scope of SoC camera. The CSI-2 and parallel busses are
> still quite generic.

Disagree, O'm quite happy with the current flag system, used for both 
capabilities and configuration.

> That said, at this point I'm not exactly sure what configuration should be
> board specific and what should be dynamically configurable. The lane setup I
> mentioned earlier, for example, is something that would be good to be
> dynamically configurable in the future. Even if there are e.g. two lanes the
> user still might want to use just one. In this case you need to be able to
> tell the hardware has that two lanes but you only use one of them.

Yes, but what exactly do you want to make configurable? The _number_ of 
lanes, or their routing?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Sakari Ailus]

Hi Guennadi,

On Mon, Jun 27, 2011 at 10:54:11AM +0200, Guennadi Liakhovetski wrote:
> Hi Sakari
> 
> On Mon, 27 Jun 2011, Sakari Ailus wrote:
> 
> > On Fri, Jun 24, 2011 at 12:35:03AM +0200, Guennadi Liakhovetski wrote:
> > > Hi Sakari
> > 
> > Hi Guennadi,
> > 
> > > On Fri, 24 Jun 2011, Sakari Ailus wrote:
> > > 
> > > > Hi Guennadi,
> > > > 
> > > > Thanks for the patch. I have a few comments below.
> > > > 
> > > > On Wed, Jun 22, 2011 at 11:26:29PM +0200, Guennadi Liakhovetski wrote:
> > 
> > [clip]
> > 
> > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > index 1562c4f..75919ef 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
> > > > >     try_mbus_fmt: try to set a pixel format on a video data source
> > > > >  
> > > > >     s_mbus_fmt: set a pixel format on a video data source
> > > > > +
> > > > > +   g_mbus_config: get supported mediabus configurations
> > > > > +
> > > > > +   s_mbus_config: set a certain mediabus configuration
> > > > >   */
> > > > >  struct v4l2_subdev_video_ops {
> > > > >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > > > > @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
> > > > >  			    struct v4l2_mbus_framefmt *fmt);
> > > > >  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
> > > > >  			  struct v4l2_mbus_framefmt *fmt);
> > > > > +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > > > +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > > 
> > > > How would the ops be used and by whom?
> > > 
> > > In the first line I want to use them to make re-use of soc-camera and 
> > > other subdev drivers possible. Soc-camera drivers would switch to these 
> > > from their specific bus-configuration methods, other drivers are free to 
> > > use or not to use them.
> > > 
> > > > How complete configuration for CSI2 bus is the above intend to be? Complete,
> > > > I suppose, and so I think we'll also need to specify how e.g. the CSI2 clock
> > > > and data lanes have been connected between transmitter and receiver. This is
> > > > less trivial to guess than clock polarity and requires further information
> > > > from the board and lane mapping configuration capabilities of both.
> > > > Shouldn't this information be also added to CSI2 configuration?
> > > > 
> > > > Do you think a single bitmask would suffice for this in the long run?
> > > 
> > > If you have certain additions to this API I would be happy to consider 
> > > them. Otherwise, if you just suppose, that we might need more 
> > > configuration options, we can always add them in the future, when we know 
> > > more about them and have specific use-cases, with which we can test them. 
> > > E.g., I am not sure what data- and clock-lane connection possibilities you 
> > > refer to above. Don't you _have_ to connect lane 1 to 1 etc?
> > 
> > You don't need to. It actually depends on the hardware: the OMAP 3 CSI-2
> > receiver, for example, may freely configure the order of the data and clock
> > lanes. And you'll actually have to do that configuration since this is
> > purely board dependent.
> 
> Nice
> 
> > I'm not sure whether the CSI-2 specification requires this, however. I
> > wouldn't be surprised if there were sensors which could also do this.
> 
> Ok, to be flexible and consistent, we could do the same as with other 
> parameters. We could design a query like "to which pin can you route data 
> lane 1?" with possible replies "I can only route lane 1 to pin 1," or "I 
> can route lane 1 to pins 1, 2, 3, 4, or 5" (for 4 data-lanes and one clock 
> lane. Then the configuration request could do "please, route your data 
> lane 1 to pin 1." Or we can hard-code this routing in the platform data 
> for now.
> 
> I, probably, already mentioned this before, but this is also how the 
> present soc-camera API evolved in the past: in the beginning it also was 
> completely static, then I noticed, that instead of hard coding the 
> configuration, I can let drivers negotiate parameters automatically. Then 
> gradually the number of auto-negotiated parameters grew, as more flexible 
> hardware had to be handled.
> 
> We could do the same here: first hard-code this, then see, if it is 
> becoming a burden, having to hard-code these. Notice, that 
> auto-configuring these should not be a problem. This is not like 
> configuring a signal polarity, of which one can work better, than the 
> other.

I'm definitely for hard coding this kind of information. The driver may
define a default order for the lanes, and then the mapping to the actual
lanes may be stored in the platform data. The same for the receiver. So the
configuration would actually be different on transmitter and receiver: it's
not a parameter which is shared among both of them.

> > For OMAP 3 ISP driver this configuration is in struct isp_csiphy_lanes_cfg
> > in drivers/media/video/omap3isp/ispcsiphy.h at the moment.
> > 
> > The OMAP 3 ISP driver currently uses all the lanes it has, as far as I
> > understand, so you can't disable them right now.
> > 
> > > > I can see some use for the information in the set operation in lane
> > > > configuration, for example, as you mentioned. My guess would be that the
> > > > number of lanes _might_ be something that the user space would want to know
> > > > or possibly even configure --- but we'll first need to discuss low-level
> > > > sensor control interface.
> > > > 
> > > > But otherwise the configuration should likely be rather static and board
> > > > specific. Wouldn't the subdevs get this as part of the platform data, or
> > > > how?
> > > > 
> > > > I would just keep the bus configuration static board dependent information
> > > > until we have that part working and used by drivers and extend it later on.
> > > 
> > > As I said, drivers are free to not use these methods and implement a 
> > > platform-provided configuration method. In which case, as, I think, Hans 
> > > pointed out earlier, they can also choose to use this struct in their 
> > > platform data.
> > 
> > If the structures are expected to be generic I somehow feel that a field of
> > flags isn't the best way to describe the configuration of CSI-2 or other
> > busses. Why not to just use a structure with bus type and an union for
> > bus-specific configuration parameters? It'd be easier to access and also to
> > change as needed than flags in an unsigned long field.
> 
> Well, yes, a union can be a good idea, thanks.
> 
> > Also, this structure is primarily about capabilities and not configuration.
> > Having settings e.g. for active low vsync and the opposite isn't making this
> > easier for drivers. There should be just one --- more so if the matching
> > will be specific to SoC camera only.
> > 
> > What would you think of having separate structures for configuration and
> > capabilities of the busses? I think this way the mechanism would be more
> > useful outside the scope of SoC camera. The CSI-2 and parallel busses are
> > still quite generic.
> 
> Disagree, O'm quite happy with the current flag system, used for both 
> capabilities and configuration.

What do you think about use outside SoC camera where we don't have
negotiation? The bus configuration should be generic and not dependent on
SoC camera. There isn't any generic CSI-2 configuration outside SoC camera
right now but I think it would make sense to have that some day.

> > That said, at this point I'm not exactly sure what configuration should be
> > board specific and what should be dynamically configurable. The lane setup I
> > mentioned earlier, for example, is something that would be good to be
> > dynamically configurable in the future. Even if there are e.g. two lanes the
> > user still might want to use just one. In this case you need to be able to
> > tell the hardware has that two lanes but you only use one of them.
> 
> Yes, but what exactly do you want to make configurable? The _number_ of 
> lanes, or their routing?

Number of the lanes. When e.g. the sensor does binning and skipping and the
output data rate is much lower than at the native output size, one could use
less lanes than at the native size. For the user it doesn't matter which
of the lanes are being used.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

On Mon, 27 Jun 2011, Sakari Ailus wrote:

> Hi Guennadi,
> 
> On Mon, Jun 27, 2011 at 10:54:11AM +0200, Guennadi Liakhovetski wrote:
> > Hi Sakari
> > 
> > On Mon, 27 Jun 2011, Sakari Ailus wrote:
> > 
> > > On Fri, Jun 24, 2011 at 12:35:03AM +0200, Guennadi Liakhovetski wrote:
> > > > Hi Sakari
> > > 
> > > Hi Guennadi,
> > > 
> > > > On Fri, 24 Jun 2011, Sakari Ailus wrote:
> > > > 
> > > > > Hi Guennadi,
> > > > > 
> > > > > Thanks for the patch. I have a few comments below.
> > > > > 
> > > > > On Wed, Jun 22, 2011 at 11:26:29PM +0200, Guennadi Liakhovetski wrote:
> > > 
> > > [clip]
> > > 
> > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > > index 1562c4f..75919ef 100644
> > > > > > --- a/include/media/v4l2-subdev.h
> > > > > > +++ b/include/media/v4l2-subdev.h
> > > > > > @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
> > > > > >     try_mbus_fmt: try to set a pixel format on a video data source
> > > > > >  
> > > > > >     s_mbus_fmt: set a pixel format on a video data source
> > > > > > +
> > > > > > +   g_mbus_config: get supported mediabus configurations
> > > > > > +
> > > > > > +   s_mbus_config: set a certain mediabus configuration
> > > > > >   */
> > > > > >  struct v4l2_subdev_video_ops {
> > > > > >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > > > > > @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
> > > > > >  			    struct v4l2_mbus_framefmt *fmt);
> > > > > >  	int (*s_mbus_fmt)(struct v4l2_subdev *sd,
> > > > > >  			  struct v4l2_mbus_framefmt *fmt);
> > > > > > +	int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > > > > +	int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > > > 
> > > > > How would the ops be used and by whom?
> > > > 
> > > > In the first line I want to use them to make re-use of soc-camera and 
> > > > other subdev drivers possible. Soc-camera drivers would switch to these 
> > > > from their specific bus-configuration methods, other drivers are free to 
> > > > use or not to use them.
> > > > 
> > > > > How complete configuration for CSI2 bus is the above intend to be? Complete,
> > > > > I suppose, and so I think we'll also need to specify how e.g. the CSI2 clock
> > > > > and data lanes have been connected between transmitter and receiver. This is
> > > > > less trivial to guess than clock polarity and requires further information
> > > > > from the board and lane mapping configuration capabilities of both.
> > > > > Shouldn't this information be also added to CSI2 configuration?
> > > > > 
> > > > > Do you think a single bitmask would suffice for this in the long run?
> > > > 
> > > > If you have certain additions to this API I would be happy to consider 
> > > > them. Otherwise, if you just suppose, that we might need more 
> > > > configuration options, we can always add them in the future, when we know 
> > > > more about them and have specific use-cases, with which we can test them. 
> > > > E.g., I am not sure what data- and clock-lane connection possibilities you 
> > > > refer to above. Don't you _have_ to connect lane 1 to 1 etc?
> > > 
> > > You don't need to. It actually depends on the hardware: the OMAP 3 CSI-2
> > > receiver, for example, may freely configure the order of the data and clock
> > > lanes. And you'll actually have to do that configuration since this is
> > > purely board dependent.
> > 
> > Nice
> > 
> > > I'm not sure whether the CSI-2 specification requires this, however. I
> > > wouldn't be surprised if there were sensors which could also do this.
> > 
> > Ok, to be flexible and consistent, we could do the same as with other 
> > parameters. We could design a query like "to which pin can you route data 
> > lane 1?" with possible replies "I can only route lane 1 to pin 1," or "I 
> > can route lane 1 to pins 1, 2, 3, 4, or 5" (for 4 data-lanes and one clock 
> > lane. Then the configuration request could do "please, route your data 
> > lane 1 to pin 1." Or we can hard-code this routing in the platform data 
> > for now.
> > 
> > I, probably, already mentioned this before, but this is also how the 
> > present soc-camera API evolved in the past: in the beginning it also was 
> > completely static, then I noticed, that instead of hard coding the 
> > configuration, I can let drivers negotiate parameters automatically. Then 
> > gradually the number of auto-negotiated parameters grew, as more flexible 
> > hardware had to be handled.
> > 
> > We could do the same here: first hard-code this, then see, if it is 
> > becoming a burden, having to hard-code these. Notice, that 
> > auto-configuring these should not be a problem. This is not like 
> > configuring a signal polarity, of which one can work better, than the 
> > other.
> 
> I'm definitely for hard coding this kind of information. The driver may
> define a default order for the lanes, and then the mapping to the actual
> lanes may be stored in the platform data. The same for the receiver. So the
> configuration would actually be different on transmitter and receiver: it's
> not a parameter which is shared among both of them.

This would be doable, you would have to introduce some "board" lane 
numbering as a common reference for both the bridge and the sensor. But 
this seems pretty far-fetched to me too, so, I'm ok with hard-coding of 
the lane routing.

> > > For OMAP 3 ISP driver this configuration is in struct isp_csiphy_lanes_cfg
> > > in drivers/media/video/omap3isp/ispcsiphy.h at the moment.
> > > 
> > > The OMAP 3 ISP driver currently uses all the lanes it has, as far as I
> > > understand, so you can't disable them right now.
> > > 
> > > > > I can see some use for the information in the set operation in lane
> > > > > configuration, for example, as you mentioned. My guess would be that the
> > > > > number of lanes _might_ be something that the user space would want to know
> > > > > or possibly even configure --- but we'll first need to discuss low-level
> > > > > sensor control interface.
> > > > > 
> > > > > But otherwise the configuration should likely be rather static and board
> > > > > specific. Wouldn't the subdevs get this as part of the platform data, or
> > > > > how?
> > > > > 
> > > > > I would just keep the bus configuration static board dependent information
> > > > > until we have that part working and used by drivers and extend it later on.
> > > > 
> > > > As I said, drivers are free to not use these methods and implement a 
> > > > platform-provided configuration method. In which case, as, I think, Hans 
> > > > pointed out earlier, they can also choose to use this struct in their 
> > > > platform data.
> > > 
> > > If the structures are expected to be generic I somehow feel that a field of
> > > flags isn't the best way to describe the configuration of CSI-2 or other
> > > busses. Why not to just use a structure with bus type and an union for
> > > bus-specific configuration parameters? It'd be easier to access and also to
> > > change as needed than flags in an unsigned long field.
> > 
> > Well, yes, a union can be a good idea, thanks.
> > 
> > > Also, this structure is primarily about capabilities and not configuration.
> > > Having settings e.g. for active low vsync and the opposite isn't making this
> > > easier for drivers. There should be just one --- more so if the matching
> > > will be specific to SoC camera only.
> > > 
> > > What would you think of having separate structures for configuration and
> > > capabilities of the busses? I think this way the mechanism would be more
> > > useful outside the scope of SoC camera. The CSI-2 and parallel busses are
> > > still quite generic.
> > 
> > Disagree, O'm quite happy with the current flag system, used for both 
> > capabilities and configuration.
> 
> What do you think about use outside SoC camera where we don't have
> negotiation? The bus configuration should be generic and not dependent on
> SoC camera. There isn't any generic CSI-2 configuration outside SoC camera
> right now but I think it would make sense to have that some day.

I think, we have discussed this already. Other drivers do not want any 
dynamic configuration, so, they only implement the "g" method, and the 
subdevices return only one of the alternatives for each parameter. For 
example, the sensor would say "I only support HSYNC active high, PCLK 
sampled on the falling edge" etc. Don't see any problem with this.

> > > That said, at this point I'm not exactly sure what configuration should be
> > > board specific and what should be dynamically configurable. The lane setup I
> > > mentioned earlier, for example, is something that would be good to be
> > > dynamically configurable in the future. Even if there are e.g. two lanes the
> > > user still might want to use just one. In this case you need to be able to
> > > tell the hardware has that two lanes but you only use one of them.
> > 
> > Yes, but what exactly do you want to make configurable? The _number_ of 
> > lanes, or their routing?
> 
> Number of the lanes. When e.g. the sensor does binning and skipping and the
> output data rate is much lower than at the native output size, one could use
> less lanes than at the native size. For the user it doesn't matter which
> of the lanes are being used.

Ok

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

On Mon, 27 Jun 2011, Guennadi Liakhovetski wrote:

[snip]

> > If the structures are expected to be generic I somehow feel that a field of
> > flags isn't the best way to describe the configuration of CSI-2 or other
> > busses. Why not to just use a structure with bus type and an union for
> > bus-specific configuration parameters? It'd be easier to access and also to
> > change as needed than flags in an unsigned long field.
> 
> Well, yes, a union can be a good idea, thanks.

...on a second thought, we currently only have one field: flags, and it is 
common for all 3 bus types: parallel, reduced parallel (bt.656, etc.), and 
CSI-2. In the future, when we need more parameters for any of these busses 
we'll just add such a union, shouldn't be a problem.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Sakari Ailus]

Guennadi Liakhovetski wrote:
> On Mon, 27 Jun 2011, Guennadi Liakhovetski wrote:
> 
> [snip]
> 
>>> If the structures are expected to be generic I somehow feel that a field of
>>> flags isn't the best way to describe the configuration of CSI-2 or other
>>> busses. Why not to just use a structure with bus type and an union for
>>> bus-specific configuration parameters? It'd be easier to access and also to
>>> change as needed than flags in an unsigned long field.
>>
>> Well, yes, a union can be a good idea, thanks.
> 
> ...on a second thought, we currently only have one field: flags, and it is 
> common for all 3 bus types: parallel, reduced parallel (bt.656, etc.), and 
> CSI-2. In the future, when we need more parameters for any of these busses 
> we'll just add such a union, shouldn't be a problem.

What I meant above was that I would prefer to describe the capabilities
in a structure which would contain appropriate data type for the field,
not as flags or sets of flags in a bit field.

This would allow e.g. just testing for
v4l2_mbus_config.u.parallel.hsync_active_low instead of
v4l2_mbus_config.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW. This way the flags
used for the bus also express the bus explicitly rather than implicitly.

Do you see downsides with this compared to using an integer field as
flags? The other benefits of this are described in my earlier comment.

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

On Wed, 29 Jun 2011, Sakari Ailus wrote:

> Guennadi Liakhovetski wrote:
> > On Mon, 27 Jun 2011, Guennadi Liakhovetski wrote:
> > 
> > [snip]
> > 
> >>> If the structures are expected to be generic I somehow feel that a field of
> >>> flags isn't the best way to describe the configuration of CSI-2 or other
> >>> busses. Why not to just use a structure with bus type and an union for
> >>> bus-specific configuration parameters? It'd be easier to access and also to
> >>> change as needed than flags in an unsigned long field.
> >>
> >> Well, yes, a union can be a good idea, thanks.
> > 
> > ...on a second thought, we currently only have one field: flags, and it is 
> > common for all 3 bus types: parallel, reduced parallel (bt.656, etc.), and 
> > CSI-2. In the future, when we need more parameters for any of these busses 
> > we'll just add such a union, shouldn't be a problem.
> 
> What I meant above was that I would prefer to describe the capabilities
> in a structure which would contain appropriate data type for the field,
> not as flags or sets of flags in a bit field.
> 
> This would allow e.g. just testing for
> v4l2_mbus_config.u.parallel.hsync_active_low instead of
> v4l2_mbus_config.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW. This way the flags
> used for the bus also express the bus explicitly rather than implicitly.

I don't think, using flags is any less explicit, than using struct 
members. As I already explained, I'd like to use the same interface for 
querying capabilities, the present configuration, and setting a 
configuration. This is easily achieved with flags. See the 
v4l2_mbus_config_compatible() function and try to rewrite all the bitwise 
operations in it, using your struct.

Thanks
Guennadi

> 
> Do you see downsides with this compared to using an integer field as
> flags? The other benefits of this are described in my earlier comment.
> 
> Regards,
> 
> -- 
> Sakari Ailus
> sakari.ailus@iki.fi
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Sakari Ailus]

On Wed, Jun 29, 2011 at 09:28:06PM +0200, Guennadi Liakhovetski wrote:
> On Wed, 29 Jun 2011, Sakari Ailus wrote:
> 
> > Guennadi Liakhovetski wrote:
> > > On Mon, 27 Jun 2011, Guennadi Liakhovetski wrote:
> > > 
> > > [snip]
> > > 
> > >>> If the structures are expected to be generic I somehow feel that a field of
> > >>> flags isn't the best way to describe the configuration of CSI-2 or other
> > >>> busses. Why not to just use a structure with bus type and an union for
> > >>> bus-specific configuration parameters? It'd be easier to access and also to
> > >>> change as needed than flags in an unsigned long field.
> > >>
> > >> Well, yes, a union can be a good idea, thanks.
> > > 
> > > ...on a second thought, we currently only have one field: flags, and it is 
> > > common for all 3 bus types: parallel, reduced parallel (bt.656, etc.), and 
> > > CSI-2. In the future, when we need more parameters for any of these busses 
> > > we'll just add such a union, shouldn't be a problem.
> > 
> > What I meant above was that I would prefer to describe the capabilities
> > in a structure which would contain appropriate data type for the field,
> > not as flags or sets of flags in a bit field.
> > 
> > This would allow e.g. just testing for
> > v4l2_mbus_config.u.parallel.hsync_active_low instead of
> > v4l2_mbus_config.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW. This way the flags
> > used for the bus also express the bus explicitly rather than implicitly.
> 
> I don't think, using flags is any less explicit, than using struct 
> members. As I already explained, I'd like to use the same interface for 
> querying capabilities, the present configuration, and setting a 
> configuration. This is easily achieved with flags. See the 
> v4l2_mbus_config_compatible() function and try to rewrite all the bitwise 
> operations in it, using your struct.

I still don't like the use of flags this way. I do admit that
v4l2_mbus_config_compatible() might be more difficult to write. It's still a
tiny piece of code. Speaking of which, should it be part of the latest
patch?

As far as I understand, v4l2_mbus_config_compatible() is meant for SoC
camera compatibility only. I would like that the interface which is not SoC
camera dependent would not get compatibility burden right from the
beginning. That said, it's a nice property that e.g. sensor drivers written
for SoC camera would work without it as well --- is this what the patch
would achieve, or a part of it?

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2] V4L: add media bus configuration subdev operations

[Guennadi Liakhovetski]

On Thu, 30 Jun 2011, Sakari Ailus wrote:

> On Wed, Jun 29, 2011 at 09:28:06PM +0200, Guennadi Liakhovetski wrote:
> > On Wed, 29 Jun 2011, Sakari Ailus wrote:
> > 
> > > Guennadi Liakhovetski wrote:
> > > > On Mon, 27 Jun 2011, Guennadi Liakhovetski wrote:
> > > > 
> > > > [snip]
> > > > 
> > > >>> If the structures are expected to be generic I somehow feel that a field of
> > > >>> flags isn't the best way to describe the configuration of CSI-2 or other
> > > >>> busses. Why not to just use a structure with bus type and an union for
> > > >>> bus-specific configuration parameters? It'd be easier to access and also to
> > > >>> change as needed than flags in an unsigned long field.
> > > >>
> > > >> Well, yes, a union can be a good idea, thanks.
> > > > 
> > > > ...on a second thought, we currently only have one field: flags, and it is 
> > > > common for all 3 bus types: parallel, reduced parallel (bt.656, etc.), and 
> > > > CSI-2. In the future, when we need more parameters for any of these busses 
> > > > we'll just add such a union, shouldn't be a problem.
> > > 
> > > What I meant above was that I would prefer to describe the capabilities
> > > in a structure which would contain appropriate data type for the field,
> > > not as flags or sets of flags in a bit field.
> > > 
> > > This would allow e.g. just testing for
> > > v4l2_mbus_config.u.parallel.hsync_active_low instead of
> > > v4l2_mbus_config.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW. This way the flags
> > > used for the bus also express the bus explicitly rather than implicitly.
> > 
> > I don't think, using flags is any less explicit, than using struct 
> > members. As I already explained, I'd like to use the same interface for 
> > querying capabilities, the present configuration, and setting a 
> > configuration. This is easily achieved with flags. See the 
> > v4l2_mbus_config_compatible() function and try to rewrite all the bitwise 
> > operations in it, using your struct.
> 
> I still don't like the use of flags this way. I do admit that
> v4l2_mbus_config_compatible() might be more difficult to write. It's still a
> tiny piece of code. Speaking of which, should it be part of the latest
> patch?

Hm, indeed, I've dropped that hunk from the last patch version 
accidentally. Thanks for pointing out. In principle, it's the same as the 
previous version with just two cosmetic changes: flags changed to unsigned 
int and a comment added for falling through in a switch-case statement.

> As far as I understand, v4l2_mbus_config_compatible() is meant for SoC
> camera compatibility only. I would like that the interface which is not SoC
> camera dependent would not get compatibility burden right from the
> beginning. That said, it's a nice property that e.g. sensor drivers written
> for SoC camera would work without it as well --- is this what the patch
> would achieve, or a part of it?

No worries, we can drop that function and I will keep it soc-camera 
specific, if noone else finds it useful.

And, yes, I've replied several times already, we want to re-use sensor 
drivers.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/