RE: [PATCH v7 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi ,
> -Original Message-
> From: Amadeusz Sławiński
> Sent: 2021年4月12日 18:40
> To: Yuan, Perry; po...@protonmail.com; pierre-
> louis.boss...@linux.intel.com; oder_ch...@realtek.com; pe...@perex.cz;
> ti...@suse.com; hdego...@redhat.com; mgr...@linux.intel.com
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
> lgirdw...@gmail.com; platform-driver-...@vger.kernel.org;
> broo...@kernel.org; Dell Client Kernel; mario.limoncie...@outlook.com
> Subject: Re: [PATCH v7 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
> On 4/12/2021 11:19 AM, Perry Yuan wrote:
> > From: Perry Yuan
> >
>
> (...)
>
> > diff --git a/drivers/platform/x86/dell/dell-laptop.c
> > b/drivers/platform/x86/dell/dell-laptop.c
> > index 70edc5bb3a14..e7ffc0b81208 100644
> > --- a/drivers/platform/x86/dell/dell-laptop.c
> > +++ b/drivers/platform/x86/dell/dell-laptop.c
> > @@ -31,6 +31,8 @@
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
> >
> > +#include "dell-privacy-wmi.h"
> > +
> > struct quirk_entry {
> > bool touchpad_led;
> > bool kbd_led_not_present;
> > @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;
> > static struct rfkill *bluetooth_rfkill;
> > static struct rfkill *wwan_rfkill;
> > static bool force_rfkill;
> > +static bool has_privacy;
> >
> > module_param(force_rfkill, bool, 0444);
> > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted
> > models"); @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
> >
> > if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> > dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> > - micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > - ret = led_classdev_register(_device->dev,
> _led_cdev);
> > - if (ret < 0)
> > - goto fail_led;
> > + if (dell_privacy_present())
> > + has_privacy = true;
> > + else
> > + has_privacy = false;
>
> Bit, of nitpicking, but you can write above shorter:
> has_privacy = dell_privacy_present();
Good point, changed the code as you suggested.
Thank you.
Perry.
RE: [PATCH v6 2/2] ASoC: rt715:add micmute led state control supports
Hi Mark:
> -Original Message-
> From: Mark Brown
> Sent: 2021年4月7日 23:16
> To: Yuan, Perry
> Cc: po...@protonmail.com; pierre-louis.boss...@linux.intel.com;
> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario;
> lgirdw...@gmail.com; alsa-de...@alsa-project.org; linux-
> ker...@vger.kernel.org; platform-driver-...@vger.kernel.org;
> mario.limoncie...@outlook.com; Dell Client Kernel
> Subject: Re: [PATCH v6 2/2] ASoC: rt715:add micmute led state control
> supports
>
> On Sun, Apr 04, 2021 at 04:31:59PM +0800, Perry Yuan wrote:
>
> > +static bool micmute_led_set;
> > +static int dmi_matched(const struct dmi_system_id *dmi) {
> > + micmute_led_set = 1;
> > + return 1;
> > +}
>
> This isn't how we usually record DMI quirks, usually we'd query once on probe
> and store the data in the driver data struct - see other users for examples.
>
> > @@ -358,6 +388,7 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol
> *kcontrol,
> > unsigned int mask = (1 << fls(max)) - 1;
>
> > + dmi_check_system(micmute_led_dmi_table);
> > + if (invert && micmute_led_set) {
>
> This check for invert is odd and could probably use a comment.
Thanks for your review.
I add one short comments for the reason in V7 patch.
I tried not to use the invert , the LED state and mute state will be not
matching
So it has to use the invert to check if the mute state changed. Then notifiy
the led trigger driver to set the new led state.
Perry
RE: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Hans, Barnabás. Thanks for your review very much! I have no concern to change the most of the comments. Just one sysfs_emit replacement will cause problem , adding comments at below. > -Original Message- > From: Hans de Goede > Sent: Wednesday, April 7, 2021 11:16 PM > To: Barnabás Pőcze; Yuan, Perry > Cc: pierre-louis.boss...@linux.intel.com; oder_ch...@realtek.com; > pe...@perex.cz; ti...@suse.com; mgr...@linux.intel.com; Limonciello, > Mario; lgirdw...@gmail.com; broo...@kernel.org; alsa-devel@alsa- > project.org; linux-kernel@vger.kernel.org; platform-driver- > x...@vger.kernel.org; mario.limoncie...@outlook.com; Dell Client Kernel > Subject: Re: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell > hardware privacy > > > [EXTERNAL EMAIL] > > Hi Perry, Barnabás, > > Barnabás thank you for your review. I agree with all your remarks. > > Perry, Thank you for the new version, this version is looking good. > This is almost ready for merging. > > Please fix the review remarks from Barnabás. I also have a couple of small > review remarks myself below. > > On 4/4/21 6:53 PM, Barnabás Pőcze wrote: > > Hi > > > > > > 2021. április 4., vasárnap 10:31 keltezéssel, Perry Yuan írta: > > > >> From: Perry Yuan > >> > >> add support for Dell privacy driver for the Dell units equipped > >> hardware privacy design, which protect users privacy of audio and > >> camera from hardware level. Once the audio or camera privacy mode > >> activated, any applications will not get any audio or video stream > >> when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled, > >> micmute led will be also changed accordingly The micmute led is fully > >> controlled by hardware & EC(embedded controller) and camera mute > >> hotkey is Ctrl+F9. Currently design only emits SW_CAMERA_LENS_COVER > >> event while the camera lens shutter will be changed by EC & > >> HW(hardware) control > >> > >> *The flow is like this: > >> 1) User presses key. HW does stuff with this key (timeout timer is > >> started) > >> 2) WMI event is emitted from BIOS to kernel > >> 3) WMI event is received by dell-privacy > >> 4) KEY_MICMUTE emitted from dell-privacy > >> 5) Userland picks up key and modifies kcontrol for SW mute > >> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this: > >>ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? > LED_ON > >> :LED_OFF); > >> 7) If "LED" is set to on dell-privacy notifies EC, and timeout is > >> cancelled, > >>HW mic mute activated. If EC not notified, HW mic mute will also be > >>activated when timeout used up, it is just later than active ack > >> > >> Signed-off-by: Perry Yuan > >> --- > >> v5 -> v6: > >> * addressed feedback from Hans > >> * addressed feedback from Pierre > >> * optimize some debug format with dev_dbg() > >> * remove platform driver,combined privacy acpi driver into single wmi > >> driver file > >> * optimize sysfs interface with string added to be more clearly > >> reading > >> v4 -> v5: > >> * addressed feedback from Randy Dunlap > >> * addressed feedback from Pierre-Louis Bossart > >> * rebase to latest 5.12 rc4 upstream kernel > >> * fix some space alignment problem > >> v3 -> v4: > >> * fix format for Kconfig > >> * add sysfs document > >> * add flow comments to the privacy wmi/acpi driver > >> * addressed feedback from Barnabás Pőcze[Thanks very much] > >> * export privacy_valid to make the global state simpler to query > >> * fix one issue which will block the dell-laptop driver to load when > >> privacy driver invalid > >> * addressed feedback from Pierre-Louis Bossart,remove the EC ID match > >> v2 -> v3: > >> * add sysfs attributes doc > >> v1 -> v2: > >> * query EC handle from EC driver directly. > >> * fix some code style. > >> * add KEY_END to keymap array. > >> * clean platform device when cleanup called > >> * use hexadecimal format for log print in dev_dbg > >> * remove __set_bit for the report keys from probe. > >> * fix keymap leak > >> * add err_free_keymap in dell_privacy_wmi_probe > >> * wmi driver will be unregistered if privacy_acpi_init() fails > >> * add sysfs attribute files for user space query. > >> * add leds micmute driver to privacy acpi > >> * add
RE: [PATCH v6 2/2] ASoC: rt715:add micmute led state control supportspo...@protonmail.com
Hi Jaroslav:
> -Original Message-
> From: Yuan, Perry
> Sent: Sunday, April 4, 2021 4:32 PM
> To: po...@protonmail.com; pierre-louis.boss...@linux.intel.com;
> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario
> Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org;
> linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org; Yuan,
> Perry; mario.limoncie...@outlook.com; Dell Client Kernel
> Subject: [PATCH v6 2/2] ASoC: rt715:add micmute led state control supports
>
> From: Perry Yuan
>
> 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 and rt715 sdca driver to change the local
> micmute led state. Dell privacy led trigger driver will 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, then
> hardware audio circuit solution controlled by EC will switch the audio input
> source off/on
>
> Signed-off-by: Perry Yuan
>
>
> v5 -> v6:
> * addresed review comments from Jaroslav
> * add quirks for micmute led control as short term solution to control
> micmute led state change
> v4 -> v5:
> * rebase to latest 5.12 rc4 upstream kernel
> v3 -> v4:
> * remove unused debug log
> * remove compile flag of DELL privacy
> * move the micmute_led to local from rt715_priv
> * when Jaroslav upstream his gerneric LED trigger driver,I will rebase
> this patch,please consider merge this at first
> https://lore.kernel.org/alsa-devel/2021021400.1131020-1-
> pe...@perex.cz/
> v2 -> v3:
> * simplify the patch to reuse some val value
> * add more detail to the commit info
> v1 -> v2:
> * fix some format issue
>
> ---
> sound/soc/codecs/rt715-sdca.c | 41
> ++-
> sound/soc/codecs/rt715.c | 41
> +++
> 2 files changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/codecs/rt715-sdca.c b/sound/soc/codecs/rt715-sdca.c
> index 20528afbdc57..7bd7ad0ba7d7 100644
> --- a/sound/soc/codecs/rt715-sdca.c
> +++ b/sound/soc/codecs/rt715-sdca.c
> @@ -11,12 +11,14 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -344,6 +346,34 @@ static int rt715_sdca_get_volsw(struct snd_kcontrol
> *kcontrol,
> return 0;
> }
>
> +static bool micmute_led_set;
> +static int dmi_matched(const struct dmi_system_id *dmi) {
> + micmute_led_set = 1;
> + return 1;
> +}
> +
> +/* Some systems will need to use this to trigger mic mute LED state
> +changed */ static const struct dmi_system_id micmute_led_dmi_table[] = {
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Latitude 9420",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Latitude
> 9420"),
> + },
> + },
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Latitude 9520",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Latitude
> 9520"),
> + },
> + },
> + {},
> +};
> +
> static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
> struct snd_ctl_elem_value *ucontrol)
> {
> @@ -358,6 +388,7 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol
> *kcontrol,
> unsigned int mask = (1 << fls(max)) - 1;
> unsigned int invert = p->invert;
> int err;
> + bool micmute_led;
>
> for (i = 0; i < 4; i++) {
> if (ucontrol->value.integer.value[i] != rt715-
> >kctl_switch_orig[i]) { @@ -393,7 +424,15 @@ static int
> rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
> if (err < 0)
> return err;
> }
> -
RE: [PATCH v4 2/2] ASoC: rt715:add micmute led state control supports
Hi Hans,Jaroslav
> -Original Message-
> From: Hans de Goede
> Sent: 2021年3月25日 23:07
> To: Yuan, Perry; Jaroslav Kysela; Mark Brown; pierre-
> louis.boss...@linux.intel.com; Limonciello, Mario
> Cc: po...@protonmail.com; oder_ch...@realtek.com; ti...@suse.com;
> mgr...@linux.intel.com; lgirdw...@gmail.com; alsa-de...@alsa-project.org;
> linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 3/25/21 3:11 PM, Yuan, Perry wrote:
> > Hi Hans
> >
> >> -Original Message-
> >> From: Hans de Goede
> >> Sent: Monday, March 22, 2021 11:02 PM
> >> To: Jaroslav Kysela; Yuan, Perry; Mark Brown; pierre-
> >> louis.boss...@linux.intel.com; Limonciello, Mario
> >> Cc: po...@protonmail.com; oder_ch...@realtek.com; ti...@suse.com;
> >> mgr...@linux.intel.com; lgirdw...@gmail.com;
> >> alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
> >> platform-driver-...@vger.kernel.org
> >> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> >> supports
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> Hi,
> >>
> >> On 3/22/21 3:37 PM, Jaroslav Kysela wrote:
> >>> Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
> >>>> Hi Mark:
> >>>>
> >>>>> -Original Message-
> >>>>> From: Mark Brown
> >>>>> Sent: Tuesday, March 9, 2021 1:24 AM
> >>>>> To: Yuan, Perry
> >>>>> Cc: po...@protonmail.com; pierre-louis.boss...@linux.intel.com;
> >>>>> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> >>>>> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario;
> >>>>> lgirdw...@gmail.com; alsa-de...@alsa-project.org; linux-
> >>>>> ker...@vger.kernel.org; platform-driver-...@vger.kernel.org
> >>>>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state
> >>>>> control supports
> >>>>>
> >>>>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
> >>>>>
> >>>>>> + /* Micmute LED state changed by muted/unmute switch */
> >>>>>> + if (mc->invert) {
> >>>>>> + if (ucontrol->value.integer.value[0] || ucontrol-
> >>>>>> value.integer.value[1]) {
> >>>>>> + micmute_led = LED_OFF;
> >>>>>> + } else {
> >>>>>> + micmute_led = LED_ON;
> >>>>>> + }
> >>>>>> + ledtrig_audio_set(LED_AUDIO_MICMUTE,
> micmute_led);
> >>>>>> + }
> >>>>>
> >>>>> These conditionals on inversion seem weird and counterintuitive.
> >>>>> If we're going with this approach it would probably be clearer to
> >>>>> define a custom operation for the affected controls that wraps the
> >>>>> standard one and adds the LED setting rather than keying off
> >>>>> invert like
> >> this.
> >>>>
> >>>> Currently the sof soundwire driver has no generic led control yet.
> >>>> This patch can handle the led control needs for MIC mute LED,
> >>>> definitely
> >> the patch is a short term solution.
> >>>> There is a feature request discussion when we started to implement
> >>>> this
> >> solution.
> >>>> https://github.com/thesofproject/linux/issues/2496#issuecomment-
> >> 71389
> >>>> 2620
> >>>>
> >>>> The workable way for now is that we put the LED mute control to the
> >> codec driver.
> >>>> When there is new and full sound LED solution implemented, this
> >>>> part will
> >> be also optimized.
> >>>> The Hardware privacy feature needs this patch to handle the Mic
> >>>> mute led
> >> state change.
> >>>> Before that full solution ready in kernel, could we take this as
> >>>> short term
> >> solution?
> >>>
> >>> Perry, it's about the machine detection. Your code is too much
> >>> generic even for the top-level LED trigger implementation. We need
> >>> an extra check, if the proper LED's are re
RE: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Hans. Reviewing your feedback and working on next version below > -Original Message- > From: Hans de Goede > Sent: Wednesday, March 24, 2021 7:19 PM > To: Yuan, Perry; po...@protonmail.com; pierre- > louis.boss...@linux.intel.com; oder_ch...@realtek.com; pe...@perex.cz; > ti...@suse.com; mgr...@linux.intel.com; Limonciello, Mario > Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org; > linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org > Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell > hardware privacy > > > [EXTERNAL EMAIL] > > Hi, > > On 3/22/21 10:38 AM, Perry Yuan wrote: > > From: Perry Yuan > > > > add support for Dell privacy driver for the Dell units equipped > > hardware privacy design, which protect users privacy of audio and > > camera from hardware level. Once the audio or camera privacy mode > > activated, any applications will not get any audio or video stream > > when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled, > > micmute led will be also changed accordingly The micmute led is fully > > controlled by hardware & EC(embedded controller) and camera mute > > hotkey is Ctrl+F9. Currently design only emmits SW_CAMERA_LENS_COVER > > event while the camera lens shutter will be changed by EC & > > hw(hadware) control > > > > *The flow is like this: > > 1) User presses key. HW does stuff with this key (timeout timer is > > started) > > 2) WMI event is emitted from BIOS to kernel > > 3) WMI event is received by dell-privacy > > 4) KEY_MICMUTE emitted from dell-privacy > > 5) Userland picks up key and modifies kcontrol for SW mute > > 6) Codec kernel driver catches and calls ledtrig_audio_set, like this: > > ledtrig_audio_set(LED_AUDIO_MICMUTE, > > rt715->micmute_led ? LED_ON :LED_OFF); > > 7) If "LED" is set to on dell-privacy notifies EC, > > and timeout is cancelled, HW mic mute activated. > > > > Signed-off-by: Perry Yuan > > --- > > v4 -> v5: > > * addressed feedback from Randy Dunlap > > * addressed feedback from Pierre-Louis Bossart > > * rebase to latest 5.12 rc4 upstream kernel > > * fix some space alignment problem > > v3 -> v4: > > * fix format for Kconfig > > * add sysfs document > > * add flow comments to the privacy wmi/acpi driver > > * addressed feedback from Barnabás Pőcze[Thanks very much] > > * export privacy_valid to make the global state simpler to query > > * fix one issue which will block the dell-laptop driver to load when > > privacy driver invalid > > * addressed feedback from Pierre-Louis Bossart,remove the EC ID match > > v2 -> v3: > > * add sysfs attributes doc > > v1 -> v2: > > * query EC handle from EC driver directly. > > * fix some code style. > > * add KEY_END to keymap array. > > * clean platform device when cleanup called > > * use hexadecimal format for log print in dev_dbg > > * remove __set_bit for the report keys from probe. > > * fix keymap leak > > * add err_free_keymap in dell_privacy_wmi_probe > > * wmi driver will be unregistered if privacy_acpi_init() fails > > * add sysfs attribute files for user space query. > > * add leds micmute driver to privacy acpi > > * add more design info the commit info > > --- > > --- > > .../testing/sysfs-platform-dell-privacy-wmi | 32 ++ > > drivers/platform/x86/dell/Kconfig | 16 + > > drivers/platform/x86/dell/Makefile| 3 + > > drivers/platform/x86/dell/dell-laptop.c | 23 +- > > drivers/platform/x86/dell/dell-privacy-acpi.c | 164 + > > drivers/platform/x86/dell/dell-privacy-wmi.c | 340 ++ > > drivers/platform/x86/dell/dell-privacy-wmi.h | 35 ++ > > drivers/platform/x86/dell/dell-wmi.c | 27 +- > > 8 files changed, 627 insertions(+), 13 deletions(-) create mode > > 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > > create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c > > create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c > > create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h > > > > diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > > b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > > new file mode 100644 > > index ..20c15a51ec38 > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > > @@ -0,0 +1,32 @@ > > +What:
RE: [PATCH v4 2/2] ASoC: rt715:add micmute led state control supports
Hi Hans
> -Original Message-
> From: Hans de Goede
> Sent: Monday, March 22, 2021 11:02 PM
> To: Jaroslav Kysela; Yuan, Perry; Mark Brown; pierre-
> louis.boss...@linux.intel.com; Limonciello, Mario
> Cc: po...@protonmail.com; oder_ch...@realtek.com; ti...@suse.com;
> mgr...@linux.intel.com; lgirdw...@gmail.com; alsa-de...@alsa-project.org;
> linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 3/22/21 3:37 PM, Jaroslav Kysela wrote:
> > Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
> >> Hi Mark:
> >>
> >>> -Original Message-----
> >>> From: Mark Brown
> >>> Sent: Tuesday, March 9, 2021 1:24 AM
> >>> To: Yuan, Perry
> >>> Cc: po...@protonmail.com; pierre-louis.boss...@linux.intel.com;
> >>> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> >>> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario;
> >>> lgirdw...@gmail.com; alsa-de...@alsa-project.org; linux-
> >>> ker...@vger.kernel.org; platform-driver-...@vger.kernel.org
> >>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state
> >>> control supports
> >>>
> >>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
> >>>
> >>>> +/* Micmute LED state changed by muted/unmute switch */
> >>>> +if (mc->invert) {
> >>>> +if (ucontrol->value.integer.value[0] || ucontrol-
> >>>> value.integer.value[1]) {
> >>>> +micmute_led = LED_OFF;
> >>>> +} else {
> >>>> +micmute_led = LED_ON;
> >>>> +}
> >>>> +ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> >>>> +}
> >>>
> >>> These conditionals on inversion seem weird and counterintuitive. If
> >>> we're going with this approach it would probably be clearer to
> >>> define a custom operation for the affected controls that wraps the
> >>> standard one and adds the LED setting rather than keying off invert like
> this.
> >>
> >> Currently the sof soundwire driver has no generic led control yet.
> >> This patch can handle the led control needs for MIC mute LED, definitely
> the patch is a short term solution.
> >> There is a feature request discussion when we started to implement this
> solution.
> >> https://github.com/thesofproject/linux/issues/2496#issuecomment-
> 71389
> >> 2620
> >>
> >> The workable way for now is that we put the LED mute control to the
> codec driver.
> >> When there is new and full sound LED solution implemented, this part will
> be also optimized.
> >> The Hardware privacy feature needs this patch to handle the Mic mute led
> state change.
> >> Before that full solution ready in kernel, could we take this as short term
> solution?
> >
> > Perry, it's about the machine detection. Your code is too much generic
> > even for the top-level LED trigger implementation. We need an extra
> > check, if the proper LED's are really controlled on the specific
> > hardware. Other hardware may use RT715 for a different purpose. Use
> > DMI / ACPI checks to detect this hardware and don't misuse the inversion
> flag to enable this code.
>
> I think this would be a goo candidate for the new generic LED handling:
>
> https://lore.kernel.org/alsa-devel/20210317172945.842280-1-
> pe...@perex.cz/
>
> And then use a udev-rule + hwdb and/or UCM profiles to configure the LED
> trigger for specific models from userspace ?
>
> Regards,
>
> Hans
>
>
>
Because the SOF SDW design has no mic mute led control yet.
So I add one short term solution to make Dell privacy working for now
Definitely , that is new solution I can add my patch on that to test as one
user case .
We really need to take the short term solution work for some system which
support new SOF soundwire hardware which have dependence on the MIC mute
feature
Meanwhile we can continue working on the new "udev-rule + hwdb and/or UCM
profiles" solution as to replace this one.
If we agree that, I will submit another V6 addressing new feedback.
Perry
RE: [PATCH v4 2/2] ASoC: rt715:add micmute led state control supports
Hi Jaroslav:
> -Original Message-
> From: Jaroslav Kysela
> Sent: Monday, March 22, 2021 10:38 PM
> To: Yuan, Perry; Mark Brown; pierre-louis.boss...@linux.intel.com;
> Limonciello, Mario; hdego...@redhat.com
> Cc: po...@protonmail.com; oder_ch...@realtek.com; ti...@suse.com;
> mgr...@linux.intel.com; lgirdw...@gmail.com; alsa-devel@alsa-
> project.org; linux-kernel@vger.kernel.org; platform-driver-
> x...@vger.kernel.org
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
>
>
> [EXTERNAL EMAIL]
>
> Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
> > Hi Mark:
> >
> >> -Original Message-
> >> From: Mark Brown
> >> Sent: Tuesday, March 9, 2021 1:24 AM
> >> To: Yuan, Perry
> >> Cc: po...@protonmail.com; pierre-louis.boss...@linux.intel.com;
> >> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> >> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario;
> >> lgirdw...@gmail.com; alsa-de...@alsa-project.org; linux-
> >> ker...@vger.kernel.org; platform-driver-...@vger.kernel.org
> >> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> >> supports
> >>
> >> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
> >>
> >>> + /* Micmute LED state changed by muted/unmute switch */
> >>> + if (mc->invert) {
> >>> + if (ucontrol->value.integer.value[0] || ucontrol-
> >>> value.integer.value[1]) {
> >>> + micmute_led = LED_OFF;
> >>> + } else {
> >>> + micmute_led = LED_ON;
> >>> + }
> >>> + ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> >>> + }
> >>
> >> These conditionals on inversion seem weird and counterintuitive. If
> >> we're going with this approach it would probably be clearer to define
> >> a custom operation for the affected controls that wraps the standard
> >> one and adds the LED setting rather than keying off invert like this.
> >
> > Currently the sof soundwire driver has no generic led control yet.
> > This patch can handle the led control needs for MIC mute LED, definitely
> the patch is a short term solution.
> > There is a feature request discussion when we started to implement this
> solution.
> > https://github.com/thesofproject/linux/issues/2496#issuecomment-
> 713892
> > 620
> >
> > The workable way for now is that we put the LED mute control to the
> codec driver.
> > When there is new and full sound LED solution implemented, this part
> will be also optimized.
> > The Hardware privacy feature needs this patch to handle the Mic mute
> led state change.
> > Before that full solution ready in kernel, could we take this as short term
> solution?
>
> Perry, it's about the machine detection. Your code is too much generic even
> for the top-level LED trigger implementation. We need an extra check, if the
> proper LED's are really controlled on the specific hardware. Other hardware
> may use RT715 for a different purpose. Use DMI / ACPI checks to detect this
> hardware and don't misuse the inversion flag to enable this code.
>
> Jaroslav
>
> --
> Jaroslav Kysela
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
In the V2 patch, I have added the machine detection, but some guys thought that
I should remove the detection for it is harmless to other system
So I remove it in the following patches.
Is it Ok for you if I add below detection of Dell system which enable the
privacy feature ?
Then the mute led control will be called normally and Mic mute will be
successfully configured.
There is no any impaction to other systems.
+#if IS_ENABLED(CONFIG_DELL_PRIVACY)
.
+#endif
RE: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Hans.
> -Original Message-
> From: Hans de Goede
> Sent: Wednesday, March 24, 2021 3:40 AM
> To: Pierre-Louis Bossart; Yuan, Perry; po...@protonmail.com;
> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> mgr...@linux.intel.com; Limonciello, Mario
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
> lgirdw...@gmail.com; platform-driver-...@vger.kernel.org;
> broo...@kernel.org
> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
> > Minor comments below.
>
>
> >> +int __init dell_privacy_acpi_init(void)
> >
> > is the __init necessary? You call this routine from another which already
> > has
> this qualifier.
>
> Yes this is necessary, all functions which are only used during module_load /
> from the module's init function must be marked as __init so that the kernel
> can unload them after module loading is done.
>
> I do wonder if this one should not be static though (I've not looked at this
> patch in detail yet).
>
> >
> >> +{
> >> + int err;
> >> + struct platform_device *pdev;
> >> +
> >> + if (!wmi_has_guid(DELL_PRIVACY_GUID))
> >> + return -ENODEV;
> >> +
> >> + privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
> >> + if (!privacy_acpi)
> >> + return -ENOMEM;
> >> +
> >> + err = platform_driver_register(_privacy_platform_drv);
> >> + if (err)
> >> + goto pdrv_err;
> >> +
> >> + pdev = platform_device_register_simple(
> >> + PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
> >> + if (IS_ERR(pdev)) {
> >> + err = PTR_ERR(pdev);
> >> + goto pdev_err;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +pdev_err:
> >> + platform_device_unregister(pdev);
> >> +pdrv_err:
> >> + kfree(privacy_acpi);
> >> + return err;
> >> +}
> >> +
> >> +void __exit dell_privacy_acpi_exit(void)
> >
> > is the __exit required here?
>
> Idem. Also static ?
>
> Regards,
>
> Hans
>
If adding static to the function, I will be worried about that the init and
exit cannot be called by another file .
>
>
> >
> >> +{
> >> + struct platform_device *pdev =
> >> +to_platform_device(privacy_acpi->dev);
> >> +
> >> + platform_device_unregister(pdev);
> >> + platform_driver_unregister(_privacy_platform_drv);
> >> + kfree(privacy_acpi);
> >> +}
> >> +
> >> +MODULE_AUTHOR("Perry Yuan ");
> >> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> >> +MODULE_LICENSE("GPL");
> >
> >
RE: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Pierre：
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: Monday, March 22, 2021 10:50 PM
> To: Perry Yuan; Yuan, Perry; po...@protonmail.com;
> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario
> Cc: linux-kernel@vger.kernel.org; alsa-de...@alsa-project.org;
> broo...@kernel.org; lgirdw...@gmail.com; platform-driver-
> x...@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
>
> > As you suggested,I should add the alignment change in another patch.
> > But if i keep the old alignment, the code will be very odd.
> > Seems like that I have to change the below code to new alignment in
> > this patch.
> >
> > if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> > dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) { <<--- changed
> > back
> > if (!privacy_valid)
> > has_privacy = true;
> > else
> > has_privacy = false;
> > if (!has_privacy) {
> > micmute_led_cdev.brightness <<--- new alignment
> > ...
> > }
> > ...
> > }
>
> I don't get the point, sorry. The code above doesn't seem properly indented
> or there were spurious tab/spaces conversions?
Could you help to take a look the V5 patch ?
I recovery some part of original code alignment and add my new codes with new
Tabs added
Thank you !
Perry
RE: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Randy. > -Original Message- > From: Randy Dunlap > Sent: Tuesday, March 23, 2021 1:15 AM > To: Yuan, Perry; po...@protonmail.com; pierre- > louis.boss...@linux.intel.com; oder_ch...@realtek.com; pe...@perex.cz; > ti...@suse.com; hdego...@redhat.com; mgr...@linux.intel.com; > Limonciello, Mario > Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org; > linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org > Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell > hardware privacy > > > [EXTERNAL EMAIL] > > On 3/22/21 2:38 AM, Perry Yuan wrote: > > From: Perry Yuan > > > > > diff --git a/drivers/platform/x86/dell/Kconfig > > b/drivers/platform/x86/dell/Kconfig > > index e0a55337f51a..0e0f1eb35bd6 100644 > > --- a/drivers/platform/x86/dell/Kconfig > > +++ b/drivers/platform/x86/dell/Kconfig > > @@ -204,4 +204,20 @@ config DELL_WMI_SYSMAN > > To compile this driver as a module, choose M here: the module will > > be called dell-wmi-sysman. > > > > +config DELL_PRIVACY > > + tristate "Dell Hardware Privacy Support" > > + depends on ACPI > > + depends on ACPI_WMI > > + depends on INPUT > > + depends on DELL_LAPTOP > > + depends on LEDS_TRIGGER_AUDIO > > + select DELL_WMI > > + help > > + This driver provides support for the "Dell Hardware Privacy" feature > > + of Dell laptops. > > + Support for a micmute and camera mute privacy will be provided as > > better: > are provided as > > > + hardware button Ctrl+F4 and Ctrl+F9 hotkey. > > Does that say that Ctrl+F4 is a hardware button and that Ctrl+F9 is a hotkey? > If so, what's the difference? and why? Are they different hardware > implementations? Does the user care? They all use the Ctrl button, Ctrl +F4 requires that the Ctrl button and F4 button are pressed at the same time. F9+ctrl also have the same meaning. They are belonging to same hardware privacy solution, just use different keys combination to activate different privacy function . > > > + > > + To compile this driver as a module, choose M here: the module will > > + be called dell_privacy. > > endif # X86_PLATFORM_DRIVERS_DELL > thanks. > -- > ~Randy
RE: [PATCH v4 2/2] ASoC: rt715:add micmute led state control supports
Hi Mark:
> -Original Message-
> From: Mark Brown
> Sent: Tuesday, March 9, 2021 1:24 AM
> To: Yuan, Perry
> Cc: po...@protonmail.com; pierre-louis.boss...@linux.intel.com;
> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario;
> lgirdw...@gmail.com; alsa-de...@alsa-project.org; linux-
> ker...@vger.kernel.org; platform-driver-...@vger.kernel.org
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
>
> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
>
> > + /* Micmute LED state changed by muted/unmute switch */
> > + if (mc->invert) {
> > + if (ucontrol->value.integer.value[0] || ucontrol-
> >value.integer.value[1]) {
> > + micmute_led = LED_OFF;
> > + } else {
> > + micmute_led = LED_ON;
> > + }
> > + ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> > + }
>
> These conditionals on inversion seem weird and counterintuitive. If we're
> going with this approach it would probably be clearer to define a custom
> operation for the affected controls that wraps the standard one and adds the
> LED setting rather than keying off invert like this.
Currently the sof soundwire driver has no generic led control yet.
This patch can handle the led control needs for MIC mute LED, definitely the
patch is a short term solution.
There is a feature request discussion when we started to implement this
solution.
https://github.com/thesofproject/linux/issues/2496#issuecomment-713892620
The workable way for now is that we put the LED mute control to the codec
driver.
When there is new and full sound LED solution implemented, this part will be
also optimized.
The Hardware privacy feature needs this patch to handle the Mic mute led state
change.
Before that full solution ready in kernel, could we take this as short term
solution?
Perry
RE: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hello Pierre:
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: Monday, March 1, 2021 10:28 PM
> To: Yuan, Perry; po...@protonmail.com; oder_ch...@realtek.com;
> pe...@perex.cz; ti...@suse.com; hdego...@redhat.com;
> mgr...@linux.intel.com; Limonciello, Mario
> Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org;
> linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
>
> > diff --git a/drivers/platform/x86/Makefile
> > b/drivers/platform/x86/Makefile index 581475f59819..18c430456de7
> > 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -51,7 +51,9 @@ obj-$(CONFIG_DELL_WMI_DESCRIPTOR) +=
> dell-wmi-descriptor.o
> > obj-$(CONFIG_DELL_WMI_AIO)+= dell-wmi-aio.o
> > obj-$(CONFIG_DELL_WMI_LED)+= dell-wmi-led.o
> > obj-$(CONFIG_DELL_WMI_SYSMAN) += dell-wmi-sysman/
> > -
> > +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o
> > +dell-privacy-objs := dell-privacy-wmi.o \
> > + dell-privacy-acpi.o
> > # Fujitsu
> > obj-$(CONFIG_AMILO_RFKILL)+= amilo-rfkill.o
> > obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
> > diff --git a/drivers/platform/x86/dell-laptop.c
> > b/drivers/platform/x86/dell-laptop.c
> > index 70edc5bb3a14..ec0dcc7fc17c 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -31,6 +31,8 @@
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
> >
> > +#include "dell-privacy-wmi.h"
> > +
> > struct quirk_entry {
> > bool touchpad_led;
> > bool kbd_led_not_present;
> > @@ -90,10 +92,12 @@ static struct rfkill *wifi_rfkill;
> > static struct rfkill *bluetooth_rfkill;
> > static struct rfkill *wwan_rfkill;
> > static bool force_rfkill;
> > +static bool has_privacy;
> >
> > module_param(force_rfkill, bool, 0444);
> > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted
> > models");
> >
> > +
>
> spurious line change
I just want to make them separate with more space .
If it cause concern, I will remote the line in V5.
>
> > static const struct dmi_system_id dell_device_table[] __initconst = {
> > {
> > .ident = "Dell laptop",
> > @@ -2205,11 +2209,17 @@ static int __init dell_init(void)
> > dell_laptop_register_notifier(_laptop_notifier);
> >
> > if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> > - dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> > - micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > - ret = led_classdev_register(_device->dev,
> _led_cdev);
> > - if (ret < 0)
> > - goto fail_led;
> > +
> dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
>
> not sure why you changed the alignment?
The previous alignment is a little not correct.
So I adjust it
If it cause concern, will restore it to original shape.
>
> > + if (!privacy_valid)
> > + has_privacy = true;
> > + else
> > + has_privacy = false;
> > + if (!has_privacy) {
> > + micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > + ret = led_classdev_register(_device->dev,
> _led_cdev);
> > + if (ret < 0)
> > + goto fail_led;
> > + }
> > }
> >
> > if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>
> > +static struct platform_driver dell_privacy_platform_drv = {
> > + .driver = {
> > + .name = PRIVACY_PLATFORM_NAME,
> > + },
> > + .probe = dell_privacy_acpi_probe,
> > + .remove = dell_privacy_acpi_remove,
> > +};
> > +
> > +int __init dell_privacy_acpi_init(void) {
> > + int err;
> > + struct platform_device *pdev;
> > +
> > + if (!wmi_has_guid(DELL_PRIVACY_GUID))
> > + return -ENODEV;
> > +
> > + privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
> > + if (!privacy_acpi)
> > + return -ENOMEM;
> > +
> > + err = platform_driver_register(_privacy_platform_drv);
> &g
RE: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Pierre
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: 2021年2月17日 22:24
> To: Perry Yuan; Yuan, Perry; oder_ch...@realtek.com; pe...@perex.cz;
> ti...@suse.com; hdego...@redhat.com; mgr...@linux.intel.com
> Cc: alsa-de...@alsa-project.org; Limonciello, Mario; linux-
> ker...@vger.kernel.org; lgirdw...@gmail.com; platform-driver-
> x...@vger.kernel.org; broo...@kernel.org
> Subject: Re: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
>
>
> On 2/17/21 6:47 AM, Perry Yuan wrote:
> > Hi Pierre:
> > On 2021/2/16 22:56, Pierre-Louis Bossart wrote:
> >>
> >>>>> +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> >>>>> + {"PNP0C09", 0},
> >>>>> + { },
> >>>>> +};
> >>>>> +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
> >>>>> +
> >>>>> +static struct platform_driver dell_privacy_platform_drv = {
> >>>>> + .driver = {
> >>>>> + .name = PRIVACY_PLATFORM_NAME,
> >>>>> + .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
> >>>>> + },
> >>>>
> >>>> no .probe?
> >>> Originally i added the probe here, but it cause the driver .probe
> >>> called twice. after i use platform_driver_probe to register the
> >>> driver loading process, the duplicated probe issue resolved.
> >>>
> >>> I
> >>>>
> >>>>> + .remove = dell_privacy_acpi_remove, };
> >>>>> +
> >>>>> +int __init dell_privacy_acpi_init(void) {
> >>>>> + int err;
> >>>>> + struct platform_device *pdev;
> >>>>> + int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
> >>>>> +
> >>>>> + if (!wmi_has_guid(DELL_PRIVACY_GUID))
> >>>>> + return -ENODEV;
> >>>>> +
> >>>>> + privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
> >>>>> + if (!privacy_acpi)
> >>>>> + return -ENOMEM;
> >>>>> +
> >>>>> + pdev = platform_device_register_simple(
> >>>>> + PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL,
> 0);
> >>>>> + if (IS_ERR(pdev)) {
> >>>>> + err = PTR_ERR(pdev);
> >>>>> + goto pdev_err;
> >>>>> + }
> >>>>> + err = platform_driver_probe(_privacy_platform_drv,
> >>>>> + dell_privacy_acpi_probe);
> >>>>> + if (err)
> >>>>> + goto pdrv_err;
> >>>>
> >>>> why is the probe done here? Put differently, what prevents you from
> >>>> using a 'normal' platform driver, and do the leds_setup in the
> >>>> .probe()?
> >>> At first ,I used the normal platform driver framework, however tt
> >>> cause the driver .probe called twice. after i use
> >>> platform_driver_probe to register the driver loading process, the
> >>> duplicated probe issue resolved.
> >>
> >> This sounds very odd...
> >>
> >> this looks like a conflict with the ACPI subsystem finding a device
> >> and probing the driver that's associated with the PNP0C09 HID, and
> >> then this module __init creating a device manually which leads to a
> >> second probe
> >>
> >> Neither the platform_device_register_simple() nor
> >> platform_driver_probe() seem necessary?Because this privacy acpi
> >> driver file has dependency on the ec handle,
> > so i want to determine if the driver can be loaded basing on the EC ID
> > PNP0C09 matching.
> >
> > So far,It works well for me to register the privacy driver with the
> > register sequence.
> > Dose it hurt to keep current registering process with
> > platform_driver_probe used?
>
> Sorry, I don't understand why you need to list PNP0C09 HID in a matching
> table if it's not used to probe anything.
>
> The purpose of those matching tables is that when this HID is found, the core
> will invoke the probe callback with no need for any manual intervention.
>
> If you want to do things manually with the module init, that's fine, it's the
> combination of the two that I find questionable.
>
> It's like having both a manual and automatic transmission in a car, with the
> automatic transmission not coupled to the wheels.
I understand your point,I have removed the PNP0C09 ID from V4 patch.
Thanks for your suggestion very much !
Perry
RE: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer dereference
> -Original Message-
> From: Limonciello, Mario
> Sent: 2021年1月30日 1:29
> To: Hans De Goede; Mark Gross
> Cc: LKML; platform-driver-...@vger.kernel.org; Bharathi, Divya; Ksr, Prasanth;
> Yuan, Perry
> Subject: RE: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer
> dereference
>
> +Divya, +Prasanth, +Perry
>
> > -Original Message-
> > From: Limonciello, Mario
> > Sent: Friday, January 29, 2021 11:27
> > To: Hans De Goede; Mark Gross
> > Cc: LKML; platform-driver-...@vger.kernel.org; Limonciello, Mario
> > Subject: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer
> > dereference
> >
> > An upcoming Dell platform is causing a NULL pointer dereference in
> > dell-wmi-sysman initialization. Validate that the input from BIOS
> > matches correct ACPI types and abort module initialization if it
> > fails.
> >
> > This leads to a memory leak that needs to be cleaned up properly.
> >
> > Signed-off-by: Mario Limonciello
> > ---
> > drivers/platform/x86/dell-wmi-sysman/sysman.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/dell-wmi-sysman/sysman.c
> > b/drivers/platform/x86/dell-wmi-sysman/sysman.c
> > index dc6dd531c996..38b497991071 100644
> > --- a/drivers/platform/x86/dell-wmi-sysman/sysman.c
> > +++ b/drivers/platform/x86/dell-wmi-sysman/sysman.c
> > @@ -419,13 +419,19 @@ static int init_bios_attributes(int attr_type,
> > const char *guid)
> > return retval;
> > /* need to use specific instance_id and guid combination to get
> > right data */
> > obj = get_wmiobj_pointer(instance_id, guid);
> > - if (!obj)
> > + if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
> > + release_attributes_data();
> > return -ENODEV;
> > + }
> > elements = obj->package.elements;
> >
> > mutex_lock(_priv.mutex);
> > while (elements) {
> > /* sanity checking */
> > + if (elements[ATTR_NAME].type != ACPI_TYPE_STRING) {
> > + pr_debug("incorrect element type\n");
> > + goto nextobj;
> > + }
> > if (strlen(elements[ATTR_NAME].string.pointer) == 0) {
> > pr_debug("empty attribute found\n");
> > goto nextobj;
> > --
> > 2.25.1
Tested on the Dell laptop system which I found the issue.
Kernel can boot up with this new fix patch without any panic triggered.
Tested-by: Perry Yuan
RE: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports
> -Original Message-
> From: Limonciello, Mario
> Sent: Wednesday, January 20, 2021 12:33 AM
> To: Perry Yuan; Yuan, Perry; oder_ch...@realtek.com; pe...@perex.cz;
> ti...@suse.com; hdego...@redhat.com; mgr...@linux.intel.com
> Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org;
> linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org
> Subject: RE: [PATCH v3 3/3] ASoC: rt715:add micmute led state control
> supports
>
> > >> -----Original Message-
> > >> From: Yuan, Perry
> > >> Sent: Tuesday, January 12, 2021 11:18
> > >> To: oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> > >> hdego...@redhat.com; mgr...@linux.intel.com
> > >> Cc: lgirdw...@gmail.com; broo...@kernel.org;
> > >> alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
> > >> platform-driver-...@vger.kernel.org; Yuan, Perry; Limonciello,
> > >> Mario
> > >> Subject: [PATCH v3 3/3] ASoC: rt715:add micmute led state control
> > >> supports
> > >>
> > >> From: Perry Yuan
> > >>
> > >> 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
> > >>
> > >> Signed-off-by: Perry Yuan
> > >>
> > >>
> > >> v2 -> v3
> > >> * simplify the patch to reuse some val value
> > >> * add more detail to the commit info
> > >>
> > >> v1 -> v2:
> > >> * fix some format issue
> > >>
> > >> ---
> > >> sound/soc/codecs/rt715-sdca.c | 16
> > >> sound/soc/codecs/rt715-sdca.h | 1 +
> > >> sound/soc/codecs/rt715.c | 14 ++
> > >> sound/soc/codecs/rt715.h | 1 +
> > >> 4 files changed, 32 insertions(+)
> > >>
> > >> diff --git a/sound/soc/codecs/rt715-sdca.c
> > >> b/sound/soc/codecs/rt715-sdca.c index b43ac8559e45..861a0d2a8957
> > >> 100644
> > >> --- a/sound/soc/codecs/rt715-sdca.c
> > >> +++ b/sound/soc/codecs/rt715-sdca.c
> > >> @@ -12,6 +12,7 @@
> > >> #include
> > >> #include
> > >> #include
> > >> +#include
> > >> #include
> > >> #include
> > >> #include
> > >> @@ -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;
> > >> @@ -261,6 +263,7 @@ static int rt715_sdca_put_volsw(struct
> > >> snd_kcontrol *kcontrol,
> > >> struct snd_ctl_elem_value *ucontrol)
> > >> {
> > >> struct snd_soc_component *component =
> > >> snd_kcontrol_chip(kcontrol);
> > >> +struct rt715_sdca_priv *rt715 =
> > >> snd_soc_component_get_drvdata(component);
> > >> struct soc_mixer_control *mc =
> > >> (struct soc_mixer_control *)kcontrol->private_value;
> > >> unsigned int val, val2, loop_cnt = 2, i; @@ -268,6 +271,7 @@
> > >> static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
> > >> unsigned int reg2 = mc->rreg;
> > >> unsigned int reg = mc->reg;
> > >> unsigned int max = mc->max;
> > >> +unsigned int val0, val1;
> > >> int err;
> > >>
> > >
RE: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: 2021年1月13日 10:42
> To: Yuan, Perry; Limonciello, Mario; oder_ch...@realtek.com;
> pe...@perex.cz; ti...@suse.com
> Cc: alsa-de...@alsa-project.org; broo...@kernel.org; lgirdw...@gmail.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support
>
>
> [EXTERNAL EMAIL]
>
>
> >>>>> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> >>>>> + /* Privacy LED Trigger State Changed by muted/unmute switch */
> >>>>> + if (mc->invert) {
> >>>>> + val0 = ucontrol->value.integer.value[0];
> >>>>> + val1 = ucontrol->value.integer.value[1];
> >>>>> + if (val0 == 1 && val1 == 1) {
> >>>>> + rt715->micmute_led = LED_OFF;
> >>>>> + ledtrig_audio_set(LED_AUDIO_MICMUTE,
> >>>>> + rt715->micmute_led ? LED_ON :
> >>>> LED_OFF);
> >>>>> + } else if (val0 == 0 && val1 == 0) {
> >>>>> + rt715->micmute_led = LED_ON;
> >>>>> + ledtrig_audio_set(LED_AUDIO_MICMUTE,
> >>>>> + rt715->micmute_led ? LED_ON :
> >>>> LED_OFF);
> >>>>> + }
> >>>>> + }
> >>>>> +#endif
> >>>>
> >>>> Should this be activated for specific DMI quirks? This driver is
> >>>> used in
> >>> non-Dell
> >>>> platforms (I am thinking of Intel RVPs or Realtek daughterboards),
> >>>> I am not sure if a build-time behavior change makes sense.
> >>>>
> >>>> Or conversely could we just set the LEDs unconditionally if doing
> >>>> so is harmless?
> >>>
> >>> The current mic mute led setting path is not common used for other
> >>> vendors, just Dell platform support this mic mute led set operation.
> >>>
> >>> Do you think that I need to add one DMI quirk in the next version ?
> >>> If so, I can add that.
> >>>
> >>>
> >>
> >>
> >> In the HDA audio case this is modeled off of, the code runs whether
> >> or not a vendor has support for a mic mute LED. The calls to
> >> ledtrig_audio_set should be a no-op. I agree with @Pierre-Louis
> >> Bossart in this case, we should just be running it whether or not
> >> dell-privacy is compiled in. If another vendor chooses to add LED
> >> support they'll just need to set up their ledtrig supported backend and not
> change codec code.
> >
> > Hi @Pierre-Louis Bossart
> > Seems like that we have two way to go.
> > * DMI quirks,seems like that it needs to maintain the quirk list when
> > vendors
> have new system to support.
> > * We just set the mic mute led state unconditionally .
> >
> > Which way would you prefer for next patch review?
>
> Maintaining quirks is a hassle, it's much simpler and consistent with HDaudio
> if the leds are set unconditionally. Thanks!
Thank you for your confirm.
I will take this to next patch V4.
Perry Yuan
Dell | Client Software Group | CDC Linux OS
RE: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support
> -Original Message-
> From: Limonciello, Mario
> Sent: 2021年1月12日 22:48
> To: Yuan, Perry; Pierre-Louis Bossart; oder_ch...@realtek.com;
> pe...@perex.cz; ti...@suse.com
> Cc: alsa-de...@alsa-project.org; lgirdw...@gmail.com; linux-
> ker...@vger.kernel.org; broo...@kernel.org
> Subject: RE: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support
>
> > > -Original Message-
> > > From: Pierre-Louis Bossart
> > > Sent: 2021年1月12日 2:07
> > > To: Yuan, Perry; oder_ch...@realtek.com; pe...@perex.cz;
> > > ti...@suse.com
> > > Cc: alsa-de...@alsa-project.org; Limonciello, Mario;
> > > lgirdw...@gmail.com; linux-kernel@vger.kernel.org;
> > > broo...@kernel.org
> > > Subject: Re: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control
> > > support
> > >
> > >
> > > [EXTERNAL EMAIL]
> > >
> > >
> > >
> > >
> > > > @@ -268,6 +269,7 @@ static int rt715_sdca_put_volsw(struct
> > > > snd_kcontrol
> > > *kcontrol,
> > > > unsigned int reg2 = mc->rreg;
> > > > unsigned int reg = mc->reg;
> > > > unsigned int max = mc->max;
> > > > + unsigned int val0, val1;
> > > > int err;
> > > >
> > > > val = ucontrol->value.integer.value[0]; @@ -286,7 +288,22 @@
> > > > static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
> > > > if (err < 0)
> > > > return err;
> > > > }
> > > > -
> > > > +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> > > > + /* Privacy LED Trigger State Changed by muted/unmute switch */
> > > > + if (mc->invert) {
> > > > + val0 = ucontrol->value.integer.value[0];
> > > > + val1 = ucontrol->value.integer.value[1];
> > > > + if (val0 == 1 && val1 == 1) {
> > > > + rt715->micmute_led = LED_OFF;
> > > > + ledtrig_audio_set(LED_AUDIO_MICMUTE,
> > > > + rt715->micmute_led ? LED_ON :
> > > LED_OFF);
> > > > + } else if (val0 == 0 && val1 == 0) {
> > > > + rt715->micmute_led = LED_ON;
> > > > + ledtrig_audio_set(LED_AUDIO_MICMUTE,
> > > > + rt715->micmute_led ? LED_ON :
> > > LED_OFF);
> > > > + }
> > > > + }
> > > > +#endif
> > >
> > > Should this be activated for specific DMI quirks? This driver is
> > > used in
> > non-Dell
> > > platforms (I am thinking of Intel RVPs or Realtek daughterboards), I
> > > am not sure if a build-time behavior change makes sense.
> > >
> > > Or conversely could we just set the LEDs unconditionally if doing so
> > > is harmless?
> >
> > The current mic mute led setting path is not common used for other
> > vendors, just Dell platform support this mic mute led set operation.
> >
> > Do you think that I need to add one DMI quirk in the next version ?
> > If so, I can add that.
> >
> >
>
>
> In the HDA audio case this is modeled off of, the code runs whether or not a
> vendor has support for a mic mute LED. The calls to ledtrig_audio_set should
> be a no-op. I agree with @Pierre-Louis Bossart in this case, we should just
> be
> running it whether or not dell-privacy is compiled in. If another vendor
> chooses to add LED support they'll just need to set up their ledtrig supported
> backend and not change codec code.
Hi @Pierre-Louis Bossart
Seems like that we have two way to go.
* DMI quirks,seems like that it needs to maintain the quirk list when vendors
have new system to support.
* We just set the mic mute led state unconditionally .
Which way would you prefer for next patch review?
RE: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support
Hi
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: 2021年1月12日 2:07
> To: Yuan, Perry; oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com
> Cc: alsa-de...@alsa-project.org; Limonciello, Mario; lgirdw...@gmail.com;
> linux-kernel@vger.kernel.org; broo...@kernel.org
> Subject: Re: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support
>
>
> [EXTERNAL EMAIL]
>
>
>
>
> > @@ -268,6 +269,7 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol
> *kcontrol,
> > unsigned int reg2 = mc->rreg;
> > unsigned int reg = mc->reg;
> > unsigned int max = mc->max;
> > + unsigned int val0, val1;
> > int err;
> >
> > val = ucontrol->value.integer.value[0]; @@ -286,7 +288,22 @@ static
> > int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
> > if (err < 0)
> > return err;
> > }
> > -
> > +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> > + /* Privacy LED Trigger State Changed by muted/unmute switch */
> > + if (mc->invert) {
> > + val0 = ucontrol->value.integer.value[0];
> > + val1 = ucontrol->value.integer.value[1];
> > + if (val0 == 1 && val1 == 1) {
> > + rt715->micmute_led = LED_OFF;
> > + ledtrig_audio_set(LED_AUDIO_MICMUTE,
> > + rt715->micmute_led ? LED_ON :
> LED_OFF);
> > + } else if (val0 == 0 && val1 == 0) {
> > + rt715->micmute_led = LED_ON;
> > + ledtrig_audio_set(LED_AUDIO_MICMUTE,
> > + rt715->micmute_led ? LED_ON :
> LED_OFF);
> > + }
> > + }
> > +#endif
>
> Should this be activated for specific DMI quirks? This driver is used in
> non-Dell
> platforms (I am thinking of Intel RVPs or Realtek daughterboards), I am not
> sure if a build-time behavior change makes sense.
>
> Or conversely could we just set the LEDs unconditionally if doing so is
> harmless?
The current mic mute led setting path is not common used for other vendors,
just Dell platform
support this mic mute led set operation.
Do you think that I need to add one DMI quirk in the next version ?
If so, I can add that.
Perry Yuan
Dell | Client Software Group | CDC Linux OS
RE: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
> -Original Message- > From: Barnabás Pőcze > Sent: 2021年1月8日 0:04 > To: Hans de Goede > Cc: Yuan, Perry; mgr...@linux.intel.com; platform-driver- > x...@vger.kernel.org; linux-kernel@vger.kernel.org; Limonciello, Mario > Subject: Re: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell > hardware privacy > > > [EXTERNAL EMAIL] > > Hi > > > 2021. január 7., csütörtök 0:43 keltezéssel, Hans de Goede írta: > > > Hi Perry, > > > > On 12/28/20 2:28 PM, Perry Yuan wrote: > > > > > From: Perry Yuan perry_y...@dell.com add support for dell privacy > > > driver for the dell units equipped hardware privacy design, which > > > protect users privacy of audio and camera from hardware level. once > > > the audio or camera privacy mode enabled, any applications will not > > > get any audio or video stream. > > > when user pressed ctrl+F4 hotkey, audio privacy mode will be > > > enabled,Micmute led will be also changed accordingly. > > > The micmute led is fully controlled by hardware & EC. > > > and camera mute hotkey is ctrl+F9.currently design only emmit > > > SW_CAMERA_LENS_COVER event while the camera LENS shutter will be > > > changed by EC & HW control. > > > *The flow is like this: > > > > > > 1. User presses key. HW does stuff with this key (timeout is > > > started) 2. Event is emitted from FW 3. Event received by > > > dell-privacy 4. KEY_MICMUTE emitted from dell-privacy 5. Userland > > > picks up key and modifies kcontrol for SW mute 6. Codec kernel > > > driver catches and calls ledtrig_audio_set, like this: > > > ledtrig_audio_set(LED_AUDIO_MICMUTE, > > > rt715->micmute_led ? LED_ON :LED_OFF); > > > > > > 7. If "LED" is set to on dell-privacy notifies ec, > > > and timeout is cancelled,HW mic mute activated. > > > > > > > > > Signed-off-by: Perry Yuan perry_y...@dell.com > > > Signed-off-by: Limonciello Mario mario_limoncie...@dell.com > > > > Thank you for your patch, please send a new version addressing > > Barnabás' review comment and including the second patch of the series. > > [...] > > I think first something needs to be figured out regarding the integration with > the rest of the Dell modules. I feel that list is not a desirable way to do > it. > > > Regards, > Barnabás Pőcze Hi Barnabás, Hans. Thanks for your review and comments. Before I send V3, I would explain some detail why we have two files for the dell privacy arch design. Firstly, all these privacy feature are hardware level privacy solution. Dell is going to implement some other dell privacy devices using interface acpi and wmi differently. such as electronic privacy display, privacy camera, and other privacy technology that is coming soon. we need to have dell-privacy-acpi and dell-privacy-wmi files to handle the privacy device interaction with BIOS and EC(embedded controller) - the dell-privacy-wmi file handle the wmi event from BIOS and emit it to userspace , it is wmi interface related driver file. - the dell-privacy-acpi interact with ACPI interface ,dell privacy feature need to send ACK content to EC through ACPI interface. - other privacy device function will be developed according to the interface type. the two driver files will be extended for new devices. If you have any other concerns for arch design , Mario could help to explain some more details to get this clear. Perry Yuan Dell | Client Software Group | CDC Linux OS
RE: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Dell Customer Communication - Confidential > -Original Message- > From: Barnabás Pőcze > Sent: 2021年1月8日 0:04 > To: Hans de Goede > Cc: Yuan, Perry; mgr...@linux.intel.com; platform-driver- > x...@vger.kernel.org; linux-kernel@vger.kernel.org; Limonciello, Mario > Subject: Re: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell > hardware privacy > > > [EXTERNAL EMAIL] > > Hi > > > 2021. január 7., csütörtök 0:43 keltezéssel, Hans de Goede írta: > > > Hi Perry, > > > > On 12/28/20 2:28 PM, Perry Yuan wrote: > > > > > From: Perry Yuan perry_y...@dell.com add support for dell privacy > > > driver for the dell units equipped hardware privacy design, which > > > protect users privacy of audio and camera from hardware level. once > > > the audio or camera privacy mode enabled, any applications will not > > > get any audio or video stream. > > > when user pressed ctrl+F4 hotkey, audio privacy mode will be > > > enabled,Micmute led will be also changed accordingly. > > > The micmute led is fully controlled by hardware & EC. > > > and camera mute hotkey is ctrl+F9.currently design only emmit > > > SW_CAMERA_LENS_COVER event while the camera LENS shutter will be > > > changed by EC & HW control. > > > *The flow is like this: > > > > > > 1. User presses key. HW does stuff with this key (timeout is > > > started) 2. Event is emitted from FW 3. Event received by > > > dell-privacy 4. KEY_MICMUTE emitted from dell-privacy 5. Userland > > > picks up key and modifies kcontrol for SW mute 6. Codec kernel > > > driver catches and calls ledtrig_audio_set, like this: > > > ledtrig_audio_set(LED_AUDIO_MICMUTE, > > > rt715->micmute_led ? LED_ON :LED_OFF); > > > > > > 7. If "LED" is set to on dell-privacy notifies ec, > > > and timeout is cancelled,HW mic mute activated. > > > > > > > > > Signed-off-by: Perry Yuan perry_y...@dell.com > > > Signed-off-by: Limonciello Mario mario_limoncie...@dell.com > > > > Thank you for your patch, please send a new version addressing > > Barnabás' review comment and including the second patch of the series. > > [...] > > I think first something needs to be figured out regarding the integration with > the rest of the Dell modules. I feel that list is not a desirable way to do > it. > > > Regards, > Barnabás Pőcze Hi Barnabás, Hans. Thanks for your review and comments. Before I send V3, I would explain some detail why we have two files for the dell privacy arch design. Firstly, all these privacy feature are hardware level privacy solution. Dell is going to implement some other dell privacy devices using interface acpi and wmi differently. such as electronic privacy display, privacy camera, and other privacy technology that is coming soon. we need to have dell-privacy-acpi and dell-privacy-wmi files to handle the privacy device interaction with BIOS and EC(embedded controller) - the dell-privacy-wmi file handle the wmi event from BIOS and emit it to userspace , it is wmi interface related driver file. - the dell-privacy-acpi interact with ACPI interface ,dell privacy feature need to send ACK content to EC through ACPI interface. - other privacy device function will be developed according to the interface type. the two driver files will be extended for new devices. Meanwhile, actually we will consider to add one ACPI device to BIOS DSDT table in future , then dell-privacy-acpi driver can register to kernel matching that device with specific ACPI device ID. If you have any other concerns for arch design , Mario could help to explain some more details to get this clear.
RE: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support
> -Original Message- > From: Mark Brown > Sent: Tuesday, December 29, 2020 8:41 PM > To: Yuan, Perry > Cc: oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com; > lgirdw...@gmail.com; alsa-de...@alsa-project.org; linux- > ker...@vger.kernel.org; Limonciello, Mario > Subject: Re: [PATCH v2 2/2] ASoC: rt715:add Mic Mute LED control support > > On Mon, Dec 28, 2020 at 09:38:31PM +0800, Perry Yuan wrote: > > From: Perry Yuan > > > > Some new Dell system is going to support audio internal micphone > > privacy setting from hardware level with micmute led state changing > > I'm missing patch 1 of this series - what's the story with dependencies and so > on? HI Mark: Sorry to make this confused. The two patches are not in the same module, I use different cc list for each patch. I will send another v3 looping all the cc list and some new improvement. --- Begin Message --- From: Perry Yuan add support for dell privacy driver for the dell units equipped hardware privacy design, which protect users privacy of audio and camera from hardware level. once the audio or camera privacy mode enabled, any applications will not get any audio or video stream. when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled,Micmute led will be also changed accordingly. The micmute led is fully controlled by hardware & EC. and camera mute hotkey is ctrl+F9.currently design only emmit SW_CAMERA_LENS_COVER event while the camera LENS shutter will be changed by EC & HW control. *The flow is like this: 1) User presses key. HW does stuff with this key (timeout is started) 2) Event is emitted from FW 3) Event received by dell-privacy 4) KEY_MICMUTE emitted from dell-privacy 5) Userland picks up key and modifies kcontrol for SW mute 6) Codec kernel driver catches and calls ledtrig_audio_set, like this: ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON :LED_OFF); 7) If "LED" is set to on dell-privacy notifies ec, and timeout is cancelled,HW mic mute activated. Signed-off-by: Perry Yuan Signed-off-by: Limonciello Mario --- v1 -> v2: * query EC handle from EC driver directly. * fix some code style. * add KEY_END to keymap array. * clean platform device when cleanup called * use hexadecimal format for log print in dev_dbg * remove __set_bit for the report keys from probe. * fix keymap leak * add err_free_keymap in dell_privacy_wmi_probe * wmi driver will be unregistered if privacy_acpi_init() fails * add sysfs attribute files for user space query. * add leds micmute driver to privacy acpi * add more design info the commit info --- --- drivers/platform/x86/Kconfig | 17 ++ drivers/platform/x86/Makefile| 4 +- drivers/platform/x86/dell-laptop.c | 29 ++- drivers/platform/x86/dell-privacy-acpi.c | 165 drivers/platform/x86/dell-privacy-wmi.c | 309 +++ drivers/platform/x86/dell-privacy-wmi.h | 33 +++ drivers/platform/x86/dell-wmi.c | 26 +- 7 files changed, 567 insertions(+), 16 deletions(-) create mode 100644 drivers/platform/x86/dell-privacy-acpi.c create mode 100644 drivers/platform/x86/dell-privacy-wmi.c create mode 100644 drivers/platform/x86/dell-privacy-wmi.h diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 91e6176cdfbd..9d5cc2454b3e 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -491,6 +491,23 @@ config DELL_WMI_LED This adds support for the Latitude 2100 and similar notebooks that have an external LED. +config DELL_PRIVACY + tristate "Dell Hardware Privacy Support" + depends on ACPI + depends on ACPI_WMI + depends on INPUT + depends on DELL_LAPTOP + depends on LEDS_TRIGGER_AUDIO + select DELL_WMI + help + This driver provides support for the "Dell Hardware Privacy" feature + of Dell laptops. + Support for a micmute and camera mute privacy will be provided as + hardware button Ctrl+F4 and Ctrl+F9 hotkey + + To compile this driver as a module, choose M here: the module will + be called dell_privacy. + config AMILO_RFKILL tristate "Fujitsu-Siemens Amilo rfkill support" depends on RFKILL diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 581475f59819..18c430456de7 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -51,7 +51,9 @@ obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o obj-$(CONFIG_DELL_WMI_SYSMAN) += dell-wmi-sysman/ - +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o +dell-privacy-objs := dell-privacy-wmi.o \ +
RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver
> -Original Message-
> From: Hans de Goede
> Sent: Monday, November 9, 2020 7:16 PM
> To: Yuan, Perry; mgr...@linux.intel.com; mj...@srcf.ucam.org;
> p...@kernel.org
> Cc: linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org;
> Limonciello, Mario
> Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy
> driver
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 11/3/20 1:55 PM, Perry Yuan wrote:
> > From: perry_yuan
> >
> > add support for dell privacy driver for the dell units equipped
> > hardware privacy design, which protect users privacy of audio and
> > camera from hardware level. once the audio or camera privacy mode
> > enabled, any applications will not get any audio or video stream.
> > when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled
> > and camera mute hotkey is ctrl+F9.
> >
> > Signed-off-by: Perry Yuan
> > Signed-off-by: Limonciello Mario
>
> Perry, you have had multiple comments and kernel-test-robot reports about
> this patch. Please prepare a new version addressing these.
>
> Once you have send out a new version I will try to make some time to do a
> full review soon(ish).
>
> Regards,
>
> Hans
Hi Hans:
I got some review feedback from Barnabás and Matthew,Mario.
I am working on another new version and will submit v2 patch soon for your
review.
Thank you in advance.
Perry
>
>
> > ---
> > drivers/platform/x86/Kconfig | 12 ++
> > drivers/platform/x86/Makefile| 4 +-
> > drivers/platform/x86/dell-laptop.c | 41 ++--
> > drivers/platform/x86/dell-privacy-acpi.c | 139
> > drivers/platform/x86/dell-privacy-wmi.c | 259 +++
> > drivers/platform/x86/dell-privacy-wmi.h | 23 ++
> > drivers/platform/x86/dell-wmi.c | 90
> > 7 files changed, 513 insertions(+), 55 deletions(-) create mode
> > 100644 drivers/platform/x86/dell-privacy-acpi.c
> > create mode 100644 drivers/platform/x86/dell-privacy-wmi.c
> > create mode 100644 drivers/platform/x86/dell-privacy-wmi.h
> >
> > diff --git a/drivers/platform/x86/Kconfig
> > b/drivers/platform/x86/Kconfig index 40219bba6801..0cb6bf5a9565
> 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -454,6 +454,18 @@ config DELL_WMI_LED
> > This adds support for the Latitude 2100 and similar
> > notebooks that have an external LED.
> >
> > +config DELL_PRIVACY
> > +tristate "Dell Hardware Privacy Support"
> > +depends on ACPI
> > +depends on ACPI_WMI
> > +depends on INPUT
> > +depends on DELL_LAPTOP
> > +select DELL_WMI
> > +help
> > + This driver provides a driver to support messaging related to the
> > + privacy button presses on applicable Dell laptops from 2021 and
> > + newer.
> > +
> > config AMILO_RFKILL
> > tristate "Fujitsu-Siemens Amilo rfkill support"
> > depends on RFKILL
> > diff --git a/drivers/platform/x86/Makefile
> > b/drivers/platform/x86/Makefile index 5f823f7eff45..111f7215db2f
> > 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -47,7 +47,9 @@ obj-$(CONFIG_DELL_WMI)+=
> dell-wmi.o
> > obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
> > obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
> > obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
> > -
> > +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o
> > +dell-privacy-objs := dell-privacy-wmi.o \
> > + dell-privacy-acpi.o
> > # Fujitsu
> > obj-$(CONFIG_AMILO_RFKILL) += amilo-rfkill.o
> > obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
> > diff --git a/drivers/platform/x86/dell-laptop.c
> > b/drivers/platform/x86/dell-laptop.c
> > index 5e9c2296931c..12b91de09356 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -30,6 +30,7 @@
> > #include
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
> > +#include "dell-privacy-wmi.h"
> >
> > struct quirk_entry {
> > bool touchpad_led;
> > @@ -90,6 +91,7 @@ static struct rfkill *wifi_rfkill; static struct
> > rfkill *bluetooth_rfkill; static struct rfkill *wwan_rfkill; static
> > bool force_rfkill;
> > +stati
RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver
> -Original Message-
> From: Matthew Garrett
> Sent: Wednesday, November 4, 2020 9:49 AM
> To: Yuan, Perry
> Cc: hdego...@redhat.com; mgr...@linux.intel.com; p...@kernel.org; linux-
> ker...@vger.kernel.org; platform-driver-...@vger.kernel.org; Limonciello,
> Mario
> Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy
> driver
>
>
> [EXTERNAL EMAIL]
>
> On Tue, Nov 03, 2020 at 04:55:42AM -0800, Perry Yuan wrote:
>
> > +#define PRIVACY_PlATFORM_NAME "dell-privacy-acpi"
> > +#define ACPI_PRIVACY_DEVICE"\\_SB.PC00.LPCB.ECDV"
>
> This looks like the EC rather than a privacy device? If so, you probably want
> to collaborate with the EC driver to obtain the handle rather than depending
> on the path, unless it's guaranteed that this path will never change.
Thanks Matthew
I will change the path to handle as you suggested.
>
> > +static int micmute_led_set(struct led_classdev *led_cdev,
> > + enum led_brightness brightness) {
> > +acpi_status status;
> > +
> > +status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL,
> NULL);
> > +if (ACPI_FAILURE(status)) {
> > +dev_err(led_cdev->dev, "Error setting privacy audio EC ack
> value: %d\n",status);
> > +return -EIO;
> > +}
> > +return 0;
> > +}
>
> What's actually being set here? You don't seem to be passing any arguments.
Yes, it is a EC ack notification without any arguments needed.
>
> > +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> > +{"PNP0C09", 0},
>
> Oooh no please don't do this - you'll trigger autoloading on everything that
> exposes a PNP0C09 device.
Got it , I need to adjust the driver register logic.
In drivers/platform/x86/dell-privacy-wmi.c .
The privacy acpi driver will be loaded by privacy wmi driver.
The privacy wmi driver need to check if the privacy device is present.
It can avoid loading driver on non-dell-privacy system.
+static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
+{ .guid_string = DELL_PRIVACY_GUID },
+{ },
>
> --
> Matthew Garrett | mj...@srcf.ucam.org
RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
> From: Andy Shevchenko
> Sent: Wednesday, July 29, 2020 3:32 PM
> To: Yuan, Perry
> Cc: Sebastian Reichel; Matthew Garrett; Pali Rohár; Darren Hart; Andy
> Shevchenko; Limonciello, Mario; Linux PM; Linux Kernel Mailing List; Platform
> Driver
> Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds
> and charging mode switch.
>
>
> [EXTERNAL EMAIL]
>
> On Wed, Jul 29, 2020 at 9:54 AM Perry Yuan wrote:
> >
> > From: perry_yuan
>
> Fix your name, please. Or do you have it spelled in the official documents
> like
> above?
>
> > The patch control battery charging thresholds when system is under
> > custom charging mode through smbios API and driver`s sys attributes.It
> > also set the percentage bounds for custom charge.
> > Start value must lie in the range [50, 95],End value must lie in the
> > range [55, 100],END must be at least (START + 5).
> >
> > The patch also add the battery charging modes switch support.User can
> > switch the battery charging mode through the new sysfs entry.
>
> The commit message needs rewording. I would recommend reading [1] and [2].
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> (section 2)
> [2]: https://chris.beams.io/posts/git-commit/
>
> > Primary battery charging modes valid choices are:
> > ['primarily_ac', 'adaptive', 'custom', 'standard', 'express']
>
> This and documentation are different (at least in terms of case).
>
> Looks like this is something that should be agreed upon on the format and gets
> supported by Power Supply framework in the first place,
>
> > Documentation/ABI/testing/sysfs-class-power | 23 ++
>
> In any case it should be a separate patch. It has nothing to do with Dell, on
> contrary it's a certain device which relies on generic attributes.
>
> ...
>
> > #include
> > #include
>
> > #include
> > +#include
>
> Keep it ordered.
>
> > +#include
>
> And this is a generic one, should be in generic headers block.
>
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
>
> ...
>
> > +static int dell_battery_get(int *start, int *end) {
> > + struct calling_interface_buffer buffer;
> > + struct calling_interface_token *token;
> > + int ret;
> > +
>
> > + if (start) {
> > + token =
> dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_START);
> > + if (!token)
> > + return -ENODEV;
> > + dell_fill_request(, token->location, 0, 0, 0);
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > + *start = buffer.output[1];
> > + }
>
> (1)
>
> > + if (end) {
> > + token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_END);
> > + if (!token)
> > + return -ENODEV;
> > + dell_fill_request(, token->location, 0, 0, 0);
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
> > + *end = buffer.output[1];
> > + }
>
> (2)
>
> (1) and (2) look like twins. Care to provide a helper to simplify?
>
> > + return 0;
> > +}
>
> ...
>
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + if (ret)
>
> > + return -EIO;
>
> Why shadowing error code?
>
> ...
>
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
>
> Ditto.
>
> > + return ret;
>
> And here perhaps simple 'return dell_send_request();'?
>
> ...
>
> > + switch (mode) {
> > + case BAT_STANDARD_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_STANDARD_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > + case BAT_EXPRESS_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_EXPRESS_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > +
RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
> From: Matthew Garrett > Sent: Tuesday, August 4, 2020 1:54 PM > To: Yuan, Perry > Cc: kernel test robot; s...@kernel.org; p...@kernel.org; dvh...@infradead.org; > a...@infradead.org; Limonciello, Mario; kbuild-...@lists.01.org; linux- > p...@vger.kernel.org; linux-kernel@vger.kernel.org; platform-driver- > x...@vger.kernel.org > Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds > and charging mode switch. > > > [EXTERNAL EMAIL] > > On Tue, Aug 04, 2020 at 05:46:30AM +, Yuan, Perry wrote: > > > It is not patch issue, the kernel config needs to add > "CONFIG_ACPI_BATTERY=y" > > In that case you probably want to add a dependency to ACPI_BATTERY in the > DELL_LAPTOP Kconfig. > > -- > Matthew Garrett | mj...@srcf.ucam.org Thank you Matthew. I will add it to the Kconfig as DELL_LAPTOP dependency.
RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
> From: kernel test robot > Sent: Saturday, August 1, 2020 1:08 PM > To: Yuan, Perry; s...@kernel.org; mj...@srcf.ucam.org; p...@kernel.org; > dvh...@infradead.org; a...@infradead.org; Limonciello, Mario > Cc: kbuild-...@lists.01.org; linux...@vger.kernel.org; linux- > ker...@vger.kernel.org; platform-driver-...@vger.kernel.org; Yuan, Perry > Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds > and charging mode switch. > > > [EXTERNAL EMAIL] > > Hi Perry, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on power-supply/for-next] [also build test ERROR on > linux/master linus/master v5.8-rc7 next-20200731] [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] > > url:https://github.com/0day-ci/linux/commits/Perry-Yuan/platform-x86- > dell-laptop-Add-battery-charging-thresholds-and-charging-mode- > switch/20200729-150347 > base: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power- > supply.git for-next > config: i386-randconfig-a005-20200731 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 > reproduce (this is a W=1 build): > # save the attached .config to linux build tree > make W=1 ARCH=i386 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > >> ERROR: modpost: "battery_hook_unregister" [drivers/platform/x86/dell- > laptop.ko] undefined! > >> ERROR: modpost: "battery_hook_register" [drivers/platform/x86/dell- > laptop.ko] undefined! > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org It is not patch issue, the kernel config needs to add "CONFIG_ACPI_BATTERY=y"
