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.
>
> > + * @data_active: data lines active state: 1 for high, 0 for low
>
> I wonder, is there any system with active low data lines ?
>
> > + */
> > +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.
>
> > + 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
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
> v4l2_fwnode_bus_parallel should be replaced with
> v4l2_mbus_parallel_config (once we add additional fields).
...
--
Kind regards,
Sakari Ailus
