Re: [PATCH v10 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

On Saturday, 11 November 2017 02:11:13 EET Niklas Söderlund wrote:
> On 2017-11-11 00:32:27 +0200, Sakari Ailus wrote:
> > On Fri, Nov 10, 2017 at 02:31:37PM +0100, Niklas Söderlund wrote:
> >> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> >> supports the rcar-vin driver on 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@ragnatech.se>
> >> ---
> >> 
> >>  drivers/media/platform/rcar-vin/Kconfig     |  12 +
> >>  drivers/media/platform/rcar-vin/Makefile    |   1 +
> >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 934 ++++++++++++++++++++++
> >>  3 files changed, 947 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..27d09da191a09b39
> >> --- /dev/null
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c

[snip]

> >> +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv,
> >> +				 struct v4l2_subdev *source,
> >> +				 struct v4l2_mbus_framefmt *mf,
> >> +				 u32 *phypll)
> >> +{
> >> +	const struct phypll_hsfreqrange *hsfreq;
> >> +	const struct rcar_csi2_format *format;
> >> +	struct v4l2_ctrl *ctrl;
> >> +	u64 mbps;
> >> +
> >> +	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > 
> > How about LINK_FREQ instead? It'd be more straightforward to calculate
> > this. Up to you.
> 
> I need to use PIXEL_RATE as my test setup uses the adv748x driver which
> only implement that control. In the short term I would like to support
> both, but I need a setup to test that before I can add support for it.
> In the long term, maybe we should deprecate one of the controls?

The LINK_FREQ control is meant for the user to select one of the available 
link frequencies (when multiple values are possible), while the PIXEL_RATE 
control is a read-only control meant for the connected subdev to query the 
resulting pixel rate. I think both controls should be kept, and PIXEL_RATE 
should be used here.

> >> +	if (!ctrl) {
> >> +		dev_err(priv->dev, "no link freq control in subdev %s\n",
> >> +			source->name);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	format = rcar_csi2_code_to_fmt(mf->code);
> >> +	if (!format) {
> >> +		dev_err(priv->dev, "Unknown format: %d\n", mf->code);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * format->bpp;
> >> +	do_div(mbps, priv->lanes * 1000000);
> >> +
> >> +	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