Re: [alsa-devel] [RFC PATCH 1/2] thinkpad-acpi: Add mute and mic-mute LED functionality

On 10/16/2013 03:09 PM, Takashi Iwai wrote:

At Wed, 16 Oct 2013 14:15:35 +0200, David Henningsson wrote:

...

Questions / notes:

This patch supersedes the second of Alex Hung's patches posted earlier at http://www.spinics.net/lists/platform-driver-x86/msg04673.html

Not sure if thinkpad_acpi should be dropped into include/linux though, any better suggestion?

Should TPACPI_VERSION be increased because we added a new LED driver?

Signed-off-by: David Henningsson david.henningsson@canonical.com

drivers/platform/x86/thinkpad_acpi.c | 94 +++++++++++++++++++++++++++++++++- include/linux/thinkpad_acpi.h | 10 ++++ 2 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 include/linux/thinkpad_acpi.h

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 03ca6c1..ecdfeae 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -23,7 +23,7 @@

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

-#define TPACPI_VERSION "0.24" +#define TPACPI_VERSION "0.25" #define TPACPI_SYSFS_VERSION 0x020700

/* @@ -88,6 +88,7 @@

#include <linux/pci_ids.h>

+#include <linux/thinkpad_acpi.h>

/* ThinkPad CMOS commands */ #define TP_CMOS_VOLUME_DOWN 0 @@ -8350,6 +8351,93 @@ static struct ibm_struct fan_driver_data = { .resume = fan_resume, };

+/*************************************************************************

• • Mute LED subdriver
• */

+#define MUTE_LED_INDEX 0 +#define MICMUTE_LED_INDEX 1

+static int mute_led_on_off(int whichled, int state) +{

• int output;
• acpi_handle temp;
• acpi_string m;
• if (whichled == MICMUTE_LED_INDEX) {

• state = state > 0 ? 2 : 0;
  

• m = "MMTS";
  

• } else {

• state = state > 0 ? 1 : 0;
  

• m = "SSMS";
  

• }
• if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, m, &temp))) {

• pr_warn("Thinkpad ACPI has no %s interface.\n", m);
  

• return -EIO;
  

• }
• if (!acpi_evalf(hkey_handle, &output, m, "dd", state))

• return -EIO;
  

• return 0;

+}

+static unsigned int mute_led_state; +static unsigned int micmute_led_state;

+int tpacpi_mute_led_set(int on) +{

• int err;
• int state = on ? 1 : 0;
• if (mute_led_state < 0 || mute_led_state == state)

• return mute_led_state;
  

• err = mute_led_on_off(MUTE_LED_INDEX, state);
• mute_led_state = err ? err : state;
• return err;

+} +EXPORT_SYMBOL(tpacpi_mute_led_set);

+int tpacpi_micmute_led_set(int on) +{

• int err;
• int state = on ? 1 : 0;
• if (micmute_led_state < 0 || micmute_led_state == state)

• return micmute_led_state;
  

• err = mute_led_on_off(MICMUTE_LED_INDEX, state);
• micmute_led_state = err ? err : state;
• return err;

+} +EXPORT_SYMBOL(tpacpi_micmute_led_set);

+static int mute_led_init(struct ibm_init_struct *iibm) +{

• acpi_handle temp;
• if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "MMTG", &temp)))

• micmute_led_state = mute_led_on_off(MICMUTE_LED_INDEX, 0);
  

• else

• micmute_led_state = -ENODEV;
  

• if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "GSMS", &temp)))

• mute_led_state = mute_led_on_off(MUTE_LED_INDEX, 0);
  

• else

• mute_led_state = -ENODEV;
  

• return 0;

+}

+static void mute_led_exit(void) +{

• tpacpi_mute_led_set(0);
• tpacpi_micmute_led_set(0);

+}

+static struct ibm_struct mute_led_driver_data = {

• .name = "mute_led",
• .exit = mute_led_exit,

+};

How about managing with a table?

Sure, that looks a bit more elegant. I'll incorporate your suggestions into v2 of the patch (together with the documentation additions suggested by Henrique), when you have reviewed the ALSA part too.

For example,

struct tp_led_table { acpi_string name; int on_value; int off_value; int state; };

static struct tp_led_table led_tables[] = { [TP_LED_MUTE] = { .name = "SSMS", .on_value = 1, .off_value = 0, }, [TP_LED_MICMUTE] = { .name = "MMTS", .on_value = 2, .off_value = 0, }, };

static int mute_led_on_off(struct tp_led_table *t, bool state) { acpi_handle temp;

if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp))) { pr_warn("Thinkpad ACPI has no %s interface.\n", t->name); return -EIO; }

if (!acpi_evalf(hkey_handle, &output, t->name, "dd", state ? t->on_value : t->off_value)) return -EIO;

t->state = state; return state; }

int tpacpi_led_set(int whichled, bool on) { struct tp_led_table *t;

if (whichled < 0 || whichled >= TP_LED_MAX) return -EINVAL;

t = &led_tables[whichled]; if (t->state < 0 || t->state == on) return t->state; return mute_led_on_off(t, on); } EXPORT_SYMBOL_GPL(tpacpi_led_set);

static int mute_led_init(struct ibm_init_struct *iibm) { acpi_handle temp; int i;

for (i = 0; i < TP_LED_MAX; i++) { struct tp_led_table *t = &led_tables[i]; if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp))) mute_led_on_off(t, false); else t-.state = -ENODEV; } return 0; }

static void mute_led_exit(void) { int i;

for (i = 0; i < TP_LED_MAX; i++) tpacpi_led_set(i, false); }

Also, the exported symbol should be marked with *_GPL().

thanks,

Takashi