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 > > > > > > +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. again, thank you for reviewing, martin
