Hi Krzysztof,
On 2018-08-31 16:59, Krzysztof Kozlowski wrote:
> On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
>> Some registers of clock controller have to be set to certain values before
>> entering system suspend state. Till now drivers did that on their own,
>> but it will be easier to handle it by generic code and let drivers simply
>> to provide the list of registers and their state.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> ---
>> drivers/clk/samsung/clk.c | 23 +++++++++++++----------
>> drivers/clk/samsung/clk.h | 18 ++++++++++++++++--
>> 2 files changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> index 8634884aa11c..ab467a7f973a 100644
>> --- a/drivers/clk/samsung/clk.c
>> +++ b/drivers/clk/samsung/clk.c
>> @@ -290,9 +290,12 @@ static int samsung_clk_suspend(void)
>> {
>> struct samsung_clock_reg_cache *reg_cache;
>>
>> - list_for_each_entry(reg_cache, &clock_reg_cache_list, node)
>> + list_for_each_entry(reg_cache, &clock_reg_cache_list, node) {
>> samsung_clk_save(reg_cache->reg_base, reg_cache->rdump,
>> reg_cache->rd_num);
>> + samsung_clk_restore(reg_cache->reg_base, reg_cache->rsuspend,
>> + reg_cache->rsuspend_num);
>> + }
>> return 0;
>> }
>>
>> @@ -310,9 +313,11 @@ static struct syscore_ops samsung_clk_syscore_ops = {
>> .resume = samsung_clk_resume,
>> };
>>
>> -void samsung_clk_sleep_init(void __iomem *reg_base,
>> +void samsung_clk_sleep_init2(void __iomem *reg_base,
> Like Chanwoo, I also do not like the init2. Quite frankly, I do not
> see what problem you want to solve it by adding "2" suffix - not
> touching Exynos5433 code?
I didn't want to change Exynos5433 and clock drivers cleaned in patches
2-7, as I see no point adding "NULL, 0" parameters there.
> In such case more meaningful prefix would be
> better. But I think this should be avoided especially that in patch
> 9/10 you use both of them.
Okay, so it is just a matter of name. What about
samsung_clk_extended_sleep_init() ?
I don't want to add another temporary structure just to pass some
arguments to that function...
> Separate topic - It is getting confusing. The existing Exynos5433 code
> has support for suspend_regs (used in its device-level runtime PM) and
> here you are extending generic code on syscore level. Maybe this could
> be unified somehow?
Well, you can consider this patch as a first step of unification.
Implementing device based suspend/resume for OF_CLK_DECLARE() based
drivers is quite complicated now (mainly because that initialization
is done much before platform bus and dt-based devices are registered),
but I hope one day this can be also unified.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
