mbox series

[0/6] Introduce audio-mute LED trigger (and conversions to it)

Message ID 20181126171126.20280-1-tiwai@suse.de (mailing list archive)
Headers show
Series Introduce audio-mute LED trigger (and conversions to it) | expand

Message

Takashi Iwai Nov. 26, 2018, 5:11 p.m. UTC
Hi,

this is patch series I've hacked after useful conversation with
Pavel.  Basically this adds a new LED trigger audio-mute and
audio-micmute, and convert the HD-audio driver and the platform
drivers to use the LED trigger instead of the ugly direct dynamic
symbol binding.

The latest version of patches are found in topic/leds-trigger branch
in my sound git tree.
  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git

As these are cross-tree patches, the branch above is based cleanly on
v4.20-rc3, so that it can be merged well to multiple trees.

Once after getting the ACK's, I'll add tags and fixate for merges.

This patch series don't include huawei-wmi stuff; so Huawei patches
need rework.  I already have some piece of changes for huawei-wmi, so
please ping me if needed.

I checked briefly on my Dell laptop, and a Thinkpad model.
Wider tests are appreciated, of course.


thanks,

Takashi

===

Takashi Iwai (6):
  leds: trigger: Introduce audio mute LED trigger
  platform/x86: dell-laptop: Add micmute LED trigger support
  platform/x86: thinkpad_acpi: Add audio mute LED classdev support
  ALSA: hda - Support led audio trigger
  platform/x86: dell-laptop: Drop superfluous exported function
  platform/x86: thinkpad_acpi: Drop superfluous exported function

 drivers/leds/trigger/Kconfig         |  7 +++
 drivers/leds/trigger/Makefile        |  1 +
 drivers/leds/trigger/ledtrig-audio.c | 45 +++++++++++++++++++
 drivers/platform/x86/Kconfig         |  4 ++
 drivers/platform/x86/dell-laptop.c   | 27 +++++++++---
 drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++-------
 include/linux/dell-led.h             |  7 ---
 include/linux/leds.h                 | 20 +++++++++
 include/linux/thinkpad_acpi.h        | 16 -------
 sound/pci/hda/dell_wmi_helper.c      | 48 --------------------
 sound/pci/hda/hda_generic.c          | 31 +++++++++++++
 sound/pci/hda/hda_generic.h          |  2 +
 sound/pci/hda/patch_realtek.c        | 17 +++----
 sound/pci/hda/thinkpad_helper.c      | 43 +++---------------
 14 files changed, 194 insertions(+), 140 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-audio.c
 delete mode 100644 include/linux/dell-led.h
 delete mode 100644 include/linux/thinkpad_acpi.h
 delete mode 100644 sound/pci/hda/dell_wmi_helper.c

Comments

Andy Shevchenko Nov. 26, 2018, 11:09 p.m. UTC | #1
On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> Hi,
>
> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.
>
> The latest version of patches are found in topic/leds-trigger branch
> in my sound git tree.
>   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
>
> As these are cross-tree patches, the branch above is based cleanly on
> v4.20-rc3, so that it can be merged well to multiple trees.
>
> Once after getting the ACK's, I'll add tags and fixate for merges.
>
> This patch series don't include huawei-wmi stuff; so Huawei patches
> need rework.  I already have some piece of changes for huawei-wmi, so
> please ping me if needed.
>
> I checked briefly on my Dell laptop, and a Thinkpad model.
> Wider tests are appreciated, of course.
>

Few minor comments, otherwise I like this series.
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for PDx86 bits.

>
> thanks,
>
> Takashi
>
> ===
>
> Takashi Iwai (6):
>   leds: trigger: Introduce audio mute LED trigger
>   platform/x86: dell-laptop: Add micmute LED trigger support
>   platform/x86: thinkpad_acpi: Add audio mute LED classdev support
>   ALSA: hda - Support led audio trigger
>   platform/x86: dell-laptop: Drop superfluous exported function
>   platform/x86: thinkpad_acpi: Drop superfluous exported function
>
>  drivers/leds/trigger/Kconfig         |  7 +++
>  drivers/leds/trigger/Makefile        |  1 +
>  drivers/leds/trigger/ledtrig-audio.c | 45 +++++++++++++++++++
>  drivers/platform/x86/Kconfig         |  4 ++
>  drivers/platform/x86/dell-laptop.c   | 27 +++++++++---
>  drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++-------
>  include/linux/dell-led.h             |  7 ---
>  include/linux/leds.h                 | 20 +++++++++
>  include/linux/thinkpad_acpi.h        | 16 -------
>  sound/pci/hda/dell_wmi_helper.c      | 48 --------------------
>  sound/pci/hda/hda_generic.c          | 31 +++++++++++++
>  sound/pci/hda/hda_generic.h          |  2 +
>  sound/pci/hda/patch_realtek.c        | 17 +++----
>  sound/pci/hda/thinkpad_helper.c      | 43 +++---------------
>  14 files changed, 194 insertions(+), 140 deletions(-)
>  create mode 100644 drivers/leds/trigger/ledtrig-audio.c
>  delete mode 100644 include/linux/dell-led.h
>  delete mode 100644 include/linux/thinkpad_acpi.h
>  delete mode 100644 sound/pci/hda/dell_wmi_helper.c
>
> --
> 2.19.1
>
Pavel Machek Nov. 27, 2018, 8:44 a.m. UTC | #2
Hi!

> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.

Thanks a lot for doing this!

> The latest version of patches are found in topic/leds-trigger branch
> in my sound git tree.
>   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> 
> As these are cross-tree patches, the branch above is based cleanly on
> v4.20-rc3, so that it can be merged well to multiple trees.
> 
> Once after getting the ACK's, I'll add tags and fixate for merges.
> 
> This patch series don't include huawei-wmi stuff; so Huawei patches
> need rework.  I already have some piece of changes for huawei-wmi, so
> please ping me if needed.
> 
> I checked briefly on my Dell laptop, and a Thinkpad model.
> Wider tests are appreciated, of course.

Looks good... except one detail: you have "tpacpi::micmute" and
"dell::micmute". I know it follows "tradition", but we are trying to
fix that at the moment. Laptop micmute button is a laptop micmute
button, and userspace should not need to know what prefix to use
depending on vendor.

I'd suggest using "sys::micmute".

With that:

Acked-by: Pavel Machek <pavel@ucw.cz>

								Pavel
Henrique de Moraes Holschuh Nov. 27, 2018, 10:26 a.m. UTC | #3
On Mon, 26 Nov 2018, Takashi Iwai wrote:
> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.

For the thinkpad-acpi bits, provided that Andy's comments are addressed:
Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

> I checked briefly on my Dell laptop, and a Thinkpad model.

Thanks :-)
Takashi Iwai Nov. 27, 2018, 11:05 a.m. UTC | #4
On Tue, 27 Nov 2018 00:09:17 +0100,
Andy Shevchenko wrote:
> 
> On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > Hi,
> >
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> >
> > The latest version of patches are found in topic/leds-trigger branch
> > in my sound git tree.
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> >
> > As these are cross-tree patches, the branch above is based cleanly on
> > v4.20-rc3, so that it can be merged well to multiple trees.
> >
> > Once after getting the ACK's, I'll add tags and fixate for merges.
> >
> > This patch series don't include huawei-wmi stuff; so Huawei patches
> > need rework.  I already have some piece of changes for huawei-wmi, so
> > please ping me if needed.
> >
> > I checked briefly on my Dell laptop, and a Thinkpad model.
> > Wider tests are appreciated, of course.
> >
> 
> Few minor comments, otherwise I like this series.
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> for PDx86 bits.

Thanks, added your ack to platform patches.

The latest patches are found in topic/leds-trigger branch of my sound
git tree.

I'm going to resubmit v2 series tomorrow or later, then merge the
branch to for-next branch.


Takashi
Takashi Iwai Nov. 27, 2018, 11:06 a.m. UTC | #5
On Tue, 27 Nov 2018 09:44:18 +0100,
Pavel Machek wrote:
> 
> Hi!
> 
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> 
> Thanks a lot for doing this!
> 
> > The latest version of patches are found in topic/leds-trigger branch
> > in my sound git tree.
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> > 
> > As these are cross-tree patches, the branch above is based cleanly on
> > v4.20-rc3, so that it can be merged well to multiple trees.
> > 
> > Once after getting the ACK's, I'll add tags and fixate for merges.
> > 
> > This patch series don't include huawei-wmi stuff; so Huawei patches
> > need rework.  I already have some piece of changes for huawei-wmi, so
> > please ping me if needed.
> > 
> > I checked briefly on my Dell laptop, and a Thinkpad model.
> > Wider tests are appreciated, of course.
> 
> Looks good... except one detail: you have "tpacpi::micmute" and
> "dell::micmute". I know it follows "tradition", but we are trying to
> fix that at the moment. Laptop micmute button is a laptop micmute
> button, and userspace should not need to know what prefix to use
> depending on vendor.
> 
> I'd suggest using "sys::micmute".

OK, replaced them, and also add explanation in the patch logs.

> With that:
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Added now to the series in topic/leds-trigger branch.

I'm going to resubmit v2 series tomorrow or later, then merge the
branch to for-next branch.


Thanks!

Takashi
Takashi Iwai Nov. 27, 2018, 11:11 a.m. UTC | #6
On Tue, 27 Nov 2018 11:26:32 +0100,
Henrique de Moraes Holschuh wrote:
> 
> On Mon, 26 Nov 2018, Takashi Iwai wrote:
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> 
> For the thinkpad-acpi bits, provided that Andy's comments are addressed:
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

Thanks, added now to topic/leds-trigger branch.

I'm going to resubmit v2 series tomorrow or later, then merge the
branch to for-next branch.


Takashi
Pali Rohár Nov. 28, 2018, 11:14 a.m. UTC | #7
On Monday 26 November 2018 18:11:20 Takashi Iwai wrote:
> Hi,
> 
> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.
> 
> The latest version of patches are found in topic/leds-trigger branch
> in my sound git tree.
>   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> 
> As these are cross-tree patches, the branch above is based cleanly on
> v4.20-rc3, so that it can be merged well to multiple trees.
> 
> Once after getting the ACK's, I'll add tags and fixate for merges.
> 
> This patch series don't include huawei-wmi stuff; so Huawei patches
> need rework.  I already have some piece of changes for huawei-wmi, so
> please ping me if needed.
> 
> I checked briefly on my Dell laptop, and a Thinkpad model.
> Wider tests are appreciated, of course.

Hi! Thanks for patch series. It is really improvement in current mute
led situation. I quickly looked at patches and they look good. So you
can add my Acked-By: Pali Rohár <pali.rohar@gmail.com>.
Pali Rohár Nov. 28, 2018, 11:18 a.m. UTC | #8
On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> Looks good... except one detail: you have "tpacpi::micmute" and
> "dell::micmute". I know it follows "tradition", but we are trying to
> fix that at the moment. Laptop micmute button is a laptop micmute
> button, and userspace should not need to know what prefix to use
> depending on vendor.
> 
> I'd suggest using "sys::micmute".

I can imagine that in future some devices like keyboards would have also
mute led. We already have keyboards with mute key, so it is something
not unrealistic. What should be name convention for these mute leds?

Is not "sys::" prefix too generic?
Takashi Iwai Nov. 28, 2018, 11:30 a.m. UTC | #9
On Wed, 28 Nov 2018 12:14:34 +0100,
Pali Rohár wrote:
> 
> On Monday 26 November 2018 18:11:20 Takashi Iwai wrote:
> > Hi,
> > 
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> > 
> > The latest version of patches are found in topic/leds-trigger branch
> > in my sound git tree.
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> > 
> > As these are cross-tree patches, the branch above is based cleanly on
> > v4.20-rc3, so that it can be merged well to multiple trees.
> > 
> > Once after getting the ACK's, I'll add tags and fixate for merges.
> > 
> > This patch series don't include huawei-wmi stuff; so Huawei patches
> > need rework.  I already have some piece of changes for huawei-wmi, so
> > please ping me if needed.
> > 
> > I checked briefly on my Dell laptop, and a Thinkpad model.
> > Wider tests are appreciated, of course.
> 
> Hi! Thanks for patch series. It is really improvement in current mute
> led situation. I quickly looked at patches and they look good. So you
> can add my Acked-By: Pali Rohár <pali.rohar@gmail.com>.

Added your ack now.  Thanks!


Takashi
Takashi Iwai Nov. 28, 2018, 11:38 a.m. UTC | #10
On Wed, 28 Nov 2018 12:18:06 +0100,
Pali Rohár wrote:
> 
> On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> > Looks good... except one detail: you have "tpacpi::micmute" and
> > "dell::micmute". I know it follows "tradition", but we are trying to
> > fix that at the moment. Laptop micmute button is a laptop micmute
> > button, and userspace should not need to know what prefix to use
> > depending on vendor.
> > 
> > I'd suggest using "sys::micmute".
> 
> I can imagine that in future some devices like keyboards would have also
> mute led. We already have keyboards with mute key, so it is something
> not unrealistic. What should be name convention for these mute leds?
> 
> Is not "sys::" prefix too generic?

Good point.  I thought of "laptop::" but it's not always laptop.
"builtin::"?  Doesn't sound great, either.

A nice godfather is required here...


thanks,

Takashi
Pavel Machek Nov. 28, 2018, 12:25 p.m. UTC | #11
On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:
> On Wed, 28 Nov 2018 12:18:06 +0100,
> Pali Rohár wrote:
> > 
> > On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> > > Looks good... except one detail: you have "tpacpi::micmute" and
> > > "dell::micmute". I know it follows "tradition", but we are trying to
> > > fix that at the moment. Laptop micmute button is a laptop micmute
> > > button, and userspace should not need to know what prefix to use
> > > depending on vendor.
> > > 
> > > I'd suggest using "sys::micmute".
> > 
> > I can imagine that in future some devices like keyboards would have also
> > mute led. We already have keyboards with mute key, so it is something
> > not unrealistic. What should be name convention for these mute leds?
> > 
> > Is not "sys::" prefix too generic?
> 
> Good point.  I thought of "laptop::" but it's not always laptop.
> "builtin::"?  Doesn't sound great, either.
> 
> A nice godfather is required here...

Just use sys:: :-).

laptop:: would work for me, too. (It is always laptop in the cases we
are handling now, right?)

When we get a keyboard with mute led, we'll have to decide if it
should be input6::mute -- because it is on keyboard, or if it is
sys::mute -- because the key is expected to mute whole system.

									Pavel
Takashi Iwai Nov. 28, 2018, 12:58 p.m. UTC | #12
On Wed, 28 Nov 2018 13:25:05 +0100,
Pavel Machek wrote:
> 
> On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:
> > On Wed, 28 Nov 2018 12:18:06 +0100,
> > Pali Rohár wrote:
> > > 
> > > On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> > > > Looks good... except one detail: you have "tpacpi::micmute" and
> > > > "dell::micmute". I know it follows "tradition", but we are trying to
> > > > fix that at the moment. Laptop micmute button is a laptop micmute
> > > > button, and userspace should not need to know what prefix to use
> > > > depending on vendor.
> > > > 
> > > > I'd suggest using "sys::micmute".
> > > 
> > > I can imagine that in future some devices like keyboards would have also
> > > mute led. We already have keyboards with mute key, so it is something
> > > not unrealistic. What should be name convention for these mute leds?
> > > 
> > > Is not "sys::" prefix too generic?
> > 
> > Good point.  I thought of "laptop::" but it's not always laptop.
> > "builtin::"?  Doesn't sound great, either.
> > 
> > A nice godfather is required here...
> 
> Just use sys:: :-).
> 
> laptop:: would work for me, too. (It is always laptop in the cases we
> are handling now, right?)

In theory, such a thing can be on all-in-one desktops, too.

> When we get a keyboard with mute led, we'll have to decide if it
> should be input6::mute -- because it is on keyboard, or if it is
> sys::mute -- because the key is expected to mute whole system.

OK, I'll wait for more comments in today and update accordingly.


thanks,

Takashi
Andy Shevchenko Nov. 28, 2018, 1:40 p.m. UTC | #13
On Wed, Nov 28, 2018 at 2:59 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 28 Nov 2018 13:25:05 +0100,
> > On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:

> > Just use sys:: :-).
> >
> > laptop:: would work for me, too. (It is always laptop in the cases we
> > are handling now, right?)
>
> In theory, such a thing can be on all-in-one desktops, too.
>
> > When we get a keyboard with mute led, we'll have to decide if it
> > should be input6::mute -- because it is on keyboard, or if it is
> > sys::mute -- because the key is expected to mute whole system.
>
> OK, I'll wait for more comments in today and update accordingly.

sys sounds plausible to me. In any case we may revisit in the future
if something happens to it. OTOH it would be part of ABI, right?
Jacek Anaszewski Nov. 28, 2018, 7:58 p.m. UTC | #14
On 11/28/2018 01:25 PM, Pavel Machek wrote:
> On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:
>> On Wed, 28 Nov 2018 12:18:06 +0100,
>> Pali Rohár wrote:
>>>
>>> On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
>>>> Looks good... except one detail: you have "tpacpi::micmute" and
>>>> "dell::micmute". I know it follows "tradition", but we are trying to
>>>> fix that at the moment. Laptop micmute button is a laptop micmute
>>>> button, and userspace should not need to know what prefix to use
>>>> depending on vendor.
>>>>
>>>> I'd suggest using "sys::micmute".
>>>
>>> I can imagine that in future some devices like keyboards would have also
>>> mute led. We already have keyboards with mute key, so it is something
>>> not unrealistic. What should be name convention for these mute leds?
>>>
>>> Is not "sys::" prefix too generic?
>>
>> Good point.  I thought of "laptop::" but it's not always laptop.
>> "builtin::"?  Doesn't sound great, either.
>>
>> A nice godfather is required here...
> 
> Just use sys:: :-).
> 
> laptop:: would work for me, too. (It is always laptop in the cases we
> are handling now, right?)
> 
> When we get a keyboard with mute led, we'll have to decide if it
> should be input6::mute -- because it is on keyboard, or if it is
> sys::mute -- because the key is expected to mute whole system.

drivers/input/input-leds.c seems to already support mute LED.
It will be exposed as inputN::mute.

Documentation/leds/leds-class.txt defines LED naming pattern
to <devicename:color:function> and "sys" does not look as
something resembling device name.
Pavel Machek Nov. 28, 2018, 8:34 p.m. UTC | #15
Hi!

> >>>> Looks good... except one detail: you have "tpacpi::micmute" and
> >>>> "dell::micmute". I know it follows "tradition", but we are trying to
> >>>> fix that at the moment. Laptop micmute button is a laptop micmute
> >>>> button, and userspace should not need to know what prefix to use
> >>>> depending on vendor.
> >>>>
> >>>> I'd suggest using "sys::micmute".
> >>>
> >>> I can imagine that in future some devices like keyboards would have also
> >>> mute led. We already have keyboards with mute key, so it is something
> >>> not unrealistic. What should be name convention for these mute leds?
> >>>
> >>> Is not "sys::" prefix too generic?
> >>
> >> Good point.  I thought of "laptop::" but it's not always laptop.
> >> "builtin::"?  Doesn't sound great, either.
> >>
> >> A nice godfather is required here...
> > 
> > Just use sys:: :-).
> > 
> > laptop:: would work for me, too. (It is always laptop in the cases we
> > are handling now, right?)
> > 
> > When we get a keyboard with mute led, we'll have to decide if it
> > should be input6::mute -- because it is on keyboard, or if it is
> > sys::mute -- because the key is expected to mute whole system.
> 
> drivers/input/input-leds.c seems to already support mute LED.
> It will be exposed as inputN::mute.
> 
> Documentation/leds/leds-class.txt defines LED naming pattern
> to <devicename:color:function> and "sys" does not look as
> something resembling device name.

So what is your suggestion?

I don't care much as long as it is same in tpacpi and dell
case. (Neither are device names, btw :-).

Actually "::mute" would make sense, too.
									
									Pavel
Pali Rohár Nov. 28, 2018, 8:46 p.m. UTC | #16
On Wednesday 28 November 2018 21:34:10 Pavel Machek wrote:
> Hi!
> 
> > >>>> Looks good... except one detail: you have "tpacpi::micmute" and
> > >>>> "dell::micmute". I know it follows "tradition", but we are trying to
> > >>>> fix that at the moment. Laptop micmute button is a laptop micmute
> > >>>> button, and userspace should not need to know what prefix to use
> > >>>> depending on vendor.
> > >>>>
> > >>>> I'd suggest using "sys::micmute".
> > >>>
> > >>> I can imagine that in future some devices like keyboards would have also
> > >>> mute led. We already have keyboards with mute key, so it is something
> > >>> not unrealistic. What should be name convention for these mute leds?
> > >>>
> > >>> Is not "sys::" prefix too generic?
> > >>
> > >> Good point.  I thought of "laptop::" but it's not always laptop.
> > >> "builtin::"?  Doesn't sound great, either.
> > >>
> > >> A nice godfather is required here...
> > > 
> > > Just use sys:: :-).
> > > 
> > > laptop:: would work for me, too. (It is always laptop in the cases we
> > > are handling now, right?)
> > > 
> > > When we get a keyboard with mute led, we'll have to decide if it
> > > should be input6::mute -- because it is on keyboard, or if it is
> > > sys::mute -- because the key is expected to mute whole system.
> > 
> > drivers/input/input-leds.c seems to already support mute LED.
> > It will be exposed as inputN::mute.
> > 
> > Documentation/leds/leds-class.txt defines LED naming pattern
> > to <devicename:color:function> and "sys" does not look as
> > something resembling device name.
> 
> So what is your suggestion?

I guess we should follow documentation. Or update documentation if it
does not make sense to follow it.

> I don't care much as long as it is same in tpacpi and dell
> case. (Neither are device names, btw :-).
> 
> Actually "::mute" would make sense, too.

"::mute" is not a good idea due to name uniqueness.

In case you would have two drivers which both provides "mute" led, then
they need to have different name. Reason also why generic name "sys" is
not a good idea.

input subsystem seems to solved this problem by appending number after
"input" word.

I think that driver name or subsystem name would be usable together with
number.

Userspace application would be probably interested to distinguish
between "mute led which is part of laptop" and "mute led which is
available on external USB keyboard".

If external USB keyboard is identified as "input7" device, then
"input7::mute" is a good name for mute key. But "sys::mute" does not say
anything to which device or hardware it belongs nor does not solve
problem that which device/driver/subsystem should have privilege to take
this "sys" name.
Jacek Anaszewski Nov. 28, 2018, 9 p.m. UTC | #17
On 11/28/2018 09:46 PM, Pali Rohár wrote:
> On Wednesday 28 November 2018 21:34:10 Pavel Machek wrote:
>> Hi!
>>
>>>>>>> Looks good... except one detail: you have "tpacpi::micmute" and
>>>>>>> "dell::micmute". I know it follows "tradition", but we are trying to
>>>>>>> fix that at the moment. Laptop micmute button is a laptop micmute
>>>>>>> button, and userspace should not need to know what prefix to use
>>>>>>> depending on vendor.
>>>>>>>
>>>>>>> I'd suggest using "sys::micmute".
>>>>>>
>>>>>> I can imagine that in future some devices like keyboards would have also
>>>>>> mute led. We already have keyboards with mute key, so it is something
>>>>>> not unrealistic. What should be name convention for these mute leds?
>>>>>>
>>>>>> Is not "sys::" prefix too generic?
>>>>>
>>>>> Good point.  I thought of "laptop::" but it's not always laptop.
>>>>> "builtin::"?  Doesn't sound great, either.
>>>>>
>>>>> A nice godfather is required here...
>>>>
>>>> Just use sys:: :-).
>>>>
>>>> laptop:: would work for me, too. (It is always laptop in the cases we
>>>> are handling now, right?)
>>>>
>>>> When we get a keyboard with mute led, we'll have to decide if it
>>>> should be input6::mute -- because it is on keyboard, or if it is
>>>> sys::mute -- because the key is expected to mute whole system.
>>>
>>> drivers/input/input-leds.c seems to already support mute LED.
>>> It will be exposed as inputN::mute.
>>>
>>> Documentation/leds/leds-class.txt defines LED naming pattern
>>> to <devicename:color:function> and "sys" does not look as
>>> something resembling device name.
>>
>> So what is your suggestion?
> 
> I guess we should follow documentation. Or update documentation if it
> does not make sense to follow it.
> 
>> I don't care much as long as it is same in tpacpi and dell
>> case. (Neither are device names, btw :-).
>>
>> Actually "::mute" would make sense, too.
> 
> "::mute" is not a good idea due to name uniqueness.

LED core adds a numerical suffix to the original name
if it is already taken. Of course it is a last resort just
to avoid name clash.

> In case you would have two drivers which both provides "mute" led, then
> they need to have different name. Reason also why generic name "sys" is
> not a good idea.
> 
> input subsystem seems to solved this problem by appending number after
> "input" word.
> 
> I think that driver name or subsystem name would be usable together with
> number.
> 
> Userspace application would be probably interested to distinguish
> between "mute led which is part of laptop" and "mute led which is
> available on external USB keyboard".
> 
> If external USB keyboard is identified as "input7" device, then
> "input7::mute" is a good name for mute key. But "sys::mute" does not say
> anything to which device or hardware it belongs nor does not solve
> problem that which device/driver/subsystem should have privilege to take
> this "sys" name.

How about just "platform" for the LEDs being part of the device
on which the system is running?
Pavel Machek Nov. 28, 2018, 9:01 p.m. UTC | #18
Hi!

> > > >> A nice godfather is required here...
> > > > 
> > > > Just use sys:: :-).
> > > > 
> > > > laptop:: would work for me, too. (It is always laptop in the cases we
> > > > are handling now, right?)
> > > > 
> > > > When we get a keyboard with mute led, we'll have to decide if it
> > > > should be input6::mute -- because it is on keyboard, or if it is
> > > > sys::mute -- because the key is expected to mute whole system.
> > > 
> > > drivers/input/input-leds.c seems to already support mute LED.
> > > It will be exposed as inputN::mute.
> > > 
> > > Documentation/leds/leds-class.txt defines LED naming pattern
> > > to <devicename:color:function> and "sys" does not look as
> > > something resembling device name.
> > 
> > So what is your suggestion?
> 
> I guess we should follow documentation. Or update documentation if it
> does not make sense to follow it.

That's actually in progress. Let me and Jacek deal with it. We don't
need great "how to name a LED" discussion here (google: bike shed paiting).

> > I don't care much as long as it is same in tpacpi and dell
> > case. (Neither are device names, btw :-).
> > 
> > Actually "::mute" would make sense, too.
> 
> "::mute" is not a good idea due to name uniqueness.
> 
> In case you would have two drivers which both provides "mute" led, then
> they need to have different name. Reason also why generic name "sys" is
> not a good idea.

> I think that driver name or subsystem name would be usable together with
> number.
> 
> Userspace application would be probably interested to distinguish
> between "mute led which is part of laptop" and "mute led which is
> available on external USB keyboard".
> 
> If external USB keyboard is identified as "input7" device, then
> "input7::mute" is a good name for mute key. But "sys::mute" does not say
> anything to which device or hardware it belongs nor does not solve
> problem that which device/driver/subsystem should have privilege to take
> this "sys" name.

Well, "sys" says this is system-wide mute LED. There are not expected
to be two of those, and there never will be, with the drivers
currently proposed.

								Pavel
Pavel Machek Nov. 28, 2018, 9:22 p.m. UTC | #19
Hi!

> > If external USB keyboard is identified as "input7" device, then
> > "input7::mute" is a good name for mute key. But "sys::mute" does not say
> > anything to which device or hardware it belongs nor does not solve
> > problem that which device/driver/subsystem should have privilege to take
> > this "sys" name.
> 
> How about just "platform" for the LEDs being part of the device
> on which the system is running?

"platform" works for me.

Are we in agreement that this name will be used for all similar LEDs,
as long as they are on the "main box" of the device, no matter if they
are connected using acpi, gpio, i2c, ...?

Should we start Documentation file explaining common names we want
people to use?

Thanks,
									Pavel
Jacek Anaszewski Nov. 29, 2018, 9:23 p.m. UTC | #20
On 11/28/2018 10:22 PM, Pavel Machek wrote:
> Hi!
> 
>>> If external USB keyboard is identified as "input7" device, then
>>> "input7::mute" is a good name for mute key. But "sys::mute" does not say
>>> anything to which device or hardware it belongs nor does not solve
>>> problem that which device/driver/subsystem should have privilege to take
>>> this "sys" name.
>>
>> How about just "platform" for the LEDs being part of the device
>> on which the system is running?
> 
> "platform" works for me.
> 
> Are we in agreement that this name will be used for all similar LEDs,
> as long as they are on the "main box" of the device, no matter if they
> are connected using acpi, gpio, i2c, ...?

One doubt: say we have hdd activity LED on the "main box" - it
would be named "platform::disk".

Now, we add external USB disk. Previously we were considering
the naming for disk LEDs in the form e.g. "sdb::disk".

If so, there would be discrepancy between internal and external disk
LED names. Similarly in case of eth/adsl/wlan/camera LEDs.

Regarding sound devices - how would we name micmute LED for USB
microphone? I suppose that it is possible to discover some unique
identifier in runtime - this is a question to ALSA people.
Takashi Iwai Nov. 29, 2018, 9:56 p.m. UTC | #21
On Thu, 29 Nov 2018 22:23:31 +0100,
Jacek Anaszewski wrote:
> 
> On 11/28/2018 10:22 PM, Pavel Machek wrote:
> > Hi!
> > 
> >>> If external USB keyboard is identified as "input7" device, then
> >>> "input7::mute" is a good name for mute key. But "sys::mute" does not say
> >>> anything to which device or hardware it belongs nor does not solve
> >>> problem that which device/driver/subsystem should have privilege to take
> >>> this "sys" name.
> >>
> >> How about just "platform" for the LEDs being part of the device
> >> on which the system is running?
> > 
> > "platform" works for me.
> > 
> > Are we in agreement that this name will be used for all similar LEDs,
> > as long as they are on the "main box" of the device, no matter if they
> > are connected using acpi, gpio, i2c, ...?
> 
> One doubt: say we have hdd activity LED on the "main box" - it
> would be named "platform::disk".
> 
> Now, we add external USB disk. Previously we were considering
> the naming for disk LEDs in the form e.g. "sdb::disk".
> 
> If so, there would be discrepancy between internal and external disk
> LED names. Similarly in case of eth/adsl/wlan/camera LEDs.
> 
> Regarding sound devices - how would we name micmute LED for USB
> microphone? I suppose that it is possible to discover some unique
> identifier in runtime - this is a question to ALSA people.

There are little USB-audio devices supporting the LED control, so we
haven't had any chance for LED class for USB-audio.

At most, we may create LED class devices for some HD-audio (usually
driven via exotic verbs or GPIO over HD-audio), but I wouldn't go in
that direction; it'll need too make things more complex than the
direct hook as of now.


thanks,

Takashi
Andy Shevchenko Nov. 30, 2018, 10:42 a.m. UTC | #22
On Thu, Nov 29, 2018 at 10:23:31PM +0100, Jacek Anaszewski wrote:

> Regarding sound devices - how would we name micmute LED for USB
> microphone? I suppose that it is possible to discover some unique
> identifier in runtime - this is a question to ALSA people.

USB path (e.g. 2-1.1.2.4.3) + '::' + name ?