| Message ID | 20230825120525.1337417-1-sbinding@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] ALSA: hda: cs35l41: Support mute notifications for CS35L41 HDA | expand |
On Fri, 25 Aug 2023 14:05:25 +0200, Stefan Binding wrote: > > From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > > Some laptops require a hardware based mute system, where when a hotkey > is pressed, it forces the amp to be muted. > > For CS35L41, when the hotkey is pressed, an acpi notification is sent > to the CS35L41 Device Node. The driver needs to handle this notification > and call a _DSM function to retrieve the mute state. > > Since the amp is only muted during playback, the driver will only mute > or unmute if playback is occurring, otherwise it will save the mute > state for when playback starts. > > Only one handler can be registered for the acpi notification, but all > amps need to receive that notification, we can register a single handler > inside the Realtek HDA driver, so that it can then notify through the > component framework. > > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> We don't do normally in this way. The ACPI hot key handling is done via user-space, and user-space daemon triggers the mute of the system. Can't the ACPI notify the key event on those machines? thanks, Takashi > --- > sound/pci/hda/cs35l41_hda.c | 92 ++++++++++++++++++++++++++++++----- > sound/pci/hda/cs35l41_hda.h | 3 ++ > sound/pci/hda/hda_component.h | 3 ++ > sound/pci/hda/patch_realtek.c | 48 +++++++++++++++++- > 4 files changed, 132 insertions(+), 14 deletions(-) > > diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c > index f9b77353c266..609e63b34d6d 100644 > --- a/sound/pci/hda/cs35l41_hda.c > +++ b/sound/pci/hda/cs35l41_hda.c > @@ -33,6 +33,9 @@ > #define CAL_AMBIENT_DSP_CTL_NAME "CAL_AMBIENT" > #define CAL_DSP_CTL_TYPE 5 > #define CAL_DSP_CTL_ALG 205 > +#define CS35L41_UUID "50d90cdc-3de4-4f18-b528-c7fe3b71f40d" > +#define CS35L41_DSM_GET_MUTE 5 > +#define CS35L41_NOTIFY_EVENT 0x91 > > static bool firmware_autostart = 1; > module_param(firmware_autostart, bool, 0444); > @@ -520,6 +523,31 @@ static void cs35l41_hda_play_start(struct device *dev) > > } > > +static void cs35l41_mute(struct device *dev, bool mute) > +{ > + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > + struct regmap *reg = cs35l41->regmap; > + > + dev_dbg(dev, "Mute(%d:%d) Playback Started: %d\n", mute, cs35l41->mute_override, > + cs35l41->playback_started); > + > + if (cs35l41->playback_started) { > + if (mute || cs35l41->mute_override) { > + dev_dbg(dev, "Muting\n"); > + regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); > + } else { > + dev_dbg(dev, "Unmuting\n"); > + if (cs35l41->firmware_running) { > + regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, > + ARRAY_SIZE(cs35l41_hda_unmute_dsp)); > + } else { > + regmap_multi_reg_write(reg, cs35l41_hda_unmute, > + ARRAY_SIZE(cs35l41_hda_unmute)); > + } > + } > + } > +} > + > static void cs35l41_hda_play_done(struct device *dev) > { > struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > @@ -529,13 +557,7 @@ static void cs35l41_hda_play_done(struct device *dev) > > cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL, > cs35l41->firmware_running); > - if (cs35l41->firmware_running) { > - regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, > - ARRAY_SIZE(cs35l41_hda_unmute_dsp)); > - } else { > - regmap_multi_reg_write(reg, cs35l41_hda_unmute, > - ARRAY_SIZE(cs35l41_hda_unmute)); > - } > + cs35l41_mute(dev, false); > } > > static void cs35l41_hda_pause_start(struct device *dev) > @@ -545,7 +567,7 @@ static void cs35l41_hda_pause_start(struct device *dev) > > dev_dbg(dev, "Pause (Start)\n"); > > - regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); > + cs35l41_mute(dev, true); > cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL, > cs35l41->firmware_running); > } > @@ -1073,6 +1095,44 @@ static int cs35l41_create_controls(struct cs35l41_hda *cs35l41) > return 0; > } > > +static int cs35l41_get_acpi_mute_state(struct cs35l41_hda *cs35l41, acpi_handle handle) > +{ > + guid_t guid; > + union acpi_object *ret; > + int mute = -ENODEV; > + > + guid_parse(CS35L41_UUID, &guid); > + > + if (acpi_check_dsm(handle, &guid, 0, BIT(CS35L41_DSM_GET_MUTE))) { > + ret = acpi_evaluate_dsm(handle, &guid, 0, CS35L41_DSM_GET_MUTE, NULL); > + mute = *ret->buffer.pointer; > + dev_dbg(cs35l41->dev, "CS35L41_DSM_GET_MUTE: %d\n", mute); > + } > + > + dev_dbg(cs35l41->dev, "%s: %d\n", __func__, mute); > + > + return mute; > +} > + > +static void cs35l41_acpi_device_notify(acpi_handle handle, u32 event, struct device *dev) > +{ > + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > + int mute; > + > + if (event != CS35L41_NOTIFY_EVENT) > + return; > + > + mute = cs35l41_get_acpi_mute_state(cs35l41, handle); > + if (mute < 0) { > + dev_warn(cs35l41->dev, "Unable to retrieve mute state: %d\n", mute); > + return; > + } > + > + dev_dbg(cs35l41->dev, "Requesting mute value: %d\n", mute); > + cs35l41->mute_override = (mute > 0); > + cs35l41_mute(cs35l41->dev, cs35l41->mute_override); > +} > + > static int cs35l41_hda_bind(struct device *dev, struct device *master, void *master_data) > { > struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > @@ -1114,6 +1174,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas > comps->playback_hook = cs35l41_hda_playback_hook; > comps->pre_playback_hook = cs35l41_hda_pre_playback_hook; > comps->post_playback_hook = cs35l41_hda_post_playback_hook; > + comps->acpi_notify = cs35l41_acpi_device_notify; > + comps->adev = cs35l41->dacpi; > + > + cs35l41->mute_override = cs35l41_get_acpi_mute_state(cs35l41, > + acpi_device_handle(cs35l41->dacpi)) > 0; > > mutex_unlock(&cs35l41->fw_mutex); > > @@ -1387,8 +1452,8 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i > return -ENODEV; > } > > + cs35l41->dacpi = adev; > physdev = get_device(acpi_get_first_physical_node(adev)); > - acpi_dev_put(adev); > > sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev)); > if (IS_ERR(sub)) > @@ -1498,6 +1563,7 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i > hw_cfg->valid = false; > hw_cfg->gpio1.valid = false; > hw_cfg->gpio2.valid = false; > + acpi_dev_put(cs35l41->dacpi); > put_physdev: > put_device(physdev); > > @@ -1601,10 +1667,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i > if (ret) > goto err; > > - ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute, > - ARRAY_SIZE(cs35l41_hda_mute)); > - if (ret) > - goto err; > + cs35l41_mute(cs35l41->dev, true); > > INIT_WORK(&cs35l41->fw_load_work, cs35l41_fw_load_work); > mutex_init(&cs35l41->fw_mutex); > @@ -1641,6 +1704,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i > if (cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type)) > gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); > gpiod_put(cs35l41->reset_gpio); > + acpi_dev_put(cs35l41->dacpi); > kfree(cs35l41->acpi_subsystem_id); > > return ret; > @@ -1659,6 +1723,8 @@ void cs35l41_hda_remove(struct device *dev) > > component_del(cs35l41->dev, &cs35l41_hda_comp_ops); > > + acpi_dev_put(cs35l41->dacpi); > + > pm_runtime_put_noidle(cs35l41->dev); > > if (cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type)) > diff --git a/sound/pci/hda/cs35l41_hda.h b/sound/pci/hda/cs35l41_hda.h > index b93bf762976e..ce3f2bb6ffd0 100644 > --- a/sound/pci/hda/cs35l41_hda.h > +++ b/sound/pci/hda/cs35l41_hda.h > @@ -10,6 +10,7 @@ > #ifndef __CS35L41_HDA_H__ > #define __CS35L41_HDA_H__ > > +#include <linux/acpi.h> > #include <linux/efi.h> > #include <linux/regulator/consumer.h> > #include <linux/gpio/consumer.h> > @@ -70,6 +71,8 @@ struct cs35l41_hda { > bool halo_initialized; > bool playback_started; > struct cs_dsp cs_dsp; > + struct acpi_device *dacpi; > + bool mute_override; > }; > > enum halo_state { > diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h > index f170aec967c1..c7a9b6a660e5 100644 > --- a/sound/pci/hda/hda_component.h > +++ b/sound/pci/hda/hda_component.h > @@ -6,6 +6,7 @@ > * Cirrus Logic International Semiconductor Ltd. > */ > > +#include <linux/acpi.h> > #include <linux/component.h> > > #define HDA_MAX_COMPONENTS 4 > @@ -15,6 +16,8 @@ struct hda_component { > struct device *dev; > char name[HDA_MAX_NAME_SIZE]; > struct hda_codec *codec; > + struct acpi_device *adev; > + void (*acpi_notify)(acpi_handle handle, u32 event, struct device *dev); > void (*pre_playback_hook)(struct device *dev, int action); > void (*playback_hook)(struct device *dev, int action); > void (*post_playback_hook)(struct device *dev, int action); > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c > index a07df6f92960..fd3768e73c15 100644 > --- a/sound/pci/hda/patch_realtek.c > +++ b/sound/pci/hda/patch_realtek.c > @@ -6704,19 +6704,65 @@ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec, > } > } > > +static void comp_acpi_device_notify(acpi_handle handle, u32 event, void *data) > +{ > + struct hda_codec *cdc = data; > + struct alc_spec *spec = cdc->spec; > + int i; > + > + codec_info(cdc, "ACPI Notification %d\n", event); > + > + for (i = 0; i < HDA_MAX_COMPONENTS; i++) { > + if (spec->comps[i].dev && spec->comps[i].acpi_notify) > + spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event, > + spec->comps[i].dev); > + } > +} > + > static int comp_bind(struct device *dev) > { > struct hda_codec *cdc = dev_to_hda_codec(dev); > struct alc_spec *spec = cdc->spec; > + struct acpi_device *adev; > + int ret; > + > + ret = component_bind_all(dev, spec->comps); > + if (ret) > + return ret; > > - return component_bind_all(dev, spec->comps); > + adev = spec->comps[0].adev; > + if (!acpi_device_handle(adev)) > + return 0; > + > + ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, > + comp_acpi_device_notify, cdc); > + if (ret < 0) { > + codec_warn(cdc, "Failed to install notify handler: %d\n", ret); > + return 0; > + } > + > + codec_dbg(cdc, "Notify handler installed\n"); > + > + return 0; > } > > static void comp_unbind(struct device *dev) > { > struct hda_codec *cdc = dev_to_hda_codec(dev); > struct alc_spec *spec = cdc->spec; > + struct acpi_device *adev; > + int ret; > + > + adev = spec->comps[0].adev; > + if (!acpi_device_handle(adev)) > + goto unbind; > + > + ret = acpi_remove_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, > + comp_acpi_device_notify); > + if (ret < 0) > + codec_warn(cdc, "Failed to uninstall notify handler: %d\n", ret); > > +unbind: > component_unbind_all(dev, spec->comps); > } > > -- > 2.34.1 >
Hi Stefan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tiwai-sound/for-next]
[also build test WARNING on next-20230825]
[cannot apply to tiwai-sound/for-linus linus/master v6.5-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stefan-Binding/ALSA-hda-cs35l41-Support-mute-notifications-for-CS35L41-HDA/20230825-200835
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link: https://lore.kernel.org/r/20230825120525.1337417-1-sbinding%40opensource.cirrus.com
patch subject: [PATCH v1] ALSA: hda: cs35l41: Support mute notifications for CS35L41 HDA
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230825/202308252129.LN14FqG9-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308252129.LN14FqG9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308252129.LN14FqG9-lkp@intel.com/
All warnings (new ones prefixed by >>):
sound/pci/hda/patch_realtek.c: In function 'comp_acpi_device_notify':
sound/pci/hda/patch_realtek.c:6717:52: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_fwnode_handle'? [-Werror=implicit-function-declaration]
6717 | spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event,
| ^~~~~~~~~~~~~~~~~~
| acpi_fwnode_handle
>> sound/pci/hda/patch_realtek.c:6717:52: warning: passing argument 1 of 'spec->comps[i].acpi_notify' makes pointer from integer without a cast [-Wint-conversion]
6717 | spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| int
sound/pci/hda/patch_realtek.c:6717:52: note: expected 'acpi_handle' {aka 'void *'} but argument is of type 'int'
sound/pci/hda/patch_realtek.c: In function 'comp_bind':
sound/pci/hda/patch_realtek.c:6737:47: error: invalid use of undefined type 'struct acpi_device'
6737 | ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY,
| ^~
sound/pci/hda/patch_realtek.c: In function 'comp_unbind':
sound/pci/hda/patch_realtek.c:6760:46: error: invalid use of undefined type 'struct acpi_device'
6760 | ret = acpi_remove_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY,
| ^~
cc1: some warnings being treated as errors
vim +6717 sound/pci/hda/patch_realtek.c
6706
6707 static void comp_acpi_device_notify(acpi_handle handle, u32 event, void *data)
6708 {
6709 struct hda_codec *cdc = data;
6710 struct alc_spec *spec = cdc->spec;
6711 int i;
6712
6713 codec_info(cdc, "ACPI Notification %d\n", event);
6714
6715 for (i = 0; i < HDA_MAX_COMPONENTS; i++) {
6716 if (spec->comps[i].dev && spec->comps[i].acpi_notify)
> 6717 spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event,
6718 spec->comps[i].dev);
6719 }
6720 }
6721
On 25/08/2023 13:13, Takashi Iwai wrote: > On Fri, 25 Aug 2023 14:05:25 +0200, > Stefan Binding wrote: >> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >> >> Some laptops require a hardware based mute system, where when a hotkey >> is pressed, it forces the amp to be muted. >> >> For CS35L41, when the hotkey is pressed, an acpi notification is sent >> to the CS35L41 Device Node. The driver needs to handle this notification >> and call a _DSM function to retrieve the mute state. >> >> Since the amp is only muted during playback, the driver will only mute >> or unmute if playback is occurring, otherwise it will save the mute >> state for when playback starts. >> >> Only one handler can be registered for the acpi notification, but all >> amps need to receive that notification, we can register a single handler >> inside the Realtek HDA driver, so that it can then notify through the >> component framework. >> >> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > We don't do normally in this way. The ACPI hot key handling is done > via user-space, and user-space daemon triggers the mute of the > system. > > Can't the ACPI notify the key event on those machines? This feature is not the "normal" mute button on a keyboard, it is a custom request from a manufacturer which only mutes the audio on the speakers. On previous generations, this was achieved using a GPIO controlled by the BIOS/EC. However, since CS35L41 does not have such GPIO, we must control it by other means. Our solution, which we have to share with the Windows driver, it to use ACPI notifications to tell the driver to mute the amps when the shortcut is pressed. Does this seem like a valid exception to the typical approach? Thanks, Stefan > > > thanks, > > Takashi > >> --- >> sound/pci/hda/cs35l41_hda.c | 92 ++++++++++++++++++++++++++++++----- >> sound/pci/hda/cs35l41_hda.h | 3 ++ >> sound/pci/hda/hda_component.h | 3 ++ >> sound/pci/hda/patch_realtek.c | 48 +++++++++++++++++- >> 4 files changed, 132 insertions(+), 14 deletions(-) >> >> diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c >> index f9b77353c266..609e63b34d6d 100644 >> --- a/sound/pci/hda/cs35l41_hda.c >> +++ b/sound/pci/hda/cs35l41_hda.c >> @@ -33,6 +33,9 @@ >> #define CAL_AMBIENT_DSP_CTL_NAME "CAL_AMBIENT" >> #define CAL_DSP_CTL_TYPE 5 >> #define CAL_DSP_CTL_ALG 205 >> +#define CS35L41_UUID "50d90cdc-3de4-4f18-b528-c7fe3b71f40d" >> +#define CS35L41_DSM_GET_MUTE 5 >> +#define CS35L41_NOTIFY_EVENT 0x91 >> >> static bool firmware_autostart = 1; >> module_param(firmware_autostart, bool, 0444); >> @@ -520,6 +523,31 @@ static void cs35l41_hda_play_start(struct device *dev) >> >> } >> >> +static void cs35l41_mute(struct device *dev, bool mute) >> +{ >> + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); >> + struct regmap *reg = cs35l41->regmap; >> + >> + dev_dbg(dev, "Mute(%d:%d) Playback Started: %d\n", mute, cs35l41->mute_override, >> + cs35l41->playback_started); >> + >> + if (cs35l41->playback_started) { >> + if (mute || cs35l41->mute_override) { >> + dev_dbg(dev, "Muting\n"); >> + regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); >> + } else { >> + dev_dbg(dev, "Unmuting\n"); >> + if (cs35l41->firmware_running) { >> + regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, >> + ARRAY_SIZE(cs35l41_hda_unmute_dsp)); >> + } else { >> + regmap_multi_reg_write(reg, cs35l41_hda_unmute, >> + ARRAY_SIZE(cs35l41_hda_unmute)); >> + } >> + } >> + } >> +} >> + >> static void cs35l41_hda_play_done(struct device *dev) >> { >> struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); >> @@ -529,13 +557,7 @@ static void cs35l41_hda_play_done(struct device *dev) >> >> cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL, >> cs35l41->firmware_running); >> - if (cs35l41->firmware_running) { >> - regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, >> - ARRAY_SIZE(cs35l41_hda_unmute_dsp)); >> - } else { >> - regmap_multi_reg_write(reg, cs35l41_hda_unmute, >> - ARRAY_SIZE(cs35l41_hda_unmute)); >> - } >> + cs35l41_mute(dev, false); >> } >> >> static void cs35l41_hda_pause_start(struct device *dev) >> @@ -545,7 +567,7 @@ static void cs35l41_hda_pause_start(struct device *dev) >> >> dev_dbg(dev, "Pause (Start)\n"); >> >> - regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); >> + cs35l41_mute(dev, true); >> cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL, >> cs35l41->firmware_running); >> } >> @@ -1073,6 +1095,44 @@ static int cs35l41_create_controls(struct cs35l41_hda *cs35l41) >> return 0; >> } >> >> +static int cs35l41_get_acpi_mute_state(struct cs35l41_hda *cs35l41, acpi_handle handle) >> +{ >> + guid_t guid; >> + union acpi_object *ret; >> + int mute = -ENODEV; >> + >> + guid_parse(CS35L41_UUID, &guid); >> + >> + if (acpi_check_dsm(handle, &guid, 0, BIT(CS35L41_DSM_GET_MUTE))) { >> + ret = acpi_evaluate_dsm(handle, &guid, 0, CS35L41_DSM_GET_MUTE, NULL); >> + mute = *ret->buffer.pointer; >> + dev_dbg(cs35l41->dev, "CS35L41_DSM_GET_MUTE: %d\n", mute); >> + } >> + >> + dev_dbg(cs35l41->dev, "%s: %d\n", __func__, mute); >> + >> + return mute; >> +} >> + >> +static void cs35l41_acpi_device_notify(acpi_handle handle, u32 event, struct device *dev) >> +{ >> + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); >> + int mute; >> + >> + if (event != CS35L41_NOTIFY_EVENT) >> + return; >> + >> + mute = cs35l41_get_acpi_mute_state(cs35l41, handle); >> + if (mute < 0) { >> + dev_warn(cs35l41->dev, "Unable to retrieve mute state: %d\n", mute); >> + return; >> + } >> + >> + dev_dbg(cs35l41->dev, "Requesting mute value: %d\n", mute); >> + cs35l41->mute_override = (mute > 0); >> + cs35l41_mute(cs35l41->dev, cs35l41->mute_override); >> +} >> + >> static int cs35l41_hda_bind(struct device *dev, struct device *master, void *master_data) >> { >> struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); >> @@ -1114,6 +1174,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas >> comps->playback_hook = cs35l41_hda_playback_hook; >> comps->pre_playback_hook = cs35l41_hda_pre_playback_hook; >> comps->post_playback_hook = cs35l41_hda_post_playback_hook; >> + comps->acpi_notify = cs35l41_acpi_device_notify; >> + comps->adev = cs35l41->dacpi; >> + >> + cs35l41->mute_override = cs35l41_get_acpi_mute_state(cs35l41, >> + acpi_device_handle(cs35l41->dacpi)) > 0; >> >> mutex_unlock(&cs35l41->fw_mutex); >> >> @@ -1387,8 +1452,8 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i >> return -ENODEV; >> } >> >> + cs35l41->dacpi = adev; >> physdev = get_device(acpi_get_first_physical_node(adev)); >> - acpi_dev_put(adev); >> >> sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev)); >> if (IS_ERR(sub)) >> @@ -1498,6 +1563,7 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i >> hw_cfg->valid = false; >> hw_cfg->gpio1.valid = false; >> hw_cfg->gpio2.valid = false; >> + acpi_dev_put(cs35l41->dacpi); >> put_physdev: >> put_device(physdev); >> >> @@ -1601,10 +1667,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i >> if (ret) >> goto err; >> >> - ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute, >> - ARRAY_SIZE(cs35l41_hda_mute)); >> - if (ret) >> - goto err; >> + cs35l41_mute(cs35l41->dev, true); >> >> INIT_WORK(&cs35l41->fw_load_work, cs35l41_fw_load_work); >> mutex_init(&cs35l41->fw_mutex); >> @@ -1641,6 +1704,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i >> if (cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type)) >> gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); >> gpiod_put(cs35l41->reset_gpio); >> + acpi_dev_put(cs35l41->dacpi); >> kfree(cs35l41->acpi_subsystem_id); >> >> return ret; >> @@ -1659,6 +1723,8 @@ void cs35l41_hda_remove(struct device *dev) >> >> component_del(cs35l41->dev, &cs35l41_hda_comp_ops); >> >> + acpi_dev_put(cs35l41->dacpi); >> + >> pm_runtime_put_noidle(cs35l41->dev); >> >> if (cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type)) >> diff --git a/sound/pci/hda/cs35l41_hda.h b/sound/pci/hda/cs35l41_hda.h >> index b93bf762976e..ce3f2bb6ffd0 100644 >> --- a/sound/pci/hda/cs35l41_hda.h >> +++ b/sound/pci/hda/cs35l41_hda.h >> @@ -10,6 +10,7 @@ >> #ifndef __CS35L41_HDA_H__ >> #define __CS35L41_HDA_H__ >> >> +#include <linux/acpi.h> >> #include <linux/efi.h> >> #include <linux/regulator/consumer.h> >> #include <linux/gpio/consumer.h> >> @@ -70,6 +71,8 @@ struct cs35l41_hda { >> bool halo_initialized; >> bool playback_started; >> struct cs_dsp cs_dsp; >> + struct acpi_device *dacpi; >> + bool mute_override; >> }; >> >> enum halo_state { >> diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h >> index f170aec967c1..c7a9b6a660e5 100644 >> --- a/sound/pci/hda/hda_component.h >> +++ b/sound/pci/hda/hda_component.h >> @@ -6,6 +6,7 @@ >> * Cirrus Logic International Semiconductor Ltd. >> */ >> >> +#include <linux/acpi.h> >> #include <linux/component.h> >> >> #define HDA_MAX_COMPONENTS 4 >> @@ -15,6 +16,8 @@ struct hda_component { >> struct device *dev; >> char name[HDA_MAX_NAME_SIZE]; >> struct hda_codec *codec; >> + struct acpi_device *adev; >> + void (*acpi_notify)(acpi_handle handle, u32 event, struct device *dev); >> void (*pre_playback_hook)(struct device *dev, int action); >> void (*playback_hook)(struct device *dev, int action); >> void (*post_playback_hook)(struct device *dev, int action); >> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c >> index a07df6f92960..fd3768e73c15 100644 >> --- a/sound/pci/hda/patch_realtek.c >> +++ b/sound/pci/hda/patch_realtek.c >> @@ -6704,19 +6704,65 @@ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec, >> } >> } >> >> +static void comp_acpi_device_notify(acpi_handle handle, u32 event, void *data) >> +{ >> + struct hda_codec *cdc = data; >> + struct alc_spec *spec = cdc->spec; >> + int i; >> + >> + codec_info(cdc, "ACPI Notification %d\n", event); >> + >> + for (i = 0; i < HDA_MAX_COMPONENTS; i++) { >> + if (spec->comps[i].dev && spec->comps[i].acpi_notify) >> + spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event, >> + spec->comps[i].dev); >> + } >> +} >> + >> static int comp_bind(struct device *dev) >> { >> struct hda_codec *cdc = dev_to_hda_codec(dev); >> struct alc_spec *spec = cdc->spec; >> + struct acpi_device *adev; >> + int ret; >> + >> + ret = component_bind_all(dev, spec->comps); >> + if (ret) >> + return ret; >> >> - return component_bind_all(dev, spec->comps); >> + adev = spec->comps[0].adev; >> + if (!acpi_device_handle(adev)) >> + return 0; >> + >> + ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, >> + comp_acpi_device_notify, cdc); >> + if (ret < 0) { >> + codec_warn(cdc, "Failed to install notify handler: %d\n", ret); >> + return 0; >> + } >> + >> + codec_dbg(cdc, "Notify handler installed\n"); >> + >> + return 0; >> } >> >> static void comp_unbind(struct device *dev) >> { >> struct hda_codec *cdc = dev_to_hda_codec(dev); >> struct alc_spec *spec = cdc->spec; >> + struct acpi_device *adev; >> + int ret; >> + >> + adev = spec->comps[0].adev; >> + if (!acpi_device_handle(adev)) >> + goto unbind; >> + >> + ret = acpi_remove_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, >> + comp_acpi_device_notify); >> + if (ret < 0) >> + codec_warn(cdc, "Failed to uninstall notify handler: %d\n", ret); >> >> +unbind: >> component_unbind_all(dev, spec->comps); >> } >> >> -- >> 2.34.1 >>
On Tue, 29 Aug 2023 16:18:12 +0200, Stefan Binding wrote: > > > On 25/08/2023 13:13, Takashi Iwai wrote: > > On Fri, 25 Aug 2023 14:05:25 +0200, > > Stefan Binding wrote: > >> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >> > >> Some laptops require a hardware based mute system, where when a hotkey > >> is pressed, it forces the amp to be muted. > >> > >> For CS35L41, when the hotkey is pressed, an acpi notification is sent > >> to the CS35L41 Device Node. The driver needs to handle this notification > >> and call a _DSM function to retrieve the mute state. > >> > >> Since the amp is only muted during playback, the driver will only mute > >> or unmute if playback is occurring, otherwise it will save the mute > >> state for when playback starts. > >> > >> Only one handler can be registered for the acpi notification, but all > >> amps need to receive that notification, we can register a single handler > >> inside the Realtek HDA driver, so that it can then notify through the > >> component framework. > >> > >> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > > We don't do normally in this way. The ACPI hot key handling is done > > via user-space, and user-space daemon triggers the mute of the > > system. > > > > Can't the ACPI notify the key event on those machines? > > This feature is not the "normal" mute button on a keyboard, it is a > custom request > from a manufacturer which only mutes the audio on the speakers. > On previous generations, this was achieved using a GPIO controlled by > the BIOS/EC. > However, since CS35L41 does not have such GPIO, we must control it by > other means. > > Our solution, which we have to share with the Windows driver, it to use ACPI > notifications to tell the driver to mute the amps when the shortcut is > pressed. > > Does this seem like a valid exception to the typical approach? It's still the question whether we have to do this inevitably in the kernel in a way like that. It sounds quite unusual. Why this must be handled directly? IOW, what's the difference from the "normal" mute button? And, even if we take this approach, it leaves the device muted without exposing it to user-space. Then user wouldn't know what happens. thanks, Takashi
On 29/08/2023 15:23, Takashi Iwai wrote: > On Tue, 29 Aug 2023 16:18:12 +0200, > Stefan Binding wrote: >> >> On 25/08/2023 13:13, Takashi Iwai wrote: >>> On Fri, 25 Aug 2023 14:05:25 +0200, >>> Stefan Binding wrote: >>>> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>> >>>> Some laptops require a hardware based mute system, where when a hotkey >>>> is pressed, it forces the amp to be muted. >>>> >>>> For CS35L41, when the hotkey is pressed, an acpi notification is sent >>>> to the CS35L41 Device Node. The driver needs to handle this notification >>>> and call a _DSM function to retrieve the mute state. >>>> >>>> Since the amp is only muted during playback, the driver will only mute >>>> or unmute if playback is occurring, otherwise it will save the mute >>>> state for when playback starts. >>>> >>>> Only one handler can be registered for the acpi notification, but all >>>> amps need to receive that notification, we can register a single handler >>>> inside the Realtek HDA driver, so that it can then notify through the >>>> component framework. >>>> >>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> >>> We don't do normally in this way. The ACPI hot key handling is done >>> via user-space, and user-space daemon triggers the mute of the >>> system. >>> >>> Can't the ACPI notify the key event on those machines? >> This feature is not the "normal" mute button on a keyboard, it is a >> custom request >> from a manufacturer which only mutes the audio on the speakers. >> On previous generations, this was achieved using a GPIO controlled by >> the BIOS/EC. >> However, since CS35L41 does not have such GPIO, we must control it by >> other means. >> >> Our solution, which we have to share with the Windows driver, it to use ACPI >> notifications to tell the driver to mute the amps when the shortcut is >> pressed. >> >> Does this seem like a valid exception to the typical approach? > It's still the question whether we have to do this inevitably in the > kernel in a way like that. It sounds quite unusual. Why this must be > handled directly? IOW, what's the difference from the "normal" mute > button? > > And, even if we take this approach, it leaves the device muted without > exposing it to user-space. Then user wouldn't know what happens. > > > thanks, > > Takashi We spoke to the ODM for this system to get a more detailed explanation of this feature. The keyboard shortcut enables something called "Unobtrusive Mode". According to their explanation: - Unobtrusive mode is distinct to normal mute, as it only mutes the speakers - There is no requirement to update the volume controls, as the screen backlight will be off anyway in this mode - All other unobtrusive mode functions are enabled without user-space dependencies, and they would prefer not to make speaker mute an exception Thanks, Stefan
On Mon, 04 Sep 2023 14:00:20 +0200, Stefan Binding wrote: > > > On 29/08/2023 15:23, Takashi Iwai wrote: > > On Tue, 29 Aug 2023 16:18:12 +0200, > > Stefan Binding wrote: > >> > >> On 25/08/2023 13:13, Takashi Iwai wrote: > >>> On Fri, 25 Aug 2023 14:05:25 +0200, > >>> Stefan Binding wrote: > >>>> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >>>> > >>>> Some laptops require a hardware based mute system, where when a hotkey > >>>> is pressed, it forces the amp to be muted. > >>>> > >>>> For CS35L41, when the hotkey is pressed, an acpi notification is sent > >>>> to the CS35L41 Device Node. The driver needs to handle this notification > >>>> and call a _DSM function to retrieve the mute state. > >>>> > >>>> Since the amp is only muted during playback, the driver will only mute > >>>> or unmute if playback is occurring, otherwise it will save the mute > >>>> state for when playback starts. > >>>> > >>>> Only one handler can be registered for the acpi notification, but all > >>>> amps need to receive that notification, we can register a single handler > >>>> inside the Realtek HDA driver, so that it can then notify through the > >>>> component framework. > >>>> > >>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > >>> We don't do normally in this way. The ACPI hot key handling is done > >>> via user-space, and user-space daemon triggers the mute of the > >>> system. > >>> > >>> Can't the ACPI notify the key event on those machines? > >> This feature is not the "normal" mute button on a keyboard, it is a > >> custom request > >> from a manufacturer which only mutes the audio on the speakers. > >> On previous generations, this was achieved using a GPIO controlled by > >> the BIOS/EC. > >> However, since CS35L41 does not have such GPIO, we must control it by > >> other means. > >> > >> Our solution, which we have to share with the Windows driver, it to use ACPI > >> notifications to tell the driver to mute the amps when the shortcut is > >> pressed. > >> > >> Does this seem like a valid exception to the typical approach? > > It's still the question whether we have to do this inevitably in the > > kernel in a way like that. It sounds quite unusual. Why this must be > > handled directly? IOW, what's the difference from the "normal" mute > > button? > > > > And, even if we take this approach, it leaves the device muted without > > exposing it to user-space. Then user wouldn't know what happens. > > > > > > thanks, > > > > Takashi > We spoke to the ODM for this system to get a more detailed explanation > of this feature. > The keyboard shortcut enables something called "Unobtrusive > Mode". According to their explanation: > > - Unobtrusive mode is distinct to normal mute, as it only mutes the speakers > - There is no requirement to update the volume controls, as the screen > backlight will be off anyway in this mode > - All other unobtrusive mode functions are enabled without user-space > dependencies, and they would prefer not to make speaker mute an > exception Thanks, it gives a bit better clue. The remaining question is rather the exact behavior of this "unobtrusive mode". How is it triggered, and what's the exact expectation? e.g. It must secretly mute the speaker? That is, it must not expose the mixer state change to user-space? Or is it tied with the normal mixer state and user may unmute again? Takashi
On 04/09/2023 13:29, Takashi Iwai wrote: > On Mon, 04 Sep 2023 14:00:20 +0200, > Stefan Binding wrote: >> >> On 29/08/2023 15:23, Takashi Iwai wrote: >>> On Tue, 29 Aug 2023 16:18:12 +0200, >>> Stefan Binding wrote: >>>> On 25/08/2023 13:13, Takashi Iwai wrote: >>>>> On Fri, 25 Aug 2023 14:05:25 +0200, >>>>> Stefan Binding wrote: >>>>>> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>>>> >>>>>> Some laptops require a hardware based mute system, where when a hotkey >>>>>> is pressed, it forces the amp to be muted. >>>>>> >>>>>> For CS35L41, when the hotkey is pressed, an acpi notification is sent >>>>>> to the CS35L41 Device Node. The driver needs to handle this notification >>>>>> and call a _DSM function to retrieve the mute state. >>>>>> >>>>>> Since the amp is only muted during playback, the driver will only mute >>>>>> or unmute if playback is occurring, otherwise it will save the mute >>>>>> state for when playback starts. >>>>>> >>>>>> Only one handler can be registered for the acpi notification, but all >>>>>> amps need to receive that notification, we can register a single handler >>>>>> inside the Realtek HDA driver, so that it can then notify through the >>>>>> component framework. >>>>>> >>>>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> >>>>> We don't do normally in this way. The ACPI hot key handling is done >>>>> via user-space, and user-space daemon triggers the mute of the >>>>> system. >>>>> >>>>> Can't the ACPI notify the key event on those machines? >>>> This feature is not the "normal" mute button on a keyboard, it is a >>>> custom request >>>> from a manufacturer which only mutes the audio on the speakers. >>>> On previous generations, this was achieved using a GPIO controlled by >>>> the BIOS/EC. >>>> However, since CS35L41 does not have such GPIO, we must control it by >>>> other means. >>>> >>>> Our solution, which we have to share with the Windows driver, it to use ACPI >>>> notifications to tell the driver to mute the amps when the shortcut is >>>> pressed. >>>> >>>> Does this seem like a valid exception to the typical approach? >>> It's still the question whether we have to do this inevitably in the >>> kernel in a way like that. It sounds quite unusual. Why this must be >>> handled directly? IOW, what's the difference from the "normal" mute >>> button? >>> >>> And, even if we take this approach, it leaves the device muted without >>> exposing it to user-space. Then user wouldn't know what happens. >>> >>> >>> thanks, >>> >>> Takashi >> We spoke to the ODM for this system to get a more detailed explanation >> of this feature. >> The keyboard shortcut enables something called "Unobtrusive >> Mode". According to their explanation: >> >> - Unobtrusive mode is distinct to normal mute, as it only mutes the speakers >> - There is no requirement to update the volume controls, as the screen >> backlight will be off anyway in this mode >> - All other unobtrusive mode functions are enabled without user-space >> dependencies, and they would prefer not to make speaker mute an >> exception > Thanks, it gives a bit better clue. > The remaining question is rather the exact behavior of this > "unobtrusive mode". How is it triggered, and what's the exact > expectation? e.g. It must secretly mute the speaker? That is, it > must not expose the mixer state change to user-space? Or is it tied > with the normal mixer state and user may unmute again? > > > Takashi From what we understand, unobtrusive mode, which is activated by a keyboard shortcut (not a single key), performs several operations, such as: - muting the speaker (headphones remain unmuted) - dimming/shutting down the LCD backlight - turning off keyboard backlight and any keyboard LEDs Apart from muting the speaker, all of these operations are done in hardware, as the keyboard shortcut still works in the BIOS. Previous laptops with this feature appear to use a GPIO to mute the speaker, and we are informed that on those laptops userspace was not informed of the mute. Since CS35L41 does not have a GPIO mute, we had to use a different solution, involving ACPI notifications, which request the driver to mute. The same mechanism is used in Windows. Our understanding is that it is not intended for the mute to be overridden by userspace. Similarly, on previous laptops, userspace could not override this mute, since it was not informed of it. Thanks, Stefan
On Mon, 04 Sep 2023 15:47:49 +0200, Stefan Binding wrote: > > > On 04/09/2023 13:29, Takashi Iwai wrote: > > On Mon, 04 Sep 2023 14:00:20 +0200, > > Stefan Binding wrote: > >> > >> On 29/08/2023 15:23, Takashi Iwai wrote: > >>> On Tue, 29 Aug 2023 16:18:12 +0200, > >>> Stefan Binding wrote: > >>>> On 25/08/2023 13:13, Takashi Iwai wrote: > >>>>> On Fri, 25 Aug 2023 14:05:25 +0200, > >>>>> Stefan Binding wrote: > >>>>>> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >>>>>> > >>>>>> Some laptops require a hardware based mute system, where when a hotkey > >>>>>> is pressed, it forces the amp to be muted. > >>>>>> > >>>>>> For CS35L41, when the hotkey is pressed, an acpi notification is sent > >>>>>> to the CS35L41 Device Node. The driver needs to handle this notification > >>>>>> and call a _DSM function to retrieve the mute state. > >>>>>> > >>>>>> Since the amp is only muted during playback, the driver will only mute > >>>>>> or unmute if playback is occurring, otherwise it will save the mute > >>>>>> state for when playback starts. > >>>>>> > >>>>>> Only one handler can be registered for the acpi notification, but all > >>>>>> amps need to receive that notification, we can register a single handler > >>>>>> inside the Realtek HDA driver, so that it can then notify through the > >>>>>> component framework. > >>>>>> > >>>>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >>>>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > >>>>> We don't do normally in this way. The ACPI hot key handling is done > >>>>> via user-space, and user-space daemon triggers the mute of the > >>>>> system. > >>>>> > >>>>> Can't the ACPI notify the key event on those machines? > >>>> This feature is not the "normal" mute button on a keyboard, it is a > >>>> custom request > >>>> from a manufacturer which only mutes the audio on the speakers. > >>>> On previous generations, this was achieved using a GPIO controlled by > >>>> the BIOS/EC. > >>>> However, since CS35L41 does not have such GPIO, we must control it by > >>>> other means. > >>>> > >>>> Our solution, which we have to share with the Windows driver, it to use ACPI > >>>> notifications to tell the driver to mute the amps when the shortcut is > >>>> pressed. > >>>> > >>>> Does this seem like a valid exception to the typical approach? > >>> It's still the question whether we have to do this inevitably in the > >>> kernel in a way like that. It sounds quite unusual. Why this must be > >>> handled directly? IOW, what's the difference from the "normal" mute > >>> button? > >>> > >>> And, even if we take this approach, it leaves the device muted without > >>> exposing it to user-space. Then user wouldn't know what happens. > >>> > >>> > >>> thanks, > >>> > >>> Takashi > >> We spoke to the ODM for this system to get a more detailed explanation > >> of this feature. > >> The keyboard shortcut enables something called "Unobtrusive > >> Mode". According to their explanation: > >> > >> - Unobtrusive mode is distinct to normal mute, as it only mutes the speakers > >> - There is no requirement to update the volume controls, as the screen > >> backlight will be off anyway in this mode > >> - All other unobtrusive mode functions are enabled without user-space > >> dependencies, and they would prefer not to make speaker mute an > >> exception > > Thanks, it gives a bit better clue. > > The remaining question is rather the exact behavior of this > > "unobtrusive mode". How is it triggered, and what's the exact > > expectation? e.g. It must secretly mute the speaker? That is, it > > must not expose the mixer state change to user-space? Or is it tied > > with the normal mixer state and user may unmute again? > > > > > > Takashi > From what we understand, unobtrusive mode, which is activated by a > keyboard shortcut (not a single key), performs several operations, > such as: > - muting the speaker (headphones remain unmuted) > - dimming/shutting down the LCD backlight > - turning off keyboard backlight and any keyboard LEDs > Apart from muting the speaker, all of these operations are done in > hardware, as the keyboard shortcut still works in the BIOS. > Previous laptops with this feature appear to use a GPIO to mute the > speaker, and we are informed that on those laptops userspace was not > informed of the mute. > Since CS35L41 does not have a GPIO mute, we had to use a different > solution, involving ACPI notifications, which request the driver to > mute. > The same mechanism is used in Windows. > Our understanding is that it is not intended for the mute to be > overridden by userspace. > Similarly, on previous laptops, userspace could not override this > mute, since it was not informed of it. OK, thanks for explanation. I still don't like the idea to hide this completely, though. The mode should be somehow exposed even if the mute isn't controllable via mixer, but currently there is no indication at all. Takashi
On 04/09/2023 14:55, Takashi Iwai wrote: > On Mon, 04 Sep 2023 15:47:49 +0200, > Stefan Binding wrote: >> >> On 04/09/2023 13:29, Takashi Iwai wrote: >>> On Mon, 04 Sep 2023 14:00:20 +0200, >>> Stefan Binding wrote: >>>> On 29/08/2023 15:23, Takashi Iwai wrote: >>>>> On Tue, 29 Aug 2023 16:18:12 +0200, >>>>> Stefan Binding wrote: >>>>>> On 25/08/2023 13:13, Takashi Iwai wrote: >>>>>>> On Fri, 25 Aug 2023 14:05:25 +0200, >>>>>>> Stefan Binding wrote: >>>>>>>> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>>>>>> >>>>>>>> Some laptops require a hardware based mute system, where when a hotkey >>>>>>>> is pressed, it forces the amp to be muted. >>>>>>>> >>>>>>>> For CS35L41, when the hotkey is pressed, an acpi notification is sent >>>>>>>> to the CS35L41 Device Node. The driver needs to handle this notification >>>>>>>> and call a _DSM function to retrieve the mute state. >>>>>>>> >>>>>>>> Since the amp is only muted during playback, the driver will only mute >>>>>>>> or unmute if playback is occurring, otherwise it will save the mute >>>>>>>> state for when playback starts. >>>>>>>> >>>>>>>> Only one handler can be registered for the acpi notification, but all >>>>>>>> amps need to receive that notification, we can register a single handler >>>>>>>> inside the Realtek HDA driver, so that it can then notify through the >>>>>>>> component framework. >>>>>>>> >>>>>>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>>>>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> >>>>>>> We don't do normally in this way. The ACPI hot key handling is done >>>>>>> via user-space, and user-space daemon triggers the mute of the >>>>>>> system. >>>>>>> >>>>>>> Can't the ACPI notify the key event on those machines? >>>>>> This feature is not the "normal" mute button on a keyboard, it is a >>>>>> custom request >>>>>> from a manufacturer which only mutes the audio on the speakers. >>>>>> On previous generations, this was achieved using a GPIO controlled by >>>>>> the BIOS/EC. >>>>>> However, since CS35L41 does not have such GPIO, we must control it by >>>>>> other means. >>>>>> >>>>>> Our solution, which we have to share with the Windows driver, it to use ACPI >>>>>> notifications to tell the driver to mute the amps when the shortcut is >>>>>> pressed. >>>>>> >>>>>> Does this seem like a valid exception to the typical approach? >>>>> It's still the question whether we have to do this inevitably in the >>>>> kernel in a way like that. It sounds quite unusual. Why this must be >>>>> handled directly? IOW, what's the difference from the "normal" mute >>>>> button? >>>>> >>>>> And, even if we take this approach, it leaves the device muted without >>>>> exposing it to user-space. Then user wouldn't know what happens. >>>>> >>>>> >>>>> thanks, >>>>> >>>>> Takashi >>>> We spoke to the ODM for this system to get a more detailed explanation >>>> of this feature. >>>> The keyboard shortcut enables something called "Unobtrusive >>>> Mode". According to their explanation: >>>> >>>> - Unobtrusive mode is distinct to normal mute, as it only mutes the speakers >>>> - There is no requirement to update the volume controls, as the screen >>>> backlight will be off anyway in this mode >>>> - All other unobtrusive mode functions are enabled without user-space >>>> dependencies, and they would prefer not to make speaker mute an >>>> exception >>> Thanks, it gives a bit better clue. >>> The remaining question is rather the exact behavior of this >>> "unobtrusive mode". How is it triggered, and what's the exact >>> expectation? e.g. It must secretly mute the speaker? That is, it >>> must not expose the mixer state change to user-space? Or is it tied >>> with the normal mixer state and user may unmute again? >>> >>> >>> Takashi >> From what we understand, unobtrusive mode, which is activated by a >> keyboard shortcut (not a single key), performs several operations, >> such as: >> - muting the speaker (headphones remain unmuted) >> - dimming/shutting down the LCD backlight >> - turning off keyboard backlight and any keyboard LEDs >> Apart from muting the speaker, all of these operations are done in >> hardware, as the keyboard shortcut still works in the BIOS. >> Previous laptops with this feature appear to use a GPIO to mute the >> speaker, and we are informed that on those laptops userspace was not >> informed of the mute. >> Since CS35L41 does not have a GPIO mute, we had to use a different >> solution, involving ACPI notifications, which request the driver to >> mute. >> The same mechanism is used in Windows. >> Our understanding is that it is not intended for the mute to be >> overridden by userspace. >> Similarly, on previous laptops, userspace could not override this >> mute, since it was not informed of it. > OK, thanks for explanation. > > I still don't like the idea to hide this completely, though. The mode > should be somehow exposed even if the mute isn't controllable via > mixer, but currently there is no indication at all. > > > Takashi We could create and expose a read-only ALSA control which would display the mute status of the amp. This way its possible to see the status of the amp, without breaking the mechanism. Would this be acceptable? Thanks, Stefan
On Mon, 04 Sep 2023 16:05:59 +0200, Stefan Binding wrote: > > > On 04/09/2023 14:55, Takashi Iwai wrote: > > On Mon, 04 Sep 2023 15:47:49 +0200, > > Stefan Binding wrote: > >> > >> On 04/09/2023 13:29, Takashi Iwai wrote: > >>> On Mon, 04 Sep 2023 14:00:20 +0200, > >>> Stefan Binding wrote: > >>>> On 29/08/2023 15:23, Takashi Iwai wrote: > >>>>> On Tue, 29 Aug 2023 16:18:12 +0200, > >>>>> Stefan Binding wrote: > >>>>>> On 25/08/2023 13:13, Takashi Iwai wrote: > >>>>>>> On Fri, 25 Aug 2023 14:05:25 +0200, > >>>>>>> Stefan Binding wrote: > >>>>>>>> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >>>>>>>> > >>>>>>>> Some laptops require a hardware based mute system, where when a hotkey > >>>>>>>> is pressed, it forces the amp to be muted. > >>>>>>>> > >>>>>>>> For CS35L41, when the hotkey is pressed, an acpi notification is sent > >>>>>>>> to the CS35L41 Device Node. The driver needs to handle this notification > >>>>>>>> and call a _DSM function to retrieve the mute state. > >>>>>>>> > >>>>>>>> Since the amp is only muted during playback, the driver will only mute > >>>>>>>> or unmute if playback is occurring, otherwise it will save the mute > >>>>>>>> state for when playback starts. > >>>>>>>> > >>>>>>>> Only one handler can be registered for the acpi notification, but all > >>>>>>>> amps need to receive that notification, we can register a single handler > >>>>>>>> inside the Realtek HDA driver, so that it can then notify through the > >>>>>>>> component framework. > >>>>>>>> > >>>>>>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > >>>>>>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > >>>>>>> We don't do normally in this way. The ACPI hot key handling is done > >>>>>>> via user-space, and user-space daemon triggers the mute of the > >>>>>>> system. > >>>>>>> > >>>>>>> Can't the ACPI notify the key event on those machines? > >>>>>> This feature is not the "normal" mute button on a keyboard, it is a > >>>>>> custom request > >>>>>> from a manufacturer which only mutes the audio on the speakers. > >>>>>> On previous generations, this was achieved using a GPIO controlled by > >>>>>> the BIOS/EC. > >>>>>> However, since CS35L41 does not have such GPIO, we must control it by > >>>>>> other means. > >>>>>> > >>>>>> Our solution, which we have to share with the Windows driver, it to use ACPI > >>>>>> notifications to tell the driver to mute the amps when the shortcut is > >>>>>> pressed. > >>>>>> > >>>>>> Does this seem like a valid exception to the typical approach? > >>>>> It's still the question whether we have to do this inevitably in the > >>>>> kernel in a way like that. It sounds quite unusual. Why this must be > >>>>> handled directly? IOW, what's the difference from the "normal" mute > >>>>> button? > >>>>> > >>>>> And, even if we take this approach, it leaves the device muted without > >>>>> exposing it to user-space. Then user wouldn't know what happens. > >>>>> > >>>>> > >>>>> thanks, > >>>>> > >>>>> Takashi > >>>> We spoke to the ODM for this system to get a more detailed explanation > >>>> of this feature. > >>>> The keyboard shortcut enables something called "Unobtrusive > >>>> Mode". According to their explanation: > >>>> > >>>> - Unobtrusive mode is distinct to normal mute, as it only mutes the speakers > >>>> - There is no requirement to update the volume controls, as the screen > >>>> backlight will be off anyway in this mode > >>>> - All other unobtrusive mode functions are enabled without user-space > >>>> dependencies, and they would prefer not to make speaker mute an > >>>> exception > >>> Thanks, it gives a bit better clue. > >>> The remaining question is rather the exact behavior of this > >>> "unobtrusive mode". How is it triggered, and what's the exact > >>> expectation? e.g. It must secretly mute the speaker? That is, it > >>> must not expose the mixer state change to user-space? Or is it tied > >>> with the normal mixer state and user may unmute again? > >>> > >>> > >>> Takashi > >> From what we understand, unobtrusive mode, which is activated by a > >> keyboard shortcut (not a single key), performs several operations, > >> such as: > >> - muting the speaker (headphones remain unmuted) > >> - dimming/shutting down the LCD backlight > >> - turning off keyboard backlight and any keyboard LEDs > >> Apart from muting the speaker, all of these operations are done in > >> hardware, as the keyboard shortcut still works in the BIOS. > >> Previous laptops with this feature appear to use a GPIO to mute the > >> speaker, and we are informed that on those laptops userspace was not > >> informed of the mute. > >> Since CS35L41 does not have a GPIO mute, we had to use a different > >> solution, involving ACPI notifications, which request the driver to > >> mute. > >> The same mechanism is used in Windows. > >> Our understanding is that it is not intended for the mute to be > >> overridden by userspace. > >> Similarly, on previous laptops, userspace could not override this > >> mute, since it was not informed of it. > > OK, thanks for explanation. > > > > I still don't like the idea to hide this completely, though. The mode > > should be somehow exposed even if the mute isn't controllable via > > mixer, but currently there is no indication at all. > > > > > > Takashi > > We could create and expose a read-only ALSA control which would > display the mute status of the amp. > This way its possible to see the status of the amp, without breaking > the mechanism. > Would this be acceptable? Yeah, that's a compromise. BTW, the acpi notification handling is enabled for all devices? I don't see the conditional enablement. thanks, Takashi
On 04/09/2023 15:16, Takashi Iwai wrote: > On Mon, 04 Sep 2023 16:05:59 +0200, > Stefan Binding wrote: >> >> On 04/09/2023 14:55, Takashi Iwai wrote: >>> On Mon, 04 Sep 2023 15:47:49 +0200, >>> Stefan Binding wrote: >>>> On 04/09/2023 13:29, Takashi Iwai wrote: >>>>> On Mon, 04 Sep 2023 14:00:20 +0200, >>>>> Stefan Binding wrote: >>>>>> On 29/08/2023 15:23, Takashi Iwai wrote: >>>>>>> On Tue, 29 Aug 2023 16:18:12 +0200, >>>>>>> Stefan Binding wrote: >>>>>>>> On 25/08/2023 13:13, Takashi Iwai wrote: >>>>>>>>> On Fri, 25 Aug 2023 14:05:25 +0200, >>>>>>>>> Stefan Binding wrote: >>>>>>>>>> From: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>>>>>>>> >>>>>>>>>> Some laptops require a hardware based mute system, where when a hotkey >>>>>>>>>> is pressed, it forces the amp to be muted. >>>>>>>>>> >>>>>>>>>> For CS35L41, when the hotkey is pressed, an acpi notification is sent >>>>>>>>>> to the CS35L41 Device Node. The driver needs to handle this notification >>>>>>>>>> and call a _DSM function to retrieve the mute state. >>>>>>>>>> >>>>>>>>>> Since the amp is only muted during playback, the driver will only mute >>>>>>>>>> or unmute if playback is occurring, otherwise it will save the mute >>>>>>>>>> state for when playback starts. >>>>>>>>>> >>>>>>>>>> Only one handler can be registered for the acpi notification, but all >>>>>>>>>> amps need to receive that notification, we can register a single handler >>>>>>>>>> inside the Realtek HDA driver, so that it can then notify through the >>>>>>>>>> component framework. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> >>>>>>>>>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> >>>>>>>>> We don't do normally in this way. The ACPI hot key handling is done >>>>>>>>> via user-space, and user-space daemon triggers the mute of the >>>>>>>>> system. >>>>>>>>> >>>>>>>>> Can't the ACPI notify the key event on those machines? >>>>>>>> This feature is not the "normal" mute button on a keyboard, it is a >>>>>>>> custom request >>>>>>>> from a manufacturer which only mutes the audio on the speakers. >>>>>>>> On previous generations, this was achieved using a GPIO controlled by >>>>>>>> the BIOS/EC. >>>>>>>> However, since CS35L41 does not have such GPIO, we must control it by >>>>>>>> other means. >>>>>>>> >>>>>>>> Our solution, which we have to share with the Windows driver, it to use ACPI >>>>>>>> notifications to tell the driver to mute the amps when the shortcut is >>>>>>>> pressed. >>>>>>>> >>>>>>>> Does this seem like a valid exception to the typical approach? >>>>>>> It's still the question whether we have to do this inevitably in the >>>>>>> kernel in a way like that. It sounds quite unusual. Why this must be >>>>>>> handled directly? IOW, what's the difference from the "normal" mute >>>>>>> button? >>>>>>> >>>>>>> And, even if we take this approach, it leaves the device muted without >>>>>>> exposing it to user-space. Then user wouldn't know what happens. >>>>>>> >>>>>>> >>>>>>> thanks, >>>>>>> >>>>>>> Takashi >>>>>> We spoke to the ODM for this system to get a more detailed explanation >>>>>> of this feature. >>>>>> The keyboard shortcut enables something called "Unobtrusive >>>>>> Mode". According to their explanation: >>>>>> >>>>>> - Unobtrusive mode is distinct to normal mute, as it only mutes the speakers >>>>>> - There is no requirement to update the volume controls, as the screen >>>>>> backlight will be off anyway in this mode >>>>>> - All other unobtrusive mode functions are enabled without user-space >>>>>> dependencies, and they would prefer not to make speaker mute an >>>>>> exception >>>>> Thanks, it gives a bit better clue. >>>>> The remaining question is rather the exact behavior of this >>>>> "unobtrusive mode". How is it triggered, and what's the exact >>>>> expectation? e.g. It must secretly mute the speaker? That is, it >>>>> must not expose the mixer state change to user-space? Or is it tied >>>>> with the normal mixer state and user may unmute again? >>>>> >>>>> >>>>> Takashi >>>> From what we understand, unobtrusive mode, which is activated by a >>>> keyboard shortcut (not a single key), performs several operations, >>>> such as: >>>> - muting the speaker (headphones remain unmuted) >>>> - dimming/shutting down the LCD backlight >>>> - turning off keyboard backlight and any keyboard LEDs >>>> Apart from muting the speaker, all of these operations are done in >>>> hardware, as the keyboard shortcut still works in the BIOS. >>>> Previous laptops with this feature appear to use a GPIO to mute the >>>> speaker, and we are informed that on those laptops userspace was not >>>> informed of the mute. >>>> Since CS35L41 does not have a GPIO mute, we had to use a different >>>> solution, involving ACPI notifications, which request the driver to >>>> mute. >>>> The same mechanism is used in Windows. >>>> Our understanding is that it is not intended for the mute to be >>>> overridden by userspace. >>>> Similarly, on previous laptops, userspace could not override this >>>> mute, since it was not informed of it. >>> OK, thanks for explanation. >>> >>> I still don't like the idea to hide this completely, though. The mode >>> should be somehow exposed even if the mute isn't controllable via >>> mixer, but currently there is no indication at all. >>> >>> >>> Takashi >> We could create and expose a read-only ALSA control which would >> display the mute status of the amp. >> This way its possible to see the status of the amp, without breaking >> the mechanism. >> Would this be acceptable? > Yeah, that's a compromise. > > BTW, the acpi notification handling is enabled for all devices? I > don't see the conditional enablement. > > > thanks, > > Takashi Thanks, I re-do this patch and add the ALSA control. Whilst I dont think having the notification handler installed for all devices causes any issues, it is unneccesary for most models, so I'll add a conditional check for this. Thanks, Stefan
Hi Stefan, kernel test robot noticed the following build errors: [auto build test ERROR on tiwai-sound/for-next] [also build test ERROR on tiwai-sound/for-linus linus/master next-20230906] [cannot apply to v6.5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Stefan-Binding/ALSA-hda-cs35l41-Support-mute-notifications-for-CS35L41-HDA/20230825-200835 base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next patch link: https://lore.kernel.org/r/20230825120525.1337417-1-sbinding%40opensource.cirrus.com patch subject: [PATCH v1] ALSA: hda: cs35l41: Support mute notifications for CS35L41 HDA config: arm-defconfig (https://download.01.org/0day-ci/archive/20230906/202309062027.esqlhxcg-lkp@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230906/202309062027.esqlhxcg-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309062027.esqlhxcg-lkp@intel.com/ All errors (new ones prefixed by >>): sound/pci/hda/patch_realtek.c: In function 'comp_acpi_device_notify': >> sound/pci/hda/patch_realtek.c:6717:52: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_fwnode_handle'? [-Werror=implicit-function-declaration] 6717 | spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event, | ^~~~~~~~~~~~~~~~~~ | acpi_fwnode_handle sound/pci/hda/patch_realtek.c:6717:52: warning: passing argument 1 of 'spec->comps[i].acpi_notify' makes pointer from integer without a cast [-Wint-conversion] 6717 | spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | int sound/pci/hda/patch_realtek.c:6717:52: note: expected 'acpi_handle' {aka 'void *'} but argument is of type 'int' sound/pci/hda/patch_realtek.c: In function 'comp_bind': >> sound/pci/hda/patch_realtek.c:6737:47: error: invalid use of undefined type 'struct acpi_device' 6737 | ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, | ^~ sound/pci/hda/patch_realtek.c: In function 'comp_unbind': sound/pci/hda/patch_realtek.c:6760:46: error: invalid use of undefined type 'struct acpi_device' 6760 | ret = acpi_remove_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, | ^~ cc1: some warnings being treated as errors vim +6717 sound/pci/hda/patch_realtek.c 6706 6707 static void comp_acpi_device_notify(acpi_handle handle, u32 event, void *data) 6708 { 6709 struct hda_codec *cdc = data; 6710 struct alc_spec *spec = cdc->spec; 6711 int i; 6712 6713 codec_info(cdc, "ACPI Notification %d\n", event); 6714 6715 for (i = 0; i < HDA_MAX_COMPONENTS; i++) { 6716 if (spec->comps[i].dev && spec->comps[i].acpi_notify) > 6717 spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event, 6718 spec->comps[i].dev); 6719 } 6720 } 6721 6722 static int comp_bind(struct device *dev) 6723 { 6724 struct hda_codec *cdc = dev_to_hda_codec(dev); 6725 struct alc_spec *spec = cdc->spec; 6726 struct acpi_device *adev; 6727 int ret; 6728 6729 ret = component_bind_all(dev, spec->comps); 6730 if (ret) 6731 return ret; 6732 6733 adev = spec->comps[0].adev; 6734 if (!acpi_device_handle(adev)) 6735 return 0; 6736 > 6737 ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, 6738 comp_acpi_device_notify, cdc); 6739 if (ret < 0) { 6740 codec_warn(cdc, "Failed to install notify handler: %d\n", ret); 6741 return 0; 6742 } 6743 6744 codec_dbg(cdc, "Notify handler installed\n"); 6745 6746 return 0; 6747 } 6748
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index f9b77353c266..609e63b34d6d 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -33,6 +33,9 @@ #define CAL_AMBIENT_DSP_CTL_NAME "CAL_AMBIENT" #define CAL_DSP_CTL_TYPE 5 #define CAL_DSP_CTL_ALG 205 +#define CS35L41_UUID "50d90cdc-3de4-4f18-b528-c7fe3b71f40d" +#define CS35L41_DSM_GET_MUTE 5 +#define CS35L41_NOTIFY_EVENT 0x91 static bool firmware_autostart = 1; module_param(firmware_autostart, bool, 0444); @@ -520,6 +523,31 @@ static void cs35l41_hda_play_start(struct device *dev) } +static void cs35l41_mute(struct device *dev, bool mute) +{ + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + struct regmap *reg = cs35l41->regmap; + + dev_dbg(dev, "Mute(%d:%d) Playback Started: %d\n", mute, cs35l41->mute_override, + cs35l41->playback_started); + + if (cs35l41->playback_started) { + if (mute || cs35l41->mute_override) { + dev_dbg(dev, "Muting\n"); + regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); + } else { + dev_dbg(dev, "Unmuting\n"); + if (cs35l41->firmware_running) { + regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, + ARRAY_SIZE(cs35l41_hda_unmute_dsp)); + } else { + regmap_multi_reg_write(reg, cs35l41_hda_unmute, + ARRAY_SIZE(cs35l41_hda_unmute)); + } + } + } +} + static void cs35l41_hda_play_done(struct device *dev) { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); @@ -529,13 +557,7 @@ static void cs35l41_hda_play_done(struct device *dev) cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL, cs35l41->firmware_running); - if (cs35l41->firmware_running) { - regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, - ARRAY_SIZE(cs35l41_hda_unmute_dsp)); - } else { - regmap_multi_reg_write(reg, cs35l41_hda_unmute, - ARRAY_SIZE(cs35l41_hda_unmute)); - } + cs35l41_mute(dev, false); } static void cs35l41_hda_pause_start(struct device *dev) @@ -545,7 +567,7 @@ static void cs35l41_hda_pause_start(struct device *dev) dev_dbg(dev, "Pause (Start)\n"); - regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); + cs35l41_mute(dev, true); cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL, cs35l41->firmware_running); } @@ -1073,6 +1095,44 @@ static int cs35l41_create_controls(struct cs35l41_hda *cs35l41) return 0; } +static int cs35l41_get_acpi_mute_state(struct cs35l41_hda *cs35l41, acpi_handle handle) +{ + guid_t guid; + union acpi_object *ret; + int mute = -ENODEV; + + guid_parse(CS35L41_UUID, &guid); + + if (acpi_check_dsm(handle, &guid, 0, BIT(CS35L41_DSM_GET_MUTE))) { + ret = acpi_evaluate_dsm(handle, &guid, 0, CS35L41_DSM_GET_MUTE, NULL); + mute = *ret->buffer.pointer; + dev_dbg(cs35l41->dev, "CS35L41_DSM_GET_MUTE: %d\n", mute); + } + + dev_dbg(cs35l41->dev, "%s: %d\n", __func__, mute); + + return mute; +} + +static void cs35l41_acpi_device_notify(acpi_handle handle, u32 event, struct device *dev) +{ + struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + int mute; + + if (event != CS35L41_NOTIFY_EVENT) + return; + + mute = cs35l41_get_acpi_mute_state(cs35l41, handle); + if (mute < 0) { + dev_warn(cs35l41->dev, "Unable to retrieve mute state: %d\n", mute); + return; + } + + dev_dbg(cs35l41->dev, "Requesting mute value: %d\n", mute); + cs35l41->mute_override = (mute > 0); + cs35l41_mute(cs35l41->dev, cs35l41->mute_override); +} + static int cs35l41_hda_bind(struct device *dev, struct device *master, void *master_data) { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); @@ -1114,6 +1174,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas comps->playback_hook = cs35l41_hda_playback_hook; comps->pre_playback_hook = cs35l41_hda_pre_playback_hook; comps->post_playback_hook = cs35l41_hda_post_playback_hook; + comps->acpi_notify = cs35l41_acpi_device_notify; + comps->adev = cs35l41->dacpi; + + cs35l41->mute_override = cs35l41_get_acpi_mute_state(cs35l41, + acpi_device_handle(cs35l41->dacpi)) > 0; mutex_unlock(&cs35l41->fw_mutex); @@ -1387,8 +1452,8 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i return -ENODEV; } + cs35l41->dacpi = adev; physdev = get_device(acpi_get_first_physical_node(adev)); - acpi_dev_put(adev); sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev)); if (IS_ERR(sub)) @@ -1498,6 +1563,7 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i hw_cfg->valid = false; hw_cfg->gpio1.valid = false; hw_cfg->gpio2.valid = false; + acpi_dev_put(cs35l41->dacpi); put_physdev: put_device(physdev); @@ -1601,10 +1667,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i if (ret) goto err; - ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute, - ARRAY_SIZE(cs35l41_hda_mute)); - if (ret) - goto err; + cs35l41_mute(cs35l41->dev, true); INIT_WORK(&cs35l41->fw_load_work, cs35l41_fw_load_work); mutex_init(&cs35l41->fw_mutex); @@ -1641,6 +1704,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i if (cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type)) gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); gpiod_put(cs35l41->reset_gpio); + acpi_dev_put(cs35l41->dacpi); kfree(cs35l41->acpi_subsystem_id); return ret; @@ -1659,6 +1723,8 @@ void cs35l41_hda_remove(struct device *dev) component_del(cs35l41->dev, &cs35l41_hda_comp_ops); + acpi_dev_put(cs35l41->dacpi); + pm_runtime_put_noidle(cs35l41->dev); if (cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type)) diff --git a/sound/pci/hda/cs35l41_hda.h b/sound/pci/hda/cs35l41_hda.h index b93bf762976e..ce3f2bb6ffd0 100644 --- a/sound/pci/hda/cs35l41_hda.h +++ b/sound/pci/hda/cs35l41_hda.h @@ -10,6 +10,7 @@ #ifndef __CS35L41_HDA_H__ #define __CS35L41_HDA_H__ +#include <linux/acpi.h> #include <linux/efi.h> #include <linux/regulator/consumer.h> #include <linux/gpio/consumer.h> @@ -70,6 +71,8 @@ struct cs35l41_hda { bool halo_initialized; bool playback_started; struct cs_dsp cs_dsp; + struct acpi_device *dacpi; + bool mute_override; }; enum halo_state { diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h index f170aec967c1..c7a9b6a660e5 100644 --- a/sound/pci/hda/hda_component.h +++ b/sound/pci/hda/hda_component.h @@ -6,6 +6,7 @@ * Cirrus Logic International Semiconductor Ltd. */ +#include <linux/acpi.h> #include <linux/component.h> #define HDA_MAX_COMPONENTS 4 @@ -15,6 +16,8 @@ struct hda_component { struct device *dev; char name[HDA_MAX_NAME_SIZE]; struct hda_codec *codec; + struct acpi_device *adev; + void (*acpi_notify)(acpi_handle handle, u32 event, struct device *dev); void (*pre_playback_hook)(struct device *dev, int action); void (*playback_hook)(struct device *dev, int action); void (*post_playback_hook)(struct device *dev, int action); diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index a07df6f92960..fd3768e73c15 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -6704,19 +6704,65 @@ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec, } } +static void comp_acpi_device_notify(acpi_handle handle, u32 event, void *data) +{ + struct hda_codec *cdc = data; + struct alc_spec *spec = cdc->spec; + int i; + + codec_info(cdc, "ACPI Notification %d\n", event); + + for (i = 0; i < HDA_MAX_COMPONENTS; i++) { + if (spec->comps[i].dev && spec->comps[i].acpi_notify) + spec->comps[i].acpi_notify(acpi_device_handle(spec->comps[i].adev), event, + spec->comps[i].dev); + } +} + static int comp_bind(struct device *dev) { struct hda_codec *cdc = dev_to_hda_codec(dev); struct alc_spec *spec = cdc->spec; + struct acpi_device *adev; + int ret; + + ret = component_bind_all(dev, spec->comps); + if (ret) + return ret; - return component_bind_all(dev, spec->comps); + adev = spec->comps[0].adev; + if (!acpi_device_handle(adev)) + return 0; + + ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, + comp_acpi_device_notify, cdc); + if (ret < 0) { + codec_warn(cdc, "Failed to install notify handler: %d\n", ret); + return 0; + } + + codec_dbg(cdc, "Notify handler installed\n"); + + return 0; } static void comp_unbind(struct device *dev) { struct hda_codec *cdc = dev_to_hda_codec(dev); struct alc_spec *spec = cdc->spec; + struct acpi_device *adev; + int ret; + + adev = spec->comps[0].adev; + if (!acpi_device_handle(adev)) + goto unbind; + + ret = acpi_remove_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, + comp_acpi_device_notify); + if (ret < 0) + codec_warn(cdc, "Failed to uninstall notify handler: %d\n", ret); +unbind: component_unbind_all(dev, spec->comps); }