From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se> To: Maxime Ripard <maxime.ripard@bootlin.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 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 22:33:08 +0200 [thread overview] Message-ID: <20180415203308.GD20093@bigcity.dyn.berto.se> (raw) In-Reply-To: <20180329113039.4v5whquyrtgf5yaa@flea> Hi Maxime, Thanks for your feedback. On 2018-03-29 13:30:39 +0200, Maxime Ripard wrote: > Hi Niklas, > > On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > > + 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; > > + default: > > + return -EINVAL; > > + } > > I guess you could have a simpler construct here using this: > > phycnt = PHYCNT_ENABLECLK; > > switch (priv->lanes) { > case 4: > phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; > case 2: > phycnt |= PHYCNT_ENABLE_1; > case 1: > phycnt |= PHYCNT_ENABLE_0; > break; > > default: > return -EINVAL; > } > > But that's really up to you. Thanks for the suggestion and sparking of the discussion, I think I will go with Geert at.al approach of: phycnt = PHYCNT_ENABLECLK; phycnt |= (1 << priv->lanes) - 1; > > > +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); > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > device uninitialised at probe, and pm_runtime_get_sync will not > initialise it either if CONFIG_PM is not enabled. I guess you could > call your runtime_resume function unconditionally, and mark the device > as active in runtime_pm using pm_runtime_set_active. Yes CONFIG_PM is selected by ARCH_RENESAS. Thanks for letting me know about this in any case I was not aware of this behavior in the case CONFIG_PM where not enabled. > > Looks good otherwise, once fixed (and if relevant): > Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com> Thanks! -- Regards, Niklas Söderlund
WARNING: multiple messages have this Message-ID (diff)
From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se> To: Maxime Ripard <maxime.ripard@bootlin.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 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 22:33:08 +0200 [thread overview] Message-ID: <20180415203308.GD20093@bigcity.dyn.berto.se> (raw) In-Reply-To: <20180329113039.4v5whquyrtgf5yaa@flea> Hi Maxime, Thanks for your feedback. On 2018-03-29 13:30:39 +0200, Maxime Ripard wrote: > Hi Niklas, > > On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas S�derlund wrote: > > + 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; > > + default: > > + return -EINVAL; > > + } > > I guess you could have a simpler construct here using this: > > phycnt = PHYCNT_ENABLECLK; > > switch (priv->lanes) { > case 4: > phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; > case 2: > phycnt |= PHYCNT_ENABLE_1; > case 1: > phycnt |= PHYCNT_ENABLE_0; > break; > > default: > return -EINVAL; > } > > But that's really up to you. Thanks for the suggestion and sparking of the discussion, I think I will go with Geert at.al approach of: phycnt = PHYCNT_ENABLECLK; phycnt |= (1 << priv->lanes) - 1; > > > +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); > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > device uninitialised at probe, and pm_runtime_get_sync will not > initialise it either if CONFIG_PM is not enabled. I guess you could > call your runtime_resume function unconditionally, and mark the device > as active in runtime_pm using pm_runtime_set_active. Yes CONFIG_PM is selected by ARCH_RENESAS. Thanks for letting me know about this in any case I was not aware of this behavior in the case CONFIG_PM where not enabled. > > Looks good otherwise, once fixed (and if relevant): > Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com> Thanks! -- Regards, Niklas S�derlund
next prev parent reply other threads:[~2018-04-15 20:33 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 [this message] 2018-04-15 20:33 ` Niklas Söderlund 2018-04-04 15:15 ` Laurent Pinchart 2018-04-15 21:26 ` Niklas Söderlund 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=20180415203308.GD20093@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=maxime.ripard@bootlin.com \ --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.