| Message ID | 20220119112024.11339-2-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: media: imx7-mipi-csis: Two small fixes | expand |
Hi Jacopo, Thank you for the patch. On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote: > The mipi_csis_dump_regs() function reads and printout the interface > registers for debugging purposes. > > Trying to access the registers without proper powering up the interface > causes the chip to hang. > > Fix that by increasing the pm runtime usage count which, if necessary, > resumes the interface. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c > index 2b73fa55c938..cb54bb7491d9 100644 > --- a/drivers/staging/media/imx/imx7-mipi-csis.c > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c > @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state) > > dev_info(state->dev, "--- REGISTERS ---\n"); > > + pm_runtime_resume_and_get(state->dev); Should this have an error check ? With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > for (i = 0; i < ARRAY_SIZE(registers); i++) { > cfg = mipi_csis_read(state, registers[i].offset); > dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg); > } > > + pm_runtime_put(state->dev); > + > return 0; > } >
Hi Jacopo, On Wed, Jan 19, 2022 at 04:24:51PM +0200, Laurent Pinchart wrote: > On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote: > > The mipi_csis_dump_regs() function reads and printout the interface > > registers for debugging purposes. > > > > Trying to access the registers without proper powering up the interface > > causes the chip to hang. > > > > Fix that by increasing the pm runtime usage count which, if necessary, > > resumes the interface. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c > > index 2b73fa55c938..cb54bb7491d9 100644 > > --- a/drivers/staging/media/imx/imx7-mipi-csis.c > > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c > > @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state) > > > > dev_info(state->dev, "--- REGISTERS ---\n"); > > > > + pm_runtime_resume_and_get(state->dev); > > Should this have an error check ? With that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I just noticed that the call to mipi_csis_dump_regs() in mipi_csis_log_status() is conditioned by state->state & ST_POWERED. An alternative would thus be to add the same condition to mipi_csis_dump_regs_show() (or to move it to mipi_csis_dump_regs()), as dumping the register when then hardware is turned off is quite pointeless. Up to you. > > + > > for (i = 0; i < ARRAY_SIZE(registers); i++) { > > cfg = mipi_csis_read(state, registers[i].offset); > > dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg); > > } > > > > + pm_runtime_put(state->dev); > > + > > return 0; > > } > >
Hi Laurent, On Tue, Jan 25, 2022 at 05:18:26AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Wed, Jan 19, 2022 at 04:24:51PM +0200, Laurent Pinchart wrote: > > On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote: > > > The mipi_csis_dump_regs() function reads and printout the interface > > > registers for debugging purposes. > > > > > > Trying to access the registers without proper powering up the interface > > > causes the chip to hang. > > > > > > Fix that by increasing the pm runtime usage count which, if necessary, > > > resumes the interface. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c > > > index 2b73fa55c938..cb54bb7491d9 100644 > > > --- a/drivers/staging/media/imx/imx7-mipi-csis.c > > > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c > > > @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state) > > > > > > dev_info(state->dev, "--- REGISTERS ---\n"); > > > > > > + pm_runtime_resume_and_get(state->dev); > > > > Should this have an error check ? With that, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I just noticed that the call to mipi_csis_dump_regs() in > mipi_csis_log_status() is conditioned by state->state & ST_POWERED. An > alternative would thus be to add the same condition to > mipi_csis_dump_regs_show() (or to move it to mipi_csis_dump_regs()), as > dumping the register when then hardware is turned off is quite > pointeless. Up to you. > Tbh, I would drop this custom sysfs attribute completely. It should serve the purpose to easily dump the reg value, but it is either accessed at the right time (ie during the streaming session) otherwise all you get is POR default values (or a hang, without this patch) > > > + > > > for (i = 0; i < ARRAY_SIZE(registers); i++) { > > > cfg = mipi_csis_read(state, registers[i].offset); > > > dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg); > > > } > > > > > > + pm_runtime_put(state->dev); > > > + > > > return 0; > > > } > > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Jan 25, 2022 at 11:22:57AM +0100, Jacopo Mondi wrote: > On Tue, Jan 25, 2022 at 05:18:26AM +0200, Laurent Pinchart wrote: > > On Wed, Jan 19, 2022 at 04:24:51PM +0200, Laurent Pinchart wrote: > > > On Wed, Jan 19, 2022 at 12:20:23PM +0100, Jacopo Mondi wrote: > > > > The mipi_csis_dump_regs() function reads and printout the interface > > > > registers for debugging purposes. > > > > > > > > Trying to access the registers without proper powering up the interface > > > > causes the chip to hang. > > > > > > > > Fix that by increasing the pm runtime usage count which, if necessary, > > > > resumes the interface. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c > > > > index 2b73fa55c938..cb54bb7491d9 100644 > > > > --- a/drivers/staging/media/imx/imx7-mipi-csis.c > > > > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c > > > > @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state) > > > > > > > > dev_info(state->dev, "--- REGISTERS ---\n"); > > > > > > > > + pm_runtime_resume_and_get(state->dev); > > > > > > Should this have an error check ? With that, > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > I just noticed that the call to mipi_csis_dump_regs() in > > mipi_csis_log_status() is conditioned by state->state & ST_POWERED. An > > alternative would thus be to add the same condition to > > mipi_csis_dump_regs_show() (or to move it to mipi_csis_dump_regs()), as > > dumping the register when then hardware is turned off is quite > > pointeless. Up to you. > > Tbh, I would drop this custom sysfs attribute completely. > It should serve the purpose to easily dump the reg value, but it is > either accessed at the right time (ie during the streaming session) > otherwise all you get is POR default values (or a hang, without this > patch) The debugfs interface has served me before to diagnose problems, as it allows checking how the register values change during streaming, in particular the DPHY status. I'd like to keep it if possible. > > > > + > > > > for (i = 0; i < ARRAY_SIZE(registers); i++) { > > > > cfg = mipi_csis_read(state, registers[i].offset); > > > > dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg); > > > > } > > > > > > > > + pm_runtime_put(state->dev); > > > > + > > > > return 0; > > > > } > > > >
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c index 2b73fa55c938..cb54bb7491d9 100644 --- a/drivers/staging/media/imx/imx7-mipi-csis.c +++ b/drivers/staging/media/imx/imx7-mipi-csis.c @@ -780,11 +780,15 @@ static int mipi_csis_dump_regs(struct csi_state *state) dev_info(state->dev, "--- REGISTERS ---\n"); + pm_runtime_resume_and_get(state->dev); + for (i = 0; i < ARRAY_SIZE(registers); i++) { cfg = mipi_csis_read(state, registers[i].offset); dev_info(state->dev, "%14s: 0x%08x\n", registers[i].name, cfg); } + pm_runtime_put(state->dev); + return 0; }
The mipi_csis_dump_regs() function reads and printout the interface registers for debugging purposes. Trying to access the registers without proper powering up the interface causes the chip to hang. Fix that by increasing the pm runtime usage count which, if necessary, resumes the interface. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/staging/media/imx/imx7-mipi-csis.c | 4 ++++ 1 file changed, 4 insertions(+)