Hi Sakari, Laurent,
On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote:
> Hi Laurent,
>
> On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Apr 15, 2020 at 12:49:58PM +0200, Jacopo Mondi wrote:
> > > Introduce a new pad operation to allow retrieving the media bus
> > > configuration on a subdevice pad.
> > >
> > > The newly introduced operation reassembles the s/g_mbus_config video
> > > operation, which have been on their way to be deprecated since a long
> > > time.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > > ---
> > > include/media/v4l2-subdev.h | 69 +++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 69 insertions(+)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index a4848de59852..fc16af578471 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc {
> > > unsigned short num_entries;
> > > };
> > >
> > > +/**
> > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > + * @hsync_active: hsync active state: 1 for high, 0 for low
> > > + * @vsync_active: vsync active state: 1 for high, 0 for low
> > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling
> >
> > Is this for the driving side or the sampling side ?
>
> Is there a difference? I'd expect the configuration needs to be the same on
> both sides.
I was puzzled as well by this question, mostly because I never found
anything like this in the existing documentation, but actually yes,
the driving side clocks out data on one edge, sampling side samples on
the opposite one ? Is this what you meant Laurent ?
To me this has always been about sampling side though, and the setting
should match on both endpoints of course.
>
> >
> > > + * @data_active: data lines active state: 1 for high, 0 for low
> >
> > I wonder, is there any system with active low data lines ?
As this is part of the standard DT properties for video interfaces, I
added it here.
> >
> > > + */
> > > +struct v4l2_mbus_parallel_config {
> >
> > Is this intended to cover BT.656 too ? Either way I think it should be
> > documented.
>
> I think we should replace what directly refers to Bt.601 with something
> that refers to that, and not "parallel". Both are parallel after all. I
> think the naming is in line with that, assuming this covers both.
>
Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal
synch and field flags are specified. This implies the following DT
properties are shared between BT.601 and BT.656:
- pclk-sample
- data-active
- slave-mode
- bus-width
- data-shift
- sync-on-green-active
- data-enable-active
with
- hsync-active
- vsync-active
- field-even-active
being BT.601 only.
We could do here do the same and mention which flags apply to 601
only, or more clearly split the config structure by keeping a generic
'parallel' flag structure, with a sub-structure which is 601 specific.
I'm not sure it's worth the extra layer of indirection though.
> >
> > > + unsigned int hsync_active : 1;
> > > + unsigned int vsync_active : 1;
> > > + unsigned int pclk_rising : 1;
> > > + unsigned int data_active : 1;
> >
> > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used
> > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is
>
> I'd think it's easier to work with fields in drivers than the flags. This
I find it easier and less error prone to work with fields as well,
provided the space occupation is the same as working with flags.
> isn't uAPI so we don't need to think the ABI. The change could also be done
> to struct v4l2_fwnode_bus_parallel.
>
> > getting deprecated, it doesn't mean we can reuse the same flags if they
> > make sense. Otherwise we'll have to translate between
> > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally
Right, I agree this is not desirable. Every driver should inspect the
fwnode properties and translate to this new config when queryed
through get_mbus_format.
> > v4l2_fwnode_bus_parallel should be replaced with
> > v4l2_mbus_parallel_config (once we add additional fields).
I like this idea
We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel
struct v4l2_fwnode_bus_parallel {
unsigned int flags;
unsigned char bus_width;
unsigned char data_shift;
};
but then we should replace the whole structure.
All in all, working with the same set of flags is the less disruptive
change and would not require translations in drivers... I'm not a fan,
but currently seems the easiest way forward...
What do you think ?
>
> ...
>
> --
> Kind regards,
>
> Sakari Ailus
