Hi Laurent, On 2018-04-28 19:01:37 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Friday, 27 April 2018 02:28:32 EEST Niklas Söderlund wrote: > > On 2018-04-27 00:30:25 +0300, Laurent Pinchart wrote: > > > On Thursday, 26 April 2018 23:21:21 EEST Niklas Söderlund wrote: > > >> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > > >> supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > > >> connected between the video sources and the video grabbers (VIN). > > >> > > >> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > >> > > >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> > > >> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > >> Reviewed-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> > > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > >> > > >> --- > > >> > > >> * Changes since v13 > > >> - Change return rcar_csi2_formats + i to return &rcar_csi2_formats[i]. > > >> - Add define for PHCLM_STOPSTATECKL. > > >> - Update spelling in comments. > > >> - Update calculation in rcar_csi2_calc_phypll() according to > > >> https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one > > >> before v14 did not take into account that 2 bits per sample is > > >> transmitted. > > > > > > Just one small comment about this, please see below. > > > > > >> - Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch > > >> statement to set correct number of lanes to enable. > > >> - Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match > > >> style of rest of file. > > >> - Switch to %u instead of 0x%x when printing bus type. > > >> - Switch to %u instead of %d for priv->lanes which is unsigned. > > >> - Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in > > >> rcar_csi2_formats[]. > > >> - Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16. > > >> - Set INTSTATE after PL-11 is confirmed to match flow chart in > > >> datasheet. > > >> - Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs. > > >> - Add Maxime's and laurent's tags. > > >> --- > > >> > > >> drivers/media/platform/rcar-vin/Kconfig | 12 + > > >> drivers/media/platform/rcar-vin/Makefile | 1 + > > >> drivers/media/platform/rcar-vin/rcar-csi2.c | 883 ++++++++++++++++++++ > > >> 3 files changed, 896 insertions(+) > > >> create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > > [snip] > > > > > >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > > >> b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644 > > >> index 0000000000000000..49b29d5680f9d80b > > >> --- /dev/null > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > > [snip] > > > > > >> +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int > > >> bpp, > > >> + u32 *phypll) > > >> +{ > > >> + const struct phypll_hsfreqrange *hsfreq; > > >> + struct v4l2_subdev *source; > > >> + struct v4l2_ctrl *ctrl; > > >> + u64 mbps; > > >> + > > >> + if (!priv->remote) > > >> + return -ENODEV; > > >> + > > >> + source = priv->remote; > > >> + > > >> + /* Read the pixel rate control from remote */ > > >> + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > > >> + if (!ctrl) { > > >> + dev_err(priv->dev, "no pixel rate control in subdev %s\n", > > >> + source->name); > > >> + return -EINVAL; > > >> + } > > >> + > > >> + /* > > >> + * Calculate the phypll in mbps (from v4l2 documentation) > > >> + * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > > >> + */ > > >> + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > >> + do_div(mbps, priv->lanes * 2000000); > > > > > > pixel rate * bits per sample will give you the overall bit rate, which you > > > then divide by the number of lanes to get the bitrate per lane, and then > > > by 2 as D-PHY is a DDR PHY and transmits 2 bits per clock cycle. You then > > > end up with the link frequency, which is thus expressed in MHz, not in > > > Mbps. I would thus name the mbps variable freq, and rename the > > > phypll_hsfreqrange mbps field to freq (maybe with a small comment right > > > after the field to tell the value is expressed in MHz). > > > > I agree that freq would be a better name for what it represents. Never > > the less I prefer to stick with mbps as that is whats used in the > > datasheet. See Table '25.9 HSFREQRANGE Bit Set Values'. > > > > With this in mind if you still feel strongly about renaming it I could > > do so. But at lest for me it feels more useful to easily be able to map > > the variable to the datasheet tables :-) > > After reading the datasheet I don't care too strongly about a rename, but I > now care about getting it right :-) The datasheet clearly specifies the > maximum data rate to be 1.5 Gbps / lane (section 25.1.1 Features), and figure > 25.9 corroborates that and shows a corresponding frequency of 750 MHz. I thus > believe that the values in table 25.9 and 25.10 are indeed bitrates (in Mbps / > lane), so I think you shouldn't divide by 2 in the above formula. As this now appears to have been fully sorted out on the adv748x end I'm now in full agreement with you and I will not divide by 2 to get the bitrate here for the CSI-2 driver. > > > >> + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) > > >> + if (hsfreq->mbps >= mbps) > > >> + break; > > >> + > > >> + if (!hsfreq->mbps) { > > >> + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps); > > >> + return -ERANGE; > > >> + } > > >> + > > >> + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n", > > >> mbps, > > >> + hsfreq->mbps); > > >> + > > >> + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg); > > >> + > > >> + return 0; > > >> +} > > > > > > [snip] > > -- > Regards, > > Laurent Pinchart > > > -- Regards, Niklas Söderlund
