Re: [RFC PATCH 04/16] ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 4/15/20 4:55 AM, Andy Shevchenko wrote:
> On Tue, Apr 14, 2020 at 01:02:12PM -0500, Pierre-Louis Bossart wrote:
>> On 4/14/20 12:20 PM, Andy Shevchenko wrote:
>>> On Thu, Apr 09, 2020 at 02:58:29PM -0500, Pierre-Louis Bossart wrote:
> 
> ...
> 
>>>> +	ctx->sclk = devm_clk_get(rtd->card->dev, "sclk");
>>>
>>> Is this in the bindings?
>>
>> Not for now. the 'sclk' part is only used by me myself and I in an ACPI
>> context. I can add this description if desired.
> 
> Unfortunately you need to add this to the bindings, because it's a part of it
> and somebody may use it outside of your scope.

ok


>>>> +	if (IS_ERR(ctx->sclk)) {
>>>
>>>> +		dev_info(dev, "Could not get SCLK, will operate in SOC master mode\n");
>>>
>>> Sounds like devm_clk_get_optional().
>>
>> I am not sure about the semantic here. This driver selects the one which
>> implements this clock, so if we get a -ENOENT return it's a very bad sign.
>> Not sure what suppressing the error and converting to NULL would do?
> 
> Same as per GPIO.
> Can it work without this clock? How did it work before your change?
> 
> When you add any hard dependency always ask yourself above questions.

The clock is not required in codec slave mode, it's provided by the SOC.

The clock is required when the codec is configured as master. Without 
these GPIO selection there will be no audio. So yes we could move this 
to devm_clk_get_optional() and change the test to IS_ERR_OR_NULL.



>>>> +		goto skip_dacpro;
>>>> +	}
>