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
:-)
[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@xxxxxxxxxxxxxxxx>
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,
Niklas Söderlund
