Re: [PATCH] clk: vc5: Add suspend/resume support

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

Hi Marek,

On Wednesday, 5 December 2018 14:29:22 EET Marek Vasut wrote:
> On 12/05/2018 06:21 AM, Laurent Pinchart wrote:
> > On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote:
> >> On 12/04/2018 09:52 PM, Stephen Boyd wrote:
> >>> Quoting Marek Vasut (2018-12-04 10:27:21)
> >>> 
> >>>> diff --git a/drivers/clk/clk-versaclock5.c
> >>>> b/drivers/clk/clk-versaclock5.c
> >>>> index decffb3826ec..ac90fb36af1a 100644
> >>>> --- a/drivers/clk/clk-versaclock5.c
> >>>> +++ b/drivers/clk/clk-versaclock5.c
> >>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
> >>>>         return 0;
> >>>>  }
> >>>> 
> >>>> +#ifdef CONFIG_PM_SLEEP
> >>>> +static int vc5_suspend(struct device *dev)
> >>> 
> >>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
> >>> 
> >>>> +{
> >>>> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = regcache_sync(vc5->regmap);
> >>>> +       if (ret != 0) {
> >>>> +               dev_err(dev, "Failed to save register map: %d\n", ret);
> >>>> +               return ret;
> >>> 
> >>> Do we need to block suspend if we can't save the register map away? Or
> >>> can we just throw up our hands and not restore on resume?
> >> 
> >> Some hardware will fail on resume, so I'd say -- yes ?
> > 
> > But why do you need to sync on suspend in the first place ? What could
> > cause the map to be dirty at this stage, and require syncing before
> > suspend, that couldn't work with the sync be delayed to resume time ?
> 
> Possibly a configuration coming from eg. bootloader time , or some other
> configuration not done by Linux.

I still don't get it. As far as I know, regcache_sync() will write the content 
of the regmap to the hardware, not the other way around. You call it at resume 
time, so the hardware should be programmed properly, regardless of what the 
boot loader or the firmware does when resuming.

Could you please explain why this is needed at suspend time ?

-- 
Regards,

Laurent Pinchart