From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Hans Verkuil <hverkuil@xs4all.nl>, linux-media@vger.kernel.org, Sakari Ailus <sakari.ailus@linux.intel.com>, linux-renesas-soc@vger.kernel.org, tomoharu.fukawa.eb@renesas.com, Kieran Bingham <kieran.bingham@ideasonboard.com>, Geert Uytterhoeven <geert@linux-m68k.org> Subject: Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver Date: Sun, 15 Apr 2018 23:26:06 +0200 [thread overview] Message-ID: <20180415212606.GG20093@bigcity.dyn.berto.se> (raw) In-Reply-To: <5149348.Rp98f1K5qJ@avalon> 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@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, Niklas Söderlund
WARNING: multiple messages have this Message-ID (diff)
From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Hans Verkuil <hverkuil@xs4all.nl>, linux-media@vger.kernel.org, Sakari Ailus <sakari.ailus@linux.intel.com>, linux-renesas-soc@vger.kernel.org, tomoharu.fukawa.eb@renesas.com, Kieran Bingham <kieran.bingham@ideasonboard.com>, Geert Uytterhoeven <geert@linux-m68k.org> Subject: Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver Date: Sun, 15 Apr 2018 23:26:06 +0200 [thread overview] Message-ID: <20180415212606.GG20093@bigcity.dyn.berto.se> (raw) In-Reply-To: <5149348.Rp98f1K5qJ@avalon> 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@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, Niklas S�derlund
next prev parent reply other threads:[~2018-04-15 21:26 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-12 23:01 [PATCH v13 0/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 Niklas Söderlund 2018-02-12 23:01 ` [PATCH v13 1/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver documentation Niklas Söderlund 2018-04-04 14:49 ` Laurent Pinchart 2018-02-12 23:01 ` [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver Niklas Söderlund 2018-03-13 22:23 ` Kieran Bingham 2018-04-04 15:25 ` Laurent Pinchart 2018-04-15 18:50 ` Niklas Söderlund 2018-04-15 18:50 ` Niklas Söderlund 2018-04-15 18:48 ` Niklas Söderlund 2018-04-15 18:48 ` Niklas Söderlund 2018-03-29 11:30 ` Maxime Ripard 2018-04-04 15:26 ` Laurent Pinchart 2018-04-05 7:33 ` Geert Uytterhoeven 2018-04-05 8:26 ` Laurent Pinchart 2018-04-15 20:35 ` Niklas Söderlund 2018-04-15 20:35 ` Niklas Söderlund 2018-04-04 20:13 ` Sakari Ailus 2018-04-04 20:13 ` Sakari Ailus 2018-04-15 20:47 ` Niklas Söderlund 2018-04-15 20:47 ` Niklas Söderlund 2018-04-16 9:30 ` Sakari Ailus 2018-04-16 9:30 ` Sakari Ailus 2018-04-15 20:33 ` Niklas Söderlund 2018-04-15 20:33 ` Niklas Söderlund 2018-04-04 15:15 ` Laurent Pinchart 2018-04-15 21:26 ` Niklas Söderlund [this message] 2018-04-15 21:26 ` Niklas Söderlund 2018-04-23 23:23 ` Laurent Pinchart 2018-04-05 9:10 ` jacopo mondi 2018-04-05 13:06 ` jacopo mondi 2018-04-15 23:16 ` Niklas Söderlund 2018-04-15 23:16 ` Niklas Söderlund 2018-04-16 12:46 ` jacopo mondi 2018-04-17 0:05 ` Niklas Söderlund 2018-04-17 0:05 ` Niklas Söderlund
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180415212606.GG20093@bigcity.dyn.berto.se \ --to=niklas.soderlund@ragnatech.se \ --cc=geert@linux-m68k.org \ --cc=hverkuil@xs4all.nl \ --cc=kieran.bingham@ideasonboard.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-media@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=sakari.ailus@linux.intel.com \ --cc=tomoharu.fukawa.eb@renesas.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.