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

[Hans de Goede]

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.

I will rebase and submit a new version with a comment added.
Do you want me to split this patch into separate codec and machine
driver patches while at it?

> It'll work but it doesn't seem to match expectations for how firmware
> data is provided which is what's bugging me here.

The problem here is that the data is not coming from the firmware,
which is a reality we have to deal with leading to some sort of
compromise.

> It at least needs a
> "here be dragons" style comment in the code.

Ack.

> Ideally we'd have a general way to massage the ACPI data at init time
> and could have board files that are instantiated from DMI to do that
> (like are needed even more for SFI) but that's a long way off.

Something like DT overlays for ACPI? With the overlays being triggered
by a DMI match? Interesting concept, but given how much discussion there
still is surrounding DT overlays I don't see this happening anytime soon.

Regards,

Hans