Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

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

Hi,

On 01-03-18 23:35, Hans de Goede wrote:
> Hi,
> 
> On 01-03-18 20:30, Mark Brown wrote:
>> On Sun, Feb 25, 2018 at 11:46:47AM +0100, Hans de Goede wrote:
>>> Configure the jack-detect source through a device-property which can be
>>> set by code outside of the codec driver. Rather then putting platform
>>> specific DMI quirks inside the generic codec driver.
>>
>>> And move the jack-detect-source quirk for the KIANO SlimNote 14.2, which
>>> was present inside the codec driver to the machine driver, where we can
>>> bundle it together with the other quirks already present for this laptop.
>>
>> Multiple things in a patch again :/
> 
> Yes because the property replaces the quirk, I can split the changes between
> the codec and machine driver into 2 patches but then the intermediate state
> will be that the jack-detection no longer works.
> 
>> The property itself is fine but...
>>
>>> @@ -360,6 +377,13 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
>>>               dev_err(card->dev, "unable to set MCLK rate\n");
>>>       }
>>> +    props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source",
>>> +                BYT_RT5651_JDSRC(byt_rt5651_quirk));
>>> +
>>> +    ret = device_add_properties(codec->dev, props);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>       ret = snd_soc_card_jack_new(runtime->card, "Headset",
>>
>> I'm having a hard time geting comfortable with this; it's all a bit
>> fragile feeling to me - someone deciding to centralise the property
>> parsing (eg, if they are using a platform where platform data makes
>> sense or want to cross-validate with some other property on probe) could
>> easily break things and there's not even a comment in the CODEC driver.
> 
> It is not _that_ fragile, but I agree that it would be good to add
> a comment about not moving the property-parsing to the codec driver.

So one other solution which comes to mind here is to move the
snd_soc_card_jack_new() call into the codec driver's
rt5651_apply_properties() function (conditional on a jack src being
set in the properties).

This would make the machine driver code look like this:

	props[cnt++] = PROPERTY_ENTRY_...
	props[cnt++] = PROPERTY_ENTRY_...
	props[cnt++] = PROPERTY_ENTRY_...
	props[cnt++] = PROPERTY_ENTRY_...

	ret = device_add_properties(codec->dev, props);
	if (ret)
		return ret;

	ret = rt5651_apply_properties(codec);
	if (ret)
		return ret;

Which makes the ordering really clear without needing any
comments.

This will also make the jack-detect device-properties work with
devicetree platforms without any changes to their platform code,
which currently is not the case since the non Intel platform-code
does not call snd_soc_component_set_jack().

Thinking more about this I believe that doing the
snd_soc_card_jack_new() call inside the codec driver based on
device-properties is a better solution then doing it in the
machine driver.

Please let me know if you agree then I will rework the remaining
patches accordingly.

Regards,

Hans