Message-ID: <c9e7799c4b02210c5be29d1c18c4eabd2fe0194b.camel@puri.sm>
Subject: Re: [PATCH] media: i2c: dw9714: use pm_runtime_force_suspend/resume
 for system suspend
From:   Martin Kepplinger <martin.kepplinger@puri.sm>
To:     Sakari Ailus <sakari.ailus@linux.intel.com>
Cc:     mchehab@kernel.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel@puri.sm,
        linux-pm@vger.kernel.org
Date:   Thu, 18 Nov 2021 17:33:04 +0100
In-Reply-To: <YZPqEu4W+JnY6LMY@paasikivi.fi.intel.com>
References: <20211109125505.2682553-1-martin.kepplinger@puri.sm>
         <YZPqEu4W+JnY6LMY@paasikivi.fi.intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.3-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Am Dienstag, dem 16.11.2021 um 19:27 +0200 schrieb Sakari Ailus:
> Hi Martin,
> 
> On Tue, Nov 09, 2021 at 01:55:05PM +0100, Martin Kepplinger wrote:
> > Using the same suspend function for runtime and system suspend
> > doesn't
> > work in this case (when controlling regulators and clocks).
> > suspend() and
> > resume() are clearly meant to stay balanced.
> > 
> > Use the pm_runtime_force_* helpers for system suspend and fix error
> > like the
> > following when transitioning to system suspend:
> > 
> > [  559.685921] dw9714 3-000c: I2C write fail
> > [  559.685941] dw9714 3-000c: dw9714_vcm_suspend I2C failure: -5
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> >  drivers/media/i2c/dw9714.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/dw9714.c
> > b/drivers/media/i2c/dw9714.c
> > index fcbebb3558b5..f6ddd7c4a1ff 100644
> > --- a/drivers/media/i2c/dw9714.c
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -267,7 +267,8 @@ static const struct of_device_id
> > dw9714_of_table[] = {
> >  MODULE_DEVICE_TABLE(of, dw9714_of_table);
> >  
> >  static const struct dev_pm_ops dw9714_pm_ops = {
> > -       SET_SYSTEM_SLEEP_PM_OPS(dw9714_vcm_suspend,
> > dw9714_vcm_resume)
> > +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +                               pm_runtime_force_resume)
> >         SET_RUNTIME_PM_OPS(dw9714_vcm_suspend, dw9714_vcm_resume,
> > NULL)
> >  };
> 
> The purpose of the vcm suspend / resume callbacks is to gently move
> the
> lens to the resting position without hitting the stopper.
> 
> The problem currently appears to be that during system suspend, the
> VCM may
> well be powered off and the driver isn't checking for that. How about
> adding that check?
> 

thanks for having a look. Actually, I forgot to add support for a power
supply regulator, so this patch (description) is wrong and I'll send a
different one that adds optional vcc regulator support and splits up
system/runtime suspend functions to handle the regulator correctly.

thank you!

                              martin