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@xxxxxxxxxxxxx> >> --- >> 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 > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
