Hi Henrique,
Here's a first set of changes. The point of these here was to
separate out a function from volume_write to set the volume/mute. (I
also added an init for the volume subdriver and a mutex).
There is a bit of a behavioural change. It's hopefully imperceptible
to the user. Before, sending multiple commands to /proc/.../volume
would execute them each in turn. Now, the consequences of the
multiple commands are accumulated to a final result and then changed
once.
Is there any reason why the volume is prefered to be changed via the
CMOS? It seems the code would be much nicer if it was all done
through the EC.
Cheers,
Lorne
On Sat, Jan 24, 2009 at 7:29 AM, Henrique de Moraes Holschuh
<hmh@xxxxxxxxxx> wrote:
> On Sat, 24 Jan 2009, Lorne Applebaum wrote:
>> I've got an ALSA mixer for the speaker volume working in
>> thinkpad_acpi.c on my laptop. Is this where I should post if I'm
>> interested in getting the code good enough to get upstream?
>
> Sure, post it here for comments.
>
> --
> "One disk to rule them all, One disk to find them. One disk to bring
> them all and in the darkness grind them. In the Land of Redmond
> where the shadows lie." -- The Silicon Valley Tarot
> Henrique Holschuh
>
=== modified file 'drivers/misc/thinkpad_acpi.c'
--- old/drivers/misc/thinkpad_acpi.c 2009-01-09 18:43:12 +0000
+++ new/drivers/misc/thinkpad_acpi.c 2009-01-10 17:26:33 +0000
@@ -5060,6 +5060,63 @@
*/
static int volume_offset = 0x30;
+static struct mutex volume_write_mutex;
+
+static int __init volume_init(struct ibm_init_struct *iibm)
+{
+ vdbg_printk(TPACPI_DBG_INIT, "initializing volume subdriver\n");
+
+ mutex_init(&volume_write_mutex);
+
+ return 0;
+}
+
+static int volume_cmos_set_level_mute(u8 new_level, u8 new_mute)
+{
+ int cmos_cmd, inc, i;
+ u8 old_level, old_mute;
+
+ if (!acpi_ec_read(volume_offset, &old_level))
+ return -EIO;
+ old_mute = old_level & 0x40;
+ old_level = old_level & 0xf;
+
+ if (new_level != old_level) {
+ /* Also takes care of mute */
+
+ cmos_cmd = (new_level > old_level) ?
+ TP_CMOS_VOLUME_UP : TP_CMOS_VOLUME_DOWN;
+ inc = new_level > old_level ? 1 : -1;
+
+ /* If mute is enabled the first CMOS command will unmute.
+ Get it over and done with. */
+ if (old_mute && (issue_thinkpad_cmos_command(cmos_cmd)))
+ return -EIO;
+
+ for (i = old_level; i != new_level; i += inc)
+ if (issue_thinkpad_cmos_command(cmos_cmd))
+ return -EIO;
+
+ /* If mute should still be set. Fix the unmuting by the
+ commands */
+ if (new_mute && (issue_thinkpad_cmos_command(TP_CMOS_VOLUME_MUTE)))
+ return -EIO;
+ /* The mute possibilities have been taken care of. Can return. */
+ return 0;
+ }
+
+ if (new_mute != old_mute) {
+ /* level doesn't change */
+
+ cmos_cmd = (new_mute) ?
+ TP_CMOS_VOLUME_MUTE : TP_CMOS_VOLUME_UP;
+
+ if (issue_thinkpad_cmos_command(cmos_cmd))
+ return -EIO;
+ }
+
+ return 0;
+}
static int volume_read(char *p)
{
@@ -5081,69 +5138,44 @@
static int volume_write(char *buf)
{
- int cmos_cmd, inc, i;
- u8 level, mute;
+ u8 level;
int new_level, new_mute;
char *cmd;
- while ((cmd = next_cmd(&buf))) {
- if (!acpi_ec_read(volume_offset, &level))
- return -EIO;
- new_mute = mute = level & 0x40;
- new_level = level = level & 0xf;
-
+ if (!acpi_ec_read(volume_offset, &level))
+ return -EIO;
+ new_mute = level & 0x40;
+ new_level = level & 0xf;
+
+ while((cmd = next_cmd(&buf))){
if (strlencmp(cmd, "up") == 0) {
- if (mute)
- new_mute = 0;
- else
- new_level = level == 15 ? 15 : level + 1;
+ /* Only change the level if not unmuting */
+ if(!new_mute)
+ new_level = (new_level >= 15) ? 15 : new_level + 1;
+ new_mute = 0;
} else if (strlencmp(cmd, "down") == 0) {
- if (mute)
- new_mute = 0;
- else
- new_level = level == 0 ? 0 : level - 1;
+ /* Only change the level if not unmuting */
+ if(!new_mute)
+ new_level = (new_level <= 0) ? 0 : new_level - 1;
+ new_mute = 0;
} else if (sscanf(cmd, "level %d", &new_level) == 1 &&
new_level >= 0 && new_level <= 15) {
- /* new_level set */
+ /* Level is set but can be changed by commands later in
+ the buffer */
} else if (strlencmp(cmd, "mute") == 0) {
new_mute = 0x40;
} else
- return -EINVAL;
-
- if (new_level != level) {
- /* mute doesn't change */
-
- cmos_cmd = (new_level > level) ?
- TP_CMOS_VOLUME_UP : TP_CMOS_VOLUME_DOWN;
- inc = new_level > level ? 1 : -1;
-
- if (mute && (issue_thinkpad_cmos_command(cmos_cmd) ||
- !acpi_ec_write(volume_offset, level)))
- return -EIO;
-
- for (i = level; i != new_level; i += inc)
- if (issue_thinkpad_cmos_command(cmos_cmd) ||
- !acpi_ec_write(volume_offset, i + inc))
- return -EIO;
-
- if (mute &&
- (issue_thinkpad_cmos_command(TP_CMOS_VOLUME_MUTE) ||
- !acpi_ec_write(volume_offset, new_level + mute))) {
- return -EIO;
- }
- }
-
- if (new_mute != mute) {
- /* level doesn't change */
-
- cmos_cmd = (new_mute) ?
- TP_CMOS_VOLUME_MUTE : TP_CMOS_VOLUME_UP;
-
- if (issue_thinkpad_cmos_command(cmos_cmd) ||
- !acpi_ec_write(volume_offset, level + new_mute))
- return -EIO;
- }
- }
+ return -EINVAL;
+ }
+
+ mutex_lock(&volume_write_mutex);
+ if(volume_cmos_set_level_mute(new_level, new_mute) ||
+ !acpi_ec_write(volume_offset, new_level + new_mute))
+ {
+ mutex_unlock(&volume_write_mutex);
+ return -EIO;
+ }
+ mutex_unlock(&volume_write_mutex);
return 0;
}
@@ -6513,6 +6545,7 @@
.data = &brightness_driver_data,
},
{
+ .init = volume_init,
.data = &volume_driver_data,
},
{
------------------------------------------------------------------------------
This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
