Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ideasonboard.com>
> > ---
> >  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