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

[Richard Fitzgerald <rf@opensource.cirrus.com>]

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
>