Re: [RFC 1/3] v4l: Add pixel clock to struct v4l2_mbus_framefmt

[Sylwester Nawrocki <snjw23@gmail.com>]

Hi Sakari,

thanks for the patch.

On 12/14/2011 04:22 PM, Sakari Ailus wrote:
> Pixel clock is an essential part of the image data parameters. Add this.
> Together, the current parameters also define the frame rate.
> 
> Sensors do not have a concept of frame rate; pixel clock is much more
> meaningful in this context. Also, it is best to combine the pixel clock with
> the other format parameters since there are dependencies between them.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  Documentation/DocBook/media/v4l/subdev-formats.xml |    9 ++++++++-
>  include/linux/v4l2-mediabus.h                      |    4 +++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
> index 49c532e..b4591ef 100644
> --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
> +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
> @@ -35,7 +35,14 @@
>  	</row>
>  	<row>
>  	  <entry>__u32</entry>
> -	  <entry><structfield>reserved</structfield>[7]</entry>
> +	  <entry><structfield>pixel_clock</structfield></entry>
> +	  <entry>Pixel clock in kHz. This clock is the maximum rate at
> +	  which pixels are transferred on the bus. The pixel_clock
> +	  field is read-only.</entry>

I searched a couple of datasheets to find out where I could use this pixel_clock
field but didn't find any so far. I haven't tried too hard though ;)
There seems to be more benefits from having the link frequency control.

It might be easy to confuse pixel_clock with the bus clock. The bus clock is
often referred in datasheets as Pixel Clock (PCLK, AFAIU it's described with
link frequency in your RFC). IMHO your original proposal was better, i.e.
using more explicit pixel_rate. Also why it is in kHz ? Doesn't it make more
sense to use bits or pixels  per second ?


> +	</row>
> +	<row>
> +	  <entry>__u32</entry>
> +	  <entry><structfield>reserved</structfield>[6]</entry>
>  	  <entry>Reserved for future extensions. Applications and drivers must
>  	  set the array to zero.</entry>
>  	</row>
> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> index 5ea7f75..76a0df2 100644
> --- a/include/linux/v4l2-mediabus.h
> +++ b/include/linux/v4l2-mediabus.h
> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
>   * @code:	data format code (from enum v4l2_mbus_pixelcode)
>   * @field:	used interlacing type (from enum v4l2_field)
>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
> + * @pixel_clock: pixel clock, in kHz
>   */
>  struct v4l2_mbus_framefmt {
>  	__u32			width;
> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
>  	__u32			code;
>  	__u32			field;
>  	__u32			colorspace;
> -	__u32			reserved[7];
> +	__u32			pixel_clock;

I'm wondering, whether it is worth to make it 'pixelclock' for consistency
with other fields? Perhaps it would make more sense to have color_space and
pixel_clock.
             		
> +	__u32			reserved[6];
>  };
>  
>  #endif

--

Regards,
Sylwester