Hello,
On Mon, May 11, 2020 at 11:03:54PM +0300, Sakari Ailus wrote:
> On Mon, May 11, 2020 at 01:32:39PM +0200, Jacopo Mondi wrote:
> > On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote:
> > > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote:
> > > > 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 ?
Yes, that's what I meant, sorry for the confusion.
> > To me this has always been about sampling side though, and the setting
> > should match on both endpoints of course.
Can we make it explicit ? How about naming the field pclk_sample_edge,
and adding macros name *_RISING and *_FALLING ? See
include/drm/drm_connector.h for an example (DRM_BUS_FLAG_PIXDATA_*).
> > > > > + * @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
Isn't this a property of analog signals ?
> > - data-enable-active
Does BT.656 have a data enable signal ?
> >
> > 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.
Possibly not, I would be fine with just documenting the structure and
fields in details.
> > > > > + 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.
I'm probably influenced by DRM_BUS_FLAG_* here :-) Especially for the
signal polarity, being able to say
->flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
in the driver the transmitter driver, and
if (->flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE)
...
in the receiver driver is very nice. I won't push for it though.
> > > 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.
Do you mean with a helper function ?
> > > > 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 ?
>
> Could we use a struct instead, say:
>
> struct v4l2_parallel_flags {
> unsigned int hsync_active:1;
> /* and so on */
> };
>
> And then you'd add this to struct v4l2_fwnode_bus_parallel as a field. No
> defines would be needed this way, and it'd be slightly safer because you
> get types checked by the compilter.
>
> I don't have strong opinion either way. Both would work just fine.
That's fine with me. As I wrote above I think flags can increase
readability in some cases, but I won't insist.
--
Regards,
Laurent Pinchart
