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

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

Hi Niklas,

On Monday, 16 April 2018 00:26:06 EEST Niklas Söderlund wrote:
> Hi Laurent,
> 
> Thanks for your feedback.
> 
> I have addressed all your comment's but one for the next version. Please
> indicate if you are fine with this and I can still keep your review tag
> 
> :-)
> 
> On 2018-04-04 18:15:16 +0300, Laurent Pinchart wrote:
> 
> [snip]
> 
> > > +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);
> > 
> > You could store the format pointer iin the rcar_csi2 structure to avoid
> > looking it up here.
> 
> I could do that, but then I would duplicate the storage of the code
> between the two cached values priv->mf and priv-><cached pointer to
> rcar_csi2>. I could find that acceptable but if you don't strongly
> disagree I would prefer to keep the current way of looking it up here
> :-)

I'm OK with that, even if I think that duplicating the code would be a very 
small price to pay for not having to look up the format information structure.

> [snip]
> 
> > > +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;
> > 
> > You don't seem to use the IRQ. Is this meant to catch invalid DT that
> > don't
> > specify an IRQ, to make sure we'll always have one available when we'll
> > need to later ?
> 
> Yes, as you deducted this is currently only to catch invalid DT. In the
> DT documentation I list it as a mandatory property. I think there might
> be potential use-case to at some point add interrupt support of for the
> some of the error interrupts which can be enabled, specially now when we
> seen similar patches for VIN floating around.
> 
> > > +
> > > +	return 0;
> > > +
> > > +error:
> > > +	v4l2_async_notifier_unregister(&priv->notifier);
> > > +	v4l2_async_notifier_cleanup(&priv->notifier);
> > > +
> > > +	return ret;
> > > +}
> > 
> > [snip]
> > 
> > With these small issues fixed and Kieran's and Maxime's comments addressed
> > as you see fit,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks, I will hold of adding it until you indicate if you are OK with
> the one comment I'm not fully agreeing with you on.


-- 
Regards,

Laurent Pinchart