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

[David Henningsson <david.henningsson@canonical.com>]

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
> 



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic