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

[Hans Verkuil <hansverk@cisco.com>]

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/
>