Hi Laurent
On Fri, Mar 11, 2022 at 07:09:25PM +0200, Laurent Pinchart wrote:
> 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 ?
>
I sent two patches in reply to this series for you to collect on v2 if
desired.
Please add
Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
to all patches in v2.
Thanks
j
> > > 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
