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
