Re: [PATCH 2/2] mfd: madera: Add support for requesting the supply clocks

From: Lee Jones
Date: Mon Aug 12 2019 - 06:39:00 EST


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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/mfd/madera-core.c | 24 +++++++++++++++++++++++-
> include/linux/mfd/madera/core.h | 11 +++++++++++
> 2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
> index 29540cbf75934..8d7ab1c7bf9f7 100644
> --- a/drivers/mfd/madera-core.c
> +++ b/drivers/mfd/madera-core.c
> @@ -428,6 +428,7 @@ static void madera_set_micbias_info(struct madera *madera)
>
> 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.

> + 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.

> + 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?

> 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?

> err_reset:
> madera_enable_hard_reset(madera);
> regulator_disable(madera->dcvdd);
> @@ -713,6 +733,8 @@ int madera_dev_exit(struct madera *madera)
> */
> pm_runtime_disable(madera->dev);
>
> + clk_disable_unprepare(madera->mclk[MADERA_MCLK2]);
> +
> regulator_disable(madera->dcvdd);
> regulator_put(madera->dcvdd);
>
> diff --git a/include/linux/mfd/madera/core.h b/include/linux/mfd/madera/core.h
> index 7ffa696cce7ca..2b6c83fe221dc 100644
> --- a/include/linux/mfd/madera/core.h
> +++ b/include/linux/mfd/madera/core.h
> @@ -8,6 +8,7 @@
> #ifndef MADERA_CORE_H
> #define MADERA_CORE_H
>
> +#include <linux/clk.h>
> #include <linux/gpio/consumer.h>
> #include <linux/interrupt.h>
> #include <linux/mfd/madera/pdata.h>
> @@ -29,6 +30,13 @@ enum madera_type {
> CS42L92 = 9,
> };
>
> +enum {
> + MADERA_MCLK1,
> + MADERA_MCLK2,
> + MADERA_MCLK3,
> + MADERA_NUM_MCLK
> +};
> +
> #define MADERA_MAX_CORE_SUPPLIES 2
> #define MADERA_MAX_GPIOS 40
>
> @@ -155,6 +163,7 @@ struct snd_soc_dapm_context;
> * @irq_dev: the irqchip child driver device
> * @irq_data: pointer to irqchip data for the child irqchip driver
> * @irq: host irq number from SPI or I2C configuration
> + * @mclk: pointers to clock supplies
> * @out_clamp: indicates output clamp state for each analogue output
> * @out_shorted: indicates short circuit state for each analogue output
> * @hp_ena: bitflags of enable state for the headphone outputs
> @@ -184,6 +193,8 @@ struct madera {
> struct regmap_irq_chip_data *irq_data;
> int irq;
>
> + struct clk *mclk[MADERA_NUM_MCLK];
> +
> unsigned int num_micbias;
> unsigned int num_childbias[MADERA_MAX_MICBIAS];
>

--
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog