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@xxxxxxx> > > > --- > > > 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
