On Thu, Sep 06, 2018 at 03:25:29PM +0200, Marek Szyprowski wrote:
> 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...
Yes, I am fine with this approach.
> > 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.
Indeed... so I will keep my fingers crossed for that work :)
Best regards,
Krzysztof
