Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10/6/21 6:37 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/6/21 11:23 AM, Andy Shevchenko wrote:
>> On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
>>> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
>>>> The devm_clk_get_optional() helper returns NULL when devm_clk_get()
>>>> returns -ENOENT. This makes things slightly cleaner. The added benefit
>>>> is mostly cosmetic.
>>
>> ...
>>
>>>>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
>>>> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
>>>> -			ret = clk_prepare_enable(priv->mclk);
>>>> -			if (ret < 0) {
>>>> -				dev_err(card->dev,
>>>> -					"could not configure MCLK state");
>>>> -				return ret;
>>>> -			}
>>>> +		ret = clk_prepare_enable(priv->mclk);
>>>> +		if (ret < 0) {
>>>> +			dev_err(card->dev, "could not configure MCLK state");
>>>> +			return ret;
>>>>  		}
>>>
>>> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
>>> see below it was designed as a fall-back mode. We don't want to return
>>> an error when we know the clock is not present/desired.
>>
>> Why should we do a unneeded test? When we switch to the optional, there
>> will be no error from these CCF APIs. Besides that it drops indentation
>> level and makes code neat.
> 
> By looking at this code only one cannot really visualize that it's a
> no-op. I personally prefer to see explicit intent rather than have to
> dig hundreds of lines below what this clock is optional.
> 
> I am also not even sure that in real products this clock is actually
> optional, the default is to make use of it:
> 
> #define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \
> 
> and the only platform without this clock is "Minnowboard Max B3" -
> probably not used by anyone. I fried mine a long time ago.
> 
> We'd need to Hans to comment on this since he's really the only one
> maintaining this code.

So as Mark wrote in his later reply:

"AIUI with the clock API the idiomatic thing is that any optionality is
handled at the point where the clock is acquired - if the clock is
optional you end up with NULL which in the clock API is a dummy clock
and ignored.  The rest of the code then doesn't need to worry about any
of this stuff and the handling can only be in one place."

Combined with there pretty much always actually being an mclk I believe
that this patch from Andy results in a nice cleanup so I'm in favor with
this. And the other cleanups also look sensible to me

I would like to run a small smoke-test with both the series to make
sure nothing regresses (should be fine but better safe then sorry).

Andy I believe that there is going to be a v2 to address a couple
of nitpicks, right ?

Note for testing I would prefer a full v2 series, even if some
patches don't change. And I assume the same applies to Mark for
applying this.

Sending partial series with only changed patches on the v2
send turns things into a puzzle, which is not ideal IMHO.

Regards,

Hans





> 
>> ...
>>
>>> same here, why was the quirk removed?
>>
>> Same answer.
>>
>> ...
>>
>>> that part in the probe looks fine, but the changes above are controversial.
>>
>> I didn't get. How controversial? Why? The whole point of _optional is to get
>> rid of unneeded checks (since they are _anyway_ be called).
> 
> It's inconsistent since you kept the following part but no longer use it:
> 
> +		/*
> +		 * Fall back to bit clock usage when clock is not
> +		 * available likely due to missing dependencies.
> +		 */
> +		if (!priv->mclk)
> +			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
> 
> 
>