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
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ń <[email protected]>
---
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
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ń <[email protected]>
---
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
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ń <[email protected]>
---
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
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ń <[email protected]>
---
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 <[email protected]>
--
2.10.2
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ń <[email protected]>
---
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
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ń <[email protected]>
---
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
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ń <[email protected]>
---
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
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
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ń <[email protected]>
> ---
> 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 <[email protected]>
Anyway, you can remove DELL_APP_GUID from other places too...
--
Pali Rohár
[email protected]
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ń <[email protected]>
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 <[email protected]>
--
Pali Rohár
[email protected]
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
[email protected]
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
[email protected]
On Sun, Dec 11, 2016 at 12:40 PM, Pali Rohár <[email protected]> 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ń <[email protected]>
>
> 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 <[email protected]>
>
> --
> Pali Rohár
> [email protected]
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 14, 2016 at 3:54 AM, Andy Shevchenko
<[email protected]> wrote:
> On Sun, Dec 11, 2016 at 12:40 PM, Pali Rohár <[email protected]> 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ń <[email protected]>
>>
>> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 8, 2016 at 2:36 PM, Michał Kępień <[email protected]> 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ń <[email protected]>
> ---
> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
> 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ń <[email protected]>
> > ---
> > 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 <[email protected]>
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ń
> 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ń
> On Wed, Dec 14, 2016 at 3:54 AM, Andy Shevchenko
> <[email protected]> wrote:
> > On Sun, Dec 11, 2016 at 12:40 PM, Pali Rohár <[email protected]> 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ń <[email protected]>
> >>
> >> 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ń
> On 12 December 2016 at 01:13, <[email protected]> wrote:
>
> > add Anthony Wong @ Canonical.
> >
> > I'm about to go on break and won't have access to hardware for a while.
> >
> > Anthony,
> >
> > Your team has this hardware more readily available than I do. Would you be
> > able to have someone on the team verify this series?
> >
> Sorry, just back from vacation.
>
> Yes, we can test it here some time next week.
That is great news, thanks! I will try to post a v2 of this patch
series within the next few days, so hopefully it will be ready before
you get around to testing. I will keep you in the loop.
--
Best regards,
Michał Kępień
> On Thu, Dec 8, 2016 at 2:36 PM, Michał Kępień <[email protected]> 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ń <[email protected]>
> > ---
> > 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ń
On Thu Dec 15 15:46:28 2016 Michał Kępień <[email protected]> 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
[email protected]
> On Thu Dec 15 15:46:28 2016 Michał Kępień <[email protected]> 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ń