[PATCH 1/2] mfd: madera: Update DT binding document to support clock supplies

[PATCH 1/2] mfd: madera: Update DT binding document to support clock supplies

[Charles Keepax]

Add the 3 input clock sources for the chip into the device tree binding
document.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 Documentation/devicetree/bindings/mfd/madera.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/madera.txt b/Documentation/devicetree/bindings/mfd/madera.txt
index cad0f28005027..47e2b8bc60519 100644
--- a/Documentation/devicetree/bindings/mfd/madera.txt
+++ b/Documentation/devicetree/bindings/mfd/madera.txt
@@ -67,6 +67,14 @@ Optional properties:
     As defined in bindings/gpio.txt.
     Although optional, it is strongly recommended to use a hardware reset
 
+  - clocks: Should reference the clocks supplied on MCLK1, MCLK2 and MCLK3
+  - clock-names: May contain up to three strings:
+      "mclk1" for the clock supplied on MCLK1, recommended to be a high
+      quality audio reference clock
+      "mclk2" for the clock supplied on MCLK2, required to be an always on
+      32k clock
+      "mclk3" for the clock supplied on MCLK3
+
   - MICBIASx : Initial data for the MICBIAS regulators, as covered in
     Documentation/devicetree/bindings/regulator/regulator.txt.
     One for each MICBIAS generator (MICBIAS1, MICBIAS2, ...)
-- 
2.11.0

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

[Charles Keepax]

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>
---
 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));
+	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]));
+			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;
+	}
+
 	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]);
 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];
 
-- 
2.11.0

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

[Lee Jones]

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

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

[Charles Keepax]

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.

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

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

[Richard Fitzgerald]

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
> 

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

[Lee Jones]

On Mon, 12 Aug 2019, 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.

It's fine in general I guess.  How likely would it be for anyone to
update either of the definitions?  Can there be more/less clocks on a
supported platform?

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

If something actually went wrong, then doesn't then become and error
and should be reported (returned)?

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

I'd prefer the code to reflect the actual situation.  If the clock is
not optional it doesn't sound correct to specify it as such.  Maybe as
an intermediary step we attempt to obtain it, but ignore missing
clocks (with a message and comment) if it is not yet specified.  We
can look to change the behaviour once users have had the chance to
update their DTs.

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

Okay.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Charles Keepax]

On Tue, Aug 13, 2019 at 08:18:14AM +0100, Lee Jones wrote:
> On Mon, 12 Aug 2019, 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.
> 
> It's fine in general I guess.  How likely would it be for anyone to
> update either of the definitions?  Can there be more/less clocks on a
> supported platform?
> 

It's not super likely but if the hardware guys make a new spin
out chip with an extra clock pin which is possible. But my
problem here is there really is no down side to this check, we
have two things that need to be in sync and if the compiler can
warn me if they are not in sync that is clearly a win.

> > > > +	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.
> 
> If something actually went wrong, then doesn't then become and error
> and should be reported (returned)?
> 

Yeah I guess its a judgement call but there is not really any
reason we need to proceed in the case of an error. I will update
to fail probe here.

> > > > +			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.
> 
> I'd prefer the code to reflect the actual situation.  If the clock is
> not optional it doesn't sound correct to specify it as such.  Maybe as
> an intermediary step we attempt to obtain it, but ignore missing
> clocks (with a message and comment) if it is not yet specified.  We
> can look to change the behaviour once users have had the chance to
> update their DTs.
> 

Ok I will add a print for a missing MCLK2.

Thanks,
Charles

Re: [PATCH 1/2] mfd: madera: Update DT binding document to support clock supplies

[Lee Jones]

On Tue, 06 Aug 2019, Charles Keepax wrote:

> Add the 3 input clock sources for the chip into the device tree binding
> document.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  Documentation/devicetree/bindings/mfd/madera.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog