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

[Marek Vasut <marek.vasut@gmail.com>]

On 12/05/2018 02:54 PM, Laurent Pinchart wrote:
> Hi Marek,

Hi,

> 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 ?

It is not, can be dropped.

-- 
Best regards,
Marek Vasut