Message-ID: <20181124104157.GA17954@amd>
Date:   Sat, 24 Nov 2018 11:41:57 +0100
From:   Pavel Machek <pavel@....cz>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Ayman Bagabas <ayman.bagabas@...il.com>,
        ALSA Development Mailing List <alsa-devel@...a-project.org>,
        Hui Wang <hui.wang@...onical.com>,
        Andy Shevchenko <andy@...radead.org>,
        Darren Hart <dvhart@...radead.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Kailang Yang <kailang@...ltek.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>
Subject: Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

Hi!

> > > > > > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > > > > > >
> > 
> > > > I'd prefer this to be normal LED and "mic muted" to become normal
> > > > trigger.
> > > 
> > > But how would you solve the existing problem?
> > > 
> > > As already mentioned, you'll need to hook the LED trigger and the
> > > actual mixer value change.  This is the biggest missing piece, and
> > > it's the very reason we have the exported symbol from the platform
> > > driver side.
> > > 
> > > So, if you prefer in that way, please implement that for the existing
> > > driver (thinkpad_acpi and dell-laptop) at first.  I'll be really happy
> > > to get rid of the present ugly solution!  But it's been there just
> > > because it's not so trivial at all.  FWIW, this must be all done
> > > inside the kernel; otherwise you'll hit a regression.
> > 
> > Ok, what about something like this?
> > 
> > Tested, and it did not work. I guess I hooked it up at the wrong place
> > in LED subsystem... or maybe thinkpad x60 is wrong machine to test on.

I meant to say "wrong place in ALSA subsystem".

> > Anyway, it looks less ugly than current code in alsa. We should not
> > really be using mixer settings to turn LED on and off.
> > 
> > Plus, it works in similar way triggers and LEDs "usually" do, and has
> > all the flexibility.
> 
> Yes, thanks, that's something similar as what I had in mind, too.
> I guess it's just a matter of thinkpad_acpi side implementation that
> differs from your expectation.
> 
> However, one remaining problem is that the state will be inconsistent
> depending on the driver module order, if we get rid of the dynamic
> symbol binding.  Then both modules become completely individual and
> thinkpad_acpi can be loaded at anytime later than HD-audio codec.
> If a mixer state is already changed before loading thinkpad_acpi, this
> event is lost and the state may be different in both sides.



> So, we'd need to record the state in the mute-trigger side, and add a
> function to return the current state.  Then thinkpad_acpi will query
> the state at the initialization time.

We want to record state at mute-trigger side, yes. Should not be
really different from other triggers.

> Also, we'll need also the normal mute LED in addition to the mic mute
> LED, so there need to be two triggers.

Yes.

> In anyway, moving to this direction requires the leds class
> implementation for dell-laptop.c as well as the new huawei stuff.  So,
> I'd really like to have the "already good working" code before
> actually hacking this, so that we can see and track the functional
> regression more obviously.

As well as thinkpad; note I did not do that part, because I don't have
that LED on my test machine.

Anyway, normally we get the architecture right and only then we merge
the drivers.

All Huawei hackers need to do is to convert their driver into normal
LED driver, and test it with some other trigger (like heartbeat). Then
we can be fairly sure it will work when we get the micmute done.

> I can start hacking this in the next week.  At least I have a Dell
> laptop and I can check the part of the changes locally.
> 
> Since the changes will be fairly cross-tree, it's better to have an
> immutable branch to start with.  I'd branch off at 4.20-rc3 (or rc4),
> apply the current patches, so that it can be merged to all relevant
> trees cleanly.

As you will not change any core LED code, I don't think any heavy
synchronization with LED tree should be neccessary. We have LED
triggers living outside drivers/leds/triggers, too, such as
drivers/usb/common/led.c .

(Actually, that's the beauty of the LED subsystem. You have standard
trigger. You have standard LED -- which says it prefers that trigger if
available. They are really independend, so you can test and merge them
separately).

Let me know if you need any more help.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)

Powered by blists - more mailing lists