2017-09-22 12:31 GMT+02:00 Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>: > On 15/09/2017 at 16:04, Romain Izard wrote: >> From: Romain Izard <romain.izard@xxxxxxxxxxxxxxxxx> >> >> 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.izard.pro@xxxxxxxxx> > > Romain, > > Some nitpicking and one comment. But on the overall patch, here is my: > Acked-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html