From: Perry Yuan <perry979106@gmail.com> To: Mark Brown <broonie@kernel.org>, Perry Yuan <Perry.Yuan@dell.com> Cc: oder_chiou@realtek.com, perex@perex.cz, tiwai@suse.com, hdegoede@redhat.com, mgross@linux.intel.com, lgirdwood@gmail.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org, Mario.Limonciello@dell.com Subject: Re: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports Date: Sun, 14 Feb 2021 14:43:10 +0800 [thread overview] Message-ID: <43507ba7-74b8-2d18-57ca-271f89a752de@gmail.com> (raw) In-Reply-To: <20210112175406.GF4646@sirena.org.uk> Hi Mark: Thanks for your review. On 2021/1/13 1:54, Mark Brown wrote: > On Wed, Jan 13, 2021 at 01:18:14AM +0800, Perry Yuan wrote: > >> Some new Dell system is going to support audio internal micphone >> privacy setting from hardware level with micmute led state changing >> When micmute hotkey pressed by user, soft mute will need to be enabled >> firstly in case of pop noise, and codec driver need to react to mic >> mute event to EC(embedded controller) notifying that SW mute is completed >> Then EC will do the hardware mute physically within the timeout reached > >> This patch allow codec rt715 driver to ack EC when micmute key pressed >> through this micphone led control interface like hda_generic provided >> ACPI method defined in dell-privacy micmute led trigger will be called >> for notifying the EC that software mute has been completed > > It feels like there's an abstraction problem here with this being hard > coded in a specific CODEC driver. > >> #include <linux/soundwire/sdw.h> >> @@ -244,6 +245,7 @@ static int rt715_sdca_get_volsw(struct snd_kcontrol *kcontrol, >> unsigned int max = mc->max; >> int val; >> >> + pr_err("++++++rt715_sdca_get_volsw++\n"); >> val = snd_soc_component_read(component, mc->reg); >> if (val < 0) >> return -EINVAL; > > This shouldn't be in the patch. Removed in V4, I forget to clear this debug code > >> @@ -287,6 +291,18 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol, >> return err; >> } >> >> +#if IS_ENABLED(CONFIG_DELL_PRIVACY) >> + /* dell privacy LED trigger state changed by muted/unmute switch */ >> + if (mc->invert) { >> + if (ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1]) { >> + rt715->micmute_led = LED_OFF; >> + } else { >> + rt715->micmute_led = LED_ON; >> + } >> + ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led); >> + } >> +#endif >> + > > This doesn't look good. There's nothing Dell specific here, and nothing > about this is conditional on any sort of runtime detection of Dell > systems, it's not obvious why this is conditional on DELL_PRIVACY or why > we only report the state if the control is inverted. > I will remove the CONFIG_DELL_PRIVACY from V4 patch and allow it to run if CONFIG_DELL_PRIVACY is not set, the result will be a no-op. > I'm also not convinced that it's a good idea to set the mute LED if only > one channel in a stereo microphone is muted, that seems likely to lead > to surprising behaviour for users. > https://github.com/thesofproject/linux/pull/2660#discussion_r555480210 There is a discussion for the channel mute changing behavior. If the anyone of value[0] or value[1] is 1, it means mic is NOT muted The muted state will be LED_ON state need to set. > TBH I don't understand why this isn't being done in generic code. > >> + bool micmute_led; > > What is this for, it never seems to be read except for in the function > where it's set? > I have moved this part code to the local definition of rt715_set_amp_gain_put and removed from rt715_priv. new code will be like this in V4. @@ -88,6 +89,7 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol, RT715_SET_GAIN_MIX_ADC2_L}; unsigned int addr_h, addr_l, val_h, val_ll, val_lr; unsigned int read_ll, read_rl, i, j, loop_cnt; + bool micmute_led;
WARNING: multiple messages have this Message-ID (diff)
From: Perry Yuan <perry979106@gmail.com> To: Mark Brown <broonie@kernel.org>, Perry Yuan <Perry.Yuan@dell.com> Cc: oder_chiou@realtek.com, alsa-devel@alsa-project.org, mgross@linux.intel.com, Mario.Limonciello@dell.com, linux-kernel@vger.kernel.org, lgirdwood@gmail.com, tiwai@suse.com, platform-driver-x86@vger.kernel.org, hdegoede@redhat.com Subject: Re: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports Date: Sun, 14 Feb 2021 14:43:10 +0800 [thread overview] Message-ID: <43507ba7-74b8-2d18-57ca-271f89a752de@gmail.com> (raw) In-Reply-To: <20210112175406.GF4646@sirena.org.uk> Hi Mark: Thanks for your review. On 2021/1/13 1:54, Mark Brown wrote: > On Wed, Jan 13, 2021 at 01:18:14AM +0800, Perry Yuan wrote: > >> Some new Dell system is going to support audio internal micphone >> privacy setting from hardware level with micmute led state changing >> When micmute hotkey pressed by user, soft mute will need to be enabled >> firstly in case of pop noise, and codec driver need to react to mic >> mute event to EC(embedded controller) notifying that SW mute is completed >> Then EC will do the hardware mute physically within the timeout reached > >> This patch allow codec rt715 driver to ack EC when micmute key pressed >> through this micphone led control interface like hda_generic provided >> ACPI method defined in dell-privacy micmute led trigger will be called >> for notifying the EC that software mute has been completed > > It feels like there's an abstraction problem here with this being hard > coded in a specific CODEC driver. > >> #include <linux/soundwire/sdw.h> >> @@ -244,6 +245,7 @@ static int rt715_sdca_get_volsw(struct snd_kcontrol *kcontrol, >> unsigned int max = mc->max; >> int val; >> >> + pr_err("++++++rt715_sdca_get_volsw++\n"); >> val = snd_soc_component_read(component, mc->reg); >> if (val < 0) >> return -EINVAL; > > This shouldn't be in the patch. Removed in V4, I forget to clear this debug code > >> @@ -287,6 +291,18 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol, >> return err; >> } >> >> +#if IS_ENABLED(CONFIG_DELL_PRIVACY) >> + /* dell privacy LED trigger state changed by muted/unmute switch */ >> + if (mc->invert) { >> + if (ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1]) { >> + rt715->micmute_led = LED_OFF; >> + } else { >> + rt715->micmute_led = LED_ON; >> + } >> + ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led); >> + } >> +#endif >> + > > This doesn't look good. There's nothing Dell specific here, and nothing > about this is conditional on any sort of runtime detection of Dell > systems, it's not obvious why this is conditional on DELL_PRIVACY or why > we only report the state if the control is inverted. > I will remove the CONFIG_DELL_PRIVACY from V4 patch and allow it to run if CONFIG_DELL_PRIVACY is not set, the result will be a no-op. > I'm also not convinced that it's a good idea to set the mute LED if only > one channel in a stereo microphone is muted, that seems likely to lead > to surprising behaviour for users. > https://github.com/thesofproject/linux/pull/2660#discussion_r555480210 There is a discussion for the channel mute changing behavior. If the anyone of value[0] or value[1] is 1, it means mic is NOT muted The muted state will be LED_ON state need to set. > TBH I don't understand why this isn't being done in generic code. > >> + bool micmute_led; > > What is this for, it never seems to be read except for in the function > where it's set? > I have moved this part code to the local definition of rt715_set_amp_gain_put and removed from rt715_priv. new code will be like this in V4. @@ -88,6 +89,7 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol, RT715_SET_GAIN_MIX_ADC2_L}; unsigned int addr_h, addr_l, val_h, val_ll, val_lr; unsigned int read_ll, read_rl, i, j, loop_cnt; + bool micmute_led;
next prev parent reply other threads:[~2021-02-14 6:44 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-12 17:18 [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports Perry Yuan 2021-01-12 17:18 ` Perry Yuan 2021-01-12 17:50 ` Limonciello, Mario 2021-01-12 17:50 ` Limonciello, Mario 2021-01-16 15:25 ` Perry Yuan 2021-01-16 15:25 ` Perry Yuan 2021-01-19 16:33 ` Limonciello, Mario 2021-01-19 16:33 ` Limonciello, Mario 2021-01-19 20:33 ` Hans de Goede 2021-01-19 20:33 ` Hans de Goede 2021-01-19 20:52 ` Takashi Iwai 2021-01-19 20:52 ` Takashi Iwai 2021-01-20 3:26 ` Yuan, Perry 2021-01-20 3:26 ` Yuan, Perry 2021-01-12 17:54 ` Mark Brown 2021-01-12 17:54 ` Mark Brown 2021-01-16 15:50 ` Perry Yuan 2021-01-16 15:50 ` Perry Yuan 2021-01-18 12:03 ` Mark Brown 2021-01-18 12:03 ` Mark Brown 2021-02-14 6:42 ` Perry Yuan 2021-02-14 6:42 ` Perry Yuan 2021-02-14 6:43 ` Perry Yuan [this message] 2021-02-14 6:43 ` Perry Yuan 2021-02-14 18:51 ` Jaroslav Kysela 2021-02-14 18:51 ` Jaroslav Kysela 2021-01-15 17:45 ` Jaroslav Kysela
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=43507ba7-74b8-2d18-57ca-271f89a752de@gmail.com \ --to=perry979106@gmail.com \ --cc=Mario.Limonciello@dell.com \ --cc=Perry.Yuan@dell.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=hdegoede@redhat.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mgross@linux.intel.com \ --cc=oder_chiou@realtek.com \ --cc=perex@perex.cz \ --cc=platform-driver-x86@vger.kernel.org \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.