Hi Jacopo,
On Fri, Mar 11, 2022 at 06:00:45PM +0100, Jacopo Mondi wrote:
> On Fri, Mar 11, 2022 at 03:55:35PM +0200, Laurent Pinchart wrote:
> > The runtime PM resume handler is guaranteed to be called on a suspended
> > device, and the suspend handler on a resumed device. The implementation
> > can thus be simplified.
> >
> > While at it, rename the mipi_csis_device state field to powered, as the
> > now state contains a single flag only.
>
> Can we instead rely on pm_runtime_get_if_in_use() instead of manual
> tracking the power state ?
>
> After all, the powered flag is only used in:
>
> static int mipi_csis_log_status(struct v4l2_subdev *sd)
> {
> struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
>
> mutex_lock(&csis->lock);
> mipi_csis_log_counters(csis, true);
> if (csis->debug.enable && csis->powered)
> mipi_csis_dump_regs(csis);
> mutex_unlock(&csis->lock);
>
> return 0;
> }
>
> which could be simplified as
>
> static int mipi_csis_log_status(struct v4l2_subdev *sd)
> {
> struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
>
> mipi_csis_log_counters(csis, true);
>
> if (!csis->debug.enable)
> return 0;
>
> mutex_lock(&csis->lock);
>
> if (!pm_runtime_get_if_in_use())
> goto unlock;
>
> mipi_csis_dump_regs(csis);
>
> pm_runtime_put();
>
> unlock:
> mutex_unlock(&csis->lock);
>
> return 0;
> }
That's a good idea. Do you mind if I do so on a patch on top of this
one, to not mix two separate changes ?
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/imx/imx-mipi-csis.c | 38 ++++++++++------------
> > 1 file changed, 17 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > index d656b8bfcc33..f6ff8d50843c 100644
> > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > @@ -248,10 +248,6 @@
> > #define MIPI_CSI2_DATA_TYPE_RAW14 0x2d
> > #define MIPI_CSI2_DATA_TYPE_USER(x) (0x30 + (x))
> >
> > -enum {
> > - ST_POWERED = 1,
> > -};
> > -
> > struct mipi_csis_event {
> > bool debug;
> > u32 mask;
> > @@ -331,10 +327,10 @@ struct mipi_csis_device {
> > u32 hs_settle;
> > u32 clk_settle;
> >
> > - struct mutex lock; /* Protect csis_fmt, format_mbus and state */
> > + struct mutex lock; /* Protect csis_fmt, format_mbus and powered */
> > const struct csis_pix_format *csis_fmt;
> > struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
> > - u32 state;
> > + bool powered;
> >
> > spinlock_t slock; /* Protect events */
> > struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> > @@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> >
> > mutex_lock(&csis->lock);
> > mipi_csis_log_counters(csis, true);
> > - if (csis->debug.enable && (csis->state & ST_POWERED))
> > + if (csis->debug.enable && csis->powered)
> > mipi_csis_dump_regs(csis);
> > mutex_unlock(&csis->lock);
> >
> > @@ -1354,13 +1350,14 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> > int ret = 0;
> >
> > mutex_lock(&csis->lock);
> > - if (csis->state & ST_POWERED) {
> > - ret = mipi_csis_phy_disable(csis);
> > - if (ret)
> > - goto unlock;
> > - mipi_csis_clk_disable(csis);
> > - csis->state &= ~ST_POWERED;
> > - }
> > +
> > + ret = mipi_csis_phy_disable(csis);
> > + if (ret)
> > + goto unlock;
> > +
> > + mipi_csis_clk_disable(csis);
> > +
> > + csis->powered = false;
> >
> > unlock:
> > mutex_unlock(&csis->lock);
> > @@ -1376,14 +1373,13 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> >
> > mutex_lock(&csis->lock);
> >
> > - if (!(csis->state & ST_POWERED)) {
> > - ret = mipi_csis_phy_enable(csis);
> > - if (ret)
> > - goto unlock;
> > + ret = mipi_csis_phy_enable(csis);
> > + if (ret)
> > + goto unlock;
> >
> > - csis->state |= ST_POWERED;
> > - mipi_csis_clk_enable(csis);
> > - }
> > + mipi_csis_clk_enable(csis);
> > +
> > + csis->powered = true;
> >
> > unlock:
> > mutex_unlock(&csis->lock);
--
Regards,
Laurent Pinchart
