[RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Stanimir Varbanov]

This RFC patch adds a new subdev sensor operation named g_interface_parms.
It is planned as a not mandatory operation and it is driver's developer
decision to use it or not.

Please share your opinions and ideas.

---
It tries to create a common API for getting the sensor interface type
- serial or parallel, modes and interface clocks. The interface clocks
then are used in the host side to calculate it's configuration, check
that the clocks are not beyond host limitations etc.

"phy_rate" in serial interface (CSI DDR clk) is used to calculate
the CSI2 PHY receiver timing parameters: ths_settle, ths_term,
clk_settle and clk_term.

As the "phy_rate" depends on current sensor mode (configuration of the
sensor's PLL and internal clock domains) it can be treated as dynamic
parameter and can vary (could be different for viewfinder and still 
capture), in this context g_interface_parms should be called after
s_fmt.

"pix_clk" for parallel interface reflects the current sensor pixel
clock. With this clock the image data is clocked out of the sensor.

"pix_clk" for serial interface reflects the current sensor pixel
clock at which image date is read from sensor matrix.

"lanes" for serial interface reflects the number of PHY lanes used from
the sensor to output image data. This should be known from the host
side before the streaming is started. For some sensor modes it's
enough to use one lane, for bigger resolutions two lanes and more
are used.

"channel" for serial interface is also needed from host side to
configure it's PHY receiver at particular virtual channel.

---
Some background and inspiration.

- Currently in the OMAP3 ISP driver we use a set of platform data
callbacks to provide the above parameters and this comes to very
complicated platform code, driver implementation and unneeded 
sensor driver <-> host driver dependences. 

- In the present time we seeing growing count of sensor drivers and
host (bridge) drivers but without standard API's for communication.
Currently the subdev sensor operations have only one operation -
g_skip_top_lines.

Stanimir Varbanov (1):
  v4l: Introduce sensor operation for getting interface configuration

 include/media/v4l2-subdev.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

[RFC/PATCH 1/1] v4l: Introduce sensor operation for getting interface configuration

[Stanimir Varbanov]

Introduce g_interface_parms sensor operation for getting sensor
interface parameters. These parameters are needed from the host side
to determine it's own configuration.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 include/media/v4l2-subdev.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b0316a7..4186cad 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -322,15 +322,57 @@ struct v4l2_subdev_vbi_ops {
 	int (*s_sliced_fmt)(struct v4l2_subdev *sd, struct v4l2_sliced_vbi_format *fmt);
 };
 
+/* Which interface the sensor use to provide it's image data */
+enum v4l2_subdev_sensor_iface {
+	V4L2_SUBDEV_SENSOR_PARALLEL,
+	V4L2_SUBDEV_SENSOR_SERIAL,
+};
+
+/* Each interface could use the following modes */
+/* Image sensor provides horizontal and vertical sync signals */
+#define V4L2_SUBDEV_SENSOR_MODE_PARALLEL_SYNC	0
+/* BT.656 interface. Embedded sync */
+#define V4L2_SUBDEV_SENSOR_MODE_PARALLEL_ITU	1
+/* MIPI CSI1 */
+#define V4L2_SUBDEV_SENSOR_MODE_SERIAL_CSI1	2
+/* MIPI CSI2 */
+#define V4L2_SUBDEV_SENSOR_MODE_SERIAL_CSI2	3
+
+struct v4l2_subdev_sensor_serial_parms {
+	unsigned char lanes;		/* number of lanes used */
+	unsigned char channel;		/* virtual channel */
+	unsigned int phy_rate;		/* output rate at CSI phy in bps */
+	unsigned int pix_clk;		/* pixel clock in Hz */
+};
+
+struct v4l2_subdev_sensor_parallel_parms {
+	unsigned int pix_clk;		/* pixel clock in Hz */
+};
+
+struct v4l2_subdev_sensor_interface_parms {
+	enum v4l2_subdev_sensor_iface if_type;
+	unsigned int if_mode;
+	union {
+		struct v4l2_subdev_sensor_serial_parms serial;
+		struct v4l2_subdev_sensor_parallel_parms parallel;
+	} parms;
+};
+
 /**
  * struct v4l2_subdev_sensor_ops - v4l2-subdev sensor operations
  * @g_skip_top_lines: number of lines at the top of the image to be skipped.
  *		      This is needed for some sensors, which always corrupt
  *		      several top lines of the output image, or which send their
  *		      metadata in them.
+ * @g_interface_parms: get sensor interface parameters. The sensor subdev should
+ *		       fill this structure with current interface params. These
+ *		       interface parameters are needed on host side to configure
+ *		       it's own hardware receivers.
  */
 struct v4l2_subdev_sensor_ops {
 	int (*g_skip_top_lines)(struct v4l2_subdev *sd, u32 *lines);
+	int (*g_interface_parms)(struct v4l2_subdev *sd,
+			struct v4l2_subdev_sensor_interface_parms *parms);
 };
 
 /*
-- 
1.6.5

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Tue, 22 Feb 2011, Stanimir Varbanov wrote:

> This RFC patch adds a new subdev sensor operation named g_interface_parms.
> It is planned as a not mandatory operation and it is driver's developer
> decision to use it or not.
> 
> Please share your opinions and ideas.

Yes, I like the idea in principle (/me pulling his bullet-proof vest on), 
as some of you might guess, because I feel it's going away from the idea, 
that I've been hard pressed to accept of hard-coding the media-bus 
configuration and in the direction of direct communication of 
bus-parameters between the (sub-)devices, e.g., a camera host and a camera 
device in soc-camera terminology.

But before reviewing the patch as such, I'd like to discuss the strategy, 
that we want to pursue here - what exactly do we want to hard-code and 
what we want to configure dynamically? As explained before, my preference 
would be to only specify the absolute minimum in the platform data, i.e., 
parameters that either are ambiguous or special for this platform. So, 
once again, my approach to configure interface parameters like signal 
polarities and edge sensitivity is:

1. if at least one side has a fixed value of the specific parameter, 
usually no need to specify it in platform data. Example: sensor only 
supports HSYNC active high, host supports both, normally "high" should be 
selected.

2. as above, but there's an inverter on the board in the signal path. The 
"invert" parameter must be specified in the platform data and the host 
will configure itself to "low" and send "high" confirmed to the sensor.

3. both are configurable. In this case the platform data has to specify, 
which polarity shall be used.

This is simple, it is implemented, it has worked that way with no problem 
for several years now.

The configuration procedure in this case looks like:

1. host requests supported interface configurations from the client 
(sensor)

2. host matches returned parameters against platform data and its own 
capabilities

3. if no suitable configuration possible - error out

4. the single possible configuration is identified and sent to the sensor 
back for its configuration

This way we need one more method: s_interface_parms.

Shortly talking to Laurent earlier today privately, he mentioned, that one 
of the reasons for this move is to support dynamic bus reconfiguration, 
e.g., the number of used CSI lanes. The same is useful for parallel 
interfaces. E.g., I had to hack the omap3spi driver to capture only 8 
(parallel) data lanes from the sensor, connected with all its 10 lanes to 
get a format, easily supported by user-space applications. Ideally you 
don't want to change anything in the code for this. If the user is 
requesting the 10-bit format, all 10 lanes are used, if only 8 - the 
interface is reconfigured accordingly.

Thanks
Guennadi

> ---
> It tries to create a common API for getting the sensor interface type
> - serial or parallel, modes and interface clocks. The interface clocks
> then are used in the host side to calculate it's configuration, check
> that the clocks are not beyond host limitations etc.
> 
> "phy_rate" in serial interface (CSI DDR clk) is used to calculate
> the CSI2 PHY receiver timing parameters: ths_settle, ths_term,
> clk_settle and clk_term.
> 
> As the "phy_rate" depends on current sensor mode (configuration of the
> sensor's PLL and internal clock domains) it can be treated as dynamic
> parameter and can vary (could be different for viewfinder and still 
> capture), in this context g_interface_parms should be called after
> s_fmt.
> 
> "pix_clk" for parallel interface reflects the current sensor pixel
> clock. With this clock the image data is clocked out of the sensor.
> 
> "pix_clk" for serial interface reflects the current sensor pixel
> clock at which image date is read from sensor matrix.
> 
> "lanes" for serial interface reflects the number of PHY lanes used from
> the sensor to output image data. This should be known from the host
> side before the streaming is started. For some sensor modes it's
> enough to use one lane, for bigger resolutions two lanes and more
> are used.
> 
> "channel" for serial interface is also needed from host side to
> configure it's PHY receiver at particular virtual channel.
> 
> ---
> Some background and inspiration.
> 
> - Currently in the OMAP3 ISP driver we use a set of platform data
> callbacks to provide the above parameters and this comes to very
> complicated platform code, driver implementation and unneeded 
> sensor driver <-> host driver dependences. 
> 
> - In the present time we seeing growing count of sensor drivers and
> host (bridge) drivers but without standard API's for communication.
> Currently the subdev sensor operations have only one operation -
> g_skip_top_lines.
> 
> Stanimir Varbanov (1):
>   v4l: Introduce sensor operation for getting interface configuration
> 
>  include/media/v4l2-subdev.h |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 42 insertions(+), 0 deletions(-)
> 

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:
> On Tue, 22 Feb 2011, Stanimir Varbanov wrote:
> 
> > This RFC patch adds a new subdev sensor operation named g_interface_parms.
> > It is planned as a not mandatory operation and it is driver's developer
> > decision to use it or not.
> > 
> > Please share your opinions and ideas.

Stanimir, thanks for the RFC. I think it is time that we create a good 
solution for this. This is currently the last remaining issue preventing soc-
camera subdevs from being used generally. (Control handling is also still 
special, but this is being worked on.)

> Yes, I like the idea in principle (/me pulling his bullet-proof vest on), 

:-)

> as some of you might guess, because I feel it's going away from the idea, 
> that I've been hard pressed to accept of hard-coding the media-bus 
> configuration and in the direction of direct communication of 
> bus-parameters between the (sub-)devices, e.g., a camera host and a camera 
> device in soc-camera terminology.
> 
> But before reviewing the patch as such, I'd like to discuss the strategy, 
> that we want to pursue here - what exactly do we want to hard-code and 
> what we want to configure dynamically? As explained before, my preference 
> would be to only specify the absolute minimum in the platform data, i.e., 
> parameters that either are ambiguous or special for this platform. So, 
> once again, my approach to configure interface parameters like signal 
> polarities and edge sensitivity is:
> 
> 1. if at least one side has a fixed value of the specific parameter, 
> usually no need to specify it in platform data. Example: sensor only 
> supports HSYNC active high, host supports both, normally "high" should be 
> selected.
> 
> 2. as above, but there's an inverter on the board in the signal path. The 
> "invert" parameter must be specified in the platform data and the host 
> will configure itself to "low" and send "high" confirmed to the sensor.
> 
> 3. both are configurable. In this case the platform data has to specify, 
> which polarity shall be used.
> 
> This is simple, it is implemented, it has worked that way with no problem 
> for several years now.
> 
> The configuration procedure in this case looks like:
> 
> 1. host requests supported interface configurations from the client 
> (sensor)
> 
> 2. host matches returned parameters against platform data and its own 
> capabilities
> 
> 3. if no suitable configuration possible - error out
> 
> 4. the single possible configuration is identified and sent to the sensor 
> back for its configuration
> 
> This way we need one more method: s_interface_parms.
> 
> Shortly talking to Laurent earlier today privately, he mentioned, that one 
> of the reasons for this move is to support dynamic bus reconfiguration, 
> e.g., the number of used CSI lanes. The same is useful for parallel 
> interfaces. E.g., I had to hack the omap3spi driver to capture only 8 
> (parallel) data lanes from the sensor, connected with all its 10 lanes to 
> get a format, easily supported by user-space applications. Ideally you 
> don't want to change anything in the code for this. If the user is 
> requesting the 10-bit format, all 10 lanes are used, if only 8 - the 
> interface is reconfigured accordingly.

I have no problems with dynamic bus reconfiguration as such. So if the host 
driver wants to do lane reconfiguration, then that's fine by me.

When it comes to signal integrity (polarity, rising/falling edge), then I 
remain convinced that this should be set via platform data. This is not 
something that should be negotiated since this depends not only on the sensor 
and host devices, but also on the routing of the lines between them on the 
actual board, how much noise there is on those lines, the quality of the clock 
signal, etc. Not really an issue with PAL/NTSC type signals, but when you get 
to 1080p60 and up, then such things become much more important.

So these settings should not be negotiated, but set explicitly.

It actually doesn't have to be done through platform data (although that makes 
the most sense), as long as it is explicitly set based on board-specific data.

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
> > ---
> > It tries to create a common API for getting the sensor interface type
> > - serial or parallel, modes and interface clocks. The interface clocks
> > then are used in the host side to calculate it's configuration, check
> > that the clocks are not beyond host limitations etc.
> > 
> > "phy_rate" in serial interface (CSI DDR clk) is used to calculate
> > the CSI2 PHY receiver timing parameters: ths_settle, ths_term,
> > clk_settle and clk_term.
> > 
> > As the "phy_rate" depends on current sensor mode (configuration of the
> > sensor's PLL and internal clock domains) it can be treated as dynamic
> > parameter and can vary (could be different for viewfinder and still 
> > capture), in this context g_interface_parms should be called after
> > s_fmt.
> > 
> > "pix_clk" for parallel interface reflects the current sensor pixel
> > clock. With this clock the image data is clocked out of the sensor.
> > 
> > "pix_clk" for serial interface reflects the current sensor pixel
> > clock at which image date is read from sensor matrix.
> > 
> > "lanes" for serial interface reflects the number of PHY lanes used from
> > the sensor to output image data. This should be known from the host
> > side before the streaming is started. For some sensor modes it's
> > enough to use one lane, for bigger resolutions two lanes and more
> > are used.
> > 
> > "channel" for serial interface is also needed from host side to
> > configure it's PHY receiver at particular virtual channel.
> > 
> > ---
> > Some background and inspiration.
> > 
> > - Currently in the OMAP3 ISP driver we use a set of platform data
> > callbacks to provide the above parameters and this comes to very
> > complicated platform code, driver implementation and unneeded 
> > sensor driver <-> host driver dependences. 
> > 
> > - In the present time we seeing growing count of sensor drivers and
> > host (bridge) drivers but without standard API's for communication.
> > Currently the subdev sensor operations have only one operation -
> > g_skip_top_lines.
> > 
> > Stanimir Varbanov (1):
> >   v4l: Introduce sensor operation for getting interface configuration
> > 
> >  include/media/v4l2-subdev.h |   42 
++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 42 insertions(+), 0 deletions(-)
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> 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
> 

RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Aguirre, Sergio]

Hi,

> -----Original Message-----
> From: Hans Verkuil [mailto:hansverk@cisco.com]
> Sent: Tuesday, February 22, 2011 7:33 AM
> To: Guennadi Liakhovetski
> Cc: Stanimir Varbanov; linux-media@vger.kernel.org;
> laurent.pinchart@ideasonboard.com; Aguirre, Sergio
> Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
> 
> On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:
> > On Tue, 22 Feb 2011, Stanimir Varbanov wrote:
> >
> > > This RFC patch adds a new subdev sensor operation named
> g_interface_parms.
> > > It is planned as a not mandatory operation and it is driver's
> developer
> > > decision to use it or not.
> > >
> > > Please share your opinions and ideas.
> 
> Stanimir, thanks for the RFC. I think it is time that we create a good
> solution for this. This is currently the last remaining issue preventing
> soc-
> camera subdevs from being used generally. (Control handling is also still
> special, but this is being worked on.)
> 
> > Yes, I like the idea in principle (/me pulling his bullet-proof vest
> on),
> 
> :-)
> 
> > as some of you might guess, because I feel it's going away from the
> idea,
> > that I've been hard pressed to accept of hard-coding the media-bus
> > configuration and in the direction of direct communication of
> > bus-parameters between the (sub-)devices, e.g., a camera host and a
> camera
> > device in soc-camera terminology.
> >
> > But before reviewing the patch as such, I'd like to discuss the
> strategy,
> > that we want to pursue here - what exactly do we want to hard-code and
> > what we want to configure dynamically? As explained before, my
> preference
> > would be to only specify the absolute minimum in the platform data,
> i.e.,
> > parameters that either are ambiguous or special for this platform. So,
> > once again, my approach to configure interface parameters like signal
> > polarities and edge sensitivity is:
> >
> > 1. if at least one side has a fixed value of the specific parameter,
> > usually no need to specify it in platform data. Example: sensor only
> > supports HSYNC active high, host supports both, normally "high" should
> be
> > selected.
> >
> > 2. as above, but there's an inverter on the board in the signal path.
> The
> > "invert" parameter must be specified in the platform data and the host
> > will configure itself to "low" and send "high" confirmed to the sensor.
> >
> > 3. both are configurable. In this case the platform data has to specify,
> > which polarity shall be used.
> >
> > This is simple, it is implemented, it has worked that way with no
> problem
> > for several years now.
> >
> > The configuration procedure in this case looks like:
> >
> > 1. host requests supported interface configurations from the client
> > (sensor)
> >
> > 2. host matches returned parameters against platform data and its own
> > capabilities
> >
> > 3. if no suitable configuration possible - error out
> >
> > 4. the single possible configuration is identified and sent to the
> sensor
> > back for its configuration
> >
> > This way we need one more method: s_interface_parms.
> >
> > Shortly talking to Laurent earlier today privately, he mentioned, that
> one
> > of the reasons for this move is to support dynamic bus reconfiguration,
> > e.g., the number of used CSI lanes. The same is useful for parallel
> > interfaces. E.g., I had to hack the omap3spi driver to capture only 8
> > (parallel) data lanes from the sensor, connected with all its 10 lanes
> to
> > get a format, easily supported by user-space applications. Ideally you
> > don't want to change anything in the code for this. If the user is
> > requesting the 10-bit format, all 10 lanes are used, if only 8 - the
> > interface is reconfigured accordingly.
> 
> I have no problems with dynamic bus reconfiguration as such. So if the
> host
> driver wants to do lane reconfiguration, then that's fine by me.
> 
> When it comes to signal integrity (polarity, rising/falling edge), then I
> remain convinced that this should be set via platform data. This is not
> something that should be negotiated since this depends not only on the
> sensor
> and host devices, but also on the routing of the lines between them on the
> actual board, how much noise there is on those lines, the quality of the
> clock
> signal, etc. Not really an issue with PAL/NTSC type signals, but when you
> get
> to 1080p60 and up, then such things become much more important.
> 
> So these settings should not be negotiated, but set explicitly.
> 
> It actually doesn't have to be done through platform data (although that
> makes
> the most sense), as long as it is explicitly set based on board-specific
> data.

My 2 cents here is that I think this consists in 2 parts, and should be
divided properly.

1. You know required # of lanes & clockspeed after you had set the format.
For example:

- VGA @ 30fps
  DDR Clk: 330 MHz
  Number of Datalanes: 1

- VGA @ 60fps
  DDR Clk: 330 MHz
  Number of Datalanes: 2

- 12MPix @ 10fps
  DDR Clk: 480 MHz
  Number of Datalanes: 2

This usually is something you know from the sensor config selected based
on your params.

So, I think this is something that the host should ask to the client
after setting size & framerate, or just before VIDIOC_STREAMON.

THIS I think must be handle dynamically by introducing it as part of
a V4l2_subdev sensor_ops entry. If this can't be done, then how?

2. Specifying polarity and lanes physical position.

For example, at least OMAP3 & 4 has the following pin pairs:

CSI2_DX0, CSI2_DY0
CSI2_DX1, CSI2_DY1
CSI2_DX2, CSI2_DY2
CSI2_DX3, CSI2_DY3
CSI2_DX4, CSI2_DY4

So, what you do is that, you can control where do you want the clock,
where do you want each datalane pair, and also the pin polarity
(X: +, Y: -, or viceversa). And this is something that is static.
THIS I think should go in the host driver's platform data.

Of course, I'm here discarding the fact that at least some sensors that
I know of can change the primary data lane position in sensor configuration
(at least some OmniVision sensors do), but I'll assume this is usually
Locked, and assume the host is better at that.

Regards,
Sergio

> 
> Regards,
> 
> 	Hans
> 
> >
> > Thanks
> > Guennadi
> >
> > > ---
> > > It tries to create a common API for getting the sensor interface type
> > > - serial or parallel, modes and interface clocks. The interface clocks
> > > then are used in the host side to calculate it's configuration, check
> > > that the clocks are not beyond host limitations etc.
> > >
> > > "phy_rate" in serial interface (CSI DDR clk) is used to calculate
> > > the CSI2 PHY receiver timing parameters: ths_settle, ths_term,
> > > clk_settle and clk_term.
> > >
> > > As the "phy_rate" depends on current sensor mode (configuration of the
> > > sensor's PLL and internal clock domains) it can be treated as dynamic
> > > parameter and can vary (could be different for viewfinder and still
> > > capture), in this context g_interface_parms should be called after
> > > s_fmt.
> > >
> > > "pix_clk" for parallel interface reflects the current sensor pixel
> > > clock. With this clock the image data is clocked out of the sensor.
> > >
> > > "pix_clk" for serial interface reflects the current sensor pixel
> > > clock at which image date is read from sensor matrix.
> > >
> > > "lanes" for serial interface reflects the number of PHY lanes used
> from
> > > the sensor to output image data. This should be known from the host
> > > side before the streaming is started. For some sensor modes it's
> > > enough to use one lane, for bigger resolutions two lanes and more
> > > are used.
> > >
> > > "channel" for serial interface is also needed from host side to
> > > configure it's PHY receiver at particular virtual channel.
> > >
> > > ---
> > > Some background and inspiration.
> > >
> > > - Currently in the OMAP3 ISP driver we use a set of platform data
> > > callbacks to provide the above parameters and this comes to very
> > > complicated platform code, driver implementation and unneeded
> > > sensor driver <-> host driver dependences.
> > >
> > > - In the present time we seeing growing count of sensor drivers and
> > > host (bridge) drivers but without standard API's for communication.
> > > Currently the subdev sensor operations have only one operation -
> > > g_skip_top_lines.
> > >
> > > Stanimir Varbanov (1):
> > >   v4l: Introduce sensor operation for getting interface configuration
> > >
> > >  include/media/v4l2-subdev.h |   42
> ++++++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 42 insertions(+), 0 deletions(-)
> > >
> >
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > --
> > 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
> >

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Tue, 22 Feb 2011, Hans Verkuil wrote:

> On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:
> > On Tue, 22 Feb 2011, Stanimir Varbanov wrote:
> > 
> > > This RFC patch adds a new subdev sensor operation named g_interface_parms.
> > > It is planned as a not mandatory operation and it is driver's developer
> > > decision to use it or not.
> > > 
> > > Please share your opinions and ideas.
> 
> Stanimir, thanks for the RFC. I think it is time that we create a good 
> solution for this. This is currently the last remaining issue preventing soc-
> camera subdevs from being used generally. (Control handling is also still 
> special, but this is being worked on.)
> 
> > Yes, I like the idea in principle (/me pulling his bullet-proof vest on), 
> 
> :-)
> 
> > as some of you might guess, because I feel it's going away from the idea, 
> > that I've been hard pressed to accept of hard-coding the media-bus 
> > configuration and in the direction of direct communication of 
> > bus-parameters between the (sub-)devices, e.g., a camera host and a camera 
> > device in soc-camera terminology.
> > 
> > But before reviewing the patch as such, I'd like to discuss the strategy, 
> > that we want to pursue here - what exactly do we want to hard-code and 
> > what we want to configure dynamically? As explained before, my preference 
> > would be to only specify the absolute minimum in the platform data, i.e., 
> > parameters that either are ambiguous or special for this platform. So, 
> > once again, my approach to configure interface parameters like signal 
> > polarities and edge sensitivity is:
> > 
> > 1. if at least one side has a fixed value of the specific parameter, 
> > usually no need to specify it in platform data. Example: sensor only 
> > supports HSYNC active high, host supports both, normally "high" should be 
> > selected.
> > 
> > 2. as above, but there's an inverter on the board in the signal path. The 
> > "invert" parameter must be specified in the platform data and the host 
> > will configure itself to "low" and send "high" confirmed to the sensor.
> > 
> > 3. both are configurable. In this case the platform data has to specify, 
> > which polarity shall be used.
> > 
> > This is simple, it is implemented, it has worked that way with no problem 
> > for several years now.
> > 
> > The configuration procedure in this case looks like:
> > 
> > 1. host requests supported interface configurations from the client 
> > (sensor)
> > 
> > 2. host matches returned parameters against platform data and its own 
> > capabilities
> > 
> > 3. if no suitable configuration possible - error out
> > 
> > 4. the single possible configuration is identified and sent to the sensor 
> > back for its configuration
> > 
> > This way we need one more method: s_interface_parms.
> > 
> > Shortly talking to Laurent earlier today privately, he mentioned, that one 
> > of the reasons for this move is to support dynamic bus reconfiguration, 
> > e.g., the number of used CSI lanes. The same is useful for parallel 
> > interfaces. E.g., I had to hack the omap3spi driver to capture only 8 
> > (parallel) data lanes from the sensor, connected with all its 10 lanes to 
> > get a format, easily supported by user-space applications. Ideally you 
> > don't want to change anything in the code for this. If the user is 
> > requesting the 10-bit format, all 10 lanes are used, if only 8 - the 
> > interface is reconfigured accordingly.
> 
> I have no problems with dynamic bus reconfiguration as such. So if the host 
> driver wants to do lane reconfiguration, then that's fine by me.
> 
> When it comes to signal integrity (polarity, rising/falling edge), then I 
> remain convinced that this should be set via platform data. This is not 
> something that should be negotiated since this depends not only on the sensor 
> and host devices, but also on the routing of the lines between them on the 
> actual board, how much noise there is on those lines, the quality of the clock 
> signal, etc. Not really an issue with PAL/NTSC type signals, but when you get 
> to 1080p60 and up, then such things become much more important.

I understand this, but my point is: forcing this parameters in the 
platform data doesn't give you any _practical_ enhancements, only 
_psychological_, meaning, that you think, that if these parameters are 
compulsory, programmers, writing board integration code, will be forced to 
think, what values to configure. Whereas if this is not compulsory, 
programmers will hope on automagic and things will break. So, this is 
purely psychological. And that's the whole question - fo we trust 
programmers, that they will anyway take care to set correct parameters, or 
do we not trust them and therefore want to punish everyone because of 
them. Besides, I'm pretty convinced, that even if those parameters will be 
compulsory, most programmers will anyway just copy-paste them from 
"similar" set ups...

Thanks
Guennadi

> So these settings should not be negotiated, but set explicitly.
> 
> It actually doesn't have to be done through platform data (although that makes 
> the most sense), as long as it is explicitly set based on board-specific data.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Thanks
> > Guennadi
> > 
> > > ---
> > > It tries to create a common API for getting the sensor interface type
> > > - serial or parallel, modes and interface clocks. The interface clocks
> > > then are used in the host side to calculate it's configuration, check
> > > that the clocks are not beyond host limitations etc.
> > > 
> > > "phy_rate" in serial interface (CSI DDR clk) is used to calculate
> > > the CSI2 PHY receiver timing parameters: ths_settle, ths_term,
> > > clk_settle and clk_term.
> > > 
> > > As the "phy_rate" depends on current sensor mode (configuration of the
> > > sensor's PLL and internal clock domains) it can be treated as dynamic
> > > parameter and can vary (could be different for viewfinder and still 
> > > capture), in this context g_interface_parms should be called after
> > > s_fmt.
> > > 
> > > "pix_clk" for parallel interface reflects the current sensor pixel
> > > clock. With this clock the image data is clocked out of the sensor.
> > > 
> > > "pix_clk" for serial interface reflects the current sensor pixel
> > > clock at which image date is read from sensor matrix.
> > > 
> > > "lanes" for serial interface reflects the number of PHY lanes used from
> > > the sensor to output image data. This should be known from the host
> > > side before the streaming is started. For some sensor modes it's
> > > enough to use one lane, for bigger resolutions two lanes and more
> > > are used.
> > > 
> > > "channel" for serial interface is also needed from host side to
> > > configure it's PHY receiver at particular virtual channel.
> > > 
> > > ---
> > > Some background and inspiration.
> > > 
> > > - Currently in the OMAP3 ISP driver we use a set of platform data
> > > callbacks to provide the above parameters and this comes to very
> > > complicated platform code, driver implementation and unneeded 
> > > sensor driver <-> host driver dependences. 
> > > 
> > > - In the present time we seeing growing count of sensor drivers and
> > > host (bridge) drivers but without standard API's for communication.
> > > Currently the subdev sensor operations have only one operation -
> > > g_skip_top_lines.
> > > 
> > > Stanimir Varbanov (1):
> > >   v4l: Introduce sensor operation for getting interface configuration
> > > 
> > >  include/media/v4l2-subdev.h |   42 
> ++++++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 42 insertions(+), 0 deletions(-)
> > > 
> > 
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > --
> > 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/

RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Tue, 22 Feb 2011, Aguirre, Sergio wrote:

> For example, at least OMAP3 & 4 has the following pin pairs:
> 
> CSI2_DX0, CSI2_DY0
> CSI2_DX1, CSI2_DY1
> CSI2_DX2, CSI2_DY2
> CSI2_DX3, CSI2_DY3
> CSI2_DX4, CSI2_DY4
> 
> So, what you do is that, you can control where do you want the clock,
> where do you want each datalane pair, and also the pin polarity
> (X: +, Y: -, or viceversa). And this is something that is static.
> THIS I think should go in the host driver's platform data.

I think, these are two different things: pin roles - yes, they are 
SoC-specific and, probably, hard-wired. But once you've assigned roles, 
you have to configure them - roles, functions, not pins. And that 
configuration is no longer SoC specific, at least some of the parameters 
are common to all such set ups - polarities and edges. So, you can use the 
same set of parameters for them on different platforms.

And yes - you have to be able to configure them dynamically. Consider two 
sensors switching to the same host by means of some board logic. So, at 
least there have to be multiple parameter sets to use, depending on the 
connection topology.

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Tuesday, February 22, 2011 15:11:49 Guennadi Liakhovetski wrote:
> On Tue, 22 Feb 2011, Hans Verkuil wrote:

<snip>

> > I have no problems with dynamic bus reconfiguration as such. So if the 
host 
> > driver wants to do lane reconfiguration, then that's fine by me.
> > 
> > When it comes to signal integrity (polarity, rising/falling edge), then I 
> > remain convinced that this should be set via platform data. This is not 
> > something that should be negotiated since this depends not only on the 
sensor 
> > and host devices, but also on the routing of the lines between them on the 
> > actual board, how much noise there is on those lines, the quality of the 
clock 
> > signal, etc. Not really an issue with PAL/NTSC type signals, but when you 
get 
> > to 1080p60 and up, then such things become much more important.
> 
> I understand this, but my point is: forcing this parameters in the 
> platform data doesn't give you any _practical_ enhancements, only 
> _psychological_, meaning, that you think, that if these parameters are 
> compulsory, programmers, writing board integration code, will be forced to 
> think, what values to configure. Whereas if this is not compulsory, 
> programmers will hope on automagic and things will break. So, this is 
> purely psychological. And that's the whole question - fo we trust 
> programmers, that they will anyway take care to set correct parameters, or 
> do we not trust them and therefore want to punish everyone because of 
> them. Besides, I'm pretty convinced, that even if those parameters will be 
> compulsory, most programmers will anyway just copy-paste them from 
> "similar" set ups...

First of all, I don't trust myself, let alone someone else :-)

So forcing people to think about it is a good thing in my mind.

Secondly, if we rely on negotiations, then someone at some time might change 
things and suddenly the negotiation gives different results which may not work 
on some boards. And such bugs can be extremely hard to track down. So that is 
why I don't want to rely on negotiations of these settings. People are free to 
copy and paste, though. I assume (and hope) that they will test before sending 
a patch, so if it works with the copy-and-pasted settings, then that's good 
enough for me.

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
> > So these settings should not be negotiated, but set explicitly.
> > 
> > It actually doesn't have to be done through platform data (although that 
makes 
> > the most sense), as long as it is explicitly set based on board-specific 
data.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > 
> > > Thanks
> > > Guennadi
> > > 
> > > > ---
> > > > It tries to create a common API for getting the sensor interface type
> > > > - serial or parallel, modes and interface clocks. The interface clocks
> > > > then are used in the host side to calculate it's configuration, check
> > > > that the clocks are not beyond host limitations etc.
> > > > 
> > > > "phy_rate" in serial interface (CSI DDR clk) is used to calculate
> > > > the CSI2 PHY receiver timing parameters: ths_settle, ths_term,
> > > > clk_settle and clk_term.
> > > > 
> > > > As the "phy_rate" depends on current sensor mode (configuration of the
> > > > sensor's PLL and internal clock domains) it can be treated as dynamic
> > > > parameter and can vary (could be different for viewfinder and still 
> > > > capture), in this context g_interface_parms should be called after
> > > > s_fmt.
> > > > 
> > > > "pix_clk" for parallel interface reflects the current sensor pixel
> > > > clock. With this clock the image data is clocked out of the sensor.
> > > > 
> > > > "pix_clk" for serial interface reflects the current sensor pixel
> > > > clock at which image date is read from sensor matrix.
> > > > 
> > > > "lanes" for serial interface reflects the number of PHY lanes used 
from
> > > > the sensor to output image data. This should be known from the host
> > > > side before the streaming is started. For some sensor modes it's
> > > > enough to use one lane, for bigger resolutions two lanes and more
> > > > are used.
> > > > 
> > > > "channel" for serial interface is also needed from host side to
> > > > configure it's PHY receiver at particular virtual channel.
> > > > 
> > > > ---
> > > > Some background and inspiration.
> > > > 
> > > > - Currently in the OMAP3 ISP driver we use a set of platform data
> > > > callbacks to provide the above parameters and this comes to very
> > > > complicated platform code, driver implementation and unneeded 
> > > > sensor driver <-> host driver dependences. 
> > > > 
> > > > - In the present time we seeing growing count of sensor drivers and
> > > > host (bridge) drivers but without standard API's for communication.
> > > > Currently the subdev sensor operations have only one operation -
> > > > g_skip_top_lines.
> > > > 
> > > > Stanimir Varbanov (1):
> > > >   v4l: Introduce sensor operation for getting interface configuration
> > > > 
> > > >  include/media/v4l2-subdev.h |   42 
> > ++++++++++++++++++++++++++++++++++++++++++
> > > >  1 files changed, 42 insertions(+), 0 deletions(-)
> > > > 
> > > 
> > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > http://www.open-technology.de/
> > > --
> > > 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/
> 
> 

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Tue, 22 Feb 2011, Hans Verkuil wrote:

> Secondly, if we rely on negotiations, then someone at some time might change 
> things and suddenly the negotiation gives different results which may not work 
> on some boards. And such bugs can be extremely hard to track down. So that is 

Sorry, there's always a chance, that someone breaks something. Always when 
changing central algorithms you have to take care, that behaviour doesn't 
change on existing configurations. Nothing new there.

> why I don't want to rely on negotiations of these settings. People are free to 
> copy and paste, though. I assume (and hope) that they will test before sending 
> a patch, so if it works with the copy-and-pasted settings, then that's good 
> enough for me.

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Stan]

Hi,

Guennadi Liakhovetski wrote:
> On Tue, 22 Feb 2011, Hans Verkuil wrote:
> 
>> On Tuesday, February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:
>>> On Tue, 22 Feb 2011, Stanimir Varbanov wrote:
>>>
>>>> This RFC patch adds a new subdev sensor operation named g_interface_parms.
>>>> It is planned as a not mandatory operation and it is driver's developer
>>>> decision to use it or not.
>>>>
>>>> Please share your opinions and ideas.
>> Stanimir, thanks for the RFC. I think it is time that we create a good 
>> solution for this. This is currently the last remaining issue preventing soc-
>> camera subdevs from being used generally. (Control handling is also still 
>> special, but this is being worked on.)
>>
>>> Yes, I like the idea in principle (/me pulling his bullet-proof vest on), 
>> :-)
>>
>>> as some of you might guess, because I feel it's going away from the idea, 
>>> that I've been hard pressed to accept of hard-coding the media-bus 
>>> configuration and in the direction of direct communication of 
>>> bus-parameters between the (sub-)devices, e.g., a camera host and a camera 
>>> device in soc-camera terminology.
>>>
>>> But before reviewing the patch as such, I'd like to discuss the strategy, 
>>> that we want to pursue here - what exactly do we want to hard-code and 
>>> what we want to configure dynamically? As explained before, my preference 
>>> would be to only specify the absolute minimum in the platform data, i.e., 
>>> parameters that either are ambiguous or special for this platform. So, 
>>> once again, my approach to configure interface parameters like signal 
>>> polarities and edge sensitivity is:
>>>
>>> 1. if at least one side has a fixed value of the specific parameter, 
>>> usually no need to specify it in platform data. Example: sensor only 
>>> supports HSYNC active high, host supports both, normally "high" should be 
>>> selected.
>>>
>>> 2. as above, but there's an inverter on the board in the signal path. The 
>>> "invert" parameter must be specified in the platform data and the host 
>>> will configure itself to "low" and send "high" confirmed to the sensor.
>>>
>>> 3. both are configurable. In this case the platform data has to specify, 
>>> which polarity shall be used.
>>>
>>> This is simple, it is implemented, it has worked that way with no problem 
>>> for several years now.
>>>
>>> The configuration procedure in this case looks like:
>>>
>>> 1. host requests supported interface configurations from the client 
>>> (sensor)
>>>
>>> 2. host matches returned parameters against platform data and its own 
>>> capabilities
>>>
>>> 3. if no suitable configuration possible - error out
>>>
>>> 4. the single possible configuration is identified and sent to the sensor 
>>> back for its configuration
>>>
>>> This way we need one more method: s_interface_parms.
>>>
>>> Shortly talking to Laurent earlier today privately, he mentioned, that one 
>>> of the reasons for this move is to support dynamic bus reconfiguration, 
>>> e.g., the number of used CSI lanes. The same is useful for parallel 
>>> interfaces. E.g., I had to hack the omap3spi driver to capture only 8 
>>> (parallel) data lanes from the sensor, connected with all its 10 lanes to 
>>> get a format, easily supported by user-space applications. Ideally you 
>>> don't want to change anything in the code for this. If the user is 
>>> requesting the 10-bit format, all 10 lanes are used, if only 8 - the 
>>> interface is reconfigured accordingly.
>> I have no problems with dynamic bus reconfiguration as such. So if the host 
>> driver wants to do lane reconfiguration, then that's fine by me.
>>
>> When it comes to signal integrity (polarity, rising/falling edge), then I 
>> remain convinced that this should be set via platform data. This is not 
>> something that should be negotiated since this depends not only on the sensor 
>> and host devices, but also on the routing of the lines between them on the 
>> actual board, how much noise there is on those lines, the quality of the clock 
>> signal, etc. Not really an issue with PAL/NTSC type signals, but when you get 
>> to 1080p60 and up, then such things become much more important.
> 

I think this could be satisfied by Guennadi's approach because he use the
platform data with preference.

> I understand this, but my point is: forcing this parameters in the 
> platform data doesn't give you any _practical_ enhancements, only 
> _psychological_, meaning, that you think, that if these parameters are 
> compulsory, programmers, writing board integration code, will be forced to 
> think, what values to configure. Whereas if this is not compulsory, 
> programmers will hope on automagic and things will break. So, this is 
> purely psychological. And that's the whole question - fo we trust 
> programmers, that they will anyway take care to set correct parameters, or 
> do we not trust them and therefore want to punish everyone because of 
> them. Besides, I'm pretty convinced, that even if those parameters will be 
> compulsory, most programmers will anyway just copy-paste them from 
> "similar" set ups...
> 

Guennadi, IMO, to force peoples to __not__ thinking about the above parameter
configurations you need to do generic functions in some place and force
peoples to use them as mandatory rule. This will be the hard part :)

In principle I agree with this bus negotiation.

 - So. let's start thinking how this could be fit to the subdev sensor
operations.
 - howto isolate your current work into some common place and reuse it,
even on platform part.
 - and is it possible.

The discussion becomes very emotional and this is not a good adviser :)

> Thanks
> Guennadi
> 
>> So these settings should not be negotiated, but set explicitly.
>>
>> It actually doesn't have to be done through platform data (although that makes 
>> the most sense), as long as it is explicitly set based on board-specific data.
>>

<snip>

-- 
Best regards,
Stan

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Tue, 22 Feb 2011, Stan wrote:

> In principle I agree with this bus negotiation.
> 
>  - So. let's start thinking how this could be fit to the subdev sensor
> operations.

Well, I'm afraid not everyone is convinced yet, so, it is a bit early to 
start designing interfaces;)

>  - howto isolate your current work into some common place and reuse it,
> even on platform part.
>  - and is it possible.
> 
> The discussion becomes very emotional and this is not a good adviser :)

No, no emotions at least on this side:) But it's also not technical, 
unfortunately. I'm prepared to discuss technical benefits or drawbacks of 
each of these approaches, but these arguments - can we trust programmers 
or can we not? or will anyone at some time in the future break it or not? 
Sorry, I am not a psychologist:) Personally, I would _exclusively_ 
consider technical arguments. Of course, things like "clean and simple 
APIs," "proper separation / layering" etc. are also important, but even 
they already can become difficult to discuss and are already on the border 
between technical issues and personal preferences... So, don't know, in 
the end, I think, it will just come down to who is making decisions and 
who is implementing them:) I just expressed my opinion, we don't have to 
agree, eventually, the maintainer will decide whether to apply patches or 
not:)

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:
> On Tue, 22 Feb 2011, Stan wrote:
> 
> > In principle I agree with this bus negotiation.
> > 
> >  - So. let's start thinking how this could be fit to the subdev sensor
> > operations.
> 
> Well, I'm afraid not everyone is convinced yet, so, it is a bit early to 
> start designing interfaces;)
> 
> >  - howto isolate your current work into some common place and reuse it,
> > even on platform part.
> >  - and is it possible.
> > 
> > The discussion becomes very emotional and this is not a good adviser :)
> 
> No, no emotions at least on this side:) But it's also not technical, 
> unfortunately. I'm prepared to discuss technical benefits or drawbacks of 
> each of these approaches, but these arguments - can we trust programmers 
> or can we not? or will anyone at some time in the future break it or not? 
> Sorry, I am not a psychologist:) Personally, I would _exclusively_ 
> consider technical arguments. Of course, things like "clean and simple 
> APIs," "proper separation / layering" etc. are also important, but even 
> they already can become difficult to discuss and are already on the border 
> between technical issues and personal preferences... So, don't know, in 
> the end, I think, it will just come down to who is making decisions and 
> who is implementing them:) I just expressed my opinion, we don't have to 
> agree, eventually, the maintainer will decide whether to apply patches or 
> not:)

In my view at least it *is* a technical argument. It makes perfect sense to
me from a technical point of view to put static, board-specific configuration
in platform_data. I don't think there would have been much, if any, discussion
about this if it wasn't for the fact that soc-camera doesn't do this but instead
negotiates it. Obviously, it isn't a pleasant prospect having to change all that.

Normally this would be enough of an argument for me to just negotiate it. The
reason that I don't want this in this particular case is that I know from
personal experience that incorrect settings can be extremely hard to find.

I also think that there is a reasonable chance that such bugs can happen. Take
a scenario like this: someone writes a new host driver. Initially there is only
support for positive polarity and detection on the rising edge, because that's
what the current board on which the driver was developed supports. This is quite
typical for an initial version of a driver.

Later someone adds support for negative polarity and falling edge. Suddenly the
polarity negotiation on the previous board results in negative instead of positive
which was never tested. Now that board starts producing pixel errors every so
often. And yes, this type of hardware problems do happen as I know from painful
experience.

Problems like this are next to impossible to debug without the aid of an
oscilloscope, so this isn't like most other bugs that are relatively easy to
debug.

It is so much easier just to avoid this by putting it in platform data. It's
simple, unambiguous and above all, unchanging.

Regards,

	Hans

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> 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
> 

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Tue, 22 Feb 2011, Hans Verkuil wrote:

> On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:
> > On Tue, 22 Feb 2011, Stan wrote:
> > 
> > > In principle I agree with this bus negotiation.
> > > 
> > >  - So. let's start thinking how this could be fit to the subdev sensor
> > > operations.
> > 
> > Well, I'm afraid not everyone is convinced yet, so, it is a bit early to 
> > start designing interfaces;)
> > 
> > >  - howto isolate your current work into some common place and reuse it,
> > > even on platform part.
> > >  - and is it possible.
> > > 
> > > The discussion becomes very emotional and this is not a good adviser :)
> > 
> > No, no emotions at least on this side:) But it's also not technical, 
> > unfortunately. I'm prepared to discuss technical benefits or drawbacks of 
> > each of these approaches, but these arguments - can we trust programmers 
> > or can we not? or will anyone at some time in the future break it or not? 
> > Sorry, I am not a psychologist:) Personally, I would _exclusively_ 
> > consider technical arguments. Of course, things like "clean and simple 
> > APIs," "proper separation / layering" etc. are also important, but even 
> > they already can become difficult to discuss and are already on the border 
> > between technical issues and personal preferences... So, don't know, in 
> > the end, I think, it will just come down to who is making decisions and 
> > who is implementing them:) I just expressed my opinion, we don't have to 
> > agree, eventually, the maintainer will decide whether to apply patches or 
> > not:)
> 
> In my view at least it *is* a technical argument. It makes perfect sense to
> me from a technical point of view to put static, board-specific configuration
> in platform_data. I don't think there would have been much, if any, discussion
> about this if it wasn't for the fact that soc-camera doesn't do this but instead
> negotiates it. Obviously, it isn't a pleasant prospect having to change all that.
> 
> Normally this would be enough of an argument for me to just negotiate it. The
> reason that I don't want this in this particular case is that I know from
> personal experience that incorrect settings can be extremely hard to find.
> 
> I also think that there is a reasonable chance that such bugs can happen. Take
> a scenario like this: someone writes a new host driver. Initially there is only
> support for positive polarity and detection on the rising edge, because that's
> what the current board on which the driver was developed supports. This is quite
> typical for an initial version of a driver.
> 
> Later someone adds support for negative polarity and falling edge. Suddenly the
> polarity negotiation on the previous board results in negative instead of positive
> which was never tested. Now that board starts producing pixel errors every so
> often. And yes, this type of hardware problems do happen as I know from painful
> experience.
> 
> Problems like this are next to impossible to debug without the aid of an
> oscilloscope, so this isn't like most other bugs that are relatively easy to
> debug.

Well, this is pretty simple to debug with the help of git bisect, as long 
as patches are sufficiently clean and properly broken down into single 
topics.

> It is so much easier just to avoid this by putting it in platform data. It's
> simple, unambiguous and above all, unchanging.

As I said, this all boils down to who does patches and who accepts them 
for mainlibe.

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Sylwester Nawrocki]

Hi everybody,

On 02/22/2011 06:00 PM, Hans Verkuil wrote:
> On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:
>> On Tue, 22 Feb 2011, Stan wrote:
>>
>>> In principle I agree with this bus negotiation.
>>>
>>>   - So. let's start thinking how this could be fit to the subdev sensor
>>> operations.
>>
>> Well, I'm afraid not everyone is convinced yet, so, it is a bit early to
>> start designing interfaces;)
>>
>>>   - howto isolate your current work into some common place and reuse it,
>>> even on platform part.
>>>   - and is it possible.
>>>
>>> The discussion becomes very emotional and this is not a good adviser :)
>>
>> No, no emotions at least on this side:) But it's also not technical,
>> unfortunately. I'm prepared to discuss technical benefits or drawbacks of
>> each of these approaches, but these arguments - can we trust programmers
>> or can we not? or will anyone at some time in the future break it or not?
>> Sorry, I am not a psychologist:) Personally, I would _exclusively_
>> consider technical arguments. Of course, things like "clean and simple
>> APIs," "proper separation / layering" etc. are also important, but even
>> they already can become difficult to discuss and are already on the border
>> between technical issues and personal preferences... So, don't know, in
>> the end, I think, it will just come down to who is making decisions and
>> who is implementing them:) I just expressed my opinion, we don't have to
>> agree, eventually, the maintainer will decide whether to apply patches or
>> not:)
> 
> In my view at least it *is* a technical argument. It makes perfect sense to
> me from a technical point of view to put static, board-specific configuration
> in platform_data. I don't think there would have been much, if any, discussion

We should not be forgetting that there often will be two or more sets 
of platform_data. For sensor, MIPI interface, for the host interface driver.. 
By negotiating setups we could avoid situations when corresponding parameters
are not matched. That is not so meaningful benefit though. 

Clock values are often being rounded at runtime and do not always reflect exactly
the numbers fixed at compile time. And negotiation could help to obtain exact
values at both sensor and host side.

I personally like the Stanimir's proposal as the parameters to be negotiated
are pretty dynamic. Only the number of lanes could be problematic as not all
lanes might be routed across different boards. Perhaps we should consider specifying
an AUTO value for some negotiated parameters. Such as in case of an attribute that
need to be fixed on some boards or can be fully negotiated on others, a fixed
value or "auto" could be respectively set up in the host's platform_data. This could
be used to override some parameters in the host driver if needed.

IMHO, as long as we negotiate only dynamic parameters there should be no special
issues.

Regards,
Sylwester 

> about this if it wasn't for the fact that soc-camera doesn't do this but instead
> negotiates it. Obviously, it isn't a pleasant prospect having to change all that.
> 
> Normally this would be enough of an argument for me to just negotiate it. The
> reason that I don't want this in this particular case is that I know from
> personal experience that incorrect settings can be extremely hard to find.
> 
> I also think that there is a reasonable chance that such bugs can happen. Take
> a scenario like this: someone writes a new host driver. Initially there is only
> support for positive polarity and detection on the rising edge, because that's
> what the current board on which the driver was developed supports. This is quite
> typical for an initial version of a driver.
> 
> Later someone adds support for negative polarity and falling edge. Suddenly the
> polarity negotiation on the previous board results in negative instead of positive
> which was never tested. Now that board starts producing pixel errors every so
> often. And yes, this type of hardware problems do happen as I know from painful
> experience.
> 
> Problems like this are next to impossible to debug without the aid of an
> oscilloscope, so this isn't like most other bugs that are relatively easy to
> debug.
> 
> It is so much easier just to avoid this by putting it in platform data. It's
> simple, unambiguous and above all, unchanging.
> 
> Regards,
> 
> 	Hans
> 
>>
>> Thanks
>> Guennadi
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>> http://www.open-technology.de/
>> --
>> 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
>>
> 

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
> Hi everybody,
> 
> On 02/22/2011 06:00 PM, Hans Verkuil wrote:
> > On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:
> >> On Tue, 22 Feb 2011, Stan wrote:
> >>
> >>> In principle I agree with this bus negotiation.
> >>>
> >>>   - So. let's start thinking how this could be fit to the subdev sensor
> >>> operations.
> >>
> >> Well, I'm afraid not everyone is convinced yet, so, it is a bit early to
> >> start designing interfaces;)
> >>
> >>>   - howto isolate your current work into some common place and reuse it,
> >>> even on platform part.
> >>>   - and is it possible.
> >>>
> >>> The discussion becomes very emotional and this is not a good adviser :)
> >>
> >> No, no emotions at least on this side:) But it's also not technical,
> >> unfortunately. I'm prepared to discuss technical benefits or drawbacks of
> >> each of these approaches, but these arguments - can we trust programmers
> >> or can we not? or will anyone at some time in the future break it or not?
> >> Sorry, I am not a psychologist:) Personally, I would _exclusively_
> >> consider technical arguments. Of course, things like "clean and simple
> >> APIs," "proper separation / layering" etc. are also important, but even
> >> they already can become difficult to discuss and are already on the border
> >> between technical issues and personal preferences... So, don't know, in
> >> the end, I think, it will just come down to who is making decisions and
> >> who is implementing them:) I just expressed my opinion, we don't have to
> >> agree, eventually, the maintainer will decide whether to apply patches or
> >> not:)
> > 
> > In my view at least it *is* a technical argument. It makes perfect sense to
> > me from a technical point of view to put static, board-specific configuration
> > in platform_data. I don't think there would have been much, if any, discussion
> 
> We should not be forgetting that there often will be two or more sets 
> of platform_data. For sensor, MIPI interface, for the host interface driver.. 
> By negotiating setups we could avoid situations when corresponding parameters
> are not matched. That is not so meaningful benefit though. 
>
> Clock values are often being rounded at runtime and do not always reflect exactly
> the numbers fixed at compile time. And negotiation could help to obtain exact
> values at both sensor and host side.

The only static data I am concerned about are those that affect signal integrity.
After thinking carefully about this I realized that there is really only one
setting that is relevant to that: the sampling edge. The polarities do not
matter in this.

Unfortunately, if a subdev is set to 'sample at rising edge', then that does
not necessarily mean that the host should sample at the same edge. Depending
on the clock line routing and the integrity of the clock signal the host may
actually have to sample on the other edge. And yes, I've seen this.

Anyway, this has been discussed to death already. I am very much opposed to
negotiating the sampling edge. During the Helsinki meeting in June last year
we decided to do this via platform data (see section 7 in the meeting
minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab).

I will formally NACK attempts to negotiate this. Mauro is of course free to
override me.

Something simple like this for subdev platform_data might be enough:

struct v4l2_bus_config {
        /* 0 - sample at falling edge, 1 - sample at rising edge */
        unsigned edge_pclock:1;
        /* 0 - host should use the same sampling edge, 1 - host should use the
           other sampling edge */
        unsigned host_invert_edge_pclock:1;
};

The host can query the bus configuration and the subdev will return:

	edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock;

We might want to add bits as well to describe whether polarities are inverted.

This old RFC gives a good overview of the possible polarities:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg09041.html

Regards,

	Hans

> I personally like the Stanimir's proposal as the parameters to be negotiated
> are pretty dynamic. Only the number of lanes could be problematic as not all
> lanes might be routed across different boards. Perhaps we should consider specifying
> an AUTO value for some negotiated parameters. Such as in case of an attribute that
> need to be fixed on some boards or can be fully negotiated on others, a fixed
> value or "auto" could be respectively set up in the host's platform_data. This could
> be used to override some parameters in the host driver if needed.
> 
> IMHO, as long as we negotiate only dynamic parameters there should be no special
> issues.
> 
> Regards,
> Sylwester 
> 
> > about this if it wasn't for the fact that soc-camera doesn't do this but instead
> > negotiates it. Obviously, it isn't a pleasant prospect having to change all that.
> > 
> > Normally this would be enough of an argument for me to just negotiate it. The
> > reason that I don't want this in this particular case is that I know from
> > personal experience that incorrect settings can be extremely hard to find.
> > 
> > I also think that there is a reasonable chance that such bugs can happen. Take
> > a scenario like this: someone writes a new host driver. Initially there is only
> > support for positive polarity and detection on the rising edge, because that's
> > what the current board on which the driver was developed supports. This is quite
> > typical for an initial version of a driver.
> > 
> > Later someone adds support for negative polarity and falling edge. Suddenly the
> > polarity negotiation on the previous board results in negative instead of positive
> > which was never tested. Now that board starts producing pixel errors every so
> > often. And yes, this type of hardware problems do happen as I know from painful
> > experience.
> > 
> > Problems like this are next to impossible to debug without the aid of an
> > oscilloscope, so this isn't like most other bugs that are relatively easy to
> > debug.
> > 
> > It is so much easier just to avoid this by putting it in platform data. It's
> > simple, unambiguous and above all, unchanging.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> >>
> >> Thanks
> >> Guennadi
> >> ---
> >> Guennadi Liakhovetski, Ph.D.
> >> Freelance Open-Source Software Developer
> >> http://www.open-technology.de/
> >> --
> >> 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
> >>
> > 
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Wed, 23 Feb 2011, Hans Verkuil wrote:

> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
> > Hi everybody,
> > 
> > On 02/22/2011 06:00 PM, Hans Verkuil wrote:
> > > On Tuesday, February 22, 2011 17:27:47 Guennadi Liakhovetski wrote:
> > >> On Tue, 22 Feb 2011, Stan wrote:
> > >>
> > >>> In principle I agree with this bus negotiation.
> > >>>
> > >>>   - So. let's start thinking how this could be fit to the subdev sensor
> > >>> operations.
> > >>
> > >> Well, I'm afraid not everyone is convinced yet, so, it is a bit early to
> > >> start designing interfaces;)
> > >>
> > >>>   - howto isolate your current work into some common place and reuse it,
> > >>> even on platform part.
> > >>>   - and is it possible.
> > >>>
> > >>> The discussion becomes very emotional and this is not a good adviser :)
> > >>
> > >> No, no emotions at least on this side:) But it's also not technical,
> > >> unfortunately. I'm prepared to discuss technical benefits or drawbacks of
> > >> each of these approaches, but these arguments - can we trust programmers
> > >> or can we not? or will anyone at some time in the future break it or not?
> > >> Sorry, I am not a psychologist:) Personally, I would _exclusively_
> > >> consider technical arguments. Of course, things like "clean and simple
> > >> APIs," "proper separation / layering" etc. are also important, but even
> > >> they already can become difficult to discuss and are already on the border
> > >> between technical issues and personal preferences... So, don't know, in
> > >> the end, I think, it will just come down to who is making decisions and
> > >> who is implementing them:) I just expressed my opinion, we don't have to
> > >> agree, eventually, the maintainer will decide whether to apply patches or
> > >> not:)
> > > 
> > > In my view at least it *is* a technical argument. It makes perfect sense to
> > > me from a technical point of view to put static, board-specific configuration
> > > in platform_data. I don't think there would have been much, if any, discussion
> > 
> > We should not be forgetting that there often will be two or more sets 
> > of platform_data. For sensor, MIPI interface, for the host interface driver.. 
> > By negotiating setups we could avoid situations when corresponding parameters
> > are not matched. That is not so meaningful benefit though. 
> >
> > Clock values are often being rounded at runtime and do not always reflect exactly
> > the numbers fixed at compile time. And negotiation could help to obtain exact
> > values at both sensor and host side.
> 
> The only static data I am concerned about are those that affect signal integrity.
> After thinking carefully about this I realized that there is really only one
> setting that is relevant to that: the sampling edge. The polarities do not
> matter in this.

Ok, this is much better! I'm still not perfectly happy having to punish 
all just for the sake of a couple of broken boards, but I can certainly 
much better live with this, than with having to hard-code each and every 
bit. Thanks, Hans!

So, I think, we can proceed with this, let's see the code now, shall we?;)

Currently soc-camera auto-configures the following parameters:

hsync polarity
vsync polarity
data polarity
master / slave mode
data bus width
pixel clock polarity

(see include/media/soc_camera.h::soc_camera_bus_param_compatible() and 
drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()). 
Removing the pixclk polarity, the rest we can use as a basis for a new 
subdev-based implementation.

Thanks
Guennadi

> Unfortunately, if a subdev is set to 'sample at rising edge', then that does
> not necessarily mean that the host should sample at the same edge. Depending
> on the clock line routing and the integrity of the clock signal the host may
> actually have to sample on the other edge. And yes, I've seen this.
> 
> Anyway, this has been discussed to death already. I am very much opposed to
> negotiating the sampling edge. During the Helsinki meeting in June last year
> we decided to do this via platform data (see section 7 in the meeting
> minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab).
> 
> I will formally NACK attempts to negotiate this. Mauro is of course free to
> override me.
> 
> Something simple like this for subdev platform_data might be enough:
> 
> struct v4l2_bus_config {
>         /* 0 - sample at falling edge, 1 - sample at rising edge */
>         unsigned edge_pclock:1;
>         /* 0 - host should use the same sampling edge, 1 - host should use the
>            other sampling edge */
>         unsigned host_invert_edge_pclock:1;
> };
> 
> The host can query the bus configuration and the subdev will return:
> 
> 	edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock;
> 
> We might want to add bits as well to describe whether polarities are inverted.
> 
> This old RFC gives a good overview of the possible polarities:
> 
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg09041.html
> 
> Regards,
> 
> 	Hans
> 
> > I personally like the Stanimir's proposal as the parameters to be negotiated
> > are pretty dynamic. Only the number of lanes could be problematic as not all
> > lanes might be routed across different boards. Perhaps we should consider specifying
> > an AUTO value for some negotiated parameters. Such as in case of an attribute that
> > need to be fixed on some boards or can be fully negotiated on others, a fixed
> > value or "auto" could be respectively set up in the host's platform_data. This could
> > be used to override some parameters in the host driver if needed.
> > 
> > IMHO, as long as we negotiate only dynamic parameters there should be no special
> > issues.
> > 
> > Regards,
> > Sylwester 
> > 
> > > about this if it wasn't for the fact that soc-camera doesn't do this but instead
> > > negotiates it. Obviously, it isn't a pleasant prospect having to change all that.
> > > 
> > > Normally this would be enough of an argument for me to just negotiate it. The
> > > reason that I don't want this in this particular case is that I know from
> > > personal experience that incorrect settings can be extremely hard to find.
> > > 
> > > I also think that there is a reasonable chance that such bugs can happen. Take
> > > a scenario like this: someone writes a new host driver. Initially there is only
> > > support for positive polarity and detection on the rising edge, because that's
> > > what the current board on which the driver was developed supports. This is quite
> > > typical for an initial version of a driver.
> > > 
> > > Later someone adds support for negative polarity and falling edge. Suddenly the
> > > polarity negotiation on the previous board results in negative instead of positive
> > > which was never tested. Now that board starts producing pixel errors every so
> > > often. And yes, this type of hardware problems do happen as I know from painful
> > > experience.
> > > 
> > > Problems like this are next to impossible to debug without the aid of an
> > > oscilloscope, so this isn't like most other bugs that are relatively easy to
> > > debug.
> > > 
> > > It is so much easier just to avoid this by putting it in platform data. It's
> > > simple, unambiguous and above all, unchanging.
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > >>
> > >> Thanks
> > >> Guennadi
> > >> ---
> > >> Guennadi Liakhovetski, Ph.D.
> > >> Freelance Open-Source Software Developer
> > >> http://www.open-technology.de/
> > >> --
> > >> 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
> > >>
> > > 
> > 
> > 
> 
> -- 
> Hans Verkuil - video4linux developer - sponsored by Cisco
> 

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Wednesday, February 23, 2011 09:10:42 Hans Verkuil wrote:
> Unfortunately, if a subdev is set to 'sample at rising edge', then that does
> not necessarily mean that the host should sample at the same edge. Depending
> on the clock line routing and the integrity of the clock signal the host may
> actually have to sample on the other edge. And yes, I've seen this.

It might be useful to give some background information regarding the sampling 
edge problems.

There are two main reasons why the sampling edge can be hardware dependent. 
The first is if the data lines go through an amplifier or something similar 
that will slightly delay the data lines compared to the clock signal. This can 
shift the edge at which you have to sample.

Actually, this may even be dependent on the clock frequency. I have not seen 
that in real life yet, but it might happen. This will complicate things even 
more since in that case you need to make a callback function in the board code 
that determines the sampling edge based on the clock frequency. I think we can 
ignore that for now, but we do need to keep it in mind.

The other is the waveform of the clock. For relatively low frequencies this 
will resemble a symmetrical square wave. But for higher frequencies this more 
resembles the bottom waveform in this picture:

http://myweb.msoe.edu/williamstm/Images/Divider2.jpg

This is asymmetric so depending on the slopes the sampling edge can make quite 
a difference.

The higher the clock frequency, the more asymmetric the waveform will look.

Regards,

	Hans

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 22 February 2011 12:40:32 Guennadi Liakhovetski wrote:
> On Tue, 22 Feb 2011, Stanimir Varbanov wrote:
> > This RFC patch adds a new subdev sensor operation named
> > g_interface_parms. It is planned as a not mandatory operation and it is
> > driver's developer decision to use it or not.
> > 
> > Please share your opinions and ideas.

[snip] (answering this in another e-mail)

> Shortly talking to Laurent earlier today privately, he mentioned, that one
> of the reasons for this move is to support dynamic bus reconfiguration,
> e.g., the number of used CSI lanes. The same is useful for parallel
> interfaces. E.g., I had to hack the omap3spi driver to capture only 8
> (parallel) data lanes from the sensor, connected with all its 10 lanes to
> get a format, easily supported by user-space applications. Ideally you
> don't want to change anything in the code for this. If the user is
> requesting the 10-bit format, all 10 lanes are used, if only 8 - the
> interface is reconfigured accordingly.

This should indeed be supported out-of-the-box. I'm waiting for Hans' opinion 
on this, but the idea is that the user should configure a 10-bit format at the 
sensor output and an 8-bit format at the CCDC input to capture 8-bit data.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

Hi Sergio,

On Tuesday 22 February 2011 15:01:57 Aguirre, Sergio wrote:
> On Tuesday, February 22, 2011 7:33 AM Hans Verkuil wrote: > > On Tuesday, 
February 22, 2011 12:40:32 Guennadi Liakhovetski wrote:
> > > On Tue, 22 Feb 2011, Stanimir Varbanov wrote:
> > > > This RFC patch adds a new subdev sensor operation named
> > > > g_interface_parms.
> > > >
> > > > It is planned as a not mandatory operation and it is driver's
> > > > developer decision to use it or not.
> > > > 
> > > > Please share your opinions and ideas.
> > 
> > Stanimir, thanks for the RFC. I think it is time that we create a good
> > solution for this. This is currently the last remaining issue preventing
> > soc-camera subdevs from being used generally. (Control handling is also
> > still special, but this is being worked on.)
> > 
> > > Yes, I like the idea in principle (/me pulling his bullet-proof vest
> > > on), :-) :
> > > as some of you might guess, because I feel it's going away from the
> > > idea, that I've been hard pressed to accept of hard-coding the media-bus
> > > configuration and in the direction of direct communication of
> > > bus-parameters between the (sub-)devices, e.g., a camera host and a
> > > camera device in soc-camera terminology.
> > > 
> > > But before reviewing the patch as such, I'd like to discuss the
> > > strategy, that we want to pursue here - what exactly do we want to
> > > hard-code and what we want to configure dynamically? As explained
> > > before, my preference would be to only specify the absolute minimum in
> > > the platform data, i.e., parameters that either are ambiguous or special
> > > for this platform. So, once again, my approach to configure interface
> > > parameters like signal polarities and edge sensitivity is:
> > > 
> > > 1. if at least one side has a fixed value of the specific parameter,
> > > usually no need to specify it in platform data. Example: sensor only
> > > supports HSYNC active high, host supports both, normally "high" should
> > > be selected.
> > > 
> > > 2. as above, but there's an inverter on the board in the signal path.
> > > The "invert" parameter must be specified in the platform data and the
> > > host will configure itself to "low" and send "high" confirmed to the
> > > sensor.
> > > 
> > > 3. both are configurable. In this case the platform data has to
> > > specify, which polarity shall be used.
> > > 
> > > This is simple, it is implemented, it has worked that way with no
> > > problem for several years now.
> > > 
> > > The configuration procedure in this case looks like:
> > > 
> > > 1. host requests supported interface configurations from the client
> > > (sensor)
> > > 
> > > 2. host matches returned parameters against platform data and its own
> > > capabilities
> > > 
> > > 3. if no suitable configuration possible - error out
> > > 
> > > 4. the single possible configuration is identified and sent to the
> > > sensor back for its configuration
> > > 
> > > This way we need one more method: s_interface_parms.
> > > 
> > > Shortly talking to Laurent earlier today privately, he mentioned, that
> > > one of the reasons for this move is to support dynamic bus
> > > reconfiguration, e.g., the number of used CSI lanes. The same is useful
> > > for parallel interfaces. E.g., I had to hack the omap3spi driver to
> > > capture only 8 (parallel) data lanes from the sensor, connected with all
> > > its 10 lanes to get a format, easily supported by user-space
> > > applications. Ideally you don't want to change anything in the code for
> > > this. If the user is requesting the 10-bit format, all 10 lanes are
> > > used, if only 8 - the interface is reconfigured accordingly.
> > 
> > I have no problems with dynamic bus reconfiguration as such. So if the
> > host driver wants to do lane reconfiguration, then that's fine by me.
> > 
> > When it comes to signal integrity (polarity, rising/falling edge), then I
> > remain convinced that this should be set via platform data. This is not
> > something that should be negotiated since this depends not only on the
> > sensor and host devices, but also on the routing of the lines between them
> > on the actual board, how much noise there is on those lines, the quality
> > of the clock signal, etc. Not really an issue with PAL/NTSC type signals,
> > but when you get to 1080p60 and up, then such things become much more
> > important.
> > 
> > So these settings should not be negotiated, but set explicitly.
> > 
> > It actually doesn't have to be done through platform data (although that
> > makes the most sense), as long as it is explicitly set based on board-
> > specific data.

I agree with Hans here. I don't think negotiation is a good idea.

> My 2 cents here is that I think this consists in 2 parts, and should be
> divided properly.
> 
> 1. You know required # of lanes & clockspeed after you had set the format.
> For example:
> 
> - VGA @ 30fps
>   DDR Clk: 330 MHz
>   Number of Datalanes: 1
> 
> - VGA @ 60fps
>   DDR Clk: 330 MHz
>   Number of Datalanes: 2
> 
> - 12MPix @ 10fps
>   DDR Clk: 480 MHz
>   Number of Datalanes: 2
> 
> This usually is something you know from the sensor config selected based
> on your params.
> 
> So, I think this is something that the host should ask to the client
> after setting size & framerate, or just before VIDIOC_STREAMON.
> 
> THIS I think must be handle dynamically by introducing it as part of
> a V4l2_subdev sensor_ops entry. If this can't be done, then how?

Sounds good to me, that's what this RFC is about. The host needs to retrieve 
interface parameters from the client at streamon time, and a new subdev 
operation looks like an easy solution. The client will obviously need to get 
the acceptable  set of parameters from platform data.

> 2. Specifying polarity and lanes physical position.
> 
> For example, at least OMAP3 & 4 has the following pin pairs:
> 
> CSI2_DX0, CSI2_DY0
> CSI2_DX1, CSI2_DY1
> CSI2_DX2, CSI2_DY2
> CSI2_DX3, CSI2_DY3
> CSI2_DX4, CSI2_DY4
> 
> So, what you do is that, you can control where do you want the clock,
> where do you want each datalane pair, and also the pin polarity
> (X: +, Y: -, or viceversa). And this is something that is static.
> THIS I think should go in the host driver's platform data.
> 
> Of course, I'm here discarding the fact that at least some sensors that
> I know of can change the primary data lane position in sensor configuration
> (at least some OmniVision sensors do), but I'll assume this is usually
> Locked, and assume the host is better at that.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

On Tuesday 22 February 2011 18:08:40 Guennadi Liakhovetski wrote:
> On Tue, 22 Feb 2011, Hans Verkuil wrote:

[snip]

> > I also think that there is a reasonable chance that such bugs can happen.
> > Take a scenario like this: someone writes a new host driver. Initially
> > there is only support for positive polarity and detection on the rising
> > edge, because that's what the current board on which the driver was
> > developed supports. This is quite typical for an initial version of a
> > driver.
> > 
> > Later someone adds support for negative polarity and falling edge.
> > Suddenly the polarity negotiation on the previous board results in
> > negative instead of positive which was never tested. Now that board
> > starts producing pixel errors every so often. And yes, this type of
> > hardware problems do happen as I know from painful experience.
> > 
> > Problems like this are next to impossible to debug without the aid of an
> > oscilloscope, so this isn't like most other bugs that are relatively easy
> > to debug.
> 
> Well, this is pretty simple to debug with the help of git bisect, as long
> as patches are sufficiently clean and properly broken down into single
> topics.

It won't be that easy, as the problems might not be easily reproduceable. 
Changing the auto-negotiation code will require testing all boards that use 
it, which is something that can't be done by the person submitting the patch. 
We will get hard to detect (and debug) breakages.

> > It is so much easier just to avoid this by putting it in platform data.
> > It's simple, unambiguous and above all, unchanging.
> 
> As I said, this all boils down to who does patches and who accepts them
> for mainlibe.

-- 
Regards,

Laurent Pinchart

RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Aguirre, Sergio]

Guennadi and Hans,

<snip>

> > The only static data I am concerned about are those that affect signal
> integrity.
> > After thinking carefully about this I realized that there is really only
> one
> > setting that is relevant to that: the sampling edge. The polarities do
> not
> > matter in this.

I respectfully disagree.

AFAIK, There is not such thing as sampling edge configuration for MIPI
Receivers, and the polarities DO matter, since it's a differential
signal.

> 
> Ok, this is much better! I'm still not perfectly happy having to punish
> all just for the sake of a couple of broken boards, but I can certainly
> much better live with this, than with having to hard-code each and every
> bit. Thanks, Hans!
> 
> So, I think, we can proceed with this, let's see the code now, shall we?;)
> 
> Currently soc-camera auto-configures the following parameters:
> 
> hsync polarity
> vsync polarity
> data polarity
> master / slave mode
> data bus width
> pixel clock polarity
> 
> (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and
> drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()).
> Removing the pixclk polarity, the rest we can use as a basis for a new
> subdev-based implementation.

Don't we need to move this out from soc_camera and make it available in
v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI
support)

That way both SoC_Camera, and Media Controller frameworks can use that.

Regards,
Sergio

> 
> Thanks
> Guennadi
> 
> > Unfortunately, if a subdev is set to 'sample at rising edge', then that
> does
> > not necessarily mean that the host should sample at the same edge.
> Depending
> > on the clock line routing and the integrity of the clock signal the host
> may
> > actually have to sample on the other edge. And yes, I've seen this.
> >
> > Anyway, this has been discussed to death already. I am very much opposed
> to
> > negotiating the sampling edge. During the Helsinki meeting in June last
> year
> > we decided to do this via platform data (see section 7 in the meeting
> > minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab).
> >
> > I will formally NACK attempts to negotiate this. Mauro is of course free
> to
> > override me.
> >
> > Something simple like this for subdev platform_data might be enough:
> >
> > struct v4l2_bus_config {
> >         /* 0 - sample at falling edge, 1 - sample at rising edge */
> >         unsigned edge_pclock:1;
> >         /* 0 - host should use the same sampling edge, 1 - host should
> use the
> >            other sampling edge */
> >         unsigned host_invert_edge_pclock:1;
> > };
> >
> > The host can query the bus configuration and the subdev will return:
> >
> > 	edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock;
> >
> > We might want to add bits as well to describe whether polarities are
> inverted.
> >
> > This old RFC gives a good overview of the possible polarities:
> >
> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg09041.html
> >
> > Regards,
> >
> > 	Hans
> >
> > > I personally like the Stanimir's proposal as the parameters to be
> negotiated
> > > are pretty dynamic. Only the number of lanes could be problematic as
> not all
> > > lanes might be routed across different boards. Perhaps we should
> consider specifying
> > > an AUTO value for some negotiated parameters. Such as in case of an
> attribute that
> > > need to be fixed on some boards or can be fully negotiated on others,
> a fixed
> > > value or "auto" could be respectively set up in the host's
> platform_data. This could
> > > be used to override some parameters in the host driver if needed.
> > >
> > > IMHO, as long as we negotiate only dynamic parameters there should be
> no special
> > > issues.
> > >
> > > Regards,
> > > Sylwester
> > >
> > > > about this if it wasn't for the fact that soc-camera doesn't do this
> but instead
> > > > negotiates it. Obviously, it isn't a pleasant prospect having to
> change all that.
> > > >
> > > > Normally this would be enough of an argument for me to just
> negotiate it. The
> > > > reason that I don't want this in this particular case is that I know
> from
> > > > personal experience that incorrect settings can be extremely hard to
> find.
> > > >
> > > > I also think that there is a reasonable chance that such bugs can
> happen. Take
> > > > a scenario like this: someone writes a new host driver. Initially
> there is only
> > > > support for positive polarity and detection on the rising edge,
> because that's
> > > > what the current board on which the driver was developed supports.
> This is quite
> > > > typical for an initial version of a driver.
> > > >
> > > > Later someone adds support for negative polarity and falling edge.
> Suddenly the
> > > > polarity negotiation on the previous board results in negative
> instead of positive
> > > > which was never tested. Now that board starts producing pixel errors
> every so
> > > > often. And yes, this type of hardware problems do happen as I know
> from painful
> > > > experience.
> > > >
> > > > Problems like this are next to impossible to debug without the aid
> of an
> > > > oscilloscope, so this isn't like most other bugs that are relatively
> easy to
> > > > debug.
> > > >
> > > > It is so much easier just to avoid this by putting it in platform
> data. It's
> > > > simple, unambiguous and above all, unchanging.
> > > >
> > > > Regards,
> > > >
> > > > 	Hans
> > > >
> > > >>
> > > >> Thanks
> > > >> Guennadi
> > > >> ---
> > > >> Guennadi Liakhovetski, Ph.D.
> > > >> Freelance Open-Source Software Developer
> > > >> http://www.open-technology.de/
> > > >> --
> > > >> 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
> > > >>
> > > >
> > >
> > >
> >
> > --
> > Hans Verkuil - video4linux developer - sponsored by Cisco
> >
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Wednesday, February 23, 2011 15:06:49 Aguirre, Sergio wrote:
> Guennadi and Hans,
> 
> <snip>
> 
> > > The only static data I am concerned about are those that affect signal
> > integrity.
> > > After thinking carefully about this I realized that there is really only
> > one
> > > setting that is relevant to that: the sampling edge. The polarities do
> > not
> > > matter in this.
> 
> I respectfully disagree.
> 
> AFAIK, There is not such thing as sampling edge configuration for MIPI
> Receivers, and the polarities DO matter, since it's a differential
> signal.

The polarities do not matter for a standard parallel bus. I cannot speak for 
MIPI or CSI busses as I have no experience there. So if you say that 
polarities matter for MIPI, then for MIPI those should be specified statically 
as well.

> 
> > 
> > Ok, this is much better! I'm still not perfectly happy having to punish
> > all just for the sake of a couple of broken boards, but I can certainly
> > much better live with this, than with having to hard-code each and every
> > bit. Thanks, Hans!
> > 
> > So, I think, we can proceed with this, let's see the code now, shall we?;)
> > 
> > Currently soc-camera auto-configures the following parameters:
> > 
> > hsync polarity
> > vsync polarity
> > data polarity
> > master / slave mode
> > data bus width
> > pixel clock polarity
> > 
> > (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and
> > drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()).
> > Removing the pixclk polarity, the rest we can use as a basis for a new
> > subdev-based implementation.
> 
> Don't we need to move this out from soc_camera and make it available in
> v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI
> support)
> 
> That way both SoC_Camera, and Media Controller frameworks can use that.

I believe that is the plan, yes.

Regards,

	Hans

> 
> Regards,
> Sergio
> 
> > 
> > Thanks
> > Guennadi
> > 
> > > Unfortunately, if a subdev is set to 'sample at rising edge', then that
> > does
> > > not necessarily mean that the host should sample at the same edge.
> > Depending
> > > on the clock line routing and the integrity of the clock signal the host
> > may
> > > actually have to sample on the other edge. And yes, I've seen this.
> > >
> > > Anyway, this has been discussed to death already. I am very much opposed
> > to
> > > negotiating the sampling edge. During the Helsinki meeting in June last
> > year
> > > we decided to do this via platform data (see section 7 in the meeting
> > > minutes: http://www.linuxtv.org/news.php?entry=2010-06-22.mchehab).
> > >
> > > I will formally NACK attempts to negotiate this. Mauro is of course free
> > to
> > > override me.
> > >
> > > Something simple like this for subdev platform_data might be enough:
> > >
> > > struct v4l2_bus_config {
> > >         /* 0 - sample at falling edge, 1 - sample at rising edge */
> > >         unsigned edge_pclock:1;
> > >         /* 0 - host should use the same sampling edge, 1 - host should
> > use the
> > >            other sampling edge */
> > >         unsigned host_invert_edge_pclock:1;
> > > };
> > >
> > > The host can query the bus configuration and the subdev will return:
> > >
> > > 	edge = host_invert_edge_pclock ? !edge_pclock : edge_pclock;
> > >
> > > We might want to add bits as well to describe whether polarities are
> > inverted.
> > >
> > > This old RFC gives a good overview of the possible polarities:
> > >
> > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg09041.html
> > >
> > > Regards,
> > >
> > > 	Hans
> > >
> > > > I personally like the Stanimir's proposal as the parameters to be
> > negotiated
> > > > are pretty dynamic. Only the number of lanes could be problematic as
> > not all
> > > > lanes might be routed across different boards. Perhaps we should
> > consider specifying
> > > > an AUTO value for some negotiated parameters. Such as in case of an
> > attribute that
> > > > need to be fixed on some boards or can be fully negotiated on others,
> > a fixed
> > > > value or "auto" could be respectively set up in the host's
> > platform_data. This could
> > > > be used to override some parameters in the host driver if needed.
> > > >
> > > > IMHO, as long as we negotiate only dynamic parameters there should be
> > no special
> > > > issues.
> > > >
> > > > Regards,
> > > > Sylwester
> > > >
> > > > > about this if it wasn't for the fact that soc-camera doesn't do this
> > but instead
> > > > > negotiates it. Obviously, it isn't a pleasant prospect having to
> > change all that.
> > > > >
> > > > > Normally this would be enough of an argument for me to just
> > negotiate it. The
> > > > > reason that I don't want this in this particular case is that I know
> > from
> > > > > personal experience that incorrect settings can be extremely hard to
> > find.
> > > > >
> > > > > I also think that there is a reasonable chance that such bugs can
> > happen. Take
> > > > > a scenario like this: someone writes a new host driver. Initially
> > there is only
> > > > > support for positive polarity and detection on the rising edge,
> > because that's
> > > > > what the current board on which the driver was developed supports.
> > This is quite
> > > > > typical for an initial version of a driver.
> > > > >
> > > > > Later someone adds support for negative polarity and falling edge.
> > Suddenly the
> > > > > polarity negotiation on the previous board results in negative
> > instead of positive
> > > > > which was never tested. Now that board starts producing pixel errors
> > every so
> > > > > often. And yes, this type of hardware problems do happen as I know
> > from painful
> > > > > experience.
> > > > >
> > > > > Problems like this are next to impossible to debug without the aid
> > of an
> > > > > oscilloscope, so this isn't like most other bugs that are relatively
> > easy to
> > > > > debug.
> > > > >
> > > > > It is so much easier just to avoid this by putting it in platform
> > data. It's
> > > > > simple, unambiguous and above all, unchanging.
> > > > >
> > > > > Regards,
> > > > >
> > > > > 	Hans
> > > > >
> > > > >>
> > > > >> Thanks
> > > > >> Guennadi
> > > > >> ---
> > > > >> Guennadi Liakhovetski, Ph.D.
> > > > >> Freelance Open-Source Software Developer
> > > > >> http://www.open-technology.de/
> > > > >> --
> > > > >> 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
> > > > >>
> > > > >
> > > >
> > > >
> > >
> > > --
> > > Hans Verkuil - video4linux developer - sponsored by Cisco
> > >
> > 
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> 

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Wed, 23 Feb 2011, Hans Verkuil wrote:

> On Wednesday, February 23, 2011 15:06:49 Aguirre, Sergio wrote:
> > Guennadi and Hans,
> > 
> > <snip>
> > 
> > > > The only static data I am concerned about are those that affect signal
> > > integrity.
> > > > After thinking carefully about this I realized that there is really only
> > > one
> > > > setting that is relevant to that: the sampling edge. The polarities do
> > > not
> > > > matter in this.
> > 
> > I respectfully disagree.
> > 
> > AFAIK, There is not such thing as sampling edge configuration for MIPI
> > Receivers, and the polarities DO matter, since it's a differential
> > signal.
> 
> The polarities do not matter for a standard parallel bus. I cannot speak for 
> MIPI or CSI busses as I have no experience there. So if you say that 
> polarities matter for MIPI, then for MIPI those should be specified statically 
> as well.

Do I misunderstand? I interpreted Hans' proposal as: clock edge 
sensitivity is critical mainly because of high frequency, at which the 
signal integrity is harder to maintain, and therefore we cannot rely on 
automagic. Whereas sync signals are much lower frequency, and therefore 
any breakage would be easier to detect. I don't otherwise understand what 
"polarities do not matter" mean - of course they do. What am I missing?

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

RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Aguirre, Sergio]

> -----Original Message-----
> From: Hans Verkuil [mailto:hansverk@cisco.com]
> Sent: Wednesday, February 23, 2011 3:32 AM
> To: Hans Verkuil
> Cc: Sylwester Nawrocki; Guennadi Liakhovetski; Stan; linux-
> media@vger.kernel.org; Laurent Pinchart; Aguirre, Sergio
> Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
> 
> On Wednesday, February 23, 2011 09:10:42 Hans Verkuil wrote:
> > Unfortunately, if a subdev is set to 'sample at rising edge', then that
> does
> > not necessarily mean that the host should sample at the same edge.
> Depending
> > on the clock line routing and the integrity of the clock signal the host
> may
> > actually have to sample on the other edge. And yes, I've seen this.
> 
> It might be useful to give some background information regarding the
> sampling
> edge problems.
> 
> There are two main reasons why the sampling edge can be hardware
> dependent.
> The first is if the data lines go through an amplifier or something
> similar
> that will slightly delay the data lines compared to the clock signal. This
> can
> shift the edge at which you have to sample.
> 
> Actually, this may even be dependent on the clock frequency. I have not
> seen
> that in real life yet, but it might happen. This will complicate things
> even
> more since in that case you need to make a callback function in the board
> code
> that determines the sampling edge based on the clock frequency. I think we
> can
> ignore that for now, but we do need to keep it in mind.
> 
> The other is the waveform of the clock. For relatively low frequencies
> this
> will resemble a symmetrical square wave. But for higher frequencies this
> more
> resembles the bottom waveform in this picture:
> 
> http://myweb.msoe.edu/williamstm/Images/Divider2.jpg
> 
> This is asymmetric so depending on the slopes the sampling edge can make
> quite
> a difference.
> 
> The higher the clock frequency, the more asymmetric the waveform will
> look.

I think this a very specific HW problem (Low slew rate), and it's something
That should be fixed in HW, or avoided in SW after proper experimentation is
done.

One example on how to fix this is described in this document:

http://www.actel.com/documents/SchmittTrigger_AN.pdf

Regards,
Sergio

> 
> Regards,
> 
> 	Hans

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote:
> <snip>
> 
> > > The only static data I am concerned about are those that affect signal
> > > integrity.
> > 
> > > After thinking carefully about this I realized that there is really
> > > only one setting that is relevant to that: the sampling edge. The
> > > polarities do not matter in this.
> 
> I respectfully disagree.

So do I. Sampling edge is related to polarities, so you need to take both into 
account.

> AFAIK, There is not such thing as sampling edge configuration for MIPI
> Receivers, and the polarities DO matter, since it's a differential
> signal.
> 
> > Ok, this is much better! I'm still not perfectly happy having to punish
> > all just for the sake of a couple of broken boards, but I can certainly
> > much better live with this, than with having to hard-code each and every
> > bit. Thanks, Hans!
> > 
> > So, I think, we can proceed with this, let's see the code now, shall
> > we?;)
> > 
> > Currently soc-camera auto-configures the following parameters:
> > 
> > hsync polarity
> > vsync polarity
> > data polarity
> > master / slave mode

What do you mean by master/slave mode ?

> > data bus width

The data bus width can already be configured through the media bus format. Do 
we need to set it explicitly ?

> > pixel clock polarity
> >
> > (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and
> > drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()).
> > Removing the pixclk polarity, the rest we can use as a basis for a new
> > subdev-based implementation.
> 
> Don't we need to move this out from soc_camera and make it available in
> v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI
> support)
> 
> That way both SoC_Camera, and Media Controller frameworks can use that.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Wed, 23 Feb 2011, Laurent Pinchart wrote:

> > > Currently soc-camera auto-configures the following parameters:
> > > 
> > > hsync polarity
> > > vsync polarity
> > > data polarity
> > > master / slave mode
> 
> What do you mean by master/slave mode ?

Many datasheets define a slave mode, in which the sensor is receiving the 
sync signals and the pixel clock from the host and is only driving the 
data lanes.

> > > data bus width
> 
> The data bus width can already be configured through the media bus format. Do 
> we need to set it explicitly ?

Maybe we'd have to think about it more, but I think, we need it: Bus 
width, specified by media bus formats tells you how the data can be 
sampled on the bus. Whereas the above parameter tells you, how the devices 
are physically connected. With one and the same physical connection you 
can get several data formats, e.g., by using shifters, like on omap3.

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:
> On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote:
> > <snip>
> > 
> > > > The only static data I am concerned about are those that affect signal
> > > > integrity.
> > > 
> > > > After thinking carefully about this I realized that there is really
> > > > only one setting that is relevant to that: the sampling edge. The
> > > > polarities do not matter in this.
> > 
> > I respectfully disagree.
> 
> So do I. Sampling edge is related to polarities, so you need to take both 
into 
> account.

When you switch polarity for data/field/hsync/vsync signals on a simple bus 
you just invert whether a 1 bit is output as high or low voltage. So you just 
change the meaning of the bit. This does not matter for signal integrity, 
since you obviously have to be able to sample both low and high voltages. It 
is *when* you sample that can have a major effect.

This might be different for differential clocks. I have no experience with 
this, so I can't say anything sensible about that.

Regards,

	Hans

> 
> > AFAIK, There is not such thing as sampling edge configuration for MIPI
> > Receivers, and the polarities DO matter, since it's a differential
> > signal.
> > 
> > > Ok, this is much better! I'm still not perfectly happy having to punish
> > > all just for the sake of a couple of broken boards, but I can certainly
> > > much better live with this, than with having to hard-code each and every
> > > bit. Thanks, Hans!
> > > 
> > > So, I think, we can proceed with this, let's see the code now, shall
> > > we?;)
> > > 
> > > Currently soc-camera auto-configures the following parameters:
> > > 
> > > hsync polarity
> > > vsync polarity
> > > data polarity
> > > master / slave mode
> 
> What do you mean by master/slave mode ?
> 
> > > data bus width
> 
> The data bus width can already be configured through the media bus format. 
Do 
> we need to set it explicitly ?
> 
> > > pixel clock polarity
> > >
> > > (see include/media/soc_camera.h::soc_camera_bus_param_compatible() and
> > > drivers/media/video/soc_camera.c::soc_camera_apply_sensor_flags()).
> > > Removing the pixclk polarity, the rest we can use as a basis for a new
> > > subdev-based implementation.
> > 
> > Don't we need to move this out from soc_camera and make it available in
> > v4l2_subdev ops? (I'm talking about both parallel and the "new" MIPI
> > support)
> > 
> > That way both SoC_Camera, and Media Controller frameworks can use that.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> --
> 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
> 

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:
> On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:
> > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote:
> > > <snip>
> > > 
> > > > > The only static data I am concerned about are those that affect
> > > > > signal integrity.
> > > > > 
> > > > > After thinking carefully about this I realized that there is really
> > > > > only one setting that is relevant to that: the sampling edge. The
> > > > > polarities do not matter in this.
> > > 
> > > I respectfully disagree.
> > 
> > So do I. Sampling edge is related to polarities, so you need to take both
> > into account.
> 
> When you switch polarity for data/field/hsync/vsync signals on a simple bus
> you just invert whether a 1 bit is output as high or low voltage. So you
> just change the meaning of the bit. This does not matter for signal
> integrity, since you obviously have to be able to sample both low and high
> voltages. It is *when* you sample that can have a major effect.

When you switch the polarity you will likely have to sample on the opposite 
edge. If, for signal integrity reasons, you can only sample on a given edge, 
you will want to use a fixed polarity and not negotiate it.

Given the very small number of parameters that are negotiated by soc-camera at 
the moment, I'm very much in favour of hardcoding all of them in platform data 
and just adding a g_interface_parms subdev operation.

> This might be different for differential clocks. I have no experience with
> this, so I can't say anything sensible about that.

-- 
Regards,

Laurent Pinchart

RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Aguirre, Sergio]

Hi,

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Wednesday, February 23, 2011 10:15 AM
> To: Hans Verkuil
> Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester
> Nawrocki; Stan; linux-media@vger.kernel.org
> Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
> 
> On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:
> > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:
> > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote:
> > > > <snip>
> > > >
> > > > > > The only static data I am concerned about are those that affect
> > > > > > signal integrity.
> > > > > >
> > > > > > After thinking carefully about this I realized that there is
> really
> > > > > > only one setting that is relevant to that: the sampling edge.
> The
> > > > > > polarities do not matter in this.
> > > >
> > > > I respectfully disagree.
> > >
> > > So do I. Sampling edge is related to polarities, so you need to take
> both
> > > into account.
> >
> > When you switch polarity for data/field/hsync/vsync signals on a simple
> bus
> > you just invert whether a 1 bit is output as high or low voltage. So you
> > just change the meaning of the bit. This does not matter for signal
> > integrity, since you obviously have to be able to sample both low and
> high
> > voltages. It is *when* you sample that can have a major effect.
> 
> When you switch the polarity you will likely have to sample on the
> opposite
> edge. If, for signal integrity reasons, you can only sample on a given
> edge,
> you will want to use a fixed polarity and not negotiate it.

I guess this should be reason enough to decide this in platform data in the
board file.

> 
> Given the very small number of parameters that are negotiated by soc-
> camera at
> the moment, I'm very much in favour of hardcoding all of them in platform
> data
> and just adding a g_interface_parms subdev operation.

I'll second that.

Negotiating all this adds unnecessary complexity, and just makes the code
More prone to errors, and even probably causing hardware damage due to
misconfiguration. It's better to keep this static and make the board config
fully conscious of it.

Regards,
Sergio

> 
> > This might be different for differential clocks. I have no experience
> with
> > this, so I can't say anything sensible about that.
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Wednesday, February 23, 2011 17:14:41 Laurent Pinchart wrote:
> On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:
> > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:
> > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote:
> > > > <snip>
> > > > 
> > > > > > The only static data I am concerned about are those that affect
> > > > > > signal integrity.
> > > > > > 
> > > > > > After thinking carefully about this I realized that there is 
really
> > > > > > only one setting that is relevant to that: the sampling edge. The
> > > > > > polarities do not matter in this.
> > > > 
> > > > I respectfully disagree.
> > > 
> > > So do I. Sampling edge is related to polarities, so you need to take 
both
> > > into account.
> > 
> > When you switch polarity for data/field/hsync/vsync signals on a simple 
bus
> > you just invert whether a 1 bit is output as high or low voltage. So you
> > just change the meaning of the bit. This does not matter for signal
> > integrity, since you obviously have to be able to sample both low and high
> > voltages. It is *when* you sample that can have a major effect.
> 
> When you switch the polarity you will likely have to sample on the opposite 
> edge. If, for signal integrity reasons, you can only sample on a given edge, 
> you will want to use a fixed polarity and not negotiate it.

You are confusing clock polarity (which is relevant) with data, field, hsync 
and vsync polarities which just invert the meaning of those pins.

I wish I had a whiteboard to draw it, much easier to explain that way.

> 
> Given the very small number of parameters that are negotiated by soc-camera 
at 
> the moment, I'm very much in favour of hardcoding all of them in platform 
data 
> and just adding a g_interface_parms subdev operation.

Just for the record: I have no problem with hardcoding these polarities. After 
all, that was my original idea. But they can be negotiated as well.

Regards,

	Hans

> 
> > This might be different for differential clocks. I have no experience with
> > this, so I can't say anything sensible about that.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

On Wednesday 23 February 2011 17:28:39 Hans Verkuil wrote:
> On Wednesday, February 23, 2011 17:14:41 Laurent Pinchart wrote:
> > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:

[snip]

> > > When you switch polarity for data/field/hsync/vsync signals on a simple
> > > bus you just invert whether a 1 bit is output as high or low voltage. So
> > > you just change the meaning of the bit. This does not matter for
> > > signal integrity, since you obviously have to be able to sample both
> > > low and high voltages. It is *when* you sample that can have a major
> > > effect.
> > 
> > When you switch the polarity you will likely have to sample on the
> > opposite edge. If, for signal integrity reasons, you can only sample on
> > a given edge, you will want to use a fixed polarity and not negotiate
> > it.
> 
> You are confusing clock polarity (which is relevant) with data, field,
> hsync and vsync polarities which just invert the meaning of those pins.

I was talking about clock polarity. Sorry for the confusion.

> I wish I had a whiteboard to draw it, much easier to explain that way.
> 
> > Given the very small number of parameters that are negotiated by
> > soc-camera at the moment, I'm very much in favour of hardcoding all of
> > them in platform data and just adding a g_interface_parms subdev
> > operation.
> 
> Just for the record: I have no problem with hardcoding these polarities.
> After all, that was my original idea. But they can be negotiated as well.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

Hi Guennadi,

On Wednesday 23 February 2011 10:31:21 Guennadi Liakhovetski wrote:

[snip]

> Currently soc-camera auto-configures the following parameters:
> 
> hsync polarity
> vsync polarity
> data polarity

Data polarity ? Are there sensors that can invert the data polarity ?

> master / slave mode
> data bus width
> pixel clock polarity

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Wed, 23 Feb 2011, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Wednesday 23 February 2011 10:31:21 Guennadi Liakhovetski wrote:
> 
> [snip]
> 
> > Currently soc-camera auto-configures the following parameters:
> > 
> > hsync polarity
> > vsync polarity
> > data polarity
> 
> Data polarity ? Are there sensors that can invert the data polarity ?

Yes.

Regards
Guennadi

> 
> > master / slave mode
> > data bus width
> > pixel clock polarity
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Wed, 23 Feb 2011, Aguirre, Sergio wrote:

> Hi,
> 
> > -----Original Message-----
> > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> > Sent: Wednesday, February 23, 2011 10:15 AM
> > To: Hans Verkuil
> > Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester
> > Nawrocki; Stan; linux-media@vger.kernel.org
> > Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
> > 
> > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:
> > > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:
> > > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote:
> > > > > <snip>
> > > > >
> > > > > > > The only static data I am concerned about are those that affect
> > > > > > > signal integrity.
> > > > > > >
> > > > > > > After thinking carefully about this I realized that there is
> > really
> > > > > > > only one setting that is relevant to that: the sampling edge.
> > The
> > > > > > > polarities do not matter in this.
> > > > >
> > > > > I respectfully disagree.
> > > >
> > > > So do I. Sampling edge is related to polarities, so you need to take
> > both
> > > > into account.
> > >
> > > When you switch polarity for data/field/hsync/vsync signals on a simple
> > bus
> > > you just invert whether a 1 bit is output as high or low voltage. So you
> > > just change the meaning of the bit. This does not matter for signal
> > > integrity, since you obviously have to be able to sample both low and
> > high
> > > voltages. It is *when* you sample that can have a major effect.
> > 
> > When you switch the polarity you will likely have to sample on the
> > opposite
> > edge. If, for signal integrity reasons, you can only sample on a given
> > edge,
> > you will want to use a fixed polarity and not negotiate it.
> 
> I guess this should be reason enough to decide this in platform data in the
> board file.
> 
> > 
> > Given the very small number of parameters that are negotiated by soc-
> > camera at
> > the moment, I'm very much in favour of hardcoding all of them in platform
> > data
> > and just adding a g_interface_parms subdev operation.
> 
> I'll second that.
> 
> Negotiating all this adds unnecessary complexity, and just makes the code
> More prone to errors, and even probably causing hardware damage due to
> misconfiguration. It's better to keep this static and make the board config
> fully conscious of it.

Sorry, I accept different opinions, and in the end only one of the two 
possibilities will be implemented, and either way it'll all work in the 
end, but, I don't buy either of these arguments. Complexity - the code is 
already there, it is working, it is simple, it has not broken since it has 
been implemented. I had it hard-coded in the beginning and I went over to 
negotiation and never regretted it.

Hardware damage - if this were the case, I'd probably be surrounded only 
by bricks. How configuring a wrong hsync polarity can damage your 
hardware?

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

RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Aguirre, Sergio]

Hi Guennadi,

> -----Original Message-----
> From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de]
> Sent: Wednesday, February 23, 2011 10:46 AM
> To: Aguirre, Sergio
> Cc: Laurent Pinchart; Hans Verkuil; Sylwester Nawrocki; Stan; linux-
> media@vger.kernel.org
> Subject: RE: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms
> 
> On Wed, 23 Feb 2011, Aguirre, Sergio wrote:
> 
> > Hi,
> >
> > > -----Original Message-----
> > > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> > > Sent: Wednesday, February 23, 2011 10:15 AM
> > > To: Hans Verkuil
> > > Cc: Aguirre, Sergio; Guennadi Liakhovetski; Hans Verkuil; Sylwester
> > > Nawrocki; Stan; linux-media@vger.kernel.org
> > > Subject: Re: [RFC/PATCH 0/1] New subdev sensor operation
> g_interface_parms
> > >
> > > On Wednesday 23 February 2011 17:02:57 Hans Verkuil wrote:
> > > > On Wednesday, February 23, 2011 16:30:42 Laurent Pinchart wrote:
> > > > > On Wednesday 23 February 2011 15:06:49 Aguirre, Sergio wrote:
> > > > > > <snip>
> > > > > >
> > > > > > > > The only static data I am concerned about are those that
> affect
> > > > > > > > signal integrity.
> > > > > > > >
> > > > > > > > After thinking carefully about this I realized that there is
> > > really
> > > > > > > > only one setting that is relevant to that: the sampling
> edge.
> > > The
> > > > > > > > polarities do not matter in this.
> > > > > >
> > > > > > I respectfully disagree.
> > > > >
> > > > > So do I. Sampling edge is related to polarities, so you need to
> take
> > > both
> > > > > into account.
> > > >
> > > > When you switch polarity for data/field/hsync/vsync signals on a
> simple
> > > bus
> > > > you just invert whether a 1 bit is output as high or low voltage. So
> you
> > > > just change the meaning of the bit. This does not matter for signal
> > > > integrity, since you obviously have to be able to sample both low
> and
> > > high
> > > > voltages. It is *when* you sample that can have a major effect.
> > >
> > > When you switch the polarity you will likely have to sample on the
> > > opposite
> > > edge. If, for signal integrity reasons, you can only sample on a given
> > > edge,
> > > you will want to use a fixed polarity and not negotiate it.
> >
> > I guess this should be reason enough to decide this in platform data in
> the
> > board file.
> >
> > >
> > > Given the very small number of parameters that are negotiated by soc-
> > > camera at
> > > the moment, I'm very much in favour of hardcoding all of them in
> platform
> > > data
> > > and just adding a g_interface_parms subdev operation.
> >
> > I'll second that.
> >
> > Negotiating all this adds unnecessary complexity, and just makes the
> code
> > More prone to errors, and even probably causing hardware damage due to
> > misconfiguration. It's better to keep this static and make the board
> config
> > fully conscious of it.
> 
> Sorry, I accept different opinions, and in the end only one of the two
> possibilities will be implemented, and either way it'll all work in the
> end, but, I don't buy either of these arguments.

> Complexity - the code is
> already there, it is working, it is simple, it has not broken since it has
> been implemented. I had it hard-coded in the beginning and I went over to
> negotiation and never regretted it.

First of all, it seems that this discussion is heavily parallel i/f
oriented, and soc_camera focused, and it's just not like that.

Now, _just_ for soc_camera framework, yeah... it works and it's there, but
still not providing a solution for other v4l2_subdev users (like Media
Controller).

Complexity comes only when trying to make this truly generic, and avoid
fragmentation of solutions (1 for soc, 1 for MC), plus adding support for
serial (MIPI) interfaces

Now, also, the patch originally proposed by Stan doesn't actually deal with
putting polarities as part of the interface parameters, which is something
you're currently negotiating in soc_camera framework, again, just for
parallel interfaces.

Now, just for the sake of clarifying my understanding, I guess what you're
saying is to make sensor driver expose all possible polarities and physical
details configurable, and make the platform data limit the actual options due to the physical layout.

For example, if in my board A, I have:

	- OV5640 sensor driver, which supports both Parallel and CSI2
        Interfaces (with up to 2 datalanes)
	- Rx subdev (or host) driver(s) which support Parallel, CSI2-A and
        CSI2-B interfaces (with 2 and 1 datalanes respectively).

I should specify in my boardfile integration details, such as the
sensor is actually wired to the CSI2-B I/f, so make the sensor
negotiate with the other side of the bus and enable CSI2 i/f with
given details, like just use 1 datalane, and match datalane
position/polarity.

Am I understanding right?

> 
> Hardware damage - if this were the case, I'd probably be surrounded only
> by bricks. How configuring a wrong hsync polarity can damage your
> hardware?

Ok, I'll regret my statement on this one. I guess I was a bit too dramatic
to point out consequences of HW mismatches. Nevermind this.

Regards,
Sergio

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Stanimir Varbanov]

Hi,

<snip>
>> Sorry, I accept different opinions, and in the end only one of the two
>> possibilities will be implemented, and either way it'll all work in the
>> end, but, I don't buy either of these arguments.
> 
>> Complexity - the code is
>> already there, it is working, it is simple, it has not broken since it has
>> been implemented. I had it hard-coded in the beginning and I went over to
>> negotiation and never regretted it.
> 
> First of all, it seems that this discussion is heavily parallel i/f
> oriented, and soc_camera focused, and it's just not like that.
> 

yes, it seems that is correct. My patch just get back on host side some
sensor dynamic parameters and it doesn't pretending for any negotiation.

> Now, _just_ for soc_camera framework, yeah... it works and it's there, but
> still not providing a solution for other v4l2_subdev users (like Media
> Controller).
> 

I already start looking into soc_camera code, but I'm so confused. :(

> Complexity comes only when trying to make this truly generic, and avoid
> fragmentation of solutions (1 for soc, 1 for MC), plus adding support for
> serial (MIPI) interfaces
> 
> Now, also, the patch originally proposed by Stan doesn't actually deal with
> putting polarities as part of the interface parameters, which is something
> you're currently negotiating in soc_camera framework, again, just for
> parallel interfaces.
> 
> Now, just for the sake of clarifying my understanding, I guess what you're
> saying is to make sensor driver expose all possible polarities and physical
> details configurable, and make the platform data limit the actual options due to the physical layout.
> 
> For example, if in my board A, I have:
> 
> 	- OV5640 sensor driver, which supports both Parallel and CSI2
>         Interfaces (with up to 2 datalanes)
> 	- Rx subdev (or host) driver(s) which support Parallel, CSI2-A and
>         CSI2-B interfaces (with 2 and 1 datalanes respectively).
> 

If the sensor is physically connected as parallel and serial the board
code should dictates the preferred interface, IMO. Or ...

> I should specify in my boardfile integration details, such as the
> sensor is actually wired to the CSI2-B I/f, so make the sensor
> negotiate with the other side of the bus and enable CSI2 i/f with
> given details, like just use 1 datalane, and match datalane
> position/polarity.
> 
> Am I understanding right?
> 
>> Hardware damage - if this were the case, I'd probably be surrounded only
>> by bricks. How configuring a wrong hsync polarity can damage your
>> hardware?
> 
> Ok, I'll regret my statement on this one. I guess I was a bit too dramatic
> to point out consequences of HW mismatches. Nevermind this.
> 

:)

> Regards,
> Sergio
> 
>> Thanks
>> Guennadi
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>> http://www.open-technology.de/
> --
> 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


-- 
Best regards,
Stan

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Sakari Ailus]

Hi Guennadi and others,

Apologies for the late reply...

Guennadi Liakhovetski wrote:
> On Wed, 23 Feb 2011, Hans Verkuil wrote:
> 
>> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
>>> Clock values are often being rounded at runtime and do not always reflect exactly
>>> the numbers fixed at compile time. And negotiation could help to obtain exact
>>> values at both sensor and host side.
>>
>> The only static data I am concerned about are those that affect signal integrity.
>> After thinking carefully about this I realized that there is really only one
>> setting that is relevant to that: the sampling edge. The polarities do not
>> matter in this.
> 
> Ok, this is much better! I'm still not perfectly happy having to punish 
> all just for the sake of a couple of broken boards, but I can certainly 
> much better live with this, than with having to hard-code each and every 
> bit. Thanks, Hans!

How much punishing would actually take place without autonegotiation?
How many boards do we have in total? I counted around 26 of
soc_camera_link declarations under arch/. Are there more?

An example of hardware which does care about clock polarity is the
N8[01]0. The parallel clock polarity is inverted since this actually
does improve reliability. In an ideal hardware this likely wouldn't
happen but sometimes the hardware is not exactly ideal. Both the sensor
and the camera block support non-inverted and inverted clock signal.

So at the very least it should be possible to provide this information
in the board code even if both ends share multiple common values for
parameters.

There have been many comments on the dangers of the autonegotiation and
I share those concerns. One of my main concerns is that it creates an
unnecessary dependency from all the boards to the negotiation code, the
behaviour of which may not change.

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:
> Hi Guennadi and others,
> 
> Apologies for the late reply...
> 
> Guennadi Liakhovetski wrote:
> > On Wed, 23 Feb 2011, Hans Verkuil wrote:
> > 
> >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
> >>> Clock values are often being rounded at runtime and do not always reflect exactly
> >>> the numbers fixed at compile time. And negotiation could help to obtain exact
> >>> values at both sensor and host side.
> >>
> >> The only static data I am concerned about are those that affect signal integrity.
> >> After thinking carefully about this I realized that there is really only one
> >> setting that is relevant to that: the sampling edge. The polarities do not
> >> matter in this.
> > 
> > Ok, this is much better! I'm still not perfectly happy having to punish 
> > all just for the sake of a couple of broken boards, but I can certainly 
> > much better live with this, than with having to hard-code each and every 
> > bit. Thanks, Hans!
> 
> How much punishing would actually take place without autonegotiation?
> How many boards do we have in total? I counted around 26 of
> soc_camera_link declarations under arch/. Are there more?
> 
> An example of hardware which does care about clock polarity is the
> N8[01]0. The parallel clock polarity is inverted since this actually
> does improve reliability. In an ideal hardware this likely wouldn't
> happen but sometimes the hardware is not exactly ideal. Both the sensor
> and the camera block support non-inverted and inverted clock signal.
> 
> So at the very least it should be possible to provide this information
> in the board code even if both ends share multiple common values for
> parameters.
> 
> There have been many comments on the dangers of the autonegotiation and
> I share those concerns. One of my main concerns is that it creates an
> unnecessary dependency from all the boards to the negotiation code, the
> behaviour of which may not change.

OK, let me summarize this and if there are no objections then Stan can start
implementing this.

1) We need two subdev ops: one reports the bus config capabilities and one that
sets it up. Note that these ops should be core ops since this functionality is
relevant for both sensors and video receive/transmit devices.

2) The clock sampling edge and polarity should not be negotiated but must be set
from board code for both subdevs and host. In the future this might even require
a callback with the clock frequency as argument.

3) We probably need a utility function that given the host and subdev capabilities
will return the required subdev/host settings.

4) soc-camera should switch to these new ops.

Of course, we also need MIPI support in this API. The same considerations apply to
MIPI as to the parallel bus: settings that depend on the hardware board design
should come from board code, others can be negotiated. Since I know next to nothing
about MIPI I will leave that to the experts...

One thing I am not sure about is if we want separate ops for parallel bus and MIPI,
or if we merge them. I am leaning towards separate ops as I think that might be
easier to implement.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Guennadi Liakhovetski]

On Sat, 26 Feb 2011, Hans Verkuil wrote:

> On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:
> > Hi Guennadi and others,
> > 
> > Apologies for the late reply...
> > 
> > Guennadi Liakhovetski wrote:
> > > On Wed, 23 Feb 2011, Hans Verkuil wrote:
> > > 
> > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
> > >>> Clock values are often being rounded at runtime and do not always reflect exactly
> > >>> the numbers fixed at compile time. And negotiation could help to obtain exact
> > >>> values at both sensor and host side.
> > >>
> > >> The only static data I am concerned about are those that affect signal integrity.
> > >> After thinking carefully about this I realized that there is really only one
> > >> setting that is relevant to that: the sampling edge. The polarities do not
> > >> matter in this.
> > > 
> > > Ok, this is much better! I'm still not perfectly happy having to punish 
> > > all just for the sake of a couple of broken boards, but I can certainly 
> > > much better live with this, than with having to hard-code each and every 
> > > bit. Thanks, Hans!
> > 
> > How much punishing would actually take place without autonegotiation?
> > How many boards do we have in total? I counted around 26 of
> > soc_camera_link declarations under arch/. Are there more?
> > 
> > An example of hardware which does care about clock polarity is the
> > N8[01]0. The parallel clock polarity is inverted since this actually
> > does improve reliability. In an ideal hardware this likely wouldn't
> > happen but sometimes the hardware is not exactly ideal. Both the sensor
> > and the camera block support non-inverted and inverted clock signal.
> > 
> > So at the very least it should be possible to provide this information
> > in the board code even if both ends share multiple common values for
> > parameters.
> > 
> > There have been many comments on the dangers of the autonegotiation and
> > I share those concerns. One of my main concerns is that it creates an
> > unnecessary dependency from all the boards to the negotiation code, the
> > behaviour of which may not change.

Sorry, didn't want to comment on this... But to me this sounds like a void 
argument... Yes, there are _many_ inter-dependencies in the kernel, and if 
you break code something will stop working... What's new about it??? But 
no, I do not want to continue this discussion endlessly...

> OK, let me summarize this and if there are no objections then Stan can start
> implementing this.
> 
> 1) We need two subdev ops: one reports the bus config capabilities and one that
> sets it up. Note that these ops should be core ops since this functionality is
> relevant for both sensors and video receive/transmit devices.
> 
> 2) The clock sampling edge and polarity should not be negotiated but must be set
> from board code for both subdevs and host. In the future this might even require
> a callback with the clock frequency as argument.
> 
> 3) We probably need a utility function that given the host and subdev capabilities
> will return the required subdev/host settings.
> 
> 4) soc-camera should switch to these new ops.

...remains only to find, who will do this;)

So, I'm in minority here, if we don't count all those X systems, 
successfully using soc-camera with its evil auto-negotiation. If you just 
decide to do this and push the changes - sure, there's nothing I can do 
against this. But if you decide to postpone a final decision on this until 
we meet personally and will not have to circulate the same arguments 100 
times - just because the delay is shorter - maybe we can find a solution, 
that will keep everyone happy.

> Of course, we also need MIPI support in this API. The same considerations apply to
> MIPI as to the parallel bus: settings that depend on the hardware board design
> should come from board code, others can be negotiated. Since I know next to nothing
> about MIPI I will leave that to the experts...
> 
> One thing I am not sure about is if we want separate ops for parallel bus and MIPI,
> or if we merge them. I am leaning towards separate ops as I think that might be
> easier to implement.

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

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Hans Verkuil]

On Saturday, February 26, 2011 14:14:29 Guennadi Liakhovetski wrote:
> On Sat, 26 Feb 2011, Hans Verkuil wrote:
> 
> > On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:
> > > Hi Guennadi and others,
> > > 
> > > Apologies for the late reply...
> > > 
> > > Guennadi Liakhovetski wrote:
> > > > On Wed, 23 Feb 2011, Hans Verkuil wrote:
> > > > 
> > > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
> > > >>> Clock values are often being rounded at runtime and do not always reflect exactly
> > > >>> the numbers fixed at compile time. And negotiation could help to obtain exact
> > > >>> values at both sensor and host side.
> > > >>
> > > >> The only static data I am concerned about are those that affect signal integrity.
> > > >> After thinking carefully about this I realized that there is really only one
> > > >> setting that is relevant to that: the sampling edge. The polarities do not
> > > >> matter in this.
> > > > 
> > > > Ok, this is much better! I'm still not perfectly happy having to punish 
> > > > all just for the sake of a couple of broken boards, but I can certainly 
> > > > much better live with this, than with having to hard-code each and every 
> > > > bit. Thanks, Hans!
> > > 
> > > How much punishing would actually take place without autonegotiation?
> > > How many boards do we have in total? I counted around 26 of
> > > soc_camera_link declarations under arch/. Are there more?
> > > 
> > > An example of hardware which does care about clock polarity is the
> > > N8[01]0. The parallel clock polarity is inverted since this actually
> > > does improve reliability. In an ideal hardware this likely wouldn't
> > > happen but sometimes the hardware is not exactly ideal. Both the sensor
> > > and the camera block support non-inverted and inverted clock signal.
> > > 
> > > So at the very least it should be possible to provide this information
> > > in the board code even if both ends share multiple common values for
> > > parameters.
> > > 
> > > There have been many comments on the dangers of the autonegotiation and
> > > I share those concerns. One of my main concerns is that it creates an
> > > unnecessary dependency from all the boards to the negotiation code, the
> > > behaviour of which may not change.
> 
> Sorry, didn't want to comment on this... But to me this sounds like a void 
> argument... Yes, there are _many_ inter-dependencies in the kernel, and if 
> you break code something will stop working... What's new about it??? But 
> no, I do not want to continue this discussion endlessly...
> 
> > OK, let me summarize this and if there are no objections then Stan can start
> > implementing this.
> > 
> > 1) We need two subdev ops: one reports the bus config capabilities and one that
> > sets it up. Note that these ops should be core ops since this functionality is
> > relevant for both sensors and video receive/transmit devices.
> > 
> > 2) The clock sampling edge and polarity should not be negotiated but must be set
> > from board code for both subdevs and host. In the future this might even require
> > a callback with the clock frequency as argument.
> > 
> > 3) We probably need a utility function that given the host and subdev capabilities
> > will return the required subdev/host settings.
> > 
> > 4) soc-camera should switch to these new ops.
> 
> ...remains only to find, who will do this;)
> 
> So, I'm in minority here, if we don't count all those X systems, 
> successfully using soc-camera with its evil auto-negotiation. If you just 
> decide to do this and push the changes - sure, there's nothing I can do 
> against this. But if you decide to postpone a final decision on this until 
> we meet personally and will not have to circulate the same arguments 100 
> times - just because the delay is shorter - maybe we can find a solution, 
> that will keep everyone happy.

No, I am no longer willing to postpone this. Sorry. Discussing this in a
brainstorm meeting or whatever won't bring us any closer. We did that in
the last Helsinki meeting already. Heck, we've gone over this for a year and a
half now, if not more. The arguments haven't changed in all that time. Enough
is enough.

Let Stan implement the new subdev core ops (Stan, please confirm that you can
work on this!), then we can use it in soc-camera for everything except the clock.
The final step will be to remove the clock negotiation from soc-camera.

By the time we are ready for that final step we'll see who can do this. It's
several months in the future anyway.

Regards,

	Hans

> > Of course, we also need MIPI support in this API. The same considerations apply to
> > MIPI as to the parallel bus: settings that depend on the hardware board design
> > should come from board code, others can be negotiated. Since I know next to nothing
> > about MIPI I will leave that to the experts...
> > 
> > One thing I am not sure about is if we want separate ops for parallel bus and MIPI,
> > or if we merge them. I am leaning towards separate ops as I think that might be
> > easier to implement.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> 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
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Sylwester Nawrocki]

On 02/26/2011 01:50 PM, Hans Verkuil wrote:
> On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:
>> Hi Guennadi and others,
>>
>> Apologies for the late reply...
>>
>> Guennadi Liakhovetski wrote:
>>> On Wed, 23 Feb 2011, Hans Verkuil wrote:
>>>
>>>> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
>>>>> Clock values are often being rounded at runtime and do not always reflect exactly
>>>>> the numbers fixed at compile time. And negotiation could help to obtain exact
>>>>> values at both sensor and host side.
>>>>
>>>> The only static data I am concerned about are those that affect signal integrity.
>>>> After thinking carefully about this I realized that there is really only one
>>>> setting that is relevant to that: the sampling edge. The polarities do not
>>>> matter in this.
>>>
>>> Ok, this is much better! I'm still not perfectly happy having to punish
>>> all just for the sake of a couple of broken boards, but I can certainly
>>> much better live with this, than with having to hard-code each and every
>>> bit. Thanks, Hans!
>>
>> How much punishing would actually take place without autonegotiation?
>> How many boards do we have in total? I counted around 26 of
>> soc_camera_link declarations under arch/. Are there more?
>>
>> An example of hardware which does care about clock polarity is the
>> N8[01]0. The parallel clock polarity is inverted since this actually
>> does improve reliability. In an ideal hardware this likely wouldn't
>> happen but sometimes the hardware is not exactly ideal. Both the sensor
>> and the camera block support non-inverted and inverted clock signal.
>>
>> So at the very least it should be possible to provide this information
>> in the board code even if both ends share multiple common values for
>> parameters.
>>
>> There have been many comments on the dangers of the autonegotiation and
>> I share those concerns. One of my main concerns is that it creates an
>> unnecessary dependency from all the boards to the negotiation code, the
>> behaviour of which may not change.
> 
> OK, let me summarize this and if there are no objections then Stan can start
> implementing this.
> 
> 1) We need two subdev ops: one reports the bus config capabilities and one that
> sets it up. Note that these ops should be core ops since this functionality is
> relevant for both sensors and video receive/transmit devices.

Sounds reasonable. In case of MIPI-CSI receiver as a separate subdev I assume 
it would allow to retrieve settings from sensor subdev and apply them to MIPI-CSI
receiver.

> 
> 2) The clock sampling edge and polarity should not be negotiated but must be set
> from board code for both subdevs and host. In the future this might even require
> a callback with the clock frequency as argument.
> 
> 3) We probably need a utility function that given the host and subdev capabilities
> will return the required subdev/host settings.
> 
> 4) soc-camera should switch to these new ops.
> 
> Of course, we also need MIPI support in this API. The same considerations apply to
> MIPI as to the parallel bus: settings that depend on the hardware board design
> should come from board code, others can be negotiated. Since I know next to nothing
> about MIPI I will leave that to the experts...
> 
> One thing I am not sure about is if we want separate ops for parallel bus and MIPI,
> or if we merge them. I am leaning towards separate ops as I think that might be
> easier to implement.

I suppose it wouldn't hurt to have same, parametrized ops for both parallel and serial
bus. Just like in the original Stan's RFC.

> 
> Regards,
> 
> 	Hans
> 

Re: [RFC/PATCH 0/1] New subdev sensor operation g_interface_parms

[Laurent Pinchart]

Hi Hans,

On Saturday 26 February 2011 13:50:12 Hans Verkuil wrote:
> On Friday, February 25, 2011 19:23:43 Sakari Ailus wrote:
> > Guennadi Liakhovetski wrote:
> > > On Wed, 23 Feb 2011, Hans Verkuil wrote:
> > >> On Tuesday, February 22, 2011 22:42:58 Sylwester Nawrocki wrote:
> > >>> Clock values are often being rounded at runtime and do not always
> > >>> reflect exactly the numbers fixed at compile time. And negotiation
> > >>> could help to obtain exact values at both sensor and host side.
> > >> 
> > >> The only static data I am concerned about are those that affect signal
> > >> integrity. After thinking carefully about this I realized that there
> > >> is really only one setting that is relevant to that: the sampling
> > >> edge. The polarities do not matter in this.
> > > 
> > > Ok, this is much better! I'm still not perfectly happy having to punish
> > > all just for the sake of a couple of broken boards, but I can certainly
> > > much better live with this, than with having to hard-code each and
> > > every bit. Thanks, Hans!
> > 
> > How much punishing would actually take place without autonegotiation?
> > How many boards do we have in total? I counted around 26 of
> > soc_camera_link declarations under arch/. Are there more?
> > 
> > An example of hardware which does care about clock polarity is the
> > N8[01]0. The parallel clock polarity is inverted since this actually
> > does improve reliability. In an ideal hardware this likely wouldn't
> > happen but sometimes the hardware is not exactly ideal. Both the sensor
> > and the camera block support non-inverted and inverted clock signal.
> > 
> > So at the very least it should be possible to provide this information
> > in the board code even if both ends share multiple common values for
> > parameters.
> > 
> > There have been many comments on the dangers of the autonegotiation and
> > I share those concerns. One of my main concerns is that it creates an
> > unnecessary dependency from all the boards to the negotiation code, the
> > behaviour of which may not change.
> 
> OK, let me summarize this and if there are no objections then Stan can
> start implementing this.
> 
> 1) We need two subdev ops: one reports the bus config capabilities and one
> that sets it up. Note that these ops should be core ops since this
> functionality is relevant for both sensors and video receive/transmit
> devices.

Could you elaborate on this ? Stan's original proposal is to report the subdev 
configuration so that the host can configure itself at streamon time. Why do 
we need an operation to setup the subdev ?

> 2) The clock sampling edge and polarity should not be negotiated but must
> be set from board code for both subdevs and host. In the future this might
> even require a callback with the clock frequency as argument.
> 
> 3) We probably need a utility function that given the host and subdev
> capabilities will return the required subdev/host settings.
> 
> 4) soc-camera should switch to these new ops.
> 
> Of course, we also need MIPI support in this API. The same considerations
> apply to MIPI as to the parallel bus: settings that depend on the hardware
> board design should come from board code, others can be negotiated. Since
> I know next to nothing about MIPI I will leave that to the experts...
> 
> One thing I am not sure about is if we want separate ops for parallel bus
> and MIPI, or if we merge them. I am leaning towards separate ops as I
> think that might be easier to implement.

-- 
Regards,

Laurent Pinchart