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

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 24065 bytes --]

A few corrections,

On Thu, Apr 05, 2018 at 11:10:01AM +0200, jacopo mondi wrote:
> Hi Niklas,
>         thanks for the VIN and CSI-2 effort!
>
> On Tue, Feb 13, 2018 at 12:01:32AM +0100, 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@ragnatech.se>
> > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/platform/rcar-vin/Kconfig     |  12 +
> >  drivers/media/platform/rcar-vin/Makefile    |   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 ++++++++++++++++++++++++++++
> >  3 files changed, 897 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> >
>
> [snip]
>
> > +
> > +static const struct rcar_csi2_format rcar_csi2_formats[] = {
> > +	{ .code = MEDIA_BUS_FMT_RGB888_1X24,	.datatype = 0x24, .bpp = 24 },
> > +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
> > +	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .bpp = 16 },
> > +	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .bpp = 16 },
>
> Shouldn't YUYV10_2X10 format have 20 bits per pixel?
>
> > +};
> > +
> > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > +		if (rcar_csi2_formats[i].code == code)
> > +			return rcar_csi2_formats + i;
> > +
> > +	return NULL;
> > +}
> > +
> > +enum rcar_csi2_pads {
> > +	RCAR_CSI2_SINK,
> > +	RCAR_CSI2_SOURCE_VC0,
> > +	RCAR_CSI2_SOURCE_VC1,
> > +	RCAR_CSI2_SOURCE_VC2,
> > +	RCAR_CSI2_SOURCE_VC3,
> > +	NR_OF_RCAR_CSI2_PAD,
> > +};
> > +
> > +struct rcar_csi2_info {
> > +	const struct phypll_hsfreqrange *hsfreqrange;
> > +	unsigned int csi0clkfreqrange;
> > +	bool clear_ulps;
> > +	bool init_phtw;
> > +};
> > +
> > +struct rcar_csi2 {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	const struct rcar_csi2_info *info;
> > +
> > +	struct v4l2_subdev subdev;
> > +	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> > +
> > +	struct v4l2_async_notifier notifier;
> > +	struct v4l2_async_subdev asd;
> > +	struct v4l2_subdev *remote;
> > +
> > +	struct v4l2_mbus_framefmt mf;
> > +
> > +	struct mutex lock;
> > +	int stream_count;
> > +
> > +	unsigned short lanes;
> > +	unsigned char lane_swap[4];
> > +};
> > +
> > +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > +{
> > +	return container_of(sd, struct rcar_csi2, subdev);
> > +}
> > +
> > +static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier *n)
> > +{
> > +	return container_of(n, struct rcar_csi2, notifier);
> > +}
> > +
> > +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg)
> > +{
> > +	return ioread32(priv->base + reg);
> > +}
> > +
> > +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> > +{
> > +	iowrite32(data, priv->base + reg);
> > +}
> > +
> > +static void rcar_csi2_reset(struct rcar_csi2 *priv)
> > +{
> > +	rcar_csi2_write(priv, SRST_REG, SRST_SRST);
> > +	usleep_range(100, 150);
> > +	rcar_csi2_write(priv, SRST_REG, 0);
> > +}
> > +
> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +	int timeout;
> > +
> > +	/* Wait for the clock and data lanes to enter LP-11 state. */
> > +	for (timeout = 100; timeout > 0; timeout--) {
> > +		const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +		if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
>
> Nitpicking:
> 		if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&
>
> Don't you prefer to provide defines also for bit fields instead of
> using magic values? In this case something like
> PHCLM_REG_STOPSTATE_CLK would do.
>
> Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
> have to set INSTATE to an hardcoded value after having validated PHDLM.
> Maybe it is not necessary, just pointing it out.
>
> > +		    (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > +			return 0;
> > +
> > +		msleep(20);
> > +	}
> > +
> > +	dev_err(priv->dev, "Timeout waiting for LP-11 state\n");
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +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 */
> > +	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * 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;
> > +}
> > +
> > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +{
> > +	const struct rcar_csi2_format *format;
> > +	u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > +		priv->mf.width, priv->mf.height,
> > +		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +
> > +	/* Code is validated in set_fmt */
> > +	format = rcar_csi2_code_to_fmt(priv->mf.code);
> > +
> > +	/*
> > +	 * Enable all Virtual Channels
> > +	 *
> > +	 * NOTE: It's not possible to get individual datatype for each
> > +	 *       source virtual channel. Once this is possible in V4L2
> > +	 *       it should be used here.
> > +	 */
> > +	for (i = 0; i < 4; i++) {
> > +		u32 vcdt_part;
> > +
> > +		vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> > +			VCDT_SEL_DT(format->datatype);
> > +
> > +		/* Store in correct reg and offset */
> > +		if (i < 2)
> > +			vcdt |= vcdt_part << ((i % 2) * 16);
> > +		else
> > +			vcdt2 |= vcdt_part << ((i % 2) * 16);
> > +	}
> > +
> > +	switch (priv->lanes) {
> > +	case 1:
> > +		phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > +		break;
> > +	case 2:
> > +		phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +		break;
> > +	case 4:
> > +		phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > +			PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +		break;
>
> Even simpler this could be written as
>
>                 phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1;

Suggested by Geert already, sorry I missed that (and I'm probably
missing () around (1 << priv->lanes) - 1

>
> > +	default:
> > +		return -EINVAL;
>
> Can this happen? You have validated priv->lanes already when parsing
> DT
>
> > +	}
> > +
> > +	ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Clear Ultra Low Power interrupt */
> > +	if (priv->info->clear_ulps)
> > +		rcar_csi2_write(priv, INTSTATE_REG,
> > +				INTSTATE_INT_ULPS_START |
> > +				INTSTATE_INT_ULPS_END);
> > +
> > +	/* Init */
> > +	rcar_csi2_write(priv, TREF_REG, TREF_TREF);
> > +	rcar_csi2_reset(priv);
> > +	rcar_csi2_write(priv, PHTC_REG, 0);
> > +
> > +	/* Configure */
> > +	rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > +			FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
>
> On the FLD_FLD_NUM(2) mask. Why 2?
> I read on the datasheet "the register must not be changed from default
> value" and I read defaul to be 0x0000
>
> Also, please consider a make as all other fields are enabled
> unconditionally.

s/make/mask

I felt like I should have corrected this, otherwise the previous
statement does not make sense :)

Thanks
   j

>
> > +	rcar_csi2_write(priv, VCDT_REG, vcdt);
> > +	rcar_csi2_write(priv, VCDT2_REG, vcdt2);
> > +	/* Lanes are zero indexed */
> > +	rcar_csi2_write(priv, LSWAP_REG,
> > +			LSWAP_L0SEL(priv->lane_swap[0] - 1) |
> > +			LSWAP_L1SEL(priv->lane_swap[1] - 1) |
> > +			LSWAP_L2SEL(priv->lane_swap[2] - 1) |
> > +			LSWAP_L3SEL(priv->lane_swap[3] - 1));
>
> EDIT:
> (This comment is way too long for the real value it has, but since I
> already wrote it, and my initial doubt clarified while I was writing,
> resulting in a much less serious issues, I'm gonna keep it all anyway.
> Sorry about this :)
>
> Why - 1 ?
> Is this because it is assumed clock lane is in position 0? Is this
> fixed by design?
>
> What I read in datasheet for LSWAP_REG is:
> L[0-3]SEL       0 = Use PHY lane 0
>                 1 = Use PHY lane 1
>                 2 = Use PHY lane 2
>                 3 = Use PHY lane 3
>
> priv->lane_swap[i] is collected parsing 'data_lanes' property and
> should reflect the actual physical lane value assigned to logical lane
> numbers. If 'data_lanes' is, say <1 2> I expect
>
> priv->lane_swap[0] = 1;
> priv->lane_swap[1] = 2;
> priv->lane_swap[1] = 3; //assigned by your parsing routine
> priv->lane_swap[1] = 4; //assigned by your parsing routine
>
> And I understand LSWAP counts instead from [0-3] so, ok, I get why you
> subtract one. But now I wonder what happens if instead, lane position
> is specified counting from 0 in DT. Ah, I see you refuse lane_swap
> values < 1! So It should be assumed clock is by HW design on lane 0,
> so wouldn't you need to mention in DT bindings that the HW has clock
> lanes fixed in position 0 and the accepted values for the 'data_lanes'
> property ranges in the [1-4] interval?
>
> > +
> > +	if (priv->info->init_phtw) {
> > +		/*
> > +		 * This is for H3 ES2.0
> > +		 *
> > +		 * NOTE: Additional logic is needed here when
> > +		 * support for V3H and/or M3-N is added
> > +		 */
> > +		rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2);
> > +		rcar_csi2_write(priv, PHTW_REG, 0x010101e3);
> > +		rcar_csi2_write(priv, PHTW_REG, 0x010101e4);
> > +		rcar_csi2_write(priv, PHTW_REG, 0x01100104);
> > +		rcar_csi2_write(priv, PHTW_REG, 0x01030100);
> > +		rcar_csi2_write(priv, PHTW_REG, 0x01800100);
> > +	}
> > +
> > +	/* Start */
> > +	rcar_csi2_write(priv, PHYPLL_REG, phypll);
> > +
> > +	/* Set frequency range if we have it */
> > +	if (priv->info->csi0clkfreqrange)
> > +		rcar_csi2_write(priv, CSI0CLKFCPR_REG,
> > +				CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange));
> > +
> > +	rcar_csi2_write(priv, PHYCNT_REG, phycnt);
> > +	rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN |
> > +			LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP);
> > +	rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
> > +	rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ |
> > +			PHYCNT_RSTZ);
>
> Nit: from tables 25.[17-20] it seems to me you do not have to re-issue
> PHYCNT_SHUTDOWNZ when writing PHYCNT_RSTZ to PHYCNT_REG.
>
> > +
> > +	return rcar_csi2_wait_phy_start(priv);
> > +}
> > +
> > +static void rcar_csi2_stop(struct rcar_csi2 *priv)
> > +{
> > +	rcar_csi2_write(priv, PHYCNT_REG, 0);
> > +
> > +	rcar_csi2_reset(priv);
> > +}
> > +
> > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +	struct v4l2_subdev *nextsd;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&priv->lock);
> > +
> > +	if (!priv->remote) {
> > +		ret = -ENODEV;
> > +		goto out;
> > +	}
>
> Can this happen?
>
> The 'bind' callback sets priv->remote and it gets assigned back to
> NULL only on 'unbind'. Wouldn't it be better to remove the link in the
> media graph and let the system return an EPIPE before calling this?
>
> > +
> > +	nextsd = priv->remote;
> > +
> > +	if (enable && priv->stream_count == 0) {
> > +		pm_runtime_get_sync(priv->dev);
> > +
> > +		ret = rcar_csi2_start(priv);
> > +		if (ret) {
> > +			pm_runtime_put(priv->dev);
> > +			goto out;
> > +		}
> > +
> > +		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> > +		if (ret) {
> > +			rcar_csi2_stop(priv);
> > +			pm_runtime_put(priv->dev);
> > +			goto out;
> > +		}
> > +	} else if (!enable && priv->stream_count == 1) {
> > +		rcar_csi2_stop(priv);
> > +		v4l2_subdev_call(nextsd, video, s_stream, 0);
> > +		pm_runtime_put(priv->dev);
> > +	}
> > +
> > +	priv->stream_count += enable ? 1 : -1;
> > +out:
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_pad_config *cfg,
> > +				    struct v4l2_subdev_format *format)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +	struct v4l2_mbus_framefmt *framefmt;
> > +
> > +	if (!rcar_csi2_code_to_fmt(format->format.code))
> > +		return -EINVAL;
> > +
> > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > +		priv->mf = format->format;
> > +	} else {
> > +		framefmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> > +		*framefmt = format->format;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_pad_config *cfg,
> > +				    struct v4l2_subdev_format *format)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		format->format = priv->mf;
> > +	else
> > +		format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> > +	.s_stream = rcar_csi2_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> > +	.set_fmt = rcar_csi2_set_pad_format,
> > +	.get_fmt = rcar_csi2_get_pad_format,
> > +};
> > +
> > +static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> > +	.video	= &rcar_csi2_video_ops,
> > +	.pad	= &rcar_csi2_pad_ops,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Async and registered of subdevices and links
> > + */
> > +
> > +static int rcar_csi2_notify_bound(struct v4l2_async_notifier *notifier,
> > +				   struct v4l2_subdev *subdev,
> > +				   struct v4l2_async_subdev *asd)
> > +{
> > +	struct rcar_csi2 *priv = notifier_to_csi2(notifier);
> > +	int pad;
> > +
> > +	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
> > +					  MEDIA_PAD_FL_SOURCE);
> > +	if (pad < 0) {
> > +		dev_err(priv->dev, "Failed to find pad for %s\n", subdev->name);
> > +		return pad;
> > +	}
> > +
> > +	priv->remote = subdev;
> > +
> > +	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
> > +
> > +	return media_create_pad_link(&subdev->entity, pad,
> > +				     &priv->subdev.entity, 0,
> > +				     MEDIA_LNK_FL_ENABLED |
> > +				     MEDIA_LNK_FL_IMMUTABLE);
> > +}
> > +
> > +static void rcar_csi2_notify_unbind(struct v4l2_async_notifier *notifier,
> > +				       struct v4l2_subdev *subdev,
> > +				       struct v4l2_async_subdev *asd)
> > +{
> > +	struct rcar_csi2 *priv = notifier_to_csi2(notifier);
> > +
> > +	priv->remote = NULL;
> > +
> > +	dev_dbg(priv->dev, "Unbind %s\n", subdev->name);
> > +}
> > +
> > +static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = {
> > +	.bound = rcar_csi2_notify_bound,
> > +	.unbind = rcar_csi2_notify_unbind,
> > +};
> > +
> > +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv,
> > +				struct v4l2_fwnode_endpoint *vep)
> > +{
> > +	unsigned int i;
> > +
> > +	/* Only port 0 endpoint 0 is valid */
> > +	if (vep->base.port || vep->base.id)
> > +		return -ENOTCONN;
> > +
> > +	if (vep->bus_type != V4L2_MBUS_CSI2) {
> > +		dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	priv->lanes = vep->bus.mipi_csi2.num_data_lanes;
> > +	if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) {
>
> Is this an HW limitation? Like the 'data_lanes' comment, if it is,
> shouldn't you mention in bindings that the accepted lane numbers is
> limited to the [1,2,4] values.
>
> > +		dev_err(priv->dev, "Unsupported number of data-lanes: %u\n",
> > +			priv->lanes);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
> > +		priv->lane_swap[i] = i < priv->lanes ?
> > +			vep->bus.mipi_csi2.data_lanes[i] : i;
> > +
> > +		/* Check for valid lane number */
> > +		if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) {
> > +			dev_err(priv->dev, "data-lanes must be in 1-4 range\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > +{
> > +	struct device_node *ep;
> > +	struct v4l2_fwnode_endpoint v4l2_ep;
> > +	int ret;
> > +
> > +	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > +	if (!ep) {
> > +		dev_err(priv->dev, "Not connected to subdevice\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> > +	if (ret) {
> > +		dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> > +		of_node_put(ep);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = rcar_csi2_parse_v4l2(priv, &v4l2_ep);
> > +	if (ret) {
> > +		of_node_put(ep);
> > +		return ret;
> > +	}
> > +
> > +	priv->asd.match.fwnode =
> > +		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > +	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +
> > +	of_node_put(ep);
> > +
> > +	priv->notifier.subdevs = devm_kzalloc(priv->dev,
> > +					      sizeof(*priv->notifier.subdevs),
> > +					      GFP_KERNEL);
> > +	if (priv->notifier.subdevs == NULL)
>
> Nit:
> you can use ! for NULL comparison. I think checkpatch --strict
> complains for this.
>
> > +		return -ENOMEM;
> > +
> > +	priv->notifier.num_subdevs = 1;
> > +	priv->notifier.subdevs[0] = &priv->asd;
> > +	priv->notifier.ops = &rcar_csi2_notify_ops;
> > +
> > +	dev_dbg(priv->dev, "Found '%pOF'\n",
> > +		to_of_node(priv->asd.match.fwnode));
> > +
> > +	return v4l2_async_subdev_notifier_register(&priv->subdev,
> > +						   &priv->notifier);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Platform Device Driver
> > + */
> > +
> > +static const struct media_entity_operations rcar_csi2_entity_ops = {
> > +	.link_validate = v4l2_subdev_link_validate,
> > +};
> > +
> > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv,
> > +				     struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +	int irq;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795 = {
> > +	.hsfreqrange = hsfreqrange_h3_v3h_m3n,
> > +	.clear_ulps = true,
> > +	.init_phtw = true,
> > +	.csi0clkfreqrange = 0x20,
> > +};
> > +
> > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = {
> > +	.hsfreqrange = hsfreqrange_m3w_h3es1,
> > +};
> > +
> > +static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = {
> > +	.hsfreqrange = hsfreqrange_m3w_h3es1,
> > +};
> > +
> > +static const struct of_device_id rcar_csi2_of_table[] = {
> > +	{
> > +		.compatible = "renesas,r8a7795-csi2",
> > +		.data = &rcar_csi2_info_r8a7795,
> > +	},
> > +	{
> > +		.compatible = "renesas,r8a7796-csi2",
> > +		.data = &rcar_csi2_info_r8a7796,
> > +	},
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, rcar_csi2_of_table);
> > +
> > +static const struct soc_device_attribute r8a7795es1[] = {
> > +	{
> > +		.soc_id = "r8a7795", .revision = "ES1.*",
> > +		.data = &rcar_csi2_info_r8a7795es1,
> > +	},
> > +	{ /* sentinel */}
> > +};
> > +
> > +static int rcar_csi2_probe(struct platform_device *pdev)
> > +{
> > +	const struct soc_device_attribute *attr;
> > +	struct rcar_csi2 *priv;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->info = of_device_get_match_data(&pdev->dev);
> > +
> > +	/* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> > +	attr = soc_device_match(r8a7795es1);
> > +	if (attr)
> > +		priv->info = attr->data;
> > +
> > +	priv->dev = &pdev->dev;
> > +
> > +	mutex_init(&priv->lock);
> > +	priv->stream_count = 0;
> > +
> > +	ret = rcar_csi2_probe_resources(priv, pdev);
> > +	if (ret) {
> > +		dev_err(priv->dev, "Failed to get resources\n");
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	ret = rcar_csi2_parse_dt(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	priv->subdev.owner = THIS_MODULE;
> > +	priv->subdev.dev = &pdev->dev;
> > +	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
> > +	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
> > +	snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> > +	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > +	priv->subdev.entity.ops = &rcar_csi2_entity_ops;
> > +
> > +	priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> > +	for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> > +		priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +	ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD,
> > +				     priv->pads);
> > +	if (ret)
> > +		goto error;
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	ret = v4l2_async_register_subdev(&priv->subdev);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> > +
> > +	return 0;
> > +
> > +error:
> > +	v4l2_async_notifier_unregister(&priv->notifier);
> > +	v4l2_async_notifier_cleanup(&priv->notifier);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rcar_csi2_remove(struct platform_device *pdev)
> > +{
> > +	struct rcar_csi2 *priv = platform_get_drvdata(pdev);
> > +
> > +	v4l2_async_notifier_unregister(&priv->notifier);
> > +	v4l2_async_notifier_cleanup(&priv->notifier);
> > +	v4l2_async_unregister_subdev(&priv->subdev);
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver __refdata rcar_csi2_pdrv = {
> > +	.remove	= rcar_csi2_remove,
> > +	.probe	= rcar_csi2_probe,
> > +	.driver	= {
> > +		.name	= "rcar-csi2",
> > +		.of_match_table	= rcar_csi2_of_table,
> > +	},
> > +};
> > +
> > +module_platform_driver(rcar_csi2_pdrv);
> > +
> > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>");
> > +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver");
> > +MODULE_LICENSE("GPL");
>
> "GPL v2" ?
>
> No serious issues though. So when fixed/clarified feel free to append my
> Reviewed-by tag, if relevant at all.
>
> Thanks
>    j
>
> > --
> > 2.16.1
> >



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]