| Message ID | 20180304143610.21125-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f0c2a330d99ef81519dc809d8b6a7dafe39b0933 |
| Headers | show |
On Sun, Mar 04, 2018 at 03:35:51PM +0100, Hans de Goede wrote: > +/* > + * Note these MUST match the values from the DT binding: > + * Documentation/devicetree/bindings/sound/rt5651.txt > + */ > enum rt5651_jd_src { > RT5651_JD_NULL, > RT5651_JD1_1, It's normal to put values like this in a header in include/dt-bindings so it's easy to get things right in actual DT bindings (and possibly ACPI tables as well if the machinery for that does the right thing) - can you send a followup doing that please? > -static const struct dmi_system_id rt5651_quirk_table[] = { > - { > - .callback = rt5651_quirk_cb, > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "KIANO"), > - DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"), > - }, > - .driver_data = (unsigned long *) RT5651_JD1_1, > - }, > - {} > -}; > - I'm still not thrilled about this bit but oh well :(
Hi, On 07-03-18 13:34, Mark Brown wrote: > On Sun, Mar 04, 2018 at 03:35:51PM +0100, Hans de Goede wrote: > >> +/* >> + * Note these MUST match the values from the DT binding: >> + * Documentation/devicetree/bindings/sound/rt5651.txt >> + */ >> enum rt5651_jd_src { >> RT5651_JD_NULL, >> RT5651_JD1_1, > > It's normal to put values like this in a header in include/dt-bindings > so it's easy to get things right in actual DT bindings (and possibly > ACPI tables as well if the machinery for that does the right thing) - > can you send a followup doing that please? Good point, yes I will send a follow-up patch. >> -static const struct dmi_system_id rt5651_quirk_table[] = { >> - { >> - .callback = rt5651_quirk_cb, >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "KIANO"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"), >> - }, >> - .driver_data = (unsigned long *) RT5651_JD1_1, >> - }, >> - {} >> -}; >> - > > I'm still not thrilled about this bit but oh well :( Hmm, I'm planning on doing something very similar for the rt5640 code. That is I'm working on jack-detect for the rt5640 too, and that will need some platform data like which pin to use and OVCD settings, I plan to once again use device-properties for this to have a platform agnostic solution at the codec driver level. And then in the Intel machine driver attach the device properties based on dmi quirks (like bytcr_rt5651 the machine driver already has DMI based quirks for a bunch of boards, which I will re-use). Although I agree that ideally we would be able to get all this directly from the firmware, to me fixing the x86 specific problem of that info missing using quirks in the Intel machine driver seems like the right place to fix this, so that we are not "poluting" generic code with these Intel specific workarounds. I know that second best would be some generic mechanism in the kernel to attach extra device-properties based on the board we're booting on, but alas currently that does not exist. One advantage of using device-properties for this as this series is doing, is that if such a mechanism gets added moving the info over should be easy. TL;DR: I know this is not the prettiest thing ever, but as you said: "oh well". So as said I plan to do something very similar for the rt5640 code, if you've any objections against that I would like to hear those objections sooner rather then later to avoid throwing away work. Regards, Hans
On Wed, Mar 07, 2018 at 02:49:05PM +0100, Hans de Goede wrote: > Although I agree that ideally we would be able to get all this > directly from the firmware, to me fixing the x86 specific problem > of that info missing using quirks in the Intel machine driver seems > like the right place to fix this, so that we are not "poluting" > generic code with these Intel specific workarounds. Yeah, my feeling here is that this is less pollution than you're seeing it as - it's too much like endorsement of the Windows one driver per machine way of doing things and splits the knowledge about how the device gets used about more. Purely a taste question.
diff --git a/include/sound/rt5651.h b/include/sound/rt5651.h index 1612462bf5ad..725b36c329d0 100644 --- a/include/sound/rt5651.h +++ b/include/sound/rt5651.h @@ -11,6 +11,10 @@ #ifndef __LINUX_SND_RT5651_H #define __LINUX_SND_RT5651_H +/* + * Note these MUST match the values from the DT binding: + * Documentation/devicetree/bindings/sound/rt5651.txt + */ enum rt5651_jd_src { RT5651_JD_NULL, RT5651_JD1_1, diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index 7bff5cda61fd..f80b3da4667f 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -19,7 +19,6 @@ #include <linux/platform_device.h> #include <linux/spi/spi.h> #include <linux/acpi.h> -#include <linux/dmi.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -32,8 +31,6 @@ #include "rl6231.h" #include "rt5651.h" -#define RT5651_JD_MAP(quirk) ((quirk) & GENMASK(7, 0)) - #define RT5651_DEVICE_ID_VALUE 0x6281 #define RT5651_PR_RANGE_BASE (0xff + 1) @@ -41,8 +38,6 @@ #define RT5651_PR_BASE (RT5651_PR_RANGE_BASE + (0 * RT5651_PR_SPACING)) -static unsigned long rt5651_quirk; - static const struct regmap_range_cfg rt5651_ranges[] = { { .name = "PR", .range_min = RT5651_PR_BASE, .range_max = RT5651_PR_BASE + 0xb4, @@ -1675,6 +1670,9 @@ static int rt5651_set_jack(struct snd_soc_component *component, */ static void rt5651_apply_properties(struct snd_soc_component *component) { + struct rt5651_priv *rt5651 = snd_soc_component_get_drvdata(component); + u32 val; + if (device_property_read_bool(component->dev, "realtek,in2-differential")) snd_soc_component_update_bits(component, RT5651_IN1_IN2, RT5651_IN_DF2, RT5651_IN_DF2); @@ -1682,6 +1680,10 @@ static void rt5651_apply_properties(struct snd_soc_component *component) if (device_property_read_bool(component->dev, "realtek,dmic-en")) snd_soc_component_update_bits(component, RT5651_GPIO_CTRL1, RT5651_GP2_PIN_MASK, RT5651_GP2_PIN_DMIC1_SCL); + + if (device_property_read_u32(component->dev, + "realtek,jack-detect-source", &val) == 0) + rt5651->jd_src = val; } static int rt5651_probe(struct snd_soc_component *component) @@ -1832,24 +1834,6 @@ static const struct i2c_device_id rt5651_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, rt5651_i2c_id); -static int rt5651_quirk_cb(const struct dmi_system_id *id) -{ - rt5651_quirk = (unsigned long) id->driver_data; - return 1; -} - -static const struct dmi_system_id rt5651_quirk_table[] = { - { - .callback = rt5651_quirk_cb, - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "KIANO"), - DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"), - }, - .driver_data = (unsigned long *) RT5651_JD1_1, - }, - {} -}; - static int rt5651_jack_detect(struct snd_soc_component *component, int jack_insert) { int jack_type; @@ -1917,9 +1901,6 @@ static int rt5651_i2c_probe(struct i2c_client *i2c, i2c_set_clientdata(i2c, rt5651); - dmi_check_system(rt5651_quirk_table); - rt5651->jd_src = RT5651_JD_MAP(rt5651_quirk); - rt5651->regmap = devm_regmap_init_i2c(i2c, &rt5651_regmap); if (IS_ERR(rt5651->regmap)) { ret = PTR_ERR(rt5651->regmap);
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v3 -Split out the machine-driver and dt-bindings changes into separate patches, keeping just the codec driver changes in this patch Changes in v2: -New patch in v2 of this patch-set --- include/sound/rt5651.h | 4 ++++ sound/soc/codecs/rt5651.c | 33 +++++++-------------------------- 2 files changed, 11 insertions(+), 26 deletions(-)