Messages in this thread | | | Subject | Re: [PATCH 2/2] mfd: madera: Add support for requesting the supply clocks | From | Richard Fitzgerald <> | Date | Mon, 12 Aug 2019 17:28:17 +0100 |
| |
On 12/08/19 17:09, Charles Keepax wrote: > On Mon, Aug 12, 2019 at 11:38:53AM +0100, Lee Jones wrote: >> On Tue, 06 Aug 2019, Charles Keepax wrote: >> >>> Add the ability to get the clock for each clock input pin of the chip >>> and enable MCLK2 since that is expected to be a permanently enabled >>> 32kHz clock. >>> >>> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> >>> --- >>> int madera_dev_init(struct madera *madera) >>> { >>> + static const char * const mclk_name[] = { "mclk1", "mclk2", "mclk3" }; >>> struct device *dev = madera->dev; >>> unsigned int hwid; >>> int (*patch_fn)(struct madera *) = NULL; >>> @@ -450,6 +451,17 @@ int madera_dev_init(struct madera *madera) >>> sizeof(madera->pdata)); >>> } >>> >>> + BUILD_BUG_ON(ARRAY_SIZE(madera->mclk) != ARRAY_SIZE(mclk_name)); >> >> Not sure how this could happen. Surely we don't need it. >> > > mclk_name is defined locally in this function and the mclk array in > include/linux/mfd/madera/core.h. This is to guard against one of > them being updated but not the other. It is by no means essential > but it feels like a good trade off given there is really limited > downside. >
I'd like to keep it if we can. Nicer to pick up a mistake at build time than using runtime checking or falling off the end of an undersized array. We use the same technique in the ASoC code.
>>> + for (i = 0; i < ARRAY_SIZE(madera->mclk); i++) { >>> + madera->mclk[i] = devm_clk_get_optional(madera->dev, >>> + mclk_name[i]); >>> + if (IS_ERR(madera->mclk[i])) { >>> + dev_warn(madera->dev, "Failed to get %s: %ld\n", >>> + mclk_name[i], PTR_ERR(madera->mclk[i])); >> >> Do we even want to warn on the non-acquisition of an optional clock? >> >> Especially with a message that looks like something actually failed. >> > > devm_clk_get_optional will return NULL if the clock was not > specified, so this is silent in that case. A warning in the case > something actually went wrong seems reasonable even if the clock > is optional as the user tried to do something and it didn't > behave as they intended. > >>> + madera->mclk[i] = NULL; >>> + } >>> + } >>> + >>> ret = madera_get_reset_gpio(madera); >>> if (ret) >>> return ret; >>> @@ -660,13 +672,19 @@ int madera_dev_init(struct madera *madera) >>> } >>> >>> /* Init 32k clock sourced from MCLK2 */ >>> + ret = clk_prepare_enable(madera->mclk[MADERA_MCLK2]); >>> + if (ret != 0) { >>> + dev_err(madera->dev, "Failed to enable 32k clock: %d\n", ret); >>> + goto err_reset; >>> + } >> >> What happened to this being optional? >> > > The device needs the clock but specifying it through DT is > optional (the clock framework functions are no-ops and return > success if the clock pointer is NULL). Normally the 32kHz > clock is always on, and more importantly no existing users of > the driver will be specifying one. > > We could remove the optional status for MCLK2, but it could break > existing users who don't yet specify the clock until they update > their DT and it will complicate the code as the other clocks are > definitely optional, so MCLK2 will need special handling. > >>> ret = regmap_update_bits(madera->regmap, >>> MADERA_CLOCK_32K_1, >>> MADERA_CLK_32K_ENA_MASK | MADERA_CLK_32K_SRC_MASK, >>> MADERA_CLK_32K_ENA | MADERA_32KZ_MCLK2); >>> if (ret) { >>> dev_err(madera->dev, "Failed to init 32k clock: %d\n", ret); >>> - goto err_reset; >>> + goto err_clock; >>> } >>> >>> pm_runtime_set_active(madera->dev); >>> @@ -687,6 +705,8 @@ int madera_dev_init(struct madera *madera) >>> >>> err_pm_runtime: >>> pm_runtime_disable(madera->dev); >>> +err_clock: >>> + clk_disable_unprepare(madera->mclk[MADERA_MCLK2]); >> >> Where are the other clocks consumed? >> > > Other clocks will be consumed by the ASoC part of the driver for > clocking the audio functionality and running the FLLs. I haven't > sent those patches yet, but was planning on doing so once this > was merged. > > Thanks, > Charles >
| |