Re: [PATCH v2 4/4] media: tc358746: add Toshiba TC358746 Parallel to CSI-2 bridge driver

[Dave Stevenson]

Hi Laurent & Marco

On Tue, 20 Sept 2022 at 12:15, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Marco,
>
> On Tue, Sep 20, 2022 at 12:48:54PM +0200, Marco Felsch wrote:
> > On 22-09-19, Laurent Pinchart wrote:
> > > On Mon, Sep 19, 2022 at 07:11:42PM +0200, Marco Felsch wrote:
> > > > On 22-09-19, Laurent Pinchart wrote:
> > > > > On Fri, Sep 16, 2022 at 03:45:35PM +0200, Marco Felsch wrote:
> > > > > > Adding support for the TC358746 parallel <-> MIPI CSI bridge. This chip
> > > > > > supports two operating modes:
> > > > > >   1st) parallel-in -> mipi-csi out
> > > > > >   2nd) mipi-csi in -> parallel out
> > > > > >
> > > > > > This patch only adds the support for the 1st mode.
> > > > > >
> > > > > > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > Changelog:
> > > > > >
> > > > > > v2:
> > > > > > - use the correct CID_LINK_FREQ control to query the sensor_pclk_rate
> > > > > > - remove now not needed tc358746_link_setup() and
> > > > > >   struct v4l2_ctrl sensor_pclk_ctrl
> > > > > > - call v4l2_subdev_link_validate_default() during link validation
> > > > > > - remove MEDIA_BUS_FMT_GBR888_1X24/YUV444 format support
> > > > > > - use subdev active_state API
> > > > > > - replace own .get_fmt with v4l2_subdev_get_fmt
> > > > > > - remove unnecessary pad checks
> > > > > > - restructure tc358746_get_format_by_code() if-case
> > > > > > - move apply_dphy_config|apply_misc_config from resume intos s_stream
> > > > > > - use goto in s_stream enable case
> > > > > > - fix error handling in suspend/resume
> > > > > > - split probe() into more sub-functions
> > > > > > - use dev_dbg() for printing successful probe
> > > > > >
> > > > > >  drivers/media/i2c/Kconfig    |   17 +
> > > > > >  drivers/media/i2c/Makefile   |    1 +
> > > > > >  drivers/media/i2c/tc358746.c | 1682 ++++++++++++++++++++++++++++++++++
<snip>
> > > > > > +
> > > > > > +     sensor = media_entity_to_v4l2_subdev(link->source->entity);
> > > > > > +     sensor_pclk_rate = v4l2_get_link_freq(sensor->ctrl_handler, 0, 0);
> > > > >
> > > > > Shouldn't you set the last two arguments to non-zero values, to support
> > > > > sources that only implement the V4L2_CID_PIXEL_RATE control ?
> > > >
> > > > Nope, I don't wanna support PIXEL_RATE right now. This can be changed
> > > > later I think.
> > >
> > > Would it be hard to support it already, given that the
> > > v4l2_get_link_freq() should make it easier ? That would avoid having to
> > > come back to this code later.
> >
> > I had the pixel-rate first, then Jacobo mentioned (correctly) that my
> > usage of pixel-rate was wrong. Supporting PIXEL_RATE as well would add
> > more complexity because we need to take core of the mbus format to get
> > the correct mul/div settings.
>
> That's right, but the required information could be stored in the
> tc358746_format structure, can't it ?
>
> > Also I think that only a few drivers
> > implementing the PIXEL_RATE correctly in case of parallel sensors _and_
> > this is just a fallback which will print a warning if triggered. All I
> > want to do here is: "give me the link frequence" :) If there are drivers
> > not supporting this but support PIXEL_RATE it shouldn't be that hard for
> > those driver to add the LINK_FREQ ctrl. This would also improve the
> > kernel quality since there are now heuristics and no warnings printed.
> >
> > Is it okay, to keep it simple and just go with LINK_FREQ. for now?
>
> OK, I won't insist much.
>
> > > > > I'd also name the variable source_link_freq, as it may not be a sensor,
> > > > > and it's a link frequency, not a pixel clock rate.
> > > >
> > > > In parallel case (which is the only supported right now) the pclk is the
> > > > link_freq. but I can change it of course.
> > >
> > > I read "pclk" as "pixel clock". That makes me think of
> > > V4L2_CID_PIXEL_RATE, which indicates the number of pixels per second.
> > > With YUV 4:2:2 2X8 media bus formats, the link frequency will be twice
> > > the pixel rate.
> >
> > Hm.. the link frequency is the frequency on the physical parallel bus,
> > as far as I understood the ctrl. In parallel use-case this is pixelclk.
> >
> > Also according PIXEL_RATE documentation, it is defined as
> > pixel-per-second. For YUV 4:2:2 those this mean mean:
> >  - y1 == 1st pixel,
> >  - u1 == 2nd pixel,
> >  - y2 == 3rd pixel,
> >  - ...
>
> YUYV8_2X8 transfers Y0, U0, Y1, V0, Y2, U2, Y3, V2, ... You need two
> cycles per pixel. That's why sensor_pclk_rate can be misleading, as it
> may refer to the sensor pixel sampling clock, or the parallel bus clock,
> and those two are different. It's just a variable naming issue to avoid
> confusion.
>
> > All I want is to get the rate/frequency of the physical bus from the
> > input device :) According my above explanation, could we please go with
> > "the LINK_FREQ ctrl only" since this would avoid possible kernel
> > warnings and is the most accurate one.

This is a bridge, so surely PIXEL_RATE is a property and control on
the source to the bridge, not on the bridge itself.
PIXEL_RATE isn't going to tell you much without HBLANK and VBLANK as
well, and those also belong to the source.

LINK_FREQ is a property that is only relevant for the output of the
bridge, and therefore it makes sense to be a control on the bridge (it
can't be represented elsewhere).

Just my 2p.
  Dave