Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934215AbdIYI0C (ORCPT ); Mon, 25 Sep 2017 04:26:02 -0400 Received: from mail-qk0-f169.google.com ([209.85.220.169]:49141 "EHLO mail-qk0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934198AbdIYIZ4 (ORCPT ); Mon, 25 Sep 2017 04:25:56 -0400 X-Google-Smtp-Source: AOwi7QAZ353KisEOLiiZWWAjRLtDomceB0e6zs/k3T35vmLl+CPfjZx7Mswq3236QjVU6WpBkvnx7IU1EpxjaixGchQ= MIME-Version: 1.0 In-Reply-To: References: <20170915140411.31716-1-romain.izard.pro@gmail.com> <20170915140411.31716-4-romain.izard.pro@gmail.com> From: Romain Izard Date: Mon, 25 Sep 2017 10:25:35 +0200 X-Google-Sender-Auth: fyBRz7UOQm_YHyxzQiPq1I9ZrB0 Message-ID: Subject: Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable clocks To: Nicolas Ferre Cc: Alexandre Belloni , Boris Brezillon , Michael Turquette , Stephen Boyd , Ludovic Desroches , Wenyou Yang , Josh Wu , David Woodhouse , Brian Norris , Marek Vasut , Cyrille Pitchen , Thierry Reding , Richard Genoud , Greg Kroah-Hartman , Alan Stern , linux-clk@vger.kernel.org, LKML , linux-mtd , linux-pwm@vger.kernel.org, linux-serial@vger.kernel.org, linux-usb@vger.kernel.org, linux-arm-kernel Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5021 Lines: 167 2017-09-22 12:31 GMT+02:00 Nicolas Ferre : > On 15/09/2017 at 16:04, Romain Izard wrote: >> From: Romain Izard >> >> When an AT91 programmable clock is declared in the device tree, register >> it into the Power Management Controller driver. On entering suspend mode, >> the driver saves and restores the Programmable Clock registers to support >> the backup mode for these clocks. >> >> Signed-off-by: Romain Izard > > Romain, > > Some nitpicking and one comment. But on the overall patch, here is my: > Acked-by: Nicolas Ferre > > See below: > >> --- >> Changes in v2: >> * register PCKs on clock startup >> >> drivers/clk/at91/clk-programmable.c | 2 ++ >> drivers/clk/at91/pmc.c | 27 +++++++++++++++++++++++++++ >> drivers/clk/at91/pmc.h | 2 ++ >> 3 files changed, 31 insertions(+) >> >> diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c >> index 85a449cf61e3..0e6aab1252fc 100644 >> --- a/drivers/clk/at91/clk-programmable.c >> +++ b/drivers/clk/at91/clk-programmable.c >> @@ -204,6 +204,8 @@ at91_clk_register_programmable(struct regmap *regmap, >> if (ret) { >> kfree(prog); >> hw = ERR_PTR(ret); > > Nit: "else" not needed. > This is a shared idiom in all the atmel clock drivers, so I prefer to keep it this way. >> + } else { >> + pmc_register_pck(id); >> } >> >> return hw; >> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c >> index 07dc2861ad3f..3910b7537152 100644 >> --- a/drivers/clk/at91/pmc.c >> +++ b/drivers/clk/at91/pmc.c >> @@ -22,6 +22,7 @@ >> #include "pmc.h" >> >> #define PMC_MAX_IDS 128 >> +#define PMC_MAX_PCKS 8 >> >> int of_at91_get_clk_range(struct device_node *np, const char *propname, >> struct clk_range *range) >> @@ -50,6 +51,7 @@ EXPORT_SYMBOL_GPL(of_at91_get_clk_range); >> static struct regmap *pmcreg; >> >> static u8 registered_ids[PMC_MAX_IDS]; >> +static u8 registered_pcks[PMC_MAX_PCKS]; >> >> static struct >> { >> @@ -66,8 +68,10 @@ static struct >> u32 pcr[PMC_MAX_IDS]; >> u32 audio_pll0; >> u32 audio_pll1; >> + u32 pckr[PMC_MAX_PCKS]; >> } pmc_cache; >> >> +/* Clock ID 0 is invalid */ > > (read: so we can use the 0 value as an indicator that this place in the > table hasn't been filled, so unused) > >> void pmc_register_id(u8 id) >> { >> int i; >> @@ -82,6 +86,21 @@ void pmc_register_id(u8 id) >> } >> } >> >> +/* Programmable Clock 0 is valid */ > > I understand the rationale behind these ^^ two comments, but I would > like that it's more explicit. Saying that you will store the pck id as > (id + 1) and that you would have to invert this operation while using > the stored id. > Maybe add a comment about this transformation to the struct definition > as well... > I will improve the comments for the next revision. > >> +void pmc_register_pck(u8 pck) >> +{ >> + int i; >> + >> + for (i = 0; i < PMC_MAX_PCKS; i++) { >> + if (registered_pcks[i] == 0) { >> + registered_pcks[i] = pck + 1; >> + break; >> + } >> + if (registered_pcks[i] == (pck + 1)) >> + break; >> + } >> +} >> + >> static int pmc_suspend(void) >> { >> int i; >> @@ -103,6 +122,10 @@ static int pmc_suspend(void) >> regmap_read(pmcreg, AT91_PMC_PCR, >> &pmc_cache.pcr[registered_ids[i]]); >> } >> + for (i = 0; registered_pcks[i]; i++) { >> + u8 num = registered_pcks[i] - 1; > > Nit: declaration are better made at the beginning of the function. This > lead to a checkpatch warning: > "WARNING: Missing a blank line after declarations" > I'll fix this as well. >> + regmap_read(pmcreg, AT91_PMC_PCKR(num), &pmc_cache.pckr[num]); >> + } >> >> return 0; >> } >> @@ -143,6 +166,10 @@ static void pmc_resume(void) >> pmc_cache.pcr[registered_ids[i]] | >> AT91_PMC_PCR_CMD); >> } >> + for (i = 0; registered_pcks[i]; i++) { >> + u8 num = registered_pcks[i] - 1; > > Ditto > >> + regmap_write(pmcreg, AT91_PMC_PCKR(num), pmc_cache.pckr[num]); >> + } >> >> if (pmc_cache.uckr & AT91_PMC_UPLLEN) >> mask |= AT91_PMC_LOCKU; >> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h >> index 858e8ef7e8db..d22b1fa9ecdc 100644 >> --- a/drivers/clk/at91/pmc.h >> +++ b/drivers/clk/at91/pmc.h >> @@ -31,8 +31,10 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname, >> >> #ifdef CONFIG_PM >> void pmc_register_id(u8 id); >> +void pmc_register_pck(u8 pck); >> #else >> static inline void pmc_register_id(u8 id) {} >> +static inline void pmc_register_pck(u8 pck) {} >> #endif >> >> #endif /* __PMC_H_ */ >> -- Romain Izard