From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Martin Kepplinger <martin.kepplinger@puri.sm> Cc: m.felsch@pengutronix.de, robh@kernel.org, devicetree@vger.kernel.org, festevam@gmail.com, kernel@pengutronix.de, kernel@puri.sm, krzk@kernel.org, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, mchehab@kernel.org, phone-devel@vger.kernel.org, shawnguo@kernel.org, slongerbeam@gmail.com Subject: Re: [PATCH v5 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx phy and controller Date: Wed, 14 Jul 2021 11:30:59 +0300 [thread overview] Message-ID: <YO6gwz7OfUAgoAMa@pendragon.ideasonboard.com> (raw) In-Reply-To: <78072a196076789745b5481e26cfb45862e935ed.camel@puri.sm> Hi Martin, On Wed, Jul 14, 2021 at 08:41:58AM +0200, Martin Kepplinger wrote: > Am Dienstag, dem 13.07.2021 um 00:52 +0300 schrieb Laurent Pinchart: > > Hi Martin, > > > > Thank you for the patch. > > thank you for reviewing! > > > On Fri, Jun 18, 2021 at 11:57:52AM +0200, Martin Kepplinger wrote: > > > Add a driver to support the i.MX8MQ MIPI CSI receiver. The hardware > > > side > > > is based on > > > > > > https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c?h=imx_5.4.70_2.3.0 > > > > > > It's built as part of VIDEO_IMX7_CSI because that's documented to > > > support > > > i.MX8M platforms. This driver adds i.MX8MQ support where currently > > > only the > > > i.MX8MM platform has been supported. > > > > Overall this is really nice work. I have quite a few review comments, > > but hopefully nothing that should be difficult to handle. > > I think so too but right now I have one question: > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > > --- > > > drivers/staging/media/imx/Makefile | 1 + > > > drivers/staging/media/imx/imx8mq-mipi-csi2.c | 963 +++++++++++++++++++ > > > 2 files changed, 964 insertions(+) > > > create mode 100644 drivers/staging/media/imx/imx8mq-mipi-csi2.c [snip] > > > +static int imx8mq_mipi_csi_pm_suspend(struct device *dev, bool runtime) > > > +{ > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > > + struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > + int ret = 0; > > > + > > > + mutex_lock(&state->lock); > > > + > > > + if (state->state & ST_POWERED) { > > > + imx8mq_mipi_csi_stop_stream(state); > > > + imx8mq_mipi_csi_clk_disable(state); > > > + state->state &= ~ST_POWERED; > > > + if (!runtime) > > > + state->state |= ST_SUSPENDED; > > > + } > > > + > > > + mutex_unlock(&state->lock); > > > + > > > + ret = icc_set_bw(state->icc_path, 0, 0); > > > + if (ret) > > > + dev_err(dev, "icc_set_bw failed with %d\n", ret); > > > + > > > + return ret ? -EAGAIN : 0; > > > +} > > > + > > > +static int imx8mq_mipi_csi_pm_resume(struct device *dev, bool runtime) > > > +{ > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > > + struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > + int ret = 0; > > > + > > > + ret = icc_set_bw(state->icc_path, 0, state->icc_path_bw); > > > + if (ret) { > > > + dev_err(dev, "icc_set_bw failed with %d\n", ret); > > > + return ret; > > > + } > > > + > > > + mutex_lock(&state->lock); > > > + > > > + if (!runtime && !(state->state & ST_SUSPENDED)) > > > + goto unlock; > > > + > > > + if (!(state->state & ST_POWERED)) { > > > + state->state |= ST_POWERED; > > > + ret = imx8mq_mipi_csi_clk_enable(state); > > > > Enabling the clocks in the PM resume handler is correct, but they should > > also be disabled in the PM suspend handler. > > imx8mq_mipi_csi_clk_disable() is called in suspend(). I don't know what > you mean here. I wonder what I was thinking... You can ignore this comment. However, imx8mq_mipi_csi_clk_disable() is also called in the error path of the probe function and in the remove function. I don't think this is right. -- Regards, Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Martin Kepplinger <martin.kepplinger@puri.sm> Cc: m.felsch@pengutronix.de, robh@kernel.org, devicetree@vger.kernel.org, festevam@gmail.com, kernel@pengutronix.de, kernel@puri.sm, krzk@kernel.org, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, mchehab@kernel.org, phone-devel@vger.kernel.org, shawnguo@kernel.org, slongerbeam@gmail.com Subject: Re: [PATCH v5 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx phy and controller Date: Wed, 14 Jul 2021 11:30:59 +0300 [thread overview] Message-ID: <YO6gwz7OfUAgoAMa@pendragon.ideasonboard.com> (raw) In-Reply-To: <78072a196076789745b5481e26cfb45862e935ed.camel@puri.sm> Hi Martin, On Wed, Jul 14, 2021 at 08:41:58AM +0200, Martin Kepplinger wrote: > Am Dienstag, dem 13.07.2021 um 00:52 +0300 schrieb Laurent Pinchart: > > Hi Martin, > > > > Thank you for the patch. > > thank you for reviewing! > > > On Fri, Jun 18, 2021 at 11:57:52AM +0200, Martin Kepplinger wrote: > > > Add a driver to support the i.MX8MQ MIPI CSI receiver. The hardware > > > side > > > is based on > > > > > > https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c?h=imx_5.4.70_2.3.0 > > > > > > It's built as part of VIDEO_IMX7_CSI because that's documented to > > > support > > > i.MX8M platforms. This driver adds i.MX8MQ support where currently > > > only the > > > i.MX8MM platform has been supported. > > > > Overall this is really nice work. I have quite a few review comments, > > but hopefully nothing that should be difficult to handle. > > I think so too but right now I have one question: > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > > --- > > > drivers/staging/media/imx/Makefile | 1 + > > > drivers/staging/media/imx/imx8mq-mipi-csi2.c | 963 +++++++++++++++++++ > > > 2 files changed, 964 insertions(+) > > > create mode 100644 drivers/staging/media/imx/imx8mq-mipi-csi2.c [snip] > > > +static int imx8mq_mipi_csi_pm_suspend(struct device *dev, bool runtime) > > > +{ > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > > + struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > + int ret = 0; > > > + > > > + mutex_lock(&state->lock); > > > + > > > + if (state->state & ST_POWERED) { > > > + imx8mq_mipi_csi_stop_stream(state); > > > + imx8mq_mipi_csi_clk_disable(state); > > > + state->state &= ~ST_POWERED; > > > + if (!runtime) > > > + state->state |= ST_SUSPENDED; > > > + } > > > + > > > + mutex_unlock(&state->lock); > > > + > > > + ret = icc_set_bw(state->icc_path, 0, 0); > > > + if (ret) > > > + dev_err(dev, "icc_set_bw failed with %d\n", ret); > > > + > > > + return ret ? -EAGAIN : 0; > > > +} > > > + > > > +static int imx8mq_mipi_csi_pm_resume(struct device *dev, bool runtime) > > > +{ > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > > + struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > + int ret = 0; > > > + > > > + ret = icc_set_bw(state->icc_path, 0, state->icc_path_bw); > > > + if (ret) { > > > + dev_err(dev, "icc_set_bw failed with %d\n", ret); > > > + return ret; > > > + } > > > + > > > + mutex_lock(&state->lock); > > > + > > > + if (!runtime && !(state->state & ST_SUSPENDED)) > > > + goto unlock; > > > + > > > + if (!(state->state & ST_POWERED)) { > > > + state->state |= ST_POWERED; > > > + ret = imx8mq_mipi_csi_clk_enable(state); > > > > Enabling the clocks in the PM resume handler is correct, but they should > > also be disabled in the PM suspend handler. > > imx8mq_mipi_csi_clk_disable() is called in suspend(). I don't know what > you mean here. I wonder what I was thinking... You can ignore this comment. However, imx8mq_mipi_csi_clk_disable() is also called in the error path of the probe function and in the remove function. I don't think this is right. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-07-14 8:31 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-18 9:57 [PATCH v5 0/3] media: imx: add support for imx8mq MIPI RX Martin Kepplinger 2021-06-18 9:57 ` Martin Kepplinger 2021-06-18 9:57 ` [PATCH v5 1/3] dt-bindings: media: document the nxp,imx8mq-mipi-csi2 receiver phy and controller Martin Kepplinger 2021-06-18 9:57 ` [PATCH v5 1/3] dt-bindings: media: document the nxp, imx8mq-mipi-csi2 " Martin Kepplinger 2021-07-12 19:11 ` [PATCH v5 1/3] dt-bindings: media: document the nxp,imx8mq-mipi-csi2 " Rob Herring 2021-07-12 19:11 ` Rob Herring 2021-06-18 9:57 ` [PATCH v5 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx " Martin Kepplinger 2021-06-18 9:57 ` Martin Kepplinger 2021-07-12 21:52 ` Laurent Pinchart 2021-07-12 21:52 ` Laurent Pinchart 2021-07-14 6:41 ` Martin Kepplinger 2021-07-14 6:41 ` Martin Kepplinger 2021-07-14 6:41 ` Martin Kepplinger 2021-07-14 8:30 ` Laurent Pinchart [this message] 2021-07-14 8:30 ` Laurent Pinchart 2021-06-18 9:57 ` [PATCH v5 3/3] arm64: dts: imx8mq: add mipi csi phy and csi bridge descriptions Martin Kepplinger 2021-06-18 9:57 ` Martin Kepplinger
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=YO6gwz7OfUAgoAMa@pendragon.ideasonboard.com \ --to=laurent.pinchart@ideasonboard.com \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=kernel@pengutronix.de \ --cc=kernel@puri.sm \ --cc=krzk@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-staging@lists.linux.dev \ --cc=m.felsch@pengutronix.de \ --cc=martin.kepplinger@puri.sm \ --cc=mchehab@kernel.org \ --cc=phone-devel@vger.kernel.org \ --cc=robh@kernel.org \ --cc=shawnguo@kernel.org \ --cc=slongerbeam@gmail.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.