* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 12:31 [PATCH/RFC] V4L: add media bus configuration subdev operations Guennadi Liakhovetski
@ 2011-06-06 12:54 ` Hans Verkuil
2011-06-06 13:06 ` Guennadi Liakhovetski
2011-06-06 14:22 ` Aguirre, Sergio
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Hans Verkuil @ 2011-06-06 12:54 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus,
Sylwester Nawrocki, Stan, Hans Verkuil, saaguirre,
Mauro Carvalho Chehab
> 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>
> ---
>
> This change would allow a re-use of soc-camera and "standard" subdev
> drivers. It is a modified and extended version of
>
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
>
> therefore the original Sob. After this we only would have to switch to the
> control framework:) Please, comment.
>
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 971c7fa..0983b7b 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -13,6 +13,76 @@
>
> #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)
> +/* Which datawidths are supported */
> +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
> +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
> +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
> +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
> +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
> +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
> +
> +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 |
> V4L2_MBUS_DATAWIDTH_8 | \
> + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
> + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
This is too limited. Video receivers for example can use 8, 10, 12, 20,
24, 30 and 36 data widths. Perhaps we should have a u64 bitmask instead.
Bit 0 is a width of 1, bit 63 is a width of 64. It's much easier to
understand.
> +
> +/* 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-contimuous 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: interface type
> + * @flags: configuration flags, depending on @type
> + * @clk: output clock, the bridge driver can try to use clk_set_parent()
> + * to specify the master clock to the client
> + */
> +struct v4l2_mbus_config {
> + enum v4l2_mbus_type type;
> + unsigned long flags;
> + struct clk *clk;
> +};
> +
> 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..6ea25f4 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_param: get supported mediabus configurations
> +
> + s_mbus_param: 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_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config
> *cfg);
The struct and op should either use the term 'config' or the term 'param',
but not mix them.
I also strongly recommend that sensor drivers can accept a struct
v4l2_mbus_config as part of their platform_data to initialize the sensor
config at load time (and allow for hardcoding in board code).
Regards,
Hans
> + int (*s_mbus_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config
> *cfg);
> };
>
> /*
> --
> 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 12:54 ` Hans Verkuil
@ 2011-06-06 13:06 ` Guennadi Liakhovetski
2011-06-06 13:24 ` Hans Verkuil
0 siblings, 1 reply; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-06-06 13:06 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus,
Sylwester Nawrocki, Stan, Hans Verkuil, saaguirre,
Mauro Carvalho Chehab
Hi Hans
Thanks for your comments
On Mon, 6 Jun 2011, Hans Verkuil 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>
> > ---
> >
> > This change would allow a re-use of soc-camera and "standard" subdev
> > drivers. It is a modified and extended version of
> >
> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
> >
> > therefore the original Sob. After this we only would have to switch to the
> > control framework:) Please, comment.
> >
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 971c7fa..0983b7b 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -13,6 +13,76 @@
> >
> > #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)
> > +/* Which datawidths are supported */
> > +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
> > +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
> > +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
> > +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
> > +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
> > +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
> > +
> > +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 |
> > V4L2_MBUS_DATAWIDTH_8 | \
> > + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
> > + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
>
> This is too limited. Video receivers for example can use 8, 10, 12, 20,
> 24, 30 and 36 data widths. Perhaps we should have a u64 bitmask instead.
> Bit 0 is a width of 1, bit 63 is a width of 64. It's much easier to
> understand.
So, you want a separate 64-bit bitmask field in struct v4l2_mbus_config
only for parallel bus width? Ok, can do that, np.
>
> > +
> > +/* 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-contimuous 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: interface type
> > + * @flags: configuration flags, depending on @type
> > + * @clk: output clock, the bridge driver can try to use clk_set_parent()
> > + * to specify the master clock to the client
> > + */
> > +struct v4l2_mbus_config {
> > + enum v4l2_mbus_type type;
> > + unsigned long flags;
> > + struct clk *clk;
> > +};
> > +
> > 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..6ea25f4 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_param: get supported mediabus configurations
> > +
> > + s_mbus_param: 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_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config
> > *cfg);
>
> The struct and op should either use the term 'config' or the term 'param',
> but not mix them.
Ok
> I also strongly recommend that sensor drivers can accept a struct
> v4l2_mbus_config as part of their platform_data to initialize the sensor
> config at load time (and allow for hardcoding in board code).
Sure, subdev drivers are free to do so. Also, these methods are designed
to be optional.
Thanks
Guennadi
> Regards,
>
> Hans
>
> > + int (*s_mbus_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config
> > *cfg);
> > };
> >
> > /*
> > --
> > 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
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 13:06 ` Guennadi Liakhovetski
@ 2011-06-06 13:24 ` Hans Verkuil
0 siblings, 0 replies; 12+ messages in thread
From: Hans Verkuil @ 2011-06-06 13:24 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus,
Sylwester Nawrocki, Stan, Hans Verkuil, saaguirre,
Mauro Carvalho Chehab
> Hi Hans
>
> Thanks for your comments
>
> On Mon, 6 Jun 2011, Hans Verkuil 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>
>> > ---
>> >
>> > This change would allow a re-use of soc-camera and "standard" subdev
>> > drivers. It is a modified and extended version of
>> >
>> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
>> >
>> > therefore the original Sob. After this we only would have to switch to
>> the
>> > control framework:) Please, comment.
>> >
>> > diff --git a/include/media/v4l2-mediabus.h
>> b/include/media/v4l2-mediabus.h
>> > index 971c7fa..0983b7b 100644
>> > --- a/include/media/v4l2-mediabus.h
>> > +++ b/include/media/v4l2-mediabus.h
>> > @@ -13,6 +13,76 @@
>> >
>> > #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)
>> > +/* Which datawidths are supported */
>> > +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
>> > +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
>> > +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
>> > +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
>> > +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
>> > +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
>> > +
>> > +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 |
>> > V4L2_MBUS_DATAWIDTH_8 | \
>> > + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
>> > + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
>>
>> This is too limited. Video receivers for example can use 8, 10, 12, 20,
>> 24, 30 and 36 data widths. Perhaps we should have a u64 bitmask instead.
>> Bit 0 is a width of 1, bit 63 is a width of 64. It's much easier to
>> understand.
>
> So, you want a separate 64-bit bitmask field in struct v4l2_mbus_config
> only for parallel bus width? Ok, can do that, np.
I've changed my mind on this. Keep it as is.
Although it would be nice if you can add support for widths 12, 20, 24, 30
and 36. That covers those video receivers/transmitters that I am aware of.
Regards,
Hans
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 12:31 [PATCH/RFC] V4L: add media bus configuration subdev operations Guennadi Liakhovetski
2011-06-06 12:54 ` Hans Verkuil
@ 2011-06-06 14:22 ` Aguirre, Sergio
2011-06-06 14:37 ` Guennadi Liakhovetski
2011-06-06 21:27 ` Sylwester Nawrocki
2011-06-07 15:25 ` Laurent Pinchart
3 siblings, 1 reply; 12+ messages in thread
From: Aguirre, Sergio @ 2011-06-06 14:22 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus,
Sylwester Nawrocki, Stan, Hans Verkuil, Mauro Carvalho Chehab
Hi Guennadi,
Thanks for the patch.
On Mon, Jun 6, 2011 at 7:31 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> 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>
> ---
>
> This change would allow a re-use of soc-camera and "standard" subdev
> drivers. It is a modified and extended version of
>
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
>
> therefore the original Sob. After this we only would have to switch to the
> control framework:) Please, comment.
>
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 971c7fa..0983b7b 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -13,6 +13,76 @@
>
> #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)
> +/* Which datawidths are supported */
> +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
> +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
> +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
> +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
> +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
> +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
> +
> +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 | V4L2_MBUS_DATAWIDTH_8 | \
> + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
> + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
> +
> +/* 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-contimuous clock mode */
Typo: non-continuous
> +#define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK (1 << 8)
Doesn't having above bit disabled, imply a non-continuous clock already?
> +#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK (1 << 9)
Regards,
Sergio
> +
> +#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: interface type
> + * @flags: configuration flags, depending on @type
> + * @clk: output clock, the bridge driver can try to use clk_set_parent()
> + * to specify the master clock to the client
> + */
> +struct v4l2_mbus_config {
> + enum v4l2_mbus_type type;
> + unsigned long flags;
> + struct clk *clk;
> +};
> +
> 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..6ea25f4 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_param: get supported mediabus configurations
> +
> + s_mbus_param: 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_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> + int (*s_mbus_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> };
>
> /*
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 14:22 ` Aguirre, Sergio
@ 2011-06-06 14:37 ` Guennadi Liakhovetski
2011-06-06 14:41 ` Aguirre, Sergio
0 siblings, 1 reply; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-06-06 14:37 UTC (permalink / raw)
To: Aguirre, Sergio
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus,
Sylwester Nawrocki, Stan, Hans Verkuil, Mauro Carvalho Chehab
Hi Sergio
On Mon, 6 Jun 2011, Aguirre, Sergio wrote:
> Hi Guennadi,
>
> Thanks for the patch.
>
> On Mon, Jun 6, 2011 at 7:31 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> 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>
> > ---
> >
> > This change would allow a re-use of soc-camera and "standard" subdev
> > drivers. It is a modified and extended version of
> >
> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
> >
> > therefore the original Sob. After this we only would have to switch to the
> > control framework:) Please, comment.
> >
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 971c7fa..0983b7b 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -13,6 +13,76 @@
> >
> > #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)
> > +/* Which datawidths are supported */
> > +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
> > +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
> > +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
> > +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
> > +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
> > +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
> > +
> > +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 | V4L2_MBUS_DATAWIDTH_8 | \
> > + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
> > + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
> > +
> > +/* 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-contimuous clock mode */
>
> Typo: non-continuous
Right, thanks:)
> > +#define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK (1 << 8)
>
> Doesn't having above bit disabled, imply a non-continuous clock already?
Well, actually, yes, we coult drop one of these, because continuous clock
mode is obligatory, so, we can just always assume, that all clients
support it and only check whether they _also_ support non-continuous.
Similarly when setting - if the non-continuous flag is not set, obviously,
the subdev has to configure the continuous mode. But if we ever encounter
a device, that only supports the non-continuous mode, we get a problem:)
Also, maybe the driver for some reason decides not to accept the
continuous mode, so, I think, it's better to keep both.
Thanks
Guennadi
>
> > +#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK (1 << 9)
>
> Regards,
> Sergio
>
> > +
> > +#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: interface type
> > + * @flags: configuration flags, depending on @type
> > + * @clk: output clock, the bridge driver can try to use clk_set_parent()
> > + * to specify the master clock to the client
> > + */
> > +struct v4l2_mbus_config {
> > + enum v4l2_mbus_type type;
> > + unsigned long flags;
> > + struct clk *clk;
> > +};
> > +
> > 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..6ea25f4 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_param: get supported mediabus configurations
> > +
> > + s_mbus_param: 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_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > + int (*s_mbus_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > };
> >
> > /*
> >
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 14:37 ` Guennadi Liakhovetski
@ 2011-06-06 14:41 ` Aguirre, Sergio
0 siblings, 0 replies; 12+ messages in thread
From: Aguirre, Sergio @ 2011-06-06 14:41 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus,
Sylwester Nawrocki, Stan, Hans Verkuil, Mauro Carvalho Chehab
Hi Guennadi,
On Mon, Jun 6, 2011 at 9:37 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi Sergio
>
> On Mon, 6 Jun 2011, Aguirre, Sergio wrote:
>
>> Hi Guennadi,
>>
>> Thanks for the patch.
>>
>> On Mon, Jun 6, 2011 at 7:31 AM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> 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>
>> > ---
>> >
>> > This change would allow a re-use of soc-camera and "standard" subdev
>> > drivers. It is a modified and extended version of
>> >
>> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
>> >
>> > therefore the original Sob. After this we only would have to switch to the
>> > control framework:) Please, comment.
>> >
>> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
>> > index 971c7fa..0983b7b 100644
>> > --- a/include/media/v4l2-mediabus.h
>> > +++ b/include/media/v4l2-mediabus.h
>> > @@ -13,6 +13,76 @@
>> >
>> > #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)
>> > +/* Which datawidths are supported */
>> > +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
>> > +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
>> > +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
>> > +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
>> > +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
>> > +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
>> > +
>> > +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 | V4L2_MBUS_DATAWIDTH_8 | \
>> > + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
>> > + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
>> > +
>> > +/* 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-contimuous clock mode */
>>
>> Typo: non-continuous
>
> Right, thanks:)
>
>> > +#define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK (1 << 8)
>>
>> Doesn't having above bit disabled, imply a non-continuous clock already?
>
> Well, actually, yes, we coult drop one of these, because continuous clock
> mode is obligatory, so, we can just always assume, that all clients
> support it and only check whether they _also_ support non-continuous.
> Similarly when setting - if the non-continuous flag is not set, obviously,
> the subdev has to configure the continuous mode. But if we ever encounter
> a device, that only supports the non-continuous mode, we get a problem:)
> Also, maybe the driver for some reason decides not to accept the
> continuous mode, so, I think, it's better to keep both.
Oh ok, I see now. I wasn't getting that before. :)
Thanks for the explanation.
Regards,
Sergio
>
> Thanks
> Guennadi
>
>>
>> > +#define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK (1 << 9)
>>
>> Regards,
>> Sergio
>>
>> > +
>> > +#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: interface type
>> > + * @flags: configuration flags, depending on @type
>> > + * @clk: output clock, the bridge driver can try to use clk_set_parent()
>> > + * to specify the master clock to the client
>> > + */
>> > +struct v4l2_mbus_config {
>> > + enum v4l2_mbus_type type;
>> > + unsigned long flags;
>> > + struct clk *clk;
>> > +};
>> > +
>> > 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..6ea25f4 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_param: get supported mediabus configurations
>> > +
>> > + s_mbus_param: 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_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
>> > + int (*s_mbus_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
>> > };
>> >
>> > /*
>> >
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 12:31 [PATCH/RFC] V4L: add media bus configuration subdev operations Guennadi Liakhovetski
2011-06-06 12:54 ` Hans Verkuil
2011-06-06 14:22 ` Aguirre, Sergio
@ 2011-06-06 21:27 ` Sylwester Nawrocki
2011-06-06 22:43 ` Guennadi Liakhovetski
2011-06-07 15:25 ` Laurent Pinchart
3 siblings, 1 reply; 12+ messages in thread
From: Sylwester Nawrocki @ 2011-06-06 21:27 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus, Stan,
Hans Verkuil, saaguirre, Mauro Carvalho Chehab
Hi Guennadi,
On 06/06/2011 02:31 PM, 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>
> ---
>
> This change would allow a re-use of soc-camera and "standard" subdev
> drivers. It is a modified and extended version of
>
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
>
> therefore the original Sob. After this we only would have to switch to the
> control framework:) Please, comment.
>
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 971c7fa..0983b7b 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -13,6 +13,76 @@
>
> #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)
> +/* Which datawidths are supported */
> +#define V4L2_MBUS_DATAWIDTH_4 (1<< 10)
> +#define V4L2_MBUS_DATAWIDTH_8 (1<< 11)
> +#define V4L2_MBUS_DATAWIDTH_9 (1<< 12)
> +#define V4L2_MBUS_DATAWIDTH_10 (1<< 13)
> +#define V4L2_MBUS_DATAWIDTH_15 (1<< 14)
> +#define V4L2_MBUS_DATAWIDTH_16 (1<< 15)
> +
> +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 | V4L2_MBUS_DATAWIDTH_8 | \
> + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
> + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
> +
> +/* 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-contimuous 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: interface type
> + * @flags: configuration flags, depending on @type
> + * @clk: output clock, the bridge driver can try to use clk_set_parent()
> + * to specify the master clock to the client
> + */
> +struct v4l2_mbus_config {
> + enum v4l2_mbus_type type;
> + unsigned long flags;
> + struct clk *clk;
This clock is interesting, however it is not quite clear to me
what was your original idea on how it should be used.
Is it supposed to be managed by the bridge drivers only?
Or is it intended to be gated by subdev drivers with the clock API?
I think embedding struct clk here might be a good solution to pass
clock frequency information around the host and subdev drivers
(e.g. to mitigate the rounding errors).
Still the host drivers would individually need to be provided
by the board code with clock frequency information per each subdev.
But as the clock usually needs to be enabled before registering a subdev
it does not seem to make sense to include the frequency information
in struct v4l2_mbus_config.
Sensors could just use clk_get_rate() to figure out what is their master
clock rate.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 21:27 ` Sylwester Nawrocki
@ 2011-06-06 22:43 ` Guennadi Liakhovetski
0 siblings, 0 replies; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-06-06 22:43 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Linux Media Mailing List, Laurent Pinchart, sakari.ailus, Stan,
Hans Verkuil, saaguirre, Mauro Carvalho Chehab
On Mon, 6 Jun 2011, Sylwester Nawrocki wrote:
> Hi Guennadi,
>
> On 06/06/2011 02:31 PM, 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>
> > ---
> >
> > This change would allow a re-use of soc-camera and "standard" subdev
> > drivers. It is a modified and extended version of
> >
> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/29408
> >
> > therefore the original Sob. After this we only would have to switch to the
> > control framework:) Please, comment.
> >
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 971c7fa..0983b7b 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -13,6 +13,76 @@
> >
> > #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)
> > +/* Which datawidths are supported */
> > +#define V4L2_MBUS_DATAWIDTH_4 (1<< 10)
> > +#define V4L2_MBUS_DATAWIDTH_8 (1<< 11)
> > +#define V4L2_MBUS_DATAWIDTH_9 (1<< 12)
> > +#define V4L2_MBUS_DATAWIDTH_10 (1<< 13)
> > +#define V4L2_MBUS_DATAWIDTH_15 (1<< 14)
> > +#define V4L2_MBUS_DATAWIDTH_16 (1<< 15)
> > +
> > +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 | V4L2_MBUS_DATAWIDTH_8 | \
> > + V4L2_MBUS_DATAWIDTH_9 | V4L2_MBUS_DATAWIDTH_10 | \
> > + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
> > +
> > +/* 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-contimuous 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: interface type
> > + * @flags: configuration flags, depending on @type
> > + * @clk: output clock, the bridge driver can try to use clk_set_parent()
> > + * to specify the master clock to the client
> > + */
> > +struct v4l2_mbus_config {
> > + enum v4l2_mbus_type type;
> > + unsigned long flags;
> > + struct clk *clk;
>
> This clock is interesting, however it is not quite clear to me
> what was your original idea on how it should be used.
> Is it supposed to be managed by the bridge drivers only?
>
> Or is it intended to be gated by subdev drivers with the clock API?
>
> I think embedding struct clk here might be a good solution to pass
> clock frequency information around the host and subdev drivers
> (e.g. to mitigate the rounding errors).
>
> Still the host drivers would individually need to be provided
> by the board code with clock frequency information per each subdev.
> But as the clock usually needs to be enabled before registering a subdev
> it does not seem to make sense to include the frequency information
> in struct v4l2_mbus_config.
> Sensors could just use clk_get_rate() to figure out what is their master
> clock rate.
The idea to include a pointer to struct clk originates from various
clocking related fields from the original patch by Stanimir. However, I
decided, that that was too unflixible and decided to use the clock API to
handle the pixel clock in "advanced" cases. In simple cases this pointer
can stay NULL and the bridge / host driver will have no information about
the pixel clock frequency. It will just rely on clock actually running and
will obtain data as fast as it arrives. This is also what many soc-camera
host drivers are currently doing. In more complex cases the subdev would
register a clock device during probing, using its platform data to decide
upon the frequency and the parent(s). Later, having retrieved the bus
configuration from the subdevice, the bridge will see a clock, then it can
_try_ to reparent it to its own master clock - if desired, if needed, it
can use the clock API to set and get the clock frequency. This scheme
should pretty well cover both simple cases, where the master clock is used
1-to-1 for the pixel clock, and complex cases, where the subdev includes
programmable PLLs and dividers, and where the board includes logic to
select one of several possible sources for the camera clock. This clock
can also be bound into the runtime PM subsystem, if desired.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-06 12:31 [PATCH/RFC] V4L: add media bus configuration subdev operations Guennadi Liakhovetski
` (2 preceding siblings ...)
2011-06-06 21:27 ` Sylwester Nawrocki
@ 2011-06-07 15:25 ` Laurent Pinchart
2011-06-08 6:40 ` Sakari Ailus
2011-06-08 16:42 ` Laurent Pinchart
3 siblings, 2 replies; 12+ messages in thread
From: Laurent Pinchart @ 2011-06-07 15:25 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Linux Media Mailing List, sakari.ailus, Sylwester Nawrocki, Stan,
Hans Verkuil, saaguirre, Mauro Carvalho Chehab
Hi Guennadi,
Thanks for the patch.
On Monday 06 June 2011 14:31:57 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>
> ---
>
> This change would allow a re-use of soc-camera and "standard" subdev
> drivers. It is a modified and extended version of
>
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/294
> 08
>
> therefore the original Sob. After this we only would have to switch to the
> control framework:) Please, comment.
I still believe we shouldn't use any set operation :-) The host/bridge driver
should just use the get operation before starting the stream to configure it's
sensor interface.
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 971c7fa..0983b7b 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -13,6 +13,76 @@
>
> #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)
> +/* Which datawidths are supported */
> +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
> +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
> +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
> +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
> +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
> +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
> +
> +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 |
> V4L2_MBUS_DATAWIDTH_8 | \ + V4L2_MBUS_DATAWIDTH_9 |
> V4L2_MBUS_DATAWIDTH_10 | \
> + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
The mbus data width is selected by the mbus format. Why do we need an explicit
width here ?
> +/* 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-contimuous 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: interface type
> + * @flags: configuration flags, depending on @type
> + * @clk: output clock, the bridge driver can try to use clk_set_parent()
> + * to specify the master clock to the client
> + */
> +struct v4l2_mbus_config {
> + enum v4l2_mbus_type type;
> + unsigned long flags;
> + struct clk *clk;
> +};
struct clk is being generalized as part of the ARM clock consolidation work,
but we're not there yet. I don't think we can use it yet.
> +
> 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..6ea25f4 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_param: get supported mediabus configurations
> +
> + s_mbus_param: 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_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config
> *cfg); + int (*s_mbus_param)(struct v4l2_subdev *sd, struct
> v4l2_mbus_config *cfg); };
>
> /*
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-07 15:25 ` Laurent Pinchart
@ 2011-06-08 6:40 ` Sakari Ailus
2011-06-08 16:42 ` Laurent Pinchart
1 sibling, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2011-06-08 6:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Guennadi Liakhovetski, Linux Media Mailing List, sakari.ailus,
Sylwester Nawrocki, Stan, Hans Verkuil, saaguirre,
Mauro Carvalho Chehab
On Tue, Jun 07, 2011 at 05:25:32PM +0200, Laurent Pinchart wrote:
> Hi Guennadi,
>
> Thanks for the patch.
>
> On Monday 06 June 2011 14:31:57 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>
> > ---
> >
> > This change would allow a re-use of soc-camera and "standard" subdev
> > drivers. It is a modified and extended version of
> >
> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/294
> > 08
> >
> > therefore the original Sob. After this we only would have to switch to the
> > control framework:) Please, comment.
>
> I still believe we shouldn't use any set operation :-) The host/bridge driver
> should just use the get operation before starting the stream to configure it's
> sensor interface.
I agree with Laurent.
What about implementing just get first?
Regards,
--
Sakari Ailus
sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH/RFC] V4L: add media bus configuration subdev operations
2011-06-07 15:25 ` Laurent Pinchart
2011-06-08 6:40 ` Sakari Ailus
@ 2011-06-08 16:42 ` Laurent Pinchart
1 sibling, 0 replies; 12+ messages in thread
From: Laurent Pinchart @ 2011-06-08 16:42 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Linux Media Mailing List, sakari.ailus, Sylwester Nawrocki, Stan,
Hans Verkuil, saaguirre, Mauro Carvalho Chehab
Replying to myself, after a chat with Guennadi.
On Tuesday 07 June 2011 17:25:32 Laurent Pinchart wrote:
> On Monday 06 June 2011 14:31:57 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>
> > ---
> >
> > This change would allow a re-use of soc-camera and "standard" subdev
> > drivers. It is a modified and extended version of
> >
> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/2
> > 94 08
> >
> > therefore the original Sob. After this we only would have to switch to
> > the control framework:) Please, comment.
>
> I still believe we shouldn't use any set operation :-) The host/bridge
> driver should just use the get operation before starting the stream to
> configure it's sensor interface.
I can be convinced to accept a set mbus config operation to allow existing
soc-camera drivers to be ported to the new API without requiring hardcoding
all the related configurations in board code in one go. However, I'm still not
convinced that we should allow new drivers to use the set operation, which
should in my (current) opinion be temporary only.
I will try this patch with an OMAP3 platform that uses CSI2 sensors and check
whether the features I require are present.
> > diff --git a/include/media/v4l2-mediabus.h
> > b/include/media/v4l2-mediabus.h index 971c7fa..0983b7b 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -13,6 +13,76 @@
> >
> > #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)
> > +/* Which datawidths are supported */
> > +#define V4L2_MBUS_DATAWIDTH_4 (1 << 10)
> > +#define V4L2_MBUS_DATAWIDTH_8 (1 << 11)
> > +#define V4L2_MBUS_DATAWIDTH_9 (1 << 12)
> > +#define V4L2_MBUS_DATAWIDTH_10 (1 << 13)
> > +#define V4L2_MBUS_DATAWIDTH_15 (1 << 14)
> > +#define V4L2_MBUS_DATAWIDTH_16 (1 << 15)
> > +
> > +#define V4L2_MBUS_DATAWIDTH_MASK (V4L2_MBUS_DATAWIDTH_4 |
> > V4L2_MBUS_DATAWIDTH_8 | \ + V4L2_MBUS_DATAWIDTH_9 |
> > V4L2_MBUS_DATAWIDTH_10 | \
> > + V4L2_MBUS_DATAWIDTH_15 | V4L2_MBUS_DATAWIDTH_16)
>
> The mbus data width is selected by the mbus format. Why do we need an
> explicit width here ?
This comment still holds.
> > +/* 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-contimuous 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: interface type
> > + * @flags: configuration flags, depending on @type
> > + * @clk: output clock, the bridge driver can try to use
clk_set_parent()
> > + * to specify the master clock to the client
> > + */
> > +struct v4l2_mbus_config {
> > + enum v4l2_mbus_type type;
> > + unsigned long flags;
> > + struct clk *clk;
> > +};
>
> struct clk is being generalized as part of the ARM clock consolidation
> work, but we're not there yet. I don't think we can use it yet.
And this one as well. There's no system-wide struct clk at the moment.
> > +
> >
> > 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..6ea25f4 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_param: get supported mediabus configurations
> > +
> > + s_mbus_param: 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_param)(struct v4l2_subdev *sd, struct v4l2_mbus_config
> > *cfg); + int (*s_mbus_param)(struct v4l2_subdev *sd, struct
> > v4l2_mbus_config *cfg); };
> >
> > /*
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread