[PATCH 0/7] Move dell-led to drivers/platform/x86

[PATCH 0/7] Move dell-led to drivers/platform/x86

[Michał Kępień]

This patch series moves the dell-led driver from the LED subsystem to
the x86 platform driver subsystem.  I decided to also CC the sound
subsystem contacts for the whole series as
sound/pci/hda/dell_wmi_helper.c is also affected.

The original motivation behind this effort was to move all code using
the dell-smbios module to the x86 platform driver subsystem.  While I
was investigating the possibilites to do that, it quickly emerged that
dell-led can and in fact should be moved to the x86 platform driver
subsystem in its entirety.

dell-led consists of two major parts:

  - the part exposing a microphone mute LED interface, introduced in
    db6d8cc ("dell-led: add mic mute led interface"); this interface is
    used by sound/pci/hda/dell_wmi_helper.c; while the original
    implementation used a WMI interface, it was changed to use
    dell-smbios in cf0d7ea ("dell-led: use dell_smbios_find_token() for
    finding mic DMI tokens") and 0c41a08 ("dell-led: use
    dell_smbios_send_request() for performing SMBIOS calls"),

  - the part handling an activity LED present in Dell Latitude 2100
    netbooks, introduced in 72dcd8d ("leds: Add Dell Business Class
    Netbook LED driver"); it binds to a specific WMI GUID and then
    registers a LED device which is controlled using WMI (i.e. it is
    basically a WMI driver).

Patches 1-4 clean up the microphone mute LED interface to minimize the
amount of code moved around.
  
Patch 5 moves the microphone mute LED interface to
drivers/platform/x86/dell-laptop.c, effectively causing
sound/pci/hda/dell_wmi_helper.c to depend on CONFIG_DELL_LAPTOP instead
of CONFIG_LEDS_DELL_NETBOOKS.
    
Patch 6 reverts dell-led to the state it was in after its initial commit
72dcd8d ("leds: Add Dell Business Class Netbook LED driver") by removing
all remnants of the microphone mute LED handling code.

Patch 7 moves all that is left of dell-led (i.e. the activity LED part,
as originally implemented), to a new module which is placed in
drivers/platform/x86/dell-wmi-led.c.

This patch series is based on linux-leds/for-4.11 as the LED subsystem
is affected by all patches except patch 3.

If anyone reading this has access to a Dell device which has an activity
LED and/or a microphone mute LED currently supported by dell-led, I
would love to hear from you as I do not have the hardware needed to
practically test this patch series.

 drivers/leds/Kconfig                               |  9 ---
 drivers/leds/Makefile                              |  1 -
 drivers/platform/x86/Kconfig                       |  8 +++
 drivers/platform/x86/Makefile                      |  1 +
 drivers/platform/x86/dell-laptop.c                 | 28 ++++++++
 .../dell-led.c => platform/x86/dell-wmi-led.c}     | 75 +++-------------------
 include/linux/dell-led.h                           |  6 +-
 sound/pci/hda/dell_wmi_helper.c                    | 18 +++---
 8 files changed, 55 insertions(+), 91 deletions(-)
 rename drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} (73%)

-- 
2.10.2

[PATCH 1/7] dell-led: remove GUID check from dell_micmute_led_set()

[Michał Kępień]

As dell_micmute_led_set() no longer uses the dell_wmi_perform_query()
method, which was removed in 0c41a08 ("dell-led: use
dell_smbios_send_request() for performing SMBIOS calls"), the
DELL_APP_GUID check is redundant and thus can be safely removed.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/leds/dell-led.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
index b3d6e9c..e8e8f67 100644
--- a/drivers/leds/dell-led.c
+++ b/drivers/leds/dell-led.c
@@ -51,9 +51,6 @@ static int dell_micmute_led_set(int state)
 	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
 
-	if (!wmi_has_guid(DELL_APP_GUID))
-		return -ENODEV;
-
 	if (state == 0)
 		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
 	else if (state == 1)
-- 
2.10.2

Re: [PATCH 1/7] dell-led: remove GUID check from dell_micmute_led_set()

[Pali Rohár]

On Thursday 08 December 2016 13:36:12 Michał Kępień wrote:
> As dell_micmute_led_set() no longer uses the dell_wmi_perform_query()
> method, which was removed in 0c41a08 ("dell-led: use
> dell_smbios_send_request() for performing SMBIOS calls"), the
> DELL_APP_GUID check is redundant and thus can be safely removed.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/leds/dell-led.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
> index b3d6e9c..e8e8f67 100644
> --- a/drivers/leds/dell-led.c
> +++ b/drivers/leds/dell-led.c
> @@ -51,9 +51,6 @@ static int dell_micmute_led_set(int state)
>  	struct calling_interface_buffer *buffer;
>  	struct calling_interface_token *token;
>  
> -	if (!wmi_has_guid(DELL_APP_GUID))
> -		return -ENODEV;
> -
>  	if (state == 0)
>  		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
>  	else if (state == 1)

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Anyway, you can remove DELL_APP_GUID from other places too...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/7] dell-led: remove GUID check from dell_micmute_led_set()

[Michał Kępień]

> On Thursday 08 December 2016 13:36:12 Michał Kępień wrote:
> > As dell_micmute_led_set() no longer uses the dell_wmi_perform_query()
> > method, which was removed in 0c41a08 ("dell-led: use
> > dell_smbios_send_request() for performing SMBIOS calls"), the
> > DELL_APP_GUID check is redundant and thus can be safely removed.
> > 
> > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > ---
> >  drivers/leds/dell-led.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
> > index b3d6e9c..e8e8f67 100644
> > --- a/drivers/leds/dell-led.c
> > +++ b/drivers/leds/dell-led.c
> > @@ -51,9 +51,6 @@ static int dell_micmute_led_set(int state)
> >  	struct calling_interface_buffer *buffer;
> >  	struct calling_interface_token *token;
> >  
> > -	if (!wmi_has_guid(DELL_APP_GUID))
> > -		return -ENODEV;
> > -
> >  	if (state == 0)
> >  		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
> >  	else if (state == 1)
> 
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Thanks for reviewing.

> Anyway, you can remove DELL_APP_GUID from other places too...

I did that in patch 6.  I singled out this one occurrence because it is
part of the code that is moved to drivers/platform/x86.  Please let me
know if you would like me to arrange this differently.

-- 
Best regards,
Michał Kępień

[PATCH 2/7] dell-led: export dell_micmute_led_set()

[Michał Kępień]

When the dell_app_wmi_led_set() method was introduced in db6d8cc
("dell-led: add mic mute led interface"), it was implemented as an
easily extensible entry point for other modules to set the state of
various LEDs.  However, almost three years later it is still only used
to control the mic mute LED, so it will be replaced with direct calls to
dell_micmute_led_set().  For this to be possible, dell_micmute_led_set()
has to be exported first.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/leds/dell-led.c  | 3 ++-
 include/linux/dell-led.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
index e8e8f67..b215248 100644
--- a/drivers/leds/dell-led.c
+++ b/drivers/leds/dell-led.c
@@ -46,7 +46,7 @@ MODULE_ALIAS("wmi:" DELL_LED_BIOS_GUID);
 #define GLOBAL_MIC_MUTE_ENABLE	0x364
 #define GLOBAL_MIC_MUTE_DISABLE	0x365
 
-static int dell_micmute_led_set(int state)
+int dell_micmute_led_set(int state)
 {
 	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
@@ -69,6 +69,7 @@ static int dell_micmute_led_set(int state)
 
 	return state;
 }
+EXPORT_SYMBOL_GPL(dell_micmute_led_set);
 
 int dell_app_wmi_led_set(int whichled, int on)
 {
diff --git a/include/linux/dell-led.h b/include/linux/dell-led.h
index 7009b8b..1b03275 100644
--- a/include/linux/dell-led.h
+++ b/include/linux/dell-led.h
@@ -5,6 +5,7 @@ enum {
 	DELL_LED_MICMUTE,
 };
 
+int dell_micmute_led_set(int on);
 int dell_app_wmi_led_set(int whichled, int on);
 
 #endif
-- 
2.10.2

[PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Michał Kępień]

When the dell_app_wmi_led_set() method was introduced in db6d8cc
("dell-led: add mic mute led interface"), it was implemented as an
easily extensible entry point for other modules to set the state of
various LEDs.  However, almost three years later it is still only used
to control the mic mute LED, so it will be replaced with direct calls to
dell_micmute_led_set().

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 sound/pci/hda/dell_wmi_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c
index 19d41da..e128c80 100644
--- a/sound/pci/hda/dell_wmi_helper.c
+++ b/sound/pci/hda/dell_wmi_helper.c
@@ -6,7 +6,7 @@
 #include <linux/dell-led.h>
 
 static int dell_led_value;
-static int (*dell_led_set_func)(int, int);
+static int (*dell_led_set_func)(int);
 static void (*dell_old_cap_hook)(struct hda_codec *,
 			         struct snd_kcontrol *,
 				 struct snd_ctl_elem_value *);
@@ -27,7 +27,7 @@ static void update_dell_wmi_micmute_led(struct hda_codec *codec,
 			return;
 		dell_led_value = val;
 		if (dell_led_set_func)
-			dell_led_set_func(DELL_LED_MICMUTE, dell_led_value);
+			dell_led_set_func(dell_led_value);
 	}
 }
 
@@ -40,14 +40,14 @@ static void alc_fixup_dell_wmi(struct hda_codec *codec,
 
 	if (action == HDA_FIXUP_ACT_PROBE) {
 		if (!dell_led_set_func)
-			dell_led_set_func = symbol_request(dell_app_wmi_led_set);
+			dell_led_set_func = symbol_request(dell_micmute_led_set);
 		if (!dell_led_set_func) {
-			codec_warn(codec, "Failed to find dell wmi symbol dell_app_wmi_led_set\n");
+			codec_warn(codec, "Failed to find dell wmi symbol dell_micmute_led_set\n");
 			return;
 		}
 
 		removefunc = true;
-		if (dell_led_set_func(DELL_LED_MICMUTE, false) >= 0) {
+		if (dell_led_set_func(false) >= 0) {
 			dell_led_value = 0;
 			if (spec->gen.num_adc_nids > 1 && !spec->gen.dyn_adc_switch)
 				codec_dbg(codec, "Skipping micmute LED control due to several ADCs");
@@ -61,7 +61,7 @@ static void alc_fixup_dell_wmi(struct hda_codec *codec,
 	}
 
 	if (dell_led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
-		symbol_put(dell_app_wmi_led_set);
+		symbol_put(dell_micmute_led_set);
 		dell_led_set_func = NULL;
 		dell_old_cap_hook = NULL;
 	}
-- 
2.10.2

Re: [PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 493 bytes --]

On Thursday 08 December 2016 13:36:14 Michał Kępień wrote:
> --- a/sound/pci/hda/dell_wmi_helper.c
> +++ b/sound/pci/hda/dell_wmi_helper.c
> @@ -6,7 +6,7 @@
>  #include <linux/dell-led.h>
> 
>  static int dell_led_value;
> -static int (*dell_led_set_func)(int, int);
> +static int (*dell_led_set_func)(int);

Suggestion: what about changing name to dell_micmute_led_set_func to 
match real function name which is used after this patch?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Michał Kępień]

> On Thursday 08 December 2016 13:36:14 Michał Kępień wrote:
> > --- a/sound/pci/hda/dell_wmi_helper.c
> > +++ b/sound/pci/hda/dell_wmi_helper.c
> > @@ -6,7 +6,7 @@
> >  #include <linux/dell-led.h>
> > 
> >  static int dell_led_value;
> > -static int (*dell_led_set_func)(int, int);
> > +static int (*dell_led_set_func)(int);
> 
> Suggestion: what about changing name to dell_micmute_led_set_func to 
> match real function name which is used after this patch?

While I like the idea itself, implementing it will double the number of
lines that this patch changes (6 vs. 12), arguably making its intention
less clear.  Please let me know if you would really like this to happen
(perhaps as a separate patch?), otherwise I will skip this idea in v2.

-- 
Best regards,
Michał Kępień

Re: [PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Pali Rohár]

On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote:
> > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote:
> > > --- a/sound/pci/hda/dell_wmi_helper.c
> > > +++ b/sound/pci/hda/dell_wmi_helper.c
> > > @@ -6,7 +6,7 @@
> > > #include <linux/dell-led.h>
> > > 
> > > static int dell_led_value;
> > > -static int (*dell_led_set_func)(int, int);
> > > +static int (*dell_led_set_func)(int);
> > 
> > Suggestion: what about changing name to dell_micmute_led_set_func to 
> > match real function name which is used after this patch?
> 
> While I like the idea itself, implementing it will double the number of
> lines that this patch changes (6 vs. 12), arguably making its intention

If some patch makes final result of code better then metric number of lines is not priority.

> less clear.   Please let me know if you would really like this to happen
> (perhaps as a separate patch?), otherwise I will skip this idea in v2.
> 
> -- 
> Best regards,
> Michał Kępień

If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Michał Kępień]

> On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote:
> > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote:
> > > > --- a/sound/pci/hda/dell_wmi_helper.c
> > > > +++ b/sound/pci/hda/dell_wmi_helper.c
> > > > @@ -6,7 +6,7 @@
> > > > #include <linux/dell-led.h>
> > > > 
> > > > static int dell_led_value;
> > > > -static int (*dell_led_set_func)(int, int);
> > > > +static int (*dell_led_set_func)(int);
> > > 
> > > Suggestion: what about changing name to dell_micmute_led_set_func to 
> > > match real function name which is used after this patch?
> > 
> > While I like the idea itself, implementing it will double the number of
> > lines that this patch changes (6 vs. 12), arguably making its intention
> 
> If some patch makes final result of code better then metric number of lines is not priority.
> 
> > less clear.   Please let me know if you would really like this to happen
> > (perhaps as a separate patch?), otherwise I will skip this idea in v2.
> > 
> > -- 
> > Best regards,
> > Michał Kępień
> 
> If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer.

Good point.  Jaroslav, Takashi, I will patiently wait for your opinion
on this patch and Pali's suggestion quoted above before I post v2 of
this series.

-- 
Best regards,
Michał Kępień

Re: [PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Michał Kępień]

> > On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote:
> > > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote:
> > > > > --- a/sound/pci/hda/dell_wmi_helper.c
> > > > > +++ b/sound/pci/hda/dell_wmi_helper.c
> > > > > @@ -6,7 +6,7 @@
> > > > > #include <linux/dell-led.h>
> > > > > 
> > > > > static int dell_led_value;
> > > > > -static int (*dell_led_set_func)(int, int);
> > > > > +static int (*dell_led_set_func)(int);
> > > > 
> > > > Suggestion: what about changing name to dell_micmute_led_set_func to 
> > > > match real function name which is used after this patch?
> > > 
> > > While I like the idea itself, implementing it will double the number of
> > > lines that this patch changes (6 vs. 12), arguably making its intention
> > 
> > If some patch makes final result of code better then metric number of lines is not priority.
> > 
> > > less clear.   Please let me know if you would really like this to happen
> > > (perhaps as a separate patch?), otherwise I will skip this idea in v2.
> > > 
> > > -- 
> > > Best regards,
> > > Michał Kępień
> > 
> > If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer.
> 
> Good point.  Jaroslav, Takashi, I will patiently wait for your opinion
> on this patch and Pali's suggestion quoted above before I post v2 of
> this series.

As it has already been a month since I posted this patch series, this is
just a reminder that I am waiting for comments from the sound subsystem
maintainers before I post v2.

Jaroslav, Takashi, I could really use your input.  Please let me know if
something is unclear.  Thanks,

-- 
Best regards,
Michał Kępień

Re: [PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Takashi Iwai]

On Mon, 09 Jan 2017 14:26:31 +0100,
Michał Kępień wrote:
> 
> > > On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote:
> > > > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote:
> > > > > > --- a/sound/pci/hda/dell_wmi_helper.c
> > > > > > +++ b/sound/pci/hda/dell_wmi_helper.c
> > > > > > @@ -6,7 +6,7 @@
> > > > > > #include <linux/dell-led.h>
> > > > > > 
> > > > > > static int dell_led_value;
> > > > > > -static int (*dell_led_set_func)(int, int);
> > > > > > +static int (*dell_led_set_func)(int);
> > > > > 
> > > > > Suggestion: what about changing name to dell_micmute_led_set_func to 
> > > > > match real function name which is used after this patch?
> > > > 
> > > > While I like the idea itself, implementing it will double the number of
> > > > lines that this patch changes (6 vs. 12), arguably making its intention
> > > 
> > > If some patch makes final result of code better then metric number of lines is not priority.
> > > 
> > > > less clear.   Please let me know if you would really like this to happen
> > > > (perhaps as a separate patch?), otherwise I will skip this idea in v2.
> > > > 
> > > > -- 
> > > > Best regards,
> > > > Michał Kępień
> > > 
> > > If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer.
> > 
> > Good point.  Jaroslav, Takashi, I will patiently wait for your opinion
> > on this patch and Pali's suggestion quoted above before I post v2 of
> > this series.
> 
> As it has already been a month since I posted this patch series, this is
> just a reminder that I am waiting for comments from the sound subsystem
> maintainers before I post v2.
> 
> Jaroslav, Takashi, I could really use your input.  Please let me know if
> something is unclear.  Thanks,

Sorry, the last post was overseen as I already went off to vacation.

The rename can be done in another patch.  Doing multiple things in a
single patch often leads to more confusion and error-prone.

Other than that, I'm fine with rename.


thanks,

Takashi

Re: [PATCH 3/7] ALSA: hda - use dell_micmute_led_set() instead of dell_app_wmi_led_set()

[Michał Kępień]

> On Mon, 09 Jan 2017 14:26:31 +0100,
> Michał Kępień wrote:
> > 
> > > > On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote:
> > > > > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote:
> > > > > > > --- a/sound/pci/hda/dell_wmi_helper.c
> > > > > > > +++ b/sound/pci/hda/dell_wmi_helper.c
> > > > > > > @@ -6,7 +6,7 @@
> > > > > > > #include <linux/dell-led.h>
> > > > > > > 
> > > > > > > static int dell_led_value;
> > > > > > > -static int (*dell_led_set_func)(int, int);
> > > > > > > +static int (*dell_led_set_func)(int);
> > > > > > 
> > > > > > Suggestion: what about changing name to dell_micmute_led_set_func to 
> > > > > > match real function name which is used after this patch?
> > > > > 
> > > > > While I like the idea itself, implementing it will double the number of
> > > > > lines that this patch changes (6 vs. 12), arguably making its intention
> > > > 
> > > > If some patch makes final result of code better then metric number of lines is not priority.
> > > > 
> > > > > less clear.   Please let me know if you would really like this to happen
> > > > > (perhaps as a separate patch?), otherwise I will skip this idea in v2.
> > > > > 
> > > > > -- 
> > > > > Best regards,
> > > > > Michał Kępień
> > > > 
> > > > If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer.
> > > 
> > > Good point.  Jaroslav, Takashi, I will patiently wait for your opinion
> > > on this patch and Pali's suggestion quoted above before I post v2 of
> > > this series.
> > 
> > As it has already been a month since I posted this patch series, this is
> > just a reminder that I am waiting for comments from the sound subsystem
> > maintainers before I post v2.
> > 
> > Jaroslav, Takashi, I could really use your input.  Please let me know if
> > something is unclear.  Thanks,
> 
> Sorry, the last post was overseen as I already went off to vacation.

No worries, thank you for getting back to me.

> The rename can be done in another patch.  Doing multiple things in a
> single patch often leads to more confusion and error-prone.
> 
> Other than that, I'm fine with rename.

Thank you for your comments, I will keep them in mind for v2.

-- 
Best regards,
Michał Kępień

[PATCH 4/7] dell-led: remove dell_app_wmi_led_set()

[Michał Kępień]

All calls to dell_app_wmi_led_set() have been replaced with direct calls
to dell_micmute_led_set(), so the former can be safely removed along
with its related enum.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/leds/dell-led.c  | 17 -----------------
 include/linux/dell-led.h |  5 -----
 2 files changed, 22 deletions(-)

diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
index b215248..f9002d9 100644
--- a/drivers/leds/dell-led.c
+++ b/drivers/leds/dell-led.c
@@ -71,23 +71,6 @@ int dell_micmute_led_set(int state)
 }
 EXPORT_SYMBOL_GPL(dell_micmute_led_set);
 
-int dell_app_wmi_led_set(int whichled, int on)
-{
-	int state = 0;
-
-	switch (whichled) {
-	case DELL_LED_MICMUTE:
-		state = dell_micmute_led_set(on);
-		break;
-	default:
-		pr_warn("led type %x is not supported\n", whichled);
-		break;
-	}
-
-	return state;
-}
-EXPORT_SYMBOL_GPL(dell_app_wmi_led_set);
-
 struct bios_args {
 	unsigned char length;
 	unsigned char result_code;
diff --git a/include/linux/dell-led.h b/include/linux/dell-led.h
index 1b03275..3f033c4 100644
--- a/include/linux/dell-led.h
+++ b/include/linux/dell-led.h
@@ -1,11 +1,6 @@
 #ifndef __DELL_LED_H__
 #define __DELL_LED_H__
 
-enum {
-	DELL_LED_MICMUTE,
-};
-
 int dell_micmute_led_set(int on);
-int dell_app_wmi_led_set(int whichled, int on);
 
 #endif
-- 
2.10.2

Re: [PATCH 4/7] dell-led: remove dell_app_wmi_led_set()

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 566 bytes --]

On Thursday 08 December 2016 13:36:15 Michał Kępień wrote:
> All calls to dell_app_wmi_led_set() have been replaced with direct
> calls to dell_micmute_led_set(), so the former can be safely removed
> along with its related enum.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>

I would suggest to squash patches 2,3,4 into one. But I let decision to 
alsa & led maintainers.

Anyway, for patches 2,3,4 you can add my Reviewed-by. It is nice cleanup

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 4/7] dell-led: remove dell_app_wmi_led_set()

[Andy Shevchenko]

On Sun, Dec 11, 2016 at 12:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 08 December 2016 13:36:15 Michał Kępień wrote:
>> All calls to dell_app_wmi_led_set() have been replaced with direct
>> calls to dell_micmute_led_set(), so the former can be safely removed
>> along with its related enum.
>>
>> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
>
> I would suggest to squash patches 2,3,4 into one. But I let decision to
> alsa & led maintainers.

I don't like the part where we are exporting something for just one
moment. So, +1 to squashed version.

>
> Anyway, for patches 2,3,4 you can add my Reviewed-by. It is nice cleanup
>
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
>
> --
> Pali Rohár
> pali.rohar@gmail.com



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] dell-led: remove dell_app_wmi_led_set()

[Andy Shevchenko]

On Wed, Dec 14, 2016 at 3:54 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Dec 11, 2016 at 12:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>> On Thursday 08 December 2016 13:36:15 Michał Kępień wrote:
>>> All calls to dell_app_wmi_led_set() have been replaced with direct
>>> calls to dell_micmute_led_set(), so the former can be safely removed
>>> along with its related enum.
>>>
>>> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
>>
>> I would suggest to squash patches 2,3,4 into one. But I let decision to
>> alsa & led maintainers.
>
> I don't like the part where we are exporting something for just one
> moment.

Oops, misread function name, though still valid vote for one patch.

> So, +1 to squashed version.
>
>>
>> Anyway, for patches 2,3,4 you can add my Reviewed-by. It is nice cleanup
>>
>> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] dell-led: remove dell_app_wmi_led_set()

[Michał Kępień]

> On Wed, Dec 14, 2016 at 3:54 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Dec 11, 2016 at 12:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> >> On Thursday 08 December 2016 13:36:15 Michał Kępień wrote:
> >>> All calls to dell_app_wmi_led_set() have been replaced with direct
> >>> calls to dell_micmute_led_set(), so the former can be safely removed
> >>> along with its related enum.
> >>>
> >>> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> >>
> >> I would suggest to squash patches 2,3,4 into one. But I let decision to
> >> alsa & led maintainers.
> >
> > I don't like the part where we are exporting something for just one
> > moment.
> 
> Oops, misread function name, though still valid vote for one patch.

Thanks, I will do that in v2 (and thanks to Pali for suggesting it).

-- 
Best regards,
Michał Kępień

[PATCH 5/7] dell-laptop: import dell_micmute_led_set() from drivers/leds/dell-led.c

[Michał Kępień]

To ensure all users of dell-smbios are in drivers/platform/x86, move the
dell_micmute_led_set() method from drivers/leds/dell-led.c to
drivers/platform/x86/dell-laptop.c.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/leds/dell-led.c            | 29 -----------------------------
 drivers/platform/x86/dell-laptop.c | 28 ++++++++++++++++++++++++++++
 sound/pci/hda/dell_wmi_helper.c    |  6 +++---
 3 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
index f9002d9..c9cc36a 100644
--- a/drivers/leds/dell-led.c
+++ b/drivers/leds/dell-led.c
@@ -16,7 +16,6 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
-#include <linux/dell-led.h>
 #include "../platform/x86/dell-smbios.h"
 
 MODULE_AUTHOR("Louis Davis/Jim Dailey");
@@ -43,34 +42,6 @@ MODULE_ALIAS("wmi:" DELL_LED_BIOS_GUID);
 #define CMD_LED_OFF	17
 #define CMD_LED_BLINK	18
 
-#define GLOBAL_MIC_MUTE_ENABLE	0x364
-#define GLOBAL_MIC_MUTE_DISABLE	0x365
-
-int dell_micmute_led_set(int state)
-{
-	struct calling_interface_buffer *buffer;
-	struct calling_interface_token *token;
-
-	if (state == 0)
-		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
-	else if (state == 1)
-		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE);
-	else
-		return -EINVAL;
-
-	if (!token)
-		return -ENODEV;
-
-	buffer = dell_smbios_get_buffer();
-	buffer->input[0] = token->location;
-	buffer->input[1] = token->value;
-	dell_smbios_send_request(1, 0);
-	dell_smbios_release_buffer();
-
-	return state;
-}
-EXPORT_SYMBOL_GPL(dell_micmute_led_set);
-
 struct bios_args {
 	unsigned char length;
 	unsigned char result_code;
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 2c2f02b..277656c 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -30,6 +30,7 @@
 #include <linux/i8042.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/dell-led.h>
 #include <acpi/video.h>
 #include "dell-rbtn.h"
 #include "dell-smbios.h"
@@ -42,6 +43,8 @@
 #define KBD_LED_AUTO_50_TOKEN 0x02EB
 #define KBD_LED_AUTO_75_TOKEN 0x02EC
 #define KBD_LED_AUTO_100_TOKEN 0x02F6
+#define GLOBAL_MIC_MUTE_ENABLE 0x364
+#define GLOBAL_MIC_MUTE_DISABLE 0x365
 
 struct quirk_entry {
 	u8 touchpad_led;
@@ -1970,6 +1973,31 @@ static void kbd_led_exit(void)
 	led_classdev_unregister(&kbd_led);
 }
 
+int dell_micmute_led_set(int state)
+{
+	struct calling_interface_buffer *buffer;
+	struct calling_interface_token *token;
+
+	if (state == 0)
+		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
+	else if (state == 1)
+		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE);
+	else
+		return -EINVAL;
+
+	if (!token)
+		return -ENODEV;
+
+	buffer = dell_smbios_get_buffer();
+	buffer->input[0] = token->location;
+	buffer->input[1] = token->value;
+	dell_smbios_send_request(1, 0);
+	dell_smbios_release_buffer();
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(dell_micmute_led_set);
+
 static int __init dell_init(void)
 {
 	struct calling_interface_buffer *buffer;
diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c
index e128c80..7983d61 100644
--- a/sound/pci/hda/dell_wmi_helper.c
+++ b/sound/pci/hda/dell_wmi_helper.c
@@ -2,7 +2,7 @@
  * to be included from codec driver
  */
 
-#if IS_ENABLED(CONFIG_LEDS_DELL_NETBOOKS)
+#if IS_ENABLED(CONFIG_DELL_LAPTOP)
 #include <linux/dell-led.h>
 
 static int dell_led_value;
@@ -67,10 +67,10 @@ static void alc_fixup_dell_wmi(struct hda_codec *codec,
 	}
 }
 
-#else /* CONFIG_LEDS_DELL_NETBOOKS */
+#else /* CONFIG_DELL_LAPTOP */
 static void alc_fixup_dell_wmi(struct hda_codec *codec,
 			       const struct hda_fixup *fix, int action)
 {
 }
 
-#endif /* CONFIG_LEDS_DELL_NETBOOKS */
+#endif /* CONFIG_DELL_LAPTOP */
-- 
2.10.2

[PATCH 6/7] dell-led: remove code related to mic mute LED

[Michał Kępień]

With dell_micmute_led_set() moved to drivers/platform/x86/dell-laptop.c,
all remnants of the mic mute LED handling code can be removed from
drivers/leds/dell-led.c, restoring it back to the state it was in before
db6d8cc ("dell-led: add mic mute led interface").

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/leds/Kconfig    |  1 -
 drivers/leds/dell-led.c | 25 +++++++------------------
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index c621cbb..f29b869 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -458,7 +458,6 @@ config LEDS_DELL_NETBOOKS
 	tristate "External LED on Dell Business Netbooks"
 	depends on LEDS_CLASS
 	depends on X86 && ACPI_WMI
-	depends on DELL_SMBIOS
 	help
 	  This adds support for the Latitude 2100 and similar
 	  notebooks that have an external LED.
diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
index c9cc36a..e5c5738 100644
--- a/drivers/leds/dell-led.c
+++ b/drivers/leds/dell-led.c
@@ -15,15 +15,12 @@
 #include <linux/leds.h>
 #include <linux/slab.h>
 #include <linux/module.h>
-#include <linux/dmi.h>
-#include "../platform/x86/dell-smbios.h"
 
 MODULE_AUTHOR("Louis Davis/Jim Dailey");
 MODULE_DESCRIPTION("Dell LED Control Driver");
 MODULE_LICENSE("GPL");
 
 #define DELL_LED_BIOS_GUID "F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396"
-#define DELL_APP_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
 MODULE_ALIAS("wmi:" DELL_LED_BIOS_GUID);
 
 /* Error Result Codes: */
@@ -184,29 +181,21 @@ static int __init dell_led_init(void)
 {
 	int error = 0;
 
-	if (!wmi_has_guid(DELL_LED_BIOS_GUID) && !wmi_has_guid(DELL_APP_GUID))
+	if (!wmi_has_guid(DELL_LED_BIOS_GUID))
 		return -ENODEV;
 
-	if (wmi_has_guid(DELL_LED_BIOS_GUID)) {
-		error = led_off();
-		if (error != 0)
-			return -ENODEV;
-
-		error = led_classdev_register(NULL, &dell_led);
-	}
+	error = led_off();
+	if (error != 0)
+		return -ENODEV;
 
-	return error;
+	return led_classdev_register(NULL, &dell_led);
 }
 
 static void __exit dell_led_exit(void)
 {
-	int error = 0;
+	led_classdev_unregister(&dell_led);
 
-	if (wmi_has_guid(DELL_LED_BIOS_GUID)) {
-		error = led_off();
-		if (error == 0)
-			led_classdev_unregister(&dell_led);
-	}
+	led_off();
 }
 
 module_init(dell_led_init);
-- 
2.10.2

[PATCH 7/7] dell-led: move driver to drivers/platform/x86/dell-wmi-led.c

[Michał Kępień]

The dell-led driver handles a specific WMI GUID present on some Dell
laptops and as such it belongs in the x86 platform driver subsystem.
Source code is moved along with the relevant Kconfig and Makefile
entries with some minor modifications:

  - Kconfig option is renamed from COFIG_LEDS_DELL_NETBOOKS to
    CONFIG_DELL_WMI_LED,

  - the X86 Kconfig dependency is removed as the whole
    drivers/platform/x86 menu depends on it, so there is no need to
    duplicate it,

  - one comment line is updated to reflect the change in the name of the
    module's source file.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/leds/Kconfig                                     | 8 --------
 drivers/leds/Makefile                                    | 1 -
 drivers/platform/x86/Kconfig                             | 8 ++++++++
 drivers/platform/x86/Makefile                            | 1 +
 drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)
 rename drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} (99%)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index f29b869..5af3fb2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -454,14 +454,6 @@ config LEDS_ADP5520
 	  To compile this driver as a module, choose M here: the module will
 	  be called leds-adp5520.
 
-config LEDS_DELL_NETBOOKS
-	tristate "External LED on Dell Business Netbooks"
-	depends on LEDS_CLASS
-	depends on X86 && ACPI_WMI
-	help
-	  This adds support for the Latitude 2100 and similar
-	  notebooks that have an external LED.
-
 config LEDS_MC13783
 	tristate "LED Support for MC13XXX PMIC"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 6b82737..558d246 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -52,7 +52,6 @@ obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
 obj-$(CONFIG_LEDS_INTEL_SS4200)		+= leds-ss4200.o
 obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
 obj-$(CONFIG_LEDS_ADP5520)		+= leds-adp5520.o
-obj-$(CONFIG_LEDS_DELL_NETBOOKS)	+= dell-led.o
 obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
 obj-$(CONFIG_LEDS_NS2)			+= leds-ns2.o
 obj-$(CONFIG_LEDS_NETXBIG)		+= leds-netxbig.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 81b8dcc..f9018e8 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -143,6 +143,14 @@ config DELL_WMI_AIO
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-wmi-aio.
 
+config DELL_WMI_LED
+	tristate "External LED on Dell Business Netbooks"
+	depends on LEDS_CLASS
+	depends on ACPI_WMI
+	help
+	  This adds support for the Latitude 2100 and similar
+	  notebooks that have an external LED.
+
 config DELL_SMO8800
 	tristate "Dell Latitude freefall driver (ACPI SMO88XX)"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2efa86d..b061817 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
 obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
 obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
 obj-$(CONFIG_DELL_WMI_AIO)	+= dell-wmi-aio.o
+obj-$(CONFIG_DELL_WMI_LED)	+= dell-wmi-led.o
 obj-$(CONFIG_DELL_SMO8800)	+= dell-smo8800.o
 obj-$(CONFIG_DELL_RBTN)		+= dell-rbtn.o
 obj-$(CONFIG_ACER_WMI)		+= acer-wmi.o
diff --git a/drivers/leds/dell-led.c b/drivers/platform/x86/dell-wmi-led.c
similarity index 99%
rename from drivers/leds/dell-led.c
rename to drivers/platform/x86/dell-wmi-led.c
index e5c5738..7486c01 100644
--- a/drivers/leds/dell-led.c
+++ b/drivers/platform/x86/dell-wmi-led.c
@@ -1,5 +1,5 @@
 /*
- * dell_led.c - Dell LED Driver
+ * dell-wmi-led.c - Dell WMI LED Driver
  *
  * Copyright (C) 2010 Dell Inc.
  * Louis Davis <louis_davis@dell.com>
-- 
2.10.2

Re: [PATCH 7/7] dell-led: move driver to drivers/platform/x86/dell-wmi-led.c

[Andy Shevchenko]

On Thu, Dec 8, 2016 at 2:36 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> The dell-led driver handles a specific WMI GUID present on some Dell
> laptops and as such it belongs in the x86 platform driver subsystem.
> Source code is moved along with the relevant Kconfig and Makefile
> entries with some minor modifications:
>
>   - Kconfig option is renamed from COFIG_LEDS_DELL_NETBOOKS to

Typo here, CONFIG_...

>     CONFIG_DELL_WMI_LED,

Do we care about current configuration or we just suggest users to
follow this by themselves?

>
>   - the X86 Kconfig dependency is removed as the whole
>     drivers/platform/x86 menu depends on it, so there is no need to
>     duplicate it,
>
>   - one comment line is updated to reflect the change in the name of the
>     module's source file.
>

While here, please follow our pattern for subject lines, i.e.
"platform/x86: driver: Description".

> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/leds/Kconfig                                     | 8 --------
>  drivers/leds/Makefile                                    | 1 -
>  drivers/platform/x86/Kconfig                             | 8 ++++++++
>  drivers/platform/x86/Makefile                            | 1 +
>  drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} | 2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
>  rename drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} (99%)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f29b869..5af3fb2 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -454,14 +454,6 @@ config LEDS_ADP5520
>           To compile this driver as a module, choose M here: the module will
>           be called leds-adp5520.
>
> -config LEDS_DELL_NETBOOKS
> -       tristate "External LED on Dell Business Netbooks"
> -       depends on LEDS_CLASS
> -       depends on X86 && ACPI_WMI
> -       help
> -         This adds support for the Latitude 2100 and similar
> -         notebooks that have an external LED.
> -
>  config LEDS_MC13783
>         tristate "LED Support for MC13XXX PMIC"
>         depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 6b82737..558d246 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -52,7 +52,6 @@ obj-$(CONFIG_LEDS_REGULATOR)          += leds-regulator.o
>  obj-$(CONFIG_LEDS_INTEL_SS4200)                += leds-ss4200.o
>  obj-$(CONFIG_LEDS_LT3593)              += leds-lt3593.o
>  obj-$(CONFIG_LEDS_ADP5520)             += leds-adp5520.o
> -obj-$(CONFIG_LEDS_DELL_NETBOOKS)       += dell-led.o
>  obj-$(CONFIG_LEDS_MC13783)             += leds-mc13783.o
>  obj-$(CONFIG_LEDS_NS2)                 += leds-ns2.o
>  obj-$(CONFIG_LEDS_NETXBIG)             += leds-netxbig.o
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 81b8dcc..f9018e8 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -143,6 +143,14 @@ config DELL_WMI_AIO
>           To compile this driver as a module, choose M here: the module will
>           be called dell-wmi-aio.
>
> +config DELL_WMI_LED
> +       tristate "External LED on Dell Business Netbooks"
> +       depends on LEDS_CLASS
> +       depends on ACPI_WMI
> +       help
> +         This adds support for the Latitude 2100 and similar
> +         notebooks that have an external LED.
> +
>  config DELL_SMO8800
>         tristate "Dell Latitude freefall driver (ACPI SMO88XX)"
>         depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 2efa86d..b061817 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_DELL_SMBIOS)     += dell-smbios.o
>  obj-$(CONFIG_DELL_LAPTOP)      += dell-laptop.o
>  obj-$(CONFIG_DELL_WMI)         += dell-wmi.o
>  obj-$(CONFIG_DELL_WMI_AIO)     += dell-wmi-aio.o
> +obj-$(CONFIG_DELL_WMI_LED)     += dell-wmi-led.o
>  obj-$(CONFIG_DELL_SMO8800)     += dell-smo8800.o
>  obj-$(CONFIG_DELL_RBTN)                += dell-rbtn.o
>  obj-$(CONFIG_ACER_WMI)         += acer-wmi.o
> diff --git a/drivers/leds/dell-led.c b/drivers/platform/x86/dell-wmi-led.c
> similarity index 99%
> rename from drivers/leds/dell-led.c
> rename to drivers/platform/x86/dell-wmi-led.c
> index e5c5738..7486c01 100644
> --- a/drivers/leds/dell-led.c
> +++ b/drivers/platform/x86/dell-wmi-led.c
> @@ -1,5 +1,5 @@
>  /*

> - * dell_led.c - Dell LED Driver
> + * dell-wmi-led.c - Dell WMI LED Driver

That's exactly the point why better to take a chance to remove file
name from the file.

>   *
>   * Copyright (C) 2010 Dell Inc.
>   * Louis Davis <louis_davis@dell.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/7] dell-led: move driver to drivers/platform/x86/dell-wmi-led.c

[Michał Kępień]

> On Thu, Dec 8, 2016 at 2:36 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> > The dell-led driver handles a specific WMI GUID present on some Dell
> > laptops and as such it belongs in the x86 platform driver subsystem.
> > Source code is moved along with the relevant Kconfig and Makefile
> > entries with some minor modifications:
> >
> >   - Kconfig option is renamed from COFIG_LEDS_DELL_NETBOOKS to
> 
> Typo here, CONFIG_...

Indeed, thanks.

> >     CONFIG_DELL_WMI_LED,
> 
> Do we care about current configuration or we just suggest users to
> follow this by themselves?

I searched the git log for a bit, looking for ideas.  Every commit I
found that renames a Kconfig option settles for doing just that, without
any further provisions to make some kind of automatic transition happen.
That being said, if there is anything I can do to make it easier for the
user to notice this change, I would be happy to hear about it.

> >
> >   - the X86 Kconfig dependency is removed as the whole
> >     drivers/platform/x86 menu depends on it, so there is no need to
> >     duplicate it,
> >
> >   - one comment line is updated to reflect the change in the name of the
> >     module's source file.
> >
> 
> While here, please follow our pattern for subject lines, i.e.
> "platform/x86: driver: Description".

Ah, sure, thanks.  It seems this pattern has changed since the last time
I submitted a patch for drivers/platform/x86.

> > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > ---
> >  drivers/leds/Kconfig                                     | 8 --------
> >  drivers/leds/Makefile                                    | 1 -
> >  drivers/platform/x86/Kconfig                             | 8 ++++++++
> >  drivers/platform/x86/Makefile                            | 1 +
> >  drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} | 2 +-
> >  5 files changed, 10 insertions(+), 10 deletions(-)
> >  rename drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} (99%)
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index f29b869..5af3fb2 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -454,14 +454,6 @@ config LEDS_ADP5520
> >           To compile this driver as a module, choose M here: the module will
> >           be called leds-adp5520.
> >
> > -config LEDS_DELL_NETBOOKS
> > -       tristate "External LED on Dell Business Netbooks"
> > -       depends on LEDS_CLASS
> > -       depends on X86 && ACPI_WMI
> > -       help
> > -         This adds support for the Latitude 2100 and similar
> > -         notebooks that have an external LED.
> > -
> >  config LEDS_MC13783
> >         tristate "LED Support for MC13XXX PMIC"
> >         depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 6b82737..558d246 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -52,7 +52,6 @@ obj-$(CONFIG_LEDS_REGULATOR)          += leds-regulator.o
> >  obj-$(CONFIG_LEDS_INTEL_SS4200)                += leds-ss4200.o
> >  obj-$(CONFIG_LEDS_LT3593)              += leds-lt3593.o
> >  obj-$(CONFIG_LEDS_ADP5520)             += leds-adp5520.o
> > -obj-$(CONFIG_LEDS_DELL_NETBOOKS)       += dell-led.o
> >  obj-$(CONFIG_LEDS_MC13783)             += leds-mc13783.o
> >  obj-$(CONFIG_LEDS_NS2)                 += leds-ns2.o
> >  obj-$(CONFIG_LEDS_NETXBIG)             += leds-netxbig.o
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 81b8dcc..f9018e8 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -143,6 +143,14 @@ config DELL_WMI_AIO
> >           To compile this driver as a module, choose M here: the module will
> >           be called dell-wmi-aio.
> >
> > +config DELL_WMI_LED
> > +       tristate "External LED on Dell Business Netbooks"
> > +       depends on LEDS_CLASS
> > +       depends on ACPI_WMI
> > +       help
> > +         This adds support for the Latitude 2100 and similar
> > +         notebooks that have an external LED.
> > +
> >  config DELL_SMO8800
> >         tristate "Dell Latitude freefall driver (ACPI SMO88XX)"
> >         depends on ACPI
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 2efa86d..b061817 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_DELL_SMBIOS)     += dell-smbios.o
> >  obj-$(CONFIG_DELL_LAPTOP)      += dell-laptop.o
> >  obj-$(CONFIG_DELL_WMI)         += dell-wmi.o
> >  obj-$(CONFIG_DELL_WMI_AIO)     += dell-wmi-aio.o
> > +obj-$(CONFIG_DELL_WMI_LED)     += dell-wmi-led.o
> >  obj-$(CONFIG_DELL_SMO8800)     += dell-smo8800.o
> >  obj-$(CONFIG_DELL_RBTN)                += dell-rbtn.o
> >  obj-$(CONFIG_ACER_WMI)         += acer-wmi.o
> > diff --git a/drivers/leds/dell-led.c b/drivers/platform/x86/dell-wmi-led.c
> > similarity index 99%
> > rename from drivers/leds/dell-led.c
> > rename to drivers/platform/x86/dell-wmi-led.c
> > index e5c5738..7486c01 100644
> > --- a/drivers/leds/dell-led.c
> > +++ b/drivers/platform/x86/dell-wmi-led.c
> > @@ -1,5 +1,5 @@
> >  /*
> 
> > - * dell_led.c - Dell LED Driver
> > + * dell-wmi-led.c - Dell WMI LED Driver
> 
> That's exactly the point why better to take a chance to remove file
> name from the file.

I will be glad to rip it out in v2, thanks.

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/7] Move dell-led to drivers/platform/x86

[Jacek Anaszewski]

Hi Michał,

Thanks for the patch set.

On 12/08/2016 01:36 PM, Michał Kępień wrote:
> This patch series moves the dell-led driver from the LED subsystem to
> the x86 platform driver subsystem.  I decided to also CC the sound
> subsystem contacts for the whole series as
> sound/pci/hda/dell_wmi_helper.c is also affected.
>
> The original motivation behind this effort was to move all code using
> the dell-smbios module to the x86 platform driver subsystem.  While I
> was investigating the possibilites to do that, it quickly emerged that
> dell-led can and in fact should be moved to the x86 platform driver
> subsystem in its entirety.
>
> dell-led consists of two major parts:
>
>   - the part exposing a microphone mute LED interface, introduced in
>     db6d8cc ("dell-led: add mic mute led interface"); this interface is
>     used by sound/pci/hda/dell_wmi_helper.c; while the original
>     implementation used a WMI interface, it was changed to use
>     dell-smbios in cf0d7ea ("dell-led: use dell_smbios_find_token() for
>     finding mic DMI tokens") and 0c41a08 ("dell-led: use
>     dell_smbios_send_request() for performing SMBIOS calls"),
>
>   - the part handling an activity LED present in Dell Latitude 2100
>     netbooks, introduced in 72dcd8d ("leds: Add Dell Business Class
>     Netbook LED driver"); it binds to a specific WMI GUID and then
>     registers a LED device which is controlled using WMI (i.e. it is
>     basically a WMI driver).
>
> Patches 1-4 clean up the microphone mute LED interface to minimize the
> amount of code moved around.
>
> Patch 5 moves the microphone mute LED interface to
> drivers/platform/x86/dell-laptop.c, effectively causing
> sound/pci/hda/dell_wmi_helper.c to depend on CONFIG_DELL_LAPTOP instead
> of CONFIG_LEDS_DELL_NETBOOKS.
>
> Patch 6 reverts dell-led to the state it was in after its initial commit
> 72dcd8d ("leds: Add Dell Business Class Netbook LED driver") by removing
> all remnants of the microphone mute LED handling code.
>
> Patch 7 moves all that is left of dell-led (i.e. the activity LED part,
> as originally implemented), to a new module which is placed in
> drivers/platform/x86/dell-wmi-led.c.
>
> This patch series is based on linux-leds/for-4.11 as the LED subsystem
> is affected by all patches except patch 3.
>
> If anyone reading this has access to a Dell device which has an activity
> LED and/or a microphone mute LED currently supported by dell-led, I
> would love to hear from you as I do not have the hardware needed to
> practically test this patch series.

I think that it is necessary to find someone who will give their
Tested-by.

What I can accept immediately is moving the driver in the current
shape to x86 platform drivers. I could expose a stable branch with
that patch for the x86 platform maintainers then.

>  drivers/leds/Kconfig                               |  9 ---
>  drivers/leds/Makefile                              |  1 -
>  drivers/platform/x86/Kconfig                       |  8 +++
>  drivers/platform/x86/Makefile                      |  1 +
>  drivers/platform/x86/dell-laptop.c                 | 28 ++++++++
>  .../dell-led.c => platform/x86/dell-wmi-led.c}     | 75 +++-------------------
>  include/linux/dell-led.h                           |  6 +-
>  sound/pci/hda/dell_wmi_helper.c                    | 18 +++---
>  8 files changed, 55 insertions(+), 91 deletions(-)
>  rename drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c} (73%)
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 0/7] Move dell-led to drivers/platform/x86

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3969 bytes --]

On Thursday 08 December 2016 15:26:37 Jacek Anaszewski wrote:
> Hi Michał,
> 
> Thanks for the patch set.
> 
> On 12/08/2016 01:36 PM, Michał Kępień wrote:
> > This patch series moves the dell-led driver from the LED subsystem
> > to the x86 platform driver subsystem.  I decided to also CC the
> > sound subsystem contacts for the whole series as
> > sound/pci/hda/dell_wmi_helper.c is also affected.
> > 
> > The original motivation behind this effort was to move all code
> > using the dell-smbios module to the x86 platform driver subsystem.
> >  While I was investigating the possibilites to do that, it quickly
> > emerged that dell-led can and in fact should be moved to the x86
> > platform driver subsystem in its entirety.
> > 
> > dell-led consists of two major parts:
> >   - the part exposing a microphone mute LED interface, introduced
> >   in
> >   
> >     db6d8cc ("dell-led: add mic mute led interface"); this
> >     interface is used by sound/pci/hda/dell_wmi_helper.c; while
> >     the original implementation used a WMI interface, it was
> >     changed to use dell-smbios in cf0d7ea ("dell-led: use
> >     dell_smbios_find_token() for finding mic DMI tokens") and
> >     0c41a08 ("dell-led: use
> >     dell_smbios_send_request() for performing SMBIOS calls"),
> >   
> >   - the part handling an activity LED present in Dell Latitude 2100
> >   
> >     netbooks, introduced in 72dcd8d ("leds: Add Dell Business Class
> >     Netbook LED driver"); it binds to a specific WMI GUID and then
> >     registers a LED device which is controlled using WMI (i.e. it
> >     is basically a WMI driver).
> > 
> > Patches 1-4 clean up the microphone mute LED interface to minimize
> > the amount of code moved around.
> > 
> > Patch 5 moves the microphone mute LED interface to
> > drivers/platform/x86/dell-laptop.c, effectively causing
> > sound/pci/hda/dell_wmi_helper.c to depend on CONFIG_DELL_LAPTOP
> > instead of CONFIG_LEDS_DELL_NETBOOKS.
> > 
> > Patch 6 reverts dell-led to the state it was in after its initial
> > commit 72dcd8d ("leds: Add Dell Business Class Netbook LED
> > driver") by removing all remnants of the microphone mute LED
> > handling code.
> > 
> > Patch 7 moves all that is left of dell-led (i.e. the activity LED
> > part, as originally implemented), to a new module which is placed
> > in drivers/platform/x86/dell-wmi-led.c.
> > 
> > This patch series is based on linux-leds/for-4.11 as the LED
> > subsystem is affected by all patches except patch 3.
> > 
> > If anyone reading this has access to a Dell device which has an
> > activity LED and/or a microphone mute LED currently supported by
> > dell-led, I would love to hear from you as I do not have the
> > hardware needed to practically test this patch series.
> 
> I think that it is necessary to find someone who will give their
> Tested-by.
> 
> What I can accept immediately is moving the driver in the current
> shape to x86 platform drivers. I could expose a stable branch with
> that patch for the x86 platform maintainers then.

Adding Mario Limonciello from @dell to discussion.

Mario, any chance you could be able to test this patch series?

> >  drivers/leds/Kconfig                               |  9 ---
> >  drivers/leds/Makefile                              |  1 -
> >  drivers/platform/x86/Kconfig                       |  8 +++
> >  drivers/platform/x86/Makefile                      |  1 +
> >  drivers/platform/x86/dell-laptop.c                 | 28 ++++++++
> >  .../dell-led.c => platform/x86/dell-wmi-led.c}     | 75
> >  +++------------------- include/linux/dell-led.h                  
> >          |  6 +- sound/pci/hda/dell_wmi_helper.c                  
> >   | 18 +++--- 8 files changed, 55 insertions(+), 91 deletions(-)
> >  rename drivers/{leds/dell-led.c => platform/x86/dell-wmi-led.c}
> >  (73%)

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]