| Message ID | 20180103184241.11690-1-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5954c4a1455c3bc42acb2c286744eae1aaa00918 |
| Headers | show |
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); >
On Wed, Jan 03, 2018 at 11:20:38PM +0100, Hans de Goede wrote: > > + 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 ? I'm not sure it's best practice to remove the entries that have been explicitly added; on the other hand if the default is jd_mode = 3 then it's not like we'll be adding any more.
On 1/4/18 5:59 AM, Mark Brown wrote: > On Wed, Jan 03, 2018 at 11:20:38PM +0100, Hans de Goede wrote: > >>> + 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 ? > > I'm not sure it's best practice to remove the entries that have been > explicitly added; on the other hand if the default is jd_mode = 3 then > it's not like we'll be adding any more. Yes, I didn't want to remove existing quirks since I can't test for non-regressions on those devices. We will most likely have to add new quirks for the capture part, there is a large variability in how the analog inputs are handled. The jd_mode3 default will help mostly identify if the device is functional out-of-the-box on the playback side (the speaker part relies on a separate amplifier so it's hit or miss). I will resubmit a patch with a clearer commit message, indeed there is no harm in reading non-existent properties. What I meant is that their absence can be used as a signal to use the default. Thanks for the quick feedback. -Pierre
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; if (quirk != -1) { rt5645->pdata.in2_diff = QUIRK_IN2_DIFF(quirk);
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. 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. 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(-)