Hi Sakari
On Mon, 27 Jun 2011, Sakari Ailus wrote:
> On Fri, Jun 24, 2011 at 12:35:03AM +0200, Guennadi Liakhovetski wrote:
> > Hi Sakari
>
> Hi Guennadi,
>
> > On Fri, 24 Jun 2011, Sakari Ailus wrote:
> >
> > > Hi Guennadi,
> > >
> > > Thanks for the patch. I have a few comments below.
> > >
> > > On Wed, Jun 22, 2011 at 11:26:29PM +0200, Guennadi Liakhovetski wrote:
>
> [clip]
>
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index 1562c4f..75919ef 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -255,6 +255,10 @@ struct v4l2_subdev_audio_ops {
> > > > try_mbus_fmt: try to set a pixel format on a video data source
> > > >
> > > > s_mbus_fmt: set a pixel format on a video data source
> > > > +
> > > > + g_mbus_config: get supported mediabus configurations
> > > > +
> > > > + s_mbus_config: set a certain mediabus configuration
> > > > */
> > > > struct v4l2_subdev_video_ops {
> > > > int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > > > @@ -294,6 +298,8 @@ struct v4l2_subdev_video_ops {
> > > > struct v4l2_mbus_framefmt *fmt);
> > > > int (*s_mbus_fmt)(struct v4l2_subdev *sd,
> > > > struct v4l2_mbus_framefmt *fmt);
> > > > + int (*g_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > > > + int (*s_mbus_config)(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg);
> > >
> > > How would the ops be used and by whom?
> >
> > In the first line I want to use them to make re-use of soc-camera and
> > other subdev drivers possible. Soc-camera drivers would switch to these
> > from their specific bus-configuration methods, other drivers are free to
> > use or not to use them.
> >
> > > How complete configuration for CSI2 bus is the above intend to be? Complete,
> > > I suppose, and so I think we'll also need to specify how e.g. the CSI2 clock
> > > and data lanes have been connected between transmitter and receiver. This is
> > > less trivial to guess than clock polarity and requires further information
> > > from the board and lane mapping configuration capabilities of both.
> > > Shouldn't this information be also added to CSI2 configuration?
> > >
> > > Do you think a single bitmask would suffice for this in the long run?
> >
> > If you have certain additions to this API I would be happy to consider
> > them. Otherwise, if you just suppose, that we might need more
> > configuration options, we can always add them in the future, when we know
> > more about them and have specific use-cases, with which we can test them.
> > E.g., I am not sure what data- and clock-lane connection possibilities you
> > refer to above. Don't you _have_ to connect lane 1 to 1 etc?
>
> You don't need to. It actually depends on the hardware: the OMAP 3 CSI-2
> receiver, for example, may freely configure the order of the data and clock
> lanes. And you'll actually have to do that configuration since this is
> purely board dependent.
Nice
> I'm not sure whether the CSI-2 specification requires this, however. I
> wouldn't be surprised if there were sensors which could also do this.
Ok, to be flexible and consistent, we could do the same as with other
parameters. We could design a query like "to which pin can you route data
lane 1?" with possible replies "I can only route lane 1 to pin 1," or "I
can route lane 1 to pins 1, 2, 3, 4, or 5" (for 4 data-lanes and one clock
lane. Then the configuration request could do "please, route your data
lane 1 to pin 1." Or we can hard-code this routing in the platform data
for now.
I, probably, already mentioned this before, but this is also how the
present soc-camera API evolved in the past: in the beginning it also was
completely static, then I noticed, that instead of hard coding the
configuration, I can let drivers negotiate parameters automatically. Then
gradually the number of auto-negotiated parameters grew, as more flexible
hardware had to be handled.
We could do the same here: first hard-code this, then see, if it is
becoming a burden, having to hard-code these. Notice, that
auto-configuring these should not be a problem. This is not like
configuring a signal polarity, of which one can work better, than the
other.
> For OMAP 3 ISP driver this configuration is in struct isp_csiphy_lanes_cfg
> in drivers/media/video/omap3isp/ispcsiphy.h at the moment.
>
> The OMAP 3 ISP driver currently uses all the lanes it has, as far as I
> understand, so you can't disable them right now.
>
> > > I can see some use for the information in the set operation in lane
> > > configuration, for example, as you mentioned. My guess would be that the
> > > number of lanes _might_ be something that the user space would want to know
> > > or possibly even configure --- but we'll first need to discuss low-level
> > > sensor control interface.
> > >
> > > But otherwise the configuration should likely be rather static and board
> > > specific. Wouldn't the subdevs get this as part of the platform data, or
> > > how?
> > >
> > > I would just keep the bus configuration static board dependent information
> > > until we have that part working and used by drivers and extend it later on.
> >
> > As I said, drivers are free to not use these methods and implement a
> > platform-provided configuration method. In which case, as, I think, Hans
> > pointed out earlier, they can also choose to use this struct in their
> > platform data.
>
> If the structures are expected to be generic I somehow feel that a field of
> flags isn't the best way to describe the configuration of CSI-2 or other
> busses. Why not to just use a structure with bus type and an union for
> bus-specific configuration parameters? It'd be easier to access and also to
> change as needed than flags in an unsigned long field.
Well, yes, a union can be a good idea, thanks.
> Also, this structure is primarily about capabilities and not configuration.
> Having settings e.g. for active low vsync and the opposite isn't making this
> easier for drivers. There should be just one --- more so if the matching
> will be specific to SoC camera only.
>
> What would you think of having separate structures for configuration and
> capabilities of the busses? I think this way the mechanism would be more
> useful outside the scope of SoC camera. The CSI-2 and parallel busses are
> still quite generic.
Disagree, O'm quite happy with the current flag system, used for both
capabilities and configuration.
> That said, at this point I'm not exactly sure what configuration should be
> board specific and what should be dynamically configurable. The lane setup I
> mentioned earlier, for example, is something that would be good to be
> dynamically configurable in the future. Even if there are e.g. two lanes the
> user still might want to use just one. In this case you need to be able to
> tell the hardware has that two lanes but you only use one of them.
Yes, but what exactly do you want to make configurable? The _number_ of
lanes, or their routing?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
