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

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

Hi Marek,

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 ?

> The rest is fixed.
> 
> >> +       }
> >> +       regcache_cache_only(vc5->regmap, true);
> >> +       regcache_mark_dirty(vc5->regmap);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int vc5_resume(struct device *dev)
> >> +{
> >> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >> +       int ret;
> >> +
> >> +       regcache_cache_only(vc5->regmap, false);
> >> +       ret = regcache_sync(vc5->regmap);
> >> +       if (ret != 0) {
> >> +               dev_err(dev, "Failed to restore register map: %d\n",
> >> ret);
> >> +               return ret;
> >> +       }
> > 
> > Simplify to
> > 
> > 	if (ret)
> > 	
> > 		dev_err()
> > 	
> > 	retun ret;
> > 	
> >> +
> >> +       return 0;
> >> +}
> >> +#endif
> >> +
> >> 
> >>  static const struct vc5_chip_info idt_5p49v5923_info = {
> >>  
> >>         .model = IDT_VC5_5P49V5923,
> >>         .clk_fod_cnt = 2,

-- 
Regards,

Laurent Pinchart