Re: [PATCH] ASoC: rt5645: add fallback case for jack detection support

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

Hi,

On 03-01-18 19:42, Pierre-Louis Bossart wrote:
> Commit 78f5605c0329 ("ASoC: rt5645: cleanup DMI matching code") did a
> lot of useful cleanups, but the logic is a bit flawed: if there is no
> DMI match on an ACPI machine, the code reads non-existent properties.

Reading non existent properties is not a problem, notice that the check
only checks if one of the properties is present, the others may still
be unset, which is fine, then the code reading the properties will leave
everything 0 / false.
> Re-add previous code that checked for the existence of those
> properties and a fallback mode to enable jd-mode3 if there is no
> pdata, device property or quirk. This will help avoid having to add
> new DMI quirks when only the jd-mode is needed for headset playback.

Note I do think that defaulting to jd_mode = 3 is a good idea,
I just believe that the commit message wrongly give the impression
that reading non-existent props is a problem.

> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Bard Liao <bardliao@realtek.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   sound/soc/codecs/rt5645.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> index ebc4f5970f24..476020445f65 100644
> --- a/sound/soc/codecs/rt5645.c
> +++ b/sound/soc/codecs/rt5645.c
> @@ -3687,6 +3687,17 @@ static const struct dmi_system_id dmi_platform_data[] = {
>   	{ }
>   };
>   
> +static bool rt5645_check_dp(struct device *dev)
> +{
> +	if (device_property_present(dev, "realtek,in2-differential") ||
> +	    device_property_present(dev, "realtek,dmic1-data-pin") ||
> +	    device_property_present(dev, "realtek,dmic2-data-pin") ||
> +	    device_property_present(dev, "realtek,jd-mode"))
> +		return true;
> +
> +	return false;
> +}
> +
>   static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device *dev)
>   {
>   	rt5645->pdata.in2_diff = device_property_read_bool(dev,
> @@ -3727,8 +3738,10 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
>   
>   	if (pdata)
>   		rt5645->pdata = *pdata;
> -	else
> +	else if (rt5645_check_dp(&i2c->dev))
>   		rt5645_parse_dt(rt5645, &i2c->dev);
> +	else
> +		rt5645->pdata = jd_mode3_platform_data;

How about:

		rt5645->pdata.jd_mode = 3;

instead and remove jd_mode3_platform_data and the 2 dmi table entries
pointing to it ?

Regards,

Hans



>   
>   	if (quirk != -1) {
>   		rt5645->pdata.in2_diff = QUIRK_IN2_DIFF(quirk);
>