| Message ID | 20180220221511.22861-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 20, 2018 at 11:15:01PM +0100, Hans de Goede wrote: > Instead of duplicating the DMI quirks between the codec and machine driver, > add a rt5651_set_pdata() codec private function which the machine driver > can use to pass in pdata. It's not clear to me that this is a great idea. The goal in general is to keep as much of the CODEC configuration in the CODEC driver as possible so we can move more towards generic machine drivers, this is moving us in the opposite drection quite dramatically. That might get in the way of improvements we could be making when we get the component based changes further on and can avoid having all the DPCM stuff in the Intel drivers. It is really unfortunate that ACPI based systems aren't embracing standards here and using this mess of open coded quirks but it feels like the way to do this is to keep the code as close as possible to well designed firmware interfaces and hope that they come round.
On Tue, Feb 20, 2018 at 10:15 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Instead of duplicating the DMI quirks between the codec and machine driver, > add a rt5651_set_pdata() codec private function which the machine driver > can use to pass in pdata. Commenting here for the whole series (I don't see the cover letter): Tested-by: Carlo Caione <carlo@endlessm.com> (On the quirked KIANO laptop). Thanks!
Hi, On 21-02-18 12:54, Mark Brown wrote: > On Tue, Feb 20, 2018 at 11:15:01PM +0100, Hans de Goede wrote: >> Instead of duplicating the DMI quirks between the codec and machine driver, >> add a rt5651_set_pdata() codec private function which the machine driver >> can use to pass in pdata. > > It's not clear to me that this is a great idea. The goal in general is > to keep as much of the CODEC configuration in the CODEC driver as > possible so we can move more towards generic machine drivers, this is > moving us in the opposite drection quite dramatically. Not really, instead of having Intel/ACPI specific quirks in the codec driver it moves everything over to pdata, so in a way this unifies the device-tree and ACPI paths. Also to me it seems that the Intel specific machine driver is the logical place to put quirks for Intel platforms rather then poluting the platform-agnostic codec driver with this. > That might get > in the way of improvements we could be making when we get the component > based changes further on and can avoid having all the DPCM stuff in the > Intel drivers. > > It is really unfortunate that ACPI based systems aren't embracing > standards here and using this mess of open coded quirks but it feels > like the way to do this is to keep the code as close as possible to well > designed firmware interfaces and hope that they come round. Both devicetree and ACPI code-paths are filling pdata and that is the only thing the codec driver cares about after this patch. so this is using a well designed interface (but a non firmware one). I can understand if you would prefer to use the functions from include/linux/property.h for this, but those simply do not work for ACPI platforms atm. If you look further in this series then quirks for at least 2 more models are added and I expect more to show up in the future, I really do not want to duplicate these over 2 files. One possible solution here would be to add device-properties to the codec i2c-device from the machine-driver using device_add_properties() and make the codec driver consume those. This has probe-ordering issues, but those can be worked around by exporting something like the rt5651_apply_pdata() function from this patch and make the machine driver call that after it has attached the properties. I'm not sure if this extra level of indirection buys us much though, the DT binding maintainers will not accept binding docs for these kind of kernel internal only device-properties until there are actual DT users of them, so for now this would mainly add a bunch of extra code for little gain. Regards, Hans
Hi, On 21-02-18 18:36, Carlo Caione wrote: > On Tue, Feb 20, 2018 at 10:15 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Instead of duplicating the DMI quirks between the codec and machine driver, >> add a rt5651_set_pdata() codec private function which the machine driver >> can use to pass in pdata. > > Commenting here for the whole series (I don't see the cover letter): > > Tested-by: Carlo Caione <carlo@endlessm.com> > > (On the quirked KIANO laptop). Great, thank you for testing, so are the issues with headset vs headphone detection not working fixed after this series? Regards, Hans
Hi Mark, On 21-02-18 20:23, Hans de Goede wrote: > Hi, > > On 21-02-18 12:54, Mark Brown wrote: >> On Tue, Feb 20, 2018 at 11:15:01PM +0100, Hans de Goede wrote: >>> Instead of duplicating the DMI quirks between the codec and machine driver, >>> add a rt5651_set_pdata() codec private function which the machine driver >>> can use to pass in pdata. >> >> It's not clear to me that this is a great idea. The goal in general is >> to keep as much of the CODEC configuration in the CODEC driver as >> possible so we can move more towards generic machine drivers, this is >> moving us in the opposite drection quite dramatically. > > Not really, instead of having Intel/ACPI specific quirks in the codec > driver it moves everything over to pdata, so in a way this unifies the > device-tree and ACPI paths. > > Also to me it seems that the Intel specific machine driver is the > logical place to put quirks for Intel platforms rather then poluting the > platform-agnostic codec driver with this. > >> That might get >> in the way of improvements we could be making when we get the component >> based changes further on and can avoid having all the DPCM stuff in the >> Intel drivers. >> >> It is really unfortunate that ACPI based systems aren't embracing >> standards here and using this mess of open coded quirks but it feels >> like the way to do this is to keep the code as close as possible to well >> designed firmware interfaces and hope that they come round. > > Both devicetree and ACPI code-paths are filling pdata and that is the > only thing the codec driver cares about after this patch. so this is > using a well designed interface (but a non firmware one). I can understand > if you would prefer to use the functions from include/linux/property.h > for this, but those simply do not work for ACPI platforms atm. > > If you look further in this series then quirks for at least 2 more models > are added and I expect more to show up in the future, I really do not want > to duplicate these over 2 files. > > One possible solution here would be to add device-properties to the codec > i2c-device from the machine-driver using device_add_properties() and make > the codec driver consume those. > > This has probe-ordering issues, but those can be worked around by exporting > something like the rt5651_apply_pdata() function from this patch and make > the machine driver call that after it has attached the properties. > > I'm not sure if this extra level of indirection buys us much though, the > DT binding maintainers will not accept binding docs for these kind of > kernel internal only device-properties until there are actual DT users > of them, so for now this would mainly add a bunch of extra code for > little gain. So thinking more about this I think that having the codec driver only check device-properties and completely killing platform_data is the best way forward / best way to clean this up. Combined with either device-tree or the machine driver setting these device properties for now, and in the future the ACPI tables can set these properties directly and we can hopefully won't need to add new quirks to the machine driver. I plan to start reworking this series this afternoon, so any feedback on the device-properties plan would be appreciated... Regards, Hans
On Wed, Feb 21, 2018 at 08:23:01PM +0100, Hans de Goede wrote: > On 21-02-18 12:54, Mark Brown wrote: > > It's not clear to me that this is a great idea. The goal in general is > > to keep as much of the CODEC configuration in the CODEC driver as > > possible so we can move more towards generic machine drivers, this is > > moving us in the opposite drection quite dramatically. > Not really, instead of having Intel/ACPI specific quirks in the codec > driver it moves everything over to pdata, so in a way this unifies the > device-tree and ACPI paths. Bear in mind that ACPI isn't x86 specific any more, and on x86 those Intel drivers really are Intel specific rather than x86 specific - AMD are doing their own completely different thing. Much ACPI, so standards, sadly :( > Also to me it seems that the Intel specific machine driver is the > logical place to put quirks for Intel platforms rather then poluting the > platform-agnostic codec driver with this. > > It is really unfortunate that ACPI based systems aren't embracing > > standards here and using this mess of open coded quirks but it feels > > like the way to do this is to keep the code as close as possible to well > > designed firmware interfaces and hope that they come round. > Both devicetree and ACPI code-paths are filling pdata and that is the > only thing the codec driver cares about after this patch. so this is > using a well designed interface (but a non firmware one). I can understand It's not just platform data, it's also putting the platform data in through a regular driver outside of the usual device model stuff. > if you would prefer to use the functions from include/linux/property.h > for this, but those simply do not work for ACPI platforms atm. Yeah, they work fine with DSD which is the official ACPI way to pass device specific data through. > One possible solution here would be to add device-properties to the codec > i2c-device from the machine-driver using device_add_properties() and make > the codec driver consume those. > This has probe-ordering issues, but those can be worked around by exporting > something like the rt5651_apply_pdata() function from this patch and make > the machine driver call that after it has attached the properties. The ordering is part of the issue, yeah. It increases fragility if we have to wait to parse the platform data until later on in init, but only on some systems. > I'm not sure if this extra level of indirection buys us much though, the > DT binding maintainers will not accept binding docs for these kind of > kernel internal only device-properties until there are actual DT users > of them, so for now this would mainly add a bunch of extra code for > little gain. I'm not aware of any such policy from the DT people, and in any case the individual driver bindings go through subsystems so...
On Thu, Feb 22, 2018 at 12:08:29PM +0100, Hans de Goede wrote: > So thinking more about this I think that having the codec driver only > check device-properties and completely killing platform_data is the best > way forward / best way to clean this up. > Combined with either device-tree or the machine driver setting these device > properties for now, and in the future the ACPI tables can set these properties > directly and we can hopefully won't need to add new quirks to the machine > driver. The device properties thing seems reasonable enough. I'm still not super convinced the machine driver is the right place, but let's see the code. I think ideally the arch code would have a way to put all the device property based quirks for these systems like we IIRC do for some DT fixups.
Hi, On 22-02-18 12:35, Mark Brown wrote: > On Thu, Feb 22, 2018 at 12:08:29PM +0100, Hans de Goede wrote: > >> So thinking more about this I think that having the codec driver only >> check device-properties and completely killing platform_data is the best >> way forward / best way to clean this up. > >> Combined with either device-tree or the machine driver setting these device >> properties for now, and in the future the ACPI tables can set these properties >> directly and we can hopefully won't need to add new quirks to the machine >> driver. > > The device properties thing seems reasonable enough. I'm still not > super convinced the machine driver is the right place, but let's see the > code. I think ideally the arch code would have a way to put all the > device property based quirks for these systems like we IIRC do for some > DT fixups. We've already tried something like that for fixing a similar situation (not enough info in ACPI for the driver to function) for silead touchscreens: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/platform/x86/silead_dmi.c Note that this device-property providing driver can only be builtin, it cannot be built as a module (otherwise we're back to having ordering issues). We (Dmitry and I) have gotten quite some flack about needlessly growing the bzImage size with data only used on a small subset of x86 devices and this construction is not something which I want to repeat (nor would advise others to repeat). We really need to have a way to have these quirks table in a module which only loads on devices which need it. So in my mind the machine driver is a good place for this. Regards, Hans
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index 45a73049cf64..c5d19a80506b 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,10 +31,6 @@ #include "rl6231.h" #include "rt5651.h" -#define RT5651_JD_MAP(quirk) ((quirk) & GENMASK(7, 0)) -#define RT5651_IN2_DIFF BIT(16) -#define RT5651_DMIC_EN BIT(17) - #define RT5651_DEVICE_ID_VALUE 0x6281 #define RT5651_PR_RANGE_BASE (0xff + 1) @@ -43,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, @@ -1585,10 +1578,88 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, return 0; } +static irqreturn_t rt5651_irq(int irq, void *data) +{ + struct rt5651_priv *rt5651 = data; + + queue_delayed_work(system_power_efficient_wq, + &rt5651->jack_detect_work, msecs_to_jiffies(250)); + + return IRQ_HANDLED; +} + +static int rt5651_set_jack(struct snd_soc_codec *codec, + struct snd_soc_jack *hp_jack, void *data) +{ + struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); + struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + int ret; + + if (!rt5651->pdata.jd_src || !rt5651->irq) + return -EINVAL; + + /* IRQ output on GPIO1 */ + regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, + RT5651_GP1_PIN_MASK, RT5651_GP1_PIN_IRQ); + + switch (rt5651->pdata.jd_src) { + case RT5651_JD1_1: + regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, + RT5651_JD_TRG_SEL_MASK, + RT5651_JD_TRG_SEL_JD1_1); + regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, + RT5651_JD1_1_IRQ_EN, + RT5651_JD1_1_IRQ_EN); + break; + case RT5651_JD1_2: + regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, + RT5651_JD_TRG_SEL_MASK, + RT5651_JD_TRG_SEL_JD1_2); + regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, + RT5651_JD1_2_IRQ_EN, + RT5651_JD1_2_IRQ_EN); + break; + case RT5651_JD2: + regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, + RT5651_JD_TRG_SEL_MASK, + RT5651_JD_TRG_SEL_JD2); + regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, + RT5651_JD2_IRQ_EN, + RT5651_JD2_IRQ_EN); + break; + case RT5651_JD_NULL: + break; + default: + dev_warn(codec->dev, "Currently only JD1_1 / JD1_2 / JD2 are supported\n"); + break; + } + + snd_soc_dapm_force_enable_pin(dapm, "JD Power"); + snd_soc_dapm_force_enable_pin(dapm, "LDO"); + snd_soc_dapm_sync(dapm); + + regmap_update_bits(rt5651->regmap, RT5651_MICBIAS, + 0x38, 0x38); + + ret = devm_request_threaded_irq(codec->dev, rt5651->irq, NULL, + rt5651_irq, + IRQF_TRIGGER_RISING | + IRQF_TRIGGER_FALLING | + IRQF_ONESHOT, "rt5651", rt5651); + if (ret) { + dev_err(codec->dev, "Failed to reguest IRQ: %d\n", ret); + return ret; + } + + rt5651->hp_jack = hp_jack; + rt5651_irq(0, rt5651); + + return 0; +} + static int rt5651_probe(struct snd_soc_codec *codec) { struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); - struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); rt5651->codec = codec; @@ -1604,15 +1675,6 @@ static int rt5651_probe(struct snd_soc_codec *codec) snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF); - if (rt5651->pdata.jd_src) { - snd_soc_dapm_force_enable_pin(dapm, "JD Power"); - snd_soc_dapm_force_enable_pin(dapm, "LDO"); - snd_soc_dapm_sync(dapm); - - regmap_update_bits(rt5651->regmap, RT5651_MICBIAS, - 0x38, 0x38); - } - return 0; } @@ -1698,6 +1760,7 @@ static const struct snd_soc_codec_driver soc_codec_dev_rt5651 = { .resume = rt5651_resume, .set_bias_level = rt5651_set_bias_level, .idle_bias_off = true, + .set_jack = rt5651_set_jack, .component_driver = { .controls = rt5651_snd_controls, .num_controls = ARRAY_SIZE(rt5651_snd_controls), @@ -1747,54 +1810,16 @@ 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_parse_dt(struct rt5651_priv *rt5651, struct device_node *np) { - if (of_property_read_bool(np, "realtek,in2-differential")) - rt5651_quirk |= RT5651_IN2_DIFF; - if (of_property_read_bool(np, "realtek,dmic-en")) - rt5651_quirk |= RT5651_DMIC_EN; + rt5651->pdata.in2_diff = + of_property_read_bool(np, "realtek,in2-differential"); + rt5651->pdata.dmic_en = + of_property_read_bool(np, "realtek,dmic-en"); return 0; } -static void rt5651_set_pdata(struct rt5651_priv *rt5651) -{ - if (rt5651_quirk & RT5651_IN2_DIFF) - rt5651->pdata.in2_diff = true; - if (rt5651_quirk & RT5651_DMIC_EN) - rt5651->pdata.dmic_en = true; - if (RT5651_JD_MAP(rt5651_quirk)) - rt5651->pdata.jd_src = RT5651_JD_MAP(rt5651_quirk); -} - -static irqreturn_t rt5651_irq(int irq, void *data) -{ - struct rt5651_priv *rt5651 = data; - - queue_delayed_work(system_power_efficient_wq, - &rt5651->jack_detect_work, msecs_to_jiffies(250)); - - return IRQ_HANDLED; -} - static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) { struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); @@ -1860,17 +1885,26 @@ static void rt5651_jack_detect_work(struct work_struct *work) snd_soc_jack_report(rt5651->hp_jack, report, SND_JACK_HEADSET); } -int rt5651_set_jack_detect(struct snd_soc_codec *codec, - struct snd_soc_jack *hp_jack) +static void rt5651_apply_pdata(struct rt5651_priv *rt5651) { - struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + if (rt5651->pdata.in2_diff) + regmap_update_bits(rt5651->regmap, RT5651_IN1_IN2, + RT5651_IN_DF2, RT5651_IN_DF2); - rt5651->hp_jack = hp_jack; - rt5651_irq(0, rt5651); + if (rt5651->pdata.dmic_en) + regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, + RT5651_GP2_PIN_MASK, RT5651_GP2_PIN_DMIC1_SCL); +} - return 0; +void rt5651_set_pdata(struct snd_soc_codec *codec, + struct rt5651_platform_data *pdata) +{ + struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + + rt5651->pdata = *pdata; + rt5651_apply_pdata(rt5651); } -EXPORT_SYMBOL_GPL(rt5651_set_jack_detect); +EXPORT_SYMBOL_GPL(rt5651_set_pdata); static int rt5651_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) @@ -1890,10 +1924,6 @@ static int rt5651_i2c_probe(struct i2c_client *i2c, rt5651->pdata = *pdata; else if (i2c->dev.of_node) rt5651_parse_dt(rt5651, i2c->dev.of_node); - else - dmi_check_system(rt5651_quirk_table); - - rt5651_set_pdata(rt5651); rt5651->regmap = devm_regmap_init_i2c(i2c, &rt5651_regmap); if (IS_ERR(rt5651->regmap)) { @@ -1917,69 +1947,13 @@ static int rt5651_i2c_probe(struct i2c_client *i2c, if (ret != 0) dev_warn(&i2c->dev, "Failed to apply regmap patch: %d\n", ret); - if (rt5651->pdata.in2_diff) - regmap_update_bits(rt5651->regmap, RT5651_IN1_IN2, - RT5651_IN_DF2, RT5651_IN_DF2); - - if (rt5651->pdata.dmic_en) - regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, - RT5651_GP2_PIN_MASK, RT5651_GP2_PIN_DMIC1_SCL); + if (pdata || i2c->dev.of_node) + rt5651_apply_pdata(rt5651); + rt5651->irq = i2c->irq; rt5651->hp_mute = 1; - - if (rt5651->pdata.jd_src) { - - /* IRQ output on GPIO1 */ - regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, - RT5651_GP1_PIN_MASK, RT5651_GP1_PIN_IRQ); - - switch (rt5651->pdata.jd_src) { - case RT5651_JD1_1: - regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, - RT5651_JD_TRG_SEL_MASK, - RT5651_JD_TRG_SEL_JD1_1); - regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, - RT5651_JD1_1_IRQ_EN, - RT5651_JD1_1_IRQ_EN); - break; - case RT5651_JD1_2: - regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, - RT5651_JD_TRG_SEL_MASK, - RT5651_JD_TRG_SEL_JD1_2); - regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, - RT5651_JD1_2_IRQ_EN, - RT5651_JD1_2_IRQ_EN); - break; - case RT5651_JD2: - regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, - RT5651_JD_TRG_SEL_MASK, - RT5651_JD_TRG_SEL_JD2); - regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, - RT5651_JD2_IRQ_EN, - RT5651_JD2_IRQ_EN); - break; - case RT5651_JD_NULL: - break; - default: - dev_warn(&i2c->dev, "Currently only JD1_1 / JD1_2 / JD2 are supported\n"); - break; - } - } - INIT_DELAYED_WORK(&rt5651->jack_detect_work, rt5651_jack_detect_work); - if (i2c->irq) { - ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, - rt5651_irq, - IRQF_TRIGGER_RISING | - IRQF_TRIGGER_FALLING | - IRQF_ONESHOT, "rt5651", rt5651); - if (ret) { - dev_err(&i2c->dev, "Failed to reguest IRQ: %d\n", ret); - return ret; - } - } - ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5651, rt5651_dai, ARRAY_SIZE(rt5651_dai)); diff --git a/sound/soc/codecs/rt5651.h b/sound/soc/codecs/rt5651.h index 4f8b202121d7..151ac92f6bad 100644 --- a/sound/soc/codecs/rt5651.h +++ b/sound/soc/codecs/rt5651.h @@ -2065,6 +2065,7 @@ struct rt5651_priv { struct snd_soc_jack *hp_jack; struct delayed_work jack_detect_work; + int irq; int sysclk; int sysclk_src; int lrck[RT5651_AIFS]; @@ -2079,6 +2080,7 @@ struct rt5651_priv { bool hp_mute; }; -int rt5651_set_jack_detect(struct snd_soc_codec *codec, - struct snd_soc_jack *hp_jack); +void rt5651_set_pdata(struct snd_soc_codec *codec, + struct rt5651_platform_data *pdata); + #endif /* __RT5651_H__ */ diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 49c538f2770a..8ef5b5500fb7 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -42,7 +42,15 @@ enum { BYT_RT5651_IN3_MAP, }; -#define BYT_RT5651_MAP(quirk) ((quirk) & GENMASK(7, 0)) +enum { + BYT_RT5651_JD_NULL = (RT5651_JD_NULL << 4), + BYT_RT5651_JD1_1 = (RT5651_JD1_1 << 4), + BYT_RT5651_JD1_2 = (RT5651_JD1_2 << 4), + BYT_RT5651_JD2 = (RT5651_JD2 << 4), +}; + +#define BYT_RT5651_MAP(quirk) ((quirk) & GENMASK(3, 0)) +#define BYT_RT5651_JDSRC(quirk) (((quirk) & GENMASK(7, 4)) >> 4) #define BYT_RT5651_DMIC_EN BIT(16) #define BYT_RT5651_MCLK_EN BIT(17) #define BYT_RT5651_MCLK_25MHZ BIT(18) @@ -53,7 +61,6 @@ struct byt_rt5651_private { }; static unsigned long byt_rt5651_quirk = BYT_RT5651_DMIC_MAP | - BYT_RT5651_DMIC_EN | BYT_RT5651_MCLK_EN; static void log_quirks(struct device *dev) @@ -66,6 +73,9 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk IN2_MAP enabled"); if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN3_MAP) dev_info(dev, "quirk IN3_MAP enabled"); + if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) + dev_info(dev, "quirk jack-detect src %ld\n", + BYT_RT5651_JDSRC(byt_rt5651_quirk)); if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN) dev_info(dev, "quirk DMIC enabled"); if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) @@ -288,6 +298,7 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"), }, .driver_data = (void *)(BYT_RT5651_MCLK_EN | + BYT_RT5651_JD1_1 | BYT_RT5651_IN1_IN2_MAP), }, {} @@ -299,6 +310,7 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) struct snd_soc_codec *codec = runtime->codec; struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card); const struct snd_soc_dapm_route *custom_map; + struct rt5651_platform_data pdata = {}; int num_routes; int ret; @@ -360,17 +372,26 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) dev_err(card->dev, "unable to set MCLK rate\n"); } - ret = snd_soc_card_jack_new(runtime->card, "Headset", - SND_JACK_HEADSET, &priv->jack, - bytcr_jack_pins, ARRAY_SIZE(bytcr_jack_pins)); - if (ret) { - dev_err(runtime->dev, "Headset jack creation failed %d\n", ret); - return ret; - } + pdata.jd_src = BYT_RT5651_JDSRC(byt_rt5651_quirk); + if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN) + pdata.dmic_en = true; + rt5651_set_pdata(codec, &pdata); + + if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) { + ret = snd_soc_card_jack_new(runtime->card, "Headset", + SND_JACK_HEADSET, &priv->jack, + bytcr_jack_pins, ARRAY_SIZE(bytcr_jack_pins)); + if (ret) { + dev_err(runtime->dev, "Jack creation failed %d\n", ret); + return ret; + } - rt5651_set_jack_detect(codec, &priv->jack); + ret = snd_soc_codec_set_jack(codec, &priv->jack, NULL); + if (ret) + return ret; + } - return ret; + return 0; } static const struct snd_soc_pcm_stream byt_rt5651_dai_params = {
Instead of duplicating the DMI quirks between the codec and machine driver, add a rt5651_set_pdata() codec private function which the machine driver can use to pass in pdata. This means we need to delay setting up the jack-detect registers, so this commits moves this to rt5651_set_jack() code. Note this is actually a good thing as before we would register the irq handler before rt5651->hp_jack was assigned, leading to a potential NULL deref if the jack_detect work runs before the machine driver has called set_jack. While at it also move over to the standard snd_soc_codec_set_jack() function instead of using a codec private function for this. Note this commit causes the machine-driver to now actually honor the BYT_RT5651_DMIC_EN quirk, which was ignored before. To avoid this causing a functional change in what is purely a refactor commit, this commit removes the quirk from the defaults. If we really want the BYT_RT5651_DMIC_EN behavior anywhere it should be specifically enabled by follow up commits. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/codecs/rt5651.c | 232 +++++++++++++++------------------- sound/soc/codecs/rt5651.h | 6 +- sound/soc/intel/boards/bytcr_rt5651.c | 43 +++++-- 3 files changed, 139 insertions(+), 142 deletions(-)