RE: [PATCH v4 2/2] ASoC: rt715:add micmute led state control supports

[Yuan, Perry]

Hi Hans,Jaroslav


> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: 2021年3月25日 23:07
> To: Yuan, Perry; Jaroslav Kysela; Mark Brown; pierre-
> louis.bossart@xxxxxxxxxxxxxxx; Limonciello, Mario
> Cc: pobrn@xxxxxxxxxxxxxx; oder_chiou@xxxxxxxxxxx; tiwai@xxxxxxxx;
> mgross@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx
> 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 <hdegoede@xxxxxxxxxx>
> >> Sent: Monday, March 22, 2021 11:02 PM
> >> To: Jaroslav Kysela; Yuan, Perry; Mark Brown; pierre-
> >> louis.bossart@xxxxxxxxxxxxxxx; Limonciello, Mario
> >> Cc: pobrn@xxxxxxxxxxxxxx; oder_chiou@xxxxxxxxxxx; tiwai@xxxxxxxx;
> >> mgross@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx;
> >> alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> platform-driver-x86@xxxxxxxxxxxxxxx
> >> 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 <broonie@xxxxxxxxxx>
> >>>>> Sent: Tuesday, March 9, 2021 1:24 AM
> >>>>> To: Yuan, Perry
> >>>>> Cc: pobrn@xxxxxxxxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx;
> >>>>> oder_chiou@xxxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx;
> >>>>> hdegoede@xxxxxxxxxx; mgross@xxxxxxxxxxxxxxx; Limonciello, Mario;
> >>>>> lgirdwood@xxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; linux-
> >>>>> kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx
> >>>>> 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-
> >> perex@xxxxxxxx/
> >>
> >> 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.
> 
> The triggering of the LED trigger and the code registering the led_classdev are
> in 2 separate subsystems; and should be merged separately.
> 
> If you post a new version of patch 1/2 addressing my review remarks then I
> can merge that.
> 
> For merging the sound side of this you need to talk to the sound folks
> (Jaroslav Kysela, Takashi Iwai, Mark Brown for files under sound/soc).
> 
> Regards,
> 
> Hans

Got it!
I am working the V6 today, will post it ASAP.
* addressing Hans Feedback 
* add runtime machine detection with DMI checking (from Jaroslav Feedback)

Perry  Yuan
Dell | Client Software Group | CDC Linux OS