Discussion:
[PATCH 0/5] toshiba_acpi: Various changes plus fixes
Azael Avalos
2014-09-05 17:14:02 UTC
Permalink
Up for review.

This series of patches introduce support for the new
keyboard backlight type found on recent Toshiba laptops,
removes the position sysfs entry and instead creates an
input polled device (joystick), and a few fixes and
additions to the keymap list.

Azael Avalos (5):
toshiba_acpi: Additional hotkey scancodes
toshiba_acpi: Fix illumination not available on certain models
toshiba_acpi: Add accelerometer input polled device
toshiba_acpi: Support new keyboard backlight type
toshiba_acpi: Change touchpad store to check for invalid values

drivers/platform/x86/toshiba_acpi.c | 277 ++++++++++++++++++++++++++----------
1 file changed, 199 insertions(+), 78 deletions(-)
--
2.0.0
Azael Avalos
2014-09-05 17:14:05 UTC
Permalink
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.

This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.

Signed-off-by: Azael Avalos <***@gmail.com>
---
drivers/platform/x86/toshiba_acpi.c | 109 +++++++++++++++++++++++++++---------
1 file changed, 84 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 4803e7b..ac1503c 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -50,6 +50,7 @@
#include <linux/backlight.h>
#include <linux/rfkill.h>
#include <linux/input.h>
+#include <linux/input-polldev.h>
#include <linux/input/sparse-keymap.h>
#include <linux/leds.h>
#include <linux/slab.h>
@@ -124,6 +125,7 @@ MODULE_LICENSE("GPL");
#define SCI_TOUCHPAD 0x050e

/* field definitions */
+#define HCI_ACCEL_DIRECTION_MASK 0x8000
#define HCI_ACCEL_MASK 0x7fff
#define HCI_HOTKEY_DISABLE 0x0b
#define HCI_HOTKEY_ENABLE 0x09
@@ -146,6 +148,7 @@ struct toshiba_acpi_dev {
const char *method_hci;
struct rfkill *bt_rfk;
struct input_dev *hotkey_dev;
+ struct input_polled_dev *ip_dev;
struct work_struct hotkey_work;
struct backlight_device *backlight_dev;
struct led_classdev led_dev;
@@ -170,6 +173,7 @@ struct toshiba_acpi_dev {
unsigned int touchpad_supported:1;
unsigned int eco_supported:1;
unsigned int accelerometer_supported:1;
+ unsigned int joystick_registered:1;
unsigned int sysfs_created:1;

struct mutex mutex;
@@ -1361,40 +1365,17 @@ static ssize_t toshiba_touchpad_show(struct device *dev,
return sprintf(buf, "%i\n", state);
}

-static ssize_t toshiba_position_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- u32 xyval, zval, tmp;
- u16 x, y, z;
- int ret;
-
- xyval = zval = 0;
- ret = toshiba_accelerometer_get(toshiba, &xyval, &zval);
- if (ret < 0)
- return ret;
-
- x = xyval & HCI_ACCEL_MASK;
- tmp = xyval >> HCI_MISC_SHIFT;
- y = tmp & HCI_ACCEL_MASK;
- z = zval & HCI_ACCEL_MASK;
-
- return sprintf(buf, "%d %d %d\n", x, y, z);
-}
-
static DEVICE_ATTR(kbd_backlight_mode, S_IRUGO | S_IWUSR,
toshiba_kbd_bl_mode_show, toshiba_kbd_bl_mode_store);
static DEVICE_ATTR(kbd_backlight_timeout, S_IRUGO | S_IWUSR,
toshiba_kbd_bl_timeout_show, toshiba_kbd_bl_timeout_store);
static DEVICE_ATTR(touchpad, S_IRUGO | S_IWUSR,
toshiba_touchpad_show, toshiba_touchpad_store);
-static DEVICE_ATTR(position, S_IRUGO, toshiba_position_show, NULL);

static struct attribute *toshiba_attributes[] = {
&dev_attr_kbd_backlight_mode.attr,
&dev_attr_kbd_backlight_timeout.attr,
&dev_attr_touchpad.attr,
- &dev_attr_position.attr,
NULL,
};

@@ -1411,8 +1392,6 @@ static umode_t toshiba_sysfs_is_visible(struct kobject *kobj,
exists = (drv->kbd_mode == SCI_KBD_MODE_AUTO) ? true : false;
else if (attr == &dev_attr_touchpad.attr)
exists = (drv->touchpad_supported) ? true : false;
- else if (attr == &dev_attr_position.attr)
- exists = (drv->accelerometer_supported) ? true : false;

return exists ? attr->mode : 0;
}
@@ -1621,6 +1600,75 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
return 0;
}

+static void toshiba_acpi_joystick_poll(struct input_polled_dev *ip_dev)
+{
+ struct toshiba_acpi_dev *dev = ip_dev->private;
+ u32 xy, zval;
+ int x, y, z;
+
+ mutex_lock(&dev->mutex);
+
+ if (toshiba_accelerometer_get(dev, &xy, &zval) < 0) {
+ pr_err("Could not get accelerometer axes");
+ mutex_unlock(&dev->mutex);
+ return;
+ }
+
+ /* Accelerometer values */
+ x = xy & HCI_ACCEL_MASK;
+ y = (xy >> HCI_MISC_SHIFT) & HCI_ACCEL_MASK;
+ z = zval & HCI_ACCEL_MASK;
+ /* Movement direction */
+ x *= xy & HCI_ACCEL_DIRECTION_MASK ? -1 : 1;
+ y *= (xy >> HCI_MISC_SHIFT) & HCI_ACCEL_DIRECTION_MASK ? -1 : 1;
+ z *= zval & HCI_ACCEL_DIRECTION_MASK ? -1 : 1;
+
+ input_report_abs(ip_dev->input, ABS_X, x);
+ input_report_abs(ip_dev->input, ABS_Y, y);
+ input_report_abs(ip_dev->input, ABS_Z, z);
+ input_sync(ip_dev->input);
+
+ mutex_unlock(&dev->mutex);
+}
+
+static int toshiba_acpi_setup_joystick(struct toshiba_acpi_dev *dev)
+{
+ struct input_dev *idev;
+ int ret;
+
+ if (dev->ip_dev)
+ return -EINVAL;
+
+ dev->ip_dev = input_allocate_polled_device();
+ if (!dev->ip_dev)
+ return -ENOMEM;
+
+ dev->ip_dev->poll = toshiba_acpi_joystick_poll;
+ dev->ip_dev->poll_interval = 50;
+ dev->ip_dev->poll_interval_min = 0;
+ dev->ip_dev->poll_interval_max = 2000;
+ dev->ip_dev->private = dev;
+ idev = dev->ip_dev->input;
+
+ idev->name = "Toshiba HAPS Accelerometer";
+ idev->phys = "toshiba_acpi/input1";
+ idev->id.bustype = BUS_HOST;
+
+ set_bit(EV_ABS, idev->evbit);
+
+ input_set_abs_params(idev, ABS_X, -512, 512, 0, 0);
+ input_set_abs_params(idev, ABS_Y, -512, 512, 0, 0);
+ input_set_abs_params(idev, ABS_Z, -716, 716, 0, 0);
+
+ ret = input_register_polled_device(dev->ip_dev);
+ if (ret) {
+ input_free_polled_device(dev->ip_dev);
+ dev->ip_dev = NULL;
+ }
+
+ return ret;
+}
+
static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
{
struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
@@ -1658,6 +1706,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
if (dev->eco_supported)
led_classdev_unregister(&dev->eco_led);

+ if (dev->joystick_registered) {
+ input_unregister_polled_device(dev->ip_dev);
+ input_free_polled_device(dev->ip_dev);
+ }
+
if (toshiba_acpi)
toshiba_acpi = NULL;

@@ -1777,6 +1830,12 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)

ret = toshiba_accelerometer_supported(dev);
dev->accelerometer_supported = !ret;
+ if (dev->accelerometer_supported) {
+ if (toshiba_acpi_setup_joystick(dev) < 0)
+ pr_err("Unable to activate joystick");
+ else
+ dev->joystick_registered = 1;
+ }

/* Determine whether or not BIOS supports fan and video interfaces */
--
2.0.0
Darren Hart
2014-09-06 02:42:54 UTC
Permalink
Post by Azael Avalos
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.
This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.
Hrm, while sysfs details can change across kernel versions, usually due to
driver core changes, we try to keep them as consistent as possible so as not to
break userspace.

That said, if we are going to try and come up with a better model for
representing an accelerometer, wouldn't treating it as an IIO device be the more
logical approach?
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-06 05:04:18 UTC
Permalink
Hi there,
Post by Darren Hart
Post by Azael Avalos
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.
This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.
Hrm, while sysfs details can change across kernel versions, usually due to
driver core changes, we try to keep them as consistent as possible so as not to
break userspace.
That said, if we are going to try and come up with a better model for
representing an accelerometer, wouldn't treating it as an IIO device be the more
logical approach?
Yes of course, but the actual accelerometer device (sensor?) is not
really exposed,
only certain "functions" it provides, and they are divided across two
different ACPI devices,
TOS620A exposes the protection, and the TOS1900 (and et. al.) only
exposes the axes.

I see your point in breaking userspace, but given the fact that it was
recently introduced,
I didn't thought it was already "adopted", that's why I decided to
remove the sysfs entry.

Then we might as well keep the sysfs entry and have the input polled
device as well.
Post by Darren Hart
--
Darren Hart
Intel Open Source Technology Center
Cheers
Azael
--
-- El mundo apesta y vosotros apestais tambien --
Darren Hart
2014-09-09 00:04:30 UTC
Permalink
Post by Azael Avalos
Hi there,
Post by Darren Hart
Post by Azael Avalos
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.
This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.
Hrm, while sysfs details can change across kernel versions, usually due to
driver core changes, we try to keep them as consistent as possible so as not to
break userspace.
That said, if we are going to try and come up with a better model for
representing an accelerometer, wouldn't treating it as an IIO device be the more
logical approach?
Yes of course, but the actual accelerometer device (sensor?) is not
really exposed,
only certain "functions" it provides, and they are divided across two
different ACPI devices,
TOS620A exposes the protection, and the TOS1900 (and et. al.) only
exposes the axes.
As I understand it, IIO defines an interface to a device, a standard sysfs set
of properties. I should think we could provide the appropriate callbacks even
for a partially implemented (or a pair of) accelerometer.

Jonathan, what are your thoughts here. Is such a "device" (ACPI accessors to
axis and threshold) a candidate for IIO, or is this input polled device more
appropriate?
Post by Azael Avalos
I see your point in breaking userspace, but given the fact that it was
recently introduced,
I didn't thought it was already "adopted", that's why I decided to
remove the sysfs entry.
Looks like since 3.15 if I read the log correctly. That is fairly recent and
this is not one of the "defined interfaces" in the sysfs documentation.

Greg, can you weigh in here - does this change count as "breaking userspace", or
is this more inline with the scheduler knobs in /proc/sched_debug which can
change from version to version.
Post by Azael Avalos
Then we might as well keep the sysfs entry and have the input polled
device as well.
Let's see what Greg has to say. If he isn't bothered by the change, I won't push
the issue.
--
Darren Hart
Intel Open Source Technology Center
Greg Kroah-Hartman
2014-09-09 01:35:53 UTC
Permalink
Post by Darren Hart
Post by Azael Avalos
Hi there,
Post by Darren Hart
Post by Azael Avalos
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.
This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.
Hrm, while sysfs details can change across kernel versions, usually due to
driver core changes, we try to keep them as consistent as possible so as not to
break userspace.
That said, if we are going to try and come up with a better model for
representing an accelerometer, wouldn't treating it as an IIO device be the more
logical approach?
Yes of course, but the actual accelerometer device (sensor?) is not
really exposed,
only certain "functions" it provides, and they are divided across two
different ACPI devices,
TOS620A exposes the protection, and the TOS1900 (and et. al.) only
exposes the axes.
As I understand it, IIO defines an interface to a device, a standard sysfs set
of properties. I should think we could provide the appropriate callbacks even
for a partially implemented (or a pair of) accelerometer.
Jonathan, what are your thoughts here. Is such a "device" (ACPI accessors to
axis and threshold) a candidate for IIO, or is this input polled device more
appropriate?
Post by Azael Avalos
I see your point in breaking userspace, but given the fact that it was
recently introduced,
I didn't thought it was already "adopted", that's why I decided to
remove the sysfs entry.
Looks like since 3.15 if I read the log correctly. That is fairly recent and
this is not one of the "defined interfaces" in the sysfs documentation.
Greg, can you weigh in here - does this change count as "breaking userspace", or
is this more inline with the scheduler knobs in /proc/sched_debug which can
change from version to version.
Post by Azael Avalos
Then we might as well keep the sysfs entry and have the input polled
device as well.
Let's see what Greg has to say. If he isn't bothered by the change, I won't push
the issue.
If it should be an IIO device, great, make it an IIO device, and move
away from a custom sysfs interface that matches nothing else.

But I really doubt it should be a joystick device, that just doesn't
make sense at all.

thanks,

greg k-h
Darren Hart
2014-09-10 03:35:20 UTC
Permalink
Post by Greg Kroah-Hartman
Post by Darren Hart
Post by Azael Avalos
Hi there,
Post by Darren Hart
Post by Azael Avalos
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.
This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.
Hrm, while sysfs details can change across kernel versions, usually due to
driver core changes, we try to keep them as consistent as possible so as not to
break userspace.
That said, if we are going to try and come up with a better model for
representing an accelerometer, wouldn't treating it as an IIO device be the more
logical approach?
Yes of course, but the actual accelerometer device (sensor?) is not
really exposed,
only certain "functions" it provides, and they are divided across two
different ACPI devices,
TOS620A exposes the protection, and the TOS1900 (and et. al.) only
exposes the axes.
As I understand it, IIO defines an interface to a device, a standard sysfs set
of properties. I should think we could provide the appropriate callbacks even
for a partially implemented (or a pair of) accelerometer.
Jonathan, what are your thoughts here. Is such a "device" (ACPI accessors to
axis and threshold) a candidate for IIO, or is this input polled device more
appropriate?
Post by Azael Avalos
I see your point in breaking userspace, but given the fact that it was
recently introduced,
I didn't thought it was already "adopted", that's why I decided to
remove the sysfs entry.
Looks like since 3.15 if I read the log correctly. That is fairly recent and
this is not one of the "defined interfaces" in the sysfs documentation.
Greg, can you weigh in here - does this change count as "breaking userspace", or
is this more inline with the scheduler knobs in /proc/sched_debug which can
change from version to version.
Post by Azael Avalos
Then we might as well keep the sysfs entry and have the input polled
device as well.
Let's see what Greg has to say. If he isn't bothered by the change, I won't push
the issue.
If it should be an IIO device, great, make it an IIO device, and move
away from a custom sysfs interface that matches nothing else.
But I really doubt it should be a joystick device, that just doesn't
make sense at all.
I immediately went to a tablet with a marble maze game and it didn't seem too
crazy, but I don't suppose that is what people are actually doing with it...

What are people actually doing with this thing Azael?
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-10 15:28:44 UTC
Permalink
Hi there,
Post by Darren Hart
I immediately went to a tablet with a marble maze game and it didn't seem too
crazy, but I don't suppose that is what people are actually doing with it...
What are people actually doing with this thing Azael?
Gaming mostly (supertuxkart anyone?), but some others (including myself)
want to use it as a movement detection (one exists for the IBM/Lenovo
Thinkpads).

Digging into platform drivers, I've found that the hdaps and also the lis3lv02d
drivers report the axes via polldev, and since I don't want to break userspace,
I'm left with two choices:

1 - Keep sysfs entry and adapt it to properly report direction, and no
polled device.

2 - Keep sysfs entry and adapt it to properly report direction, and also
a polled device.

Let me know your decision so I can send an updated patch.
Post by Darren Hart
--
Darren Hart
Intel Open Source Technology Center
Cheers
Azael
--
-- El mundo apesta y vosotros apestais tambien --
Greg Kroah-Hartman
2014-09-10 16:08:58 UTC
Permalink
Post by Azael Avalos
Hi there,
Post by Darren Hart
I immediately went to a tablet with a marble maze game and it didn't seem too
crazy, but I don't suppose that is what people are actually doing with it...
What are people actually doing with this thing Azael?
Gaming mostly (supertuxkart anyone?), but some others (including myself)
want to use it as a movement detection (one exists for the IBM/Lenovo
Thinkpads).
Digging into platform drivers, I've found that the hdaps and also the lis3lv02d
drivers report the axes via polldev, and since I don't want to break userspace,
1 - Keep sysfs entry and adapt it to properly report direction, and no
polled device.
2 - Keep sysfs entry and adapt it to properly report direction, and also
a polled device.
Let me know your decision so I can send an updated patch.
3 - use the correct api and change it to iio so that all userspace tools
can correctly interact with it, instead of dealing with a
driver-specific sysfs file.

thanks,

greg k-h
Jonathan Cameron
2014-09-17 16:36:31 UTC
Permalink
Post by Darren Hart
Post by Azael Avalos
Hi there,
Post by Darren Hart
Post by Azael Avalos
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.
This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.
Hrm, while sysfs details can change across kernel versions, usually
due to
Post by Azael Avalos
Post by Darren Hart
driver core changes, we try to keep them as consistent as possible
so as not to
Post by Azael Avalos
Post by Darren Hart
break userspace.
That said, if we are going to try and come up with a better model
for
Post by Azael Avalos
Post by Darren Hart
representing an accelerometer, wouldn't treating it as an IIO
device be the more
Post by Azael Avalos
Post by Darren Hart
logical approach?
Yes of course, but the actual accelerometer device (sensor?) is not
really exposed,
only certain "functions" it provides, and they are divided across two
different ACPI devices,
TOS620A exposes the protection, and the TOS1900 (and et. al.) only
exposes the axes.
As I understand it, IIO defines an interface to a device, a standard sysfs set
of properties. I should think we could provide the appropriate
callbacks even
for a partially implemented (or a pair of) accelerometer.
Jonathan, what are your thoughts here. Is such a "device" (ACPI
accessors to
axis and threshold) a candidate for IIO, or is this input polled device more
appropriate?
Absolutely fine in IIO.

Sorry I took so long to reply. Read the title and expected more detailed issue so queued
it up for when I had more time. Oops.

Only slight gotcha is that there is some debate over the iio timer trigger
configuration interface which would be equivalent of a polled input device.

Hence it hasn't merged yet.
Comes down to how these are instantiated. Lars-Peter Clausen is planning a configfs
proposal rather than how we do the user space trigger creation currently.

A user space trigger would work but then you loose lack of hitting sysfs files.
Post by Darren Hart
Post by Azael Avalos
I see your point in breaking userspace, but given the fact that it
was
Post by Azael Avalos
recently introduced,
I didn't thought it was already "adopted", that's why I decided to
remove the sysfs entry.
Looks like since 3.15 if I read the log correctly. That is fairly recent and
this is not one of the "defined interfaces" in the sysfs documentation.
Greg, can you weigh in here - does this change count as "breaking userspace", or
is this more inline with the scheduler knobs in /proc/sched_debug which can
change from version to version.
Post by Azael Avalos
Then we might as well keep the sysfs entry and have the input polled
device as well.
Let's see what Greg has to say. If he isn't bothered by the change, I won't push
the issue.
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Darren Hart
2014-09-17 18:38:09 UTC
Permalink
Post by Jonathan Cameron
Post by Darren Hart
Post by Azael Avalos
Hi there,
Post by Darren Hart
Post by Azael Avalos
The accelerometer sensor is very sensitive, and having userspace
poll the sysfs position entry is not very battery friendly.
This patch removes the sysfs entry and instead, it creates an
input polled device (joystick) for the built-in accelerometer.
Hrm, while sysfs details can change across kernel versions, usually
due to
Post by Azael Avalos
Post by Darren Hart
driver core changes, we try to keep them as consistent as possible
so as not to
Post by Azael Avalos
Post by Darren Hart
break userspace.
That said, if we are going to try and come up with a better model
for
Post by Azael Avalos
Post by Darren Hart
representing an accelerometer, wouldn't treating it as an IIO
device be the more
Post by Azael Avalos
Post by Darren Hart
logical approach?
Yes of course, but the actual accelerometer device (sensor?) is not
really exposed,
only certain "functions" it provides, and they are divided across two
different ACPI devices,
TOS620A exposes the protection, and the TOS1900 (and et. al.) only
exposes the axes.
As I understand it, IIO defines an interface to a device, a standard sysfs set
of properties. I should think we could provide the appropriate callbacks even
for a partially implemented (or a pair of) accelerometer.
Jonathan, what are your thoughts here. Is such a "device" (ACPI accessors to
axis and threshold) a candidate for IIO, or is this input polled device more
appropriate?
Absolutely fine in IIO.
Sorry I took so long to reply. Read the title and expected more detailed issue so queued
it up for when I had more time. Oops.
Only slight gotcha is that there is some debate over the iio timer trigger
configuration interface which would be equivalent of a polled input device.
Hence it hasn't merged yet.
Comes down to how these are instantiated. Lars-Peter Clausen is planning a configfs
proposal rather than how we do the user space trigger creation currently.
A user space trigger would work but then you loose lack of hitting sysfs files.
Thanks Jonathan,

Azael, please follow-up with the IIO folks and if you want to modify the
interface, please do so via IIO so it uses a consistent interface and we can
eliminate these custom sysfs files.

Thanks,
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-05 17:14:03 UTC
Permalink
Appart from reporting hotkeys, the INFO method is used
as a system wide event notifier for hardware or
software changes.

This patch adds additional "events" to the keymap list,
ignored by now, until we find them a good use.

Signed-off-by: Azael Avalos <***@gmail.com>
---
drivers/platform/x86/toshiba_acpi.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index b062d3d..a149bc6 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -190,6 +190,7 @@ static const struct key_entry toshiba_acpi_keymap[] = {
{ KE_KEY, 0x101, { KEY_MUTE } },
{ KE_KEY, 0x102, { KEY_ZOOMOUT } },
{ KE_KEY, 0x103, { KEY_ZOOMIN } },
+ { KE_KEY, 0x10f, { KEY_TAB } },
{ KE_KEY, 0x12c, { KEY_KBDILLUMTOGGLE } },
{ KE_KEY, 0x139, { KEY_ZOOMRESET } },
{ KE_KEY, 0x13b, { KEY_COFFEE } },
@@ -210,7 +211,11 @@ static const struct key_entry toshiba_acpi_keymap[] = {
{ KE_KEY, 0xb32, { KEY_NEXTSONG } },
{ KE_KEY, 0xb33, { KEY_PLAYPAUSE } },
{ KE_KEY, 0xb5a, { KEY_MEDIA } },
- { KE_IGNORE, 0x1430, { KEY_RESERVED } },
+ { KE_IGNORE, 0x1430, { KEY_RESERVED } }, /* Wake from sleep */
+ { KE_IGNORE, 0x1501, { KEY_RESERVED } }, /* Output changed */
+ { KE_IGNORE, 0x1502, { KEY_RESERVED } }, /* HDMI plugged/unplugged */
+ { KE_IGNORE, 0x1ABE, { KEY_RESERVED } }, /* Protection level set */
+ { KE_IGNORE, 0x1ABF, { KEY_RESERVED } }, /* Protection level off */
{ KE_END, 0 },
};
--
2.0.0
Darren Hart
2014-09-09 00:12:08 UTC
Permalink
Post by Azael Avalos
Appart from reporting hotkeys, the INFO method is used
as a system wide event notifier for hardware or
software changes.
This patch adds additional "events" to the keymap list,
ignored by now, until we find them a good use.
Queued, thanks.
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-05 17:14:07 UTC
Permalink
The function toshiba_touchpad_store is not checking
for invalid values and simply returns silently.

This patch checks for invalid values and returns accordingly.

Signed-off-by: Azael Avalos <***@gmail.com>
---
drivers/platform/x86/toshiba_acpi.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 1738171..777fb3c 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -1396,12 +1396,18 @@ static ssize_t toshiba_touchpad_store(struct device *dev,
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
int state;
+ int ret;

/* Set the TouchPad on/off, 0 - Disable | 1 - Enable */
- if (sscanf(buf, "%i", &state) == 1 && (state == 0 || state == 1)) {
- if (toshiba_touchpad_set(toshiba, state) < 0)
- return -EIO;
- }
+ ret = kstrtoint(buf, 0, &state);
+ if (ret)
+ return ret;
+ if (state != 0 || state != 1)
+ return -EINVAL;
+
+ ret = toshiba_touchpad_set(toshiba, state);
+ if (ret)
+ return ret;

return count;
}
--
2.0.0
Darren Hart
2014-09-10 04:17:40 UTC
Permalink
Post by Azael Avalos
The function toshiba_touchpad_store is not checking
for invalid values and simply returns silently.
This patch checks for invalid values and returns accordingly.
Queued, thanks.
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-05 17:14:06 UTC
Permalink
Newer Toshiba models now come with a new (and different) keyboard
backlight implementation whith three modes of operation: TIMER,
ON and OFF, and the LED is controlled internally by the firmware.

This patch adds support for that type of backlight, changing the
existing code to accomodate the new implementation.

The timeout value range is now 1-60 seconds, and the accepted
modes are now: 0 (OFF), 1 (ON or FN-Z) and 2 (AUTO or TIMER), and
the keyboard_backlight_mode entry now displays two values, the
keyboard backlight type (either 1 or 2) and the current mode.

Signed-off-by: Azael Avalos <***@gmail.com>
---
drivers/platform/x86/toshiba_acpi.c | 145 ++++++++++++++++++++++++------------
1 file changed, 98 insertions(+), 47 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index ac1503c..1738171 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -142,6 +142,8 @@ MODULE_LICENSE("GPL");
#define HCI_WIRELESS_BT_POWER 0x80
#define SCI_KBD_MODE_FNZ 0x1
#define SCI_KBD_MODE_AUTO 0x2
+#define SCI_KBD_MODE_ON 0x8
+#define SCI_KBD_MODE_OFF 0x10

struct toshiba_acpi_dev {
struct acpi_device *acpi_dev;
@@ -158,6 +160,7 @@ struct toshiba_acpi_dev {
int force_fan;
int last_key_event;
int key_event_valid;
+ int kbd_type;
int kbd_mode;
int kbd_time;

@@ -499,28 +502,36 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
}

/* KBD Illumination */
-static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
+static int toshiba_kbd_illum_available(struct toshiba_acpi_dev *dev)
{
- u32 result;
+ u32 in[HCI_WORDS] = { SCI_GET, SCI_KBD_ILLUM_STATUS, 0, 0, 0, 0 };
+ u32 out[HCI_WORDS];
acpi_status status;

if (!sci_open(dev))
- return -EIO;
+ return 0;

- status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = hci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to set KBD backlight status failed\n");
- return -EIO;
- } else if (result == HCI_NOT_SUPPORTED) {
- pr_info("Keyboard backlight status not supported\n");
- return -ENODEV;
+ if (ACPI_FAILURE(status) || out[0] == SCI_INPUT_DATA_ERROR) {
+ pr_err("ACPI call to query kbd illumination support failed\n");
+ return 0;
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
+ pr_info("Keyboard illumination not available\n");
+ return 0;
}

- return 0;
+ if (out[3] == 0x3c001a)
+ dev->kbd_type = 2;
+ else
+ dev->kbd_type = 1;
+ dev->kbd_mode = out[2] & 0x1f;
+ dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
+
+ return 1;
}

-static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
+static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
{
u32 result;
acpi_status status;
@@ -528,10 +539,10 @@ static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
if (!sci_open(dev))
return -EIO;

- status = sci_read(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
sci_close(dev);
if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to get KBD backlight status failed\n");
+ pr_err("ACPI call to set KBD backlight status failed\n");
return -EIO;
} else if (result == HCI_NOT_SUPPORTED) {
pr_info("Keyboard backlight status not supported\n");
@@ -1264,22 +1275,54 @@ static ssize_t toshiba_kbd_bl_mode_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int mode = -1;
- int time = -1;
+ int mode;
+ int time;
+ int ret;

- if (sscanf(buf, "%i", &mode) != 1 && (mode != 2 || mode != 1))
+ ret = kstrtoint(buf, 0, &mode);
+ if (ret)
+ return ret;
+ if (mode > 2 || mode < 0)
return -EINVAL;

/* Set the Keyboard Backlight Mode where:
- * Mode - Auto (2) | FN-Z (1)
+ * Mode - Auto (2) | FN-Z or ON (1) | OFF (0)
* Auto - KBD backlight turns off automatically in given time
* FN-Z - KBD backlight "toggles" when hotkey pressed
+ * ON - KBD backlight is always on
+ * OFF - KBD backlight is always off
+ */
+
+ /* Convert userspace values to internal ones,
+ * depending on the keyboard backlight type detected
*/
- if (mode != -1 && toshiba->kbd_mode != mode) {
+ if (mode == 0)
+ mode = SCI_KBD_MODE_OFF;
+ else if (mode == 1 && toshiba->kbd_type == 1)
+ mode = SCI_KBD_MODE_FNZ;
+ else if (mode == 1 && toshiba->kbd_type == 2)
+ mode = SCI_KBD_MODE_ON;
+ else if (mode == 2)
+ mode = SCI_KBD_MODE_AUTO;
+
+ /* Only make a change if the actual mode has changed */
+ if (toshiba->kbd_mode != mode) {
+ /* KBD backlight type 1 doesn't support SCI_KBD_MODE_OFF,
+ * bailout silently if set to it
+ */
+ if (toshiba->kbd_type == 1 && mode == SCI_KBD_MODE_OFF)
+ return count;
+
time = toshiba->kbd_time << HCI_MISC_SHIFT;
- time = time + toshiba->kbd_mode;
- if (toshiba_kbd_illum_status_set(toshiba, time) < 0)
- return -EIO;
+ if (toshiba->kbd_type == 1)
+ time |= toshiba->kbd_mode;
+ else if (toshiba->kbd_type == 2)
+ time |= mode;
+
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+
toshiba->kbd_mode = mode;
}

@@ -1291,12 +1334,17 @@ static ssize_t toshiba_kbd_bl_mode_show(struct device *dev,
char *buf)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- u32 time;
+ int mode;

- if (toshiba_kbd_illum_status_get(toshiba, &time) < 0)
- return -EIO;
+ if (toshiba->kbd_mode == SCI_KBD_MODE_OFF)
+ mode = 0;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_FNZ ||
+ toshiba->kbd_mode == SCI_KBD_MODE_ON)
+ mode = 1;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_AUTO)
+ mode = 2;

- return sprintf(buf, "%i\n", time & 0x07);
+ return sprintf(buf, "%i %i\n", toshiba->kbd_type, mode);
}

static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
@@ -1304,18 +1352,29 @@ static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int time = -1;
+ int time;
+ int ret;

- if (sscanf(buf, "%i", &time) != 1 && (time < 0 || time > 60))
+ ret = kstrtoint(buf, 0, &time);
+ if (ret)
+ return ret;
+ if (time < 1 || time > 60)
return -EINVAL;

- /* Set the Keyboard Backlight Timeout: 0-60 seconds */
- if (time != -1 && toshiba->kbd_time != time) {
+ /* Set the Keyboard Backlight Timeout: 1-60 seconds
+ * Only make the change if the actual timeout has changed
+ */
+ if (toshiba->kbd_time != time) {
time = time << HCI_MISC_SHIFT;
- time = (toshiba->kbd_mode == SCI_KBD_MODE_AUTO) ?
- time + 1 : time + 2;
- if (toshiba_kbd_illum_status_set(toshiba, time) < 0)
- return -EIO;
+ if (toshiba->kbd_type == 1)
+ time |= SCI_KBD_MODE_FNZ;
+ else if (toshiba->kbd_type == 2)
+ time |= SCI_KBD_MODE_AUTO;
+
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+
toshiba->kbd_time = time >> HCI_MISC_SHIFT;
}

@@ -1327,12 +1386,8 @@ static ssize_t toshiba_kbd_bl_timeout_show(struct device *dev,
char *buf)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- u32 time;
-
- if (toshiba_kbd_illum_status_get(toshiba, &time) < 0)
- return -EIO;

- return sprintf(buf, "%i\n", time >> HCI_MISC_SHIFT);
+ return sprintf(buf, "%i\n", toshiba->kbd_time);
}

static ssize_t toshiba_touchpad_store(struct device *dev,
@@ -1389,7 +1444,7 @@ static umode_t toshiba_sysfs_is_visible(struct kobject *kobj,
if (attr == &dev_attr_kbd_backlight_mode.attr)
exists = (drv->kbd_illum_supported) ? true : false;
else if (attr == &dev_attr_kbd_backlight_timeout.attr)
- exists = (drv->kbd_mode == SCI_KBD_MODE_AUTO) ? true : false;
+ exists = (drv->kbd_mode == SCI_KBD_MODE_FNZ) ? false : true;
else if (attr == &dev_attr_touchpad.attr)
exists = (drv->touchpad_supported) ? true : false;

@@ -1806,15 +1861,11 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
dev->eco_supported = 1;
}

- ret = toshiba_kbd_illum_status_get(dev, &dummy);
- if (!ret) {
- dev->kbd_time = dummy >> HCI_MISC_SHIFT;
- dev->kbd_mode = dummy & 0x07;
- }
- dev->kbd_illum_supported = !ret;
+ dev->kbd_illum_supported = toshiba_kbd_illum_available(dev);
/*
* Only register the LED if KBD illumination is supported
- * and the keyboard backlight operation mode is set to FN-Z
+ * and the keyboard backlight operation mode is set to FN-Z,
+ * keyboard backlight type 2 returns 0x8400 (HCI WRITE PROTECTED)
*/
if (dev->kbd_illum_supported && dev->kbd_mode == SCI_KBD_MODE_FNZ) {
dev->kbd_led.name = "toshiba::kbd_backlight";
--
2.0.0
Darren Hart
2014-09-10 04:11:45 UTC
Permalink
On Fri, Sep 05, 2014 at 11:14:06AM -0600, Azael Avalos wrote:

Hi Azael,

Apologies for the delay. I'm still recovering from a couple weeks of travel and
a nasty conference bug. Thanks for being patient.
Post by Azael Avalos
Newer Toshiba models now come with a new (and different) keyboard
backlight implementation whith three modes of operation: TIMER,
ON and OFF, and the LED is controlled internally by the firmware.
This patch adds support for that type of backlight, changing the
existing code to accomodate the new implementation.
The timeout value range is now 1-60 seconds, and the accepted
modes are now: 0 (OFF), 1 (ON or FN-Z) and 2 (AUTO or TIMER), and
the keyboard_backlight_mode entry now displays two values, the
keyboard backlight type (either 1 or 2) and the current mode.
Wouldn't adding a new entry make more sense than multiplexing an existing one? I
was fairly sure that was contrary to the goals of sys...
On testing, were you able to verify on new as well as previous models that this
continues to work?
Post by Azael Avalos
---
drivers/platform/x86/toshiba_acpi.c | 145 ++++++++++++++++++++++++------------
1 file changed, 98 insertions(+), 47 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index ac1503c..1738171 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -142,6 +142,8 @@ MODULE_LICENSE("GPL");
#define HCI_WIRELESS_BT_POWER 0x80
#define SCI_KBD_MODE_FNZ 0x1
#define SCI_KBD_MODE_AUTO 0x2
+#define SCI_KBD_MODE_ON 0x8
+#define SCI_KBD_MODE_OFF 0x10
struct toshiba_acpi_dev {
struct acpi_device *acpi_dev;
@@ -158,6 +160,7 @@ struct toshiba_acpi_dev {
int force_fan;
int last_key_event;
int key_event_valid;
+ int kbd_type;
Consider some defines or enum values for the types?
Post by Azael Avalos
int kbd_mode;
int kbd_time;
@@ -499,28 +502,36 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
}
/* KBD Illumination */
-static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
+static int toshiba_kbd_illum_available(struct toshiba_acpi_dev *dev)
{
- u32 result;
+ u32 in[HCI_WORDS] = { SCI_GET, SCI_KBD_ILLUM_STATUS, 0, 0, 0, 0 };
+ u32 out[HCI_WORDS];
acpi_status status;
if (!sci_open(dev))
- return -EIO;
+ return 0;
- status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = hci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to set KBD backlight status failed\n");
- return -EIO;
- } else if (result == HCI_NOT_SUPPORTED) {
- pr_info("Keyboard backlight status not supported\n");
- return -ENODEV;
+ if (ACPI_FAILURE(status) || out[0] == SCI_INPUT_DATA_ERROR) {
+ pr_err("ACPI call to query kbd illumination support failed\n");
+ return 0;
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
+ pr_info("Keyboard illumination not available\n");
+ return 0;
}
- return 0;
+ if (out[3] == 0x3c001a)
Do have any information on what this value means? It would be preferable to use
sensible defines here rather than magic hex codes if at all possible.
Post by Azael Avalos
+ dev->kbd_type = 2;
+ else
+ dev->kbd_type = 1;
A couple enum types would be useful here.
Post by Azael Avalos
+ dev->kbd_mode = out[2] & 0x1f;
define TOSHIBA_KBD_MODE_MASK maybe?
Post by Azael Avalos
+ dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
+
+ return 1;
}
-static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
+static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
{
u32 result;
acpi_status status;
@@ -528,10 +539,10 @@ static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
if (!sci_open(dev))
return -EIO;
- status = sci_read(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
sci_close(dev);
if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to get KBD backlight status failed\n");
+ pr_err("ACPI call to set KBD backlight status failed\n");
return -EIO;
} else if (result == HCI_NOT_SUPPORTED) {
pr_info("Keyboard backlight status not supported\n");
@@ -1264,22 +1275,54 @@ static ssize_t toshiba_kbd_bl_mode_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int mode = -1;
- int time = -1;
+ int mode;
+ int time;
+ int ret;
- if (sscanf(buf, "%i", &mode) != 1 && (mode != 2 || mode != 1))
+ ret = kstrtoint(buf, 0, &mode);
+ if (ret)
+ return ret;
+ if (mode > 2 || mode < 0)
return -EINVAL;
This hunk appears to be unrelated cleanup.
Post by Azael Avalos
- * Mode - Auto (2) | FN-Z (1)
+ * Mode - Auto (2) | FN-Z or ON (1) | OFF (0)
* Auto - KBD backlight turns off automatically in given time
* FN-Z - KBD backlight "toggles" when hotkey pressed
+ * ON - KBD backlight is always on
+ * OFF - KBD backlight is always off
+ */
+
+ /* Convert userspace values to internal ones,
+ * depending on the keyboard backlight type detected
*/
- if (mode != -1 && toshiba->kbd_mode != mode) {
+ if (mode == 0)
+ mode = SCI_KBD_MODE_OFF;
+ else if (mode == 1 && toshiba->kbd_type == 1)
+ mode = SCI_KBD_MODE_FNZ;
+ else if (mode == 1 && toshiba->kbd_type == 2)
The type enums would add some more confidense to this test, as my first thought
was what if kbd_type isn't 1 or 2... which of course it should never be.
Post by Azael Avalos
+ mode = SCI_KBD_MODE_ON;
+ else if (mode == 2)
+ mode = SCI_KBD_MODE_AUTO;
+
There are a number of if blocks around mode and type now. I wonder if a simple
array might make this more condensed, but of course you'd have to do bounds
checking (especially with user data as the index) which might nullify the gains.
Something to consider, I'm not insisting on it.
Post by Azael Avalos
+ /* Only make a change if the actual mode has changed */
+ if (toshiba->kbd_mode != mode) {
+ /* KBD backlight type 1 doesn't support SCI_KBD_MODE_OFF,
+ * bailout silently if set to it
+ */
+ if (toshiba->kbd_type == 1 && mode == SCI_KBD_MODE_OFF)
+ return count;
Why a silent return? Would -EINVAL not be more appropriate?
Post by Azael Avalos
+
time = toshiba->kbd_time << HCI_MISC_SHIFT;
- time = time + toshiba->kbd_mode;
- if (toshiba_kbd_illum_status_set(toshiba, time) < 0)
- return -EIO;
+ if (toshiba->kbd_type == 1)
+ time |= toshiba->kbd_mode;
+ else if (toshiba->kbd_type == 2)
+ time |= mode;
+
What? :)

I'm not following the concept of OR'ing the mode in, and am also confused by why
we use user data for type 2 and internal values for type 1...

Can you explain? And if an explanation is needed, perhaps this can be cleaned up
to be a bit more readable?
Post by Azael Avalos
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+
toshiba->kbd_mode = mode;
}
@@ -1291,12 +1334,17 @@ static ssize_t toshiba_kbd_bl_mode_show(struct device *dev,
char *buf)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- u32 time;
+ int mode;
- if (toshiba_kbd_illum_status_get(toshiba, &time) < 0)
- return -EIO;
+ if (toshiba->kbd_mode == SCI_KBD_MODE_OFF)
+ mode = 0;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_FNZ ||
+ toshiba->kbd_mode == SCI_KBD_MODE_ON)
+ mode = 1;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_AUTO)
+ mode = 2;
- return sprintf(buf, "%i\n", time & 0x07);
+ return sprintf(buf, "%i %i\n", toshiba->kbd_type, mode);
Why overload the mode==1 to mean two different things? Would it make more sense
to add a user mode value for the new modes and add those?

By adding the type you are already breaking any API, so I'm confused about why
you didn't just add a mode value and not add the type here.
Post by Azael Avalos
}
static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
@@ -1304,18 +1352,29 @@ static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int time = -1;
+ int time;
+ int ret;
- if (sscanf(buf, "%i", &time) != 1 && (time < 0 || time > 60))
+ ret = kstrtoint(buf, 0, &time);
+ if (ret)
+ return ret;
+ if (time < 1 || time > 60)
return -EINVAL;
Looks like another (mostly) cleanup block. Perhaps combine with the earlier one
into a patch to remove unecessary assignments and replacing sscanf with
kstrtoint.

Please consider the feedback above in the context of the whole patch and with
how this driver is used and prepare an updated patch.

Thanks,
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-10 16:52:28 UTC
Permalink
Hi Darren,
Post by Darren Hart
Hi Azael,
Apologies for the delay. I'm still recovering from a couple weeks of travel and
a nasty conference bug. Thanks for being patient.
Post by Azael Avalos
Newer Toshiba models now come with a new (and different) keyboard
backlight implementation whith three modes of operation: TIMER,
ON and OFF, and the LED is controlled internally by the firmware.
This patch adds support for that type of backlight, changing the
existing code to accomodate the new implementation.
The timeout value range is now 1-60 seconds, and the accepted
modes are now: 0 (OFF), 1 (ON or FN-Z) and 2 (AUTO or TIMER), and
the keyboard_backlight_mode entry now displays two values, the
keyboard backlight type (either 1 or 2) and the current mode.
Wouldn't adding a new entry make more sense than multiplexing an existing one? I
was fairly sure that was contrary to the goals of sys...
Sure, I don't want to break userspace.
Post by Darren Hart
On testing, were you able to verify on new as well as previous models that this
continues to work?
Yes, that was the first thing I did whenever I got this new implementation.
Post by Darren Hart
Post by Azael Avalos
---
drivers/platform/x86/toshiba_acpi.c | 145 ++++++++++++++++++++++++------------
1 file changed, 98 insertions(+), 47 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index ac1503c..1738171 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -142,6 +142,8 @@ MODULE_LICENSE("GPL");
#define HCI_WIRELESS_BT_POWER 0x80
#define SCI_KBD_MODE_FNZ 0x1
#define SCI_KBD_MODE_AUTO 0x2
+#define SCI_KBD_MODE_ON 0x8
+#define SCI_KBD_MODE_OFF 0x10
struct toshiba_acpi_dev {
struct acpi_device *acpi_dev;
@@ -158,6 +160,7 @@ struct toshiba_acpi_dev {
int force_fan;
int last_key_event;
int key_event_valid;
+ int kbd_type;
Consider some defines or enum values for the types?
Makes sense, in case Toshiba decides to change the keyboard backlight
modes again...
Post by Darren Hart
Post by Azael Avalos
int kbd_mode;
int kbd_time;
@@ -499,28 +502,36 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
}
/* KBD Illumination */
-static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
+static int toshiba_kbd_illum_available(struct toshiba_acpi_dev *dev)
{
- u32 result;
+ u32 in[HCI_WORDS] = { SCI_GET, SCI_KBD_ILLUM_STATUS, 0, 0, 0, 0 };
+ u32 out[HCI_WORDS];
acpi_status status;
if (!sci_open(dev))
- return -EIO;
+ return 0;
- status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = hci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to set KBD backlight status failed\n");
- return -EIO;
- } else if (result == HCI_NOT_SUPPORTED) {
- pr_info("Keyboard backlight status not supported\n");
- return -ENODEV;
+ if (ACPI_FAILURE(status) || out[0] == SCI_INPUT_DATA_ERROR) {
+ pr_err("ACPI call to query kbd illumination support failed\n");
+ return 0;
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
+ pr_info("Keyboard illumination not available\n");
+ return 0;
}
- return 0;
+ if (out[3] == 0x3c001a)
Do have any information on what this value means? It would be preferable to use
sensible defines here rather than magic hex codes if at all possible.
That is the max value the backlight method supports, and on the new
implementation, it is different from the previous one.

On reading any Toshiba method:
out[0] holds success or error
out[1] varies depending on method (usually zero)
out[2] holds the actual value
out[3] holds the max value
out[4] varies depending on method (usually zero)
out[5] varies depending on method (usually zero)
Post by Darren Hart
Post by Azael Avalos
+ dev->kbd_type = 2;
+ else
+ dev->kbd_type = 1;
A couple enum types would be useful here.
Post by Azael Avalos
+ dev->kbd_mode = out[2] & 0x1f;
define TOSHIBA_KBD_MODE_MASK maybe?
Ok
Post by Darren Hart
Post by Azael Avalos
+ dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
+
+ return 1;
}
-static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
+static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
{
u32 result;
acpi_status status;
@@ -528,10 +539,10 @@ static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
if (!sci_open(dev))
return -EIO;
- status = sci_read(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
sci_close(dev);
if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to get KBD backlight status failed\n");
+ pr_err("ACPI call to set KBD backlight status failed\n");
return -EIO;
} else if (result == HCI_NOT_SUPPORTED) {
pr_info("Keyboard backlight status not supported\n");
@@ -1264,22 +1275,54 @@ static ssize_t toshiba_kbd_bl_mode_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int mode = -1;
- int time = -1;
+ int mode;
+ int time;
+ int ret;
- if (sscanf(buf, "%i", &mode) != 1 && (mode != 2 || mode != 1))
+ ret = kstrtoint(buf, 0, &mode);
+ if (ret)
+ return ret;
+ if (mode > 2 || mode < 0)
return -EINVAL;
This hunk appears to be unrelated cleanup.
Since it was part of the keyboard backlight mode I thought I could
include it in this patch, I'll send a separete patch later for mode store
and timeout store as well.
Post by Darren Hart
Post by Azael Avalos
- * Mode - Auto (2) | FN-Z (1)
+ * Mode - Auto (2) | FN-Z or ON (1) | OFF (0)
* Auto - KBD backlight turns off automatically in given time
* FN-Z - KBD backlight "toggles" when hotkey pressed
+ * ON - KBD backlight is always on
+ * OFF - KBD backlight is always off
+ */
+
+ /* Convert userspace values to internal ones,
+ * depending on the keyboard backlight type detected
*/
- if (mode != -1 && toshiba->kbd_mode != mode) {
+ if (mode == 0)
+ mode = SCI_KBD_MODE_OFF;
+ else if (mode == 1 && toshiba->kbd_type == 1)
+ mode = SCI_KBD_MODE_FNZ;
+ else if (mode == 1 && toshiba->kbd_type == 2)
The type enums would add some more confidense to this test, as my first thought
was what if kbd_type isn't 1 or 2... which of course it should never be.
Post by Azael Avalos
+ mode = SCI_KBD_MODE_ON;
+ else if (mode == 2)
+ mode = SCI_KBD_MODE_AUTO;
+
There are a number of if blocks around mode and type now. I wonder if a simple
array might make this more condensed, but of course you'd have to do bounds
checking (especially with user data as the index) which might nullify the gains.
Something to consider, I'm not insisting on it.
I was using a switch before, but for saving a few lines I changed it to
a bunch of if-else, so perhaps I can switch back to a switch :-P
Post by Darren Hart
Post by Azael Avalos
+ /* Only make a change if the actual mode has changed */
+ if (toshiba->kbd_mode != mode) {
+ /* KBD backlight type 1 doesn't support SCI_KBD_MODE_OFF,
+ * bailout silently if set to it
+ */
+ if (toshiba->kbd_type == 1 && mode == SCI_KBD_MODE_OFF)
+ return count;
Why a silent return? Would -EINVAL not be more appropriate?
Ok
Post by Darren Hart
Post by Azael Avalos
+
time = toshiba->kbd_time << HCI_MISC_SHIFT;
- time = time + toshiba->kbd_mode;
- if (toshiba_kbd_illum_status_set(toshiba, time) < 0)
- return -EIO;
+ if (toshiba->kbd_type == 1)
+ time |= toshiba->kbd_mode;
+ else if (toshiba->kbd_type == 2)
+ time |= mode;
+
What? :)
Welcome to Toshiba's keyboard backlight implementation :-)
Post by Darren Hart
I'm not following the concept of OR'ing the mode in, and am also confused by why
we use user data for type 2 and internal values for type 1...
The first kbd bl implementation has two modes of operation, SCI_KBD_MODE_AUTO
and SCI_KBD_MODE_FNZ, to change modes you need to set the timeout value
to the current mode, either by oring or adding, both yield the same result.

On the second implementation, we now have three modes, SCI_KBD_MODE_AUTO,
SCI_KBD_MODE_ON and SCI_KBD_MODE_OFF, to change modes you need to
set the timeout value to the desired mode you want to change, again by oring
or adding.
Post by Darren Hart
Can you explain? And if an explanation is needed, perhaps this can be cleaned up
to be a bit more readable?
Sure thing, I can add a buch of comments along the way to let know others
what is happening. I'll explain below :-)

- This shifts the current timeout value stored, yielding a value from
0x10000-0x3c0000
for timeout values 1-60 seconds
time = toshiba->kbd_time << HCI_MISC_SHIFT;

- This changes modes depending on kbd bl type, if the type is one (or the first
implementation), OR the value to the current mode, yielding 0x3c0001 or
0x3c0002 for a timeout value of 60 seconds.
if (toshiba->kbd_type == 1)
time |= toshiba->kbd_mode;

- When the type is two (or the second implementation) the value yielded will be
0x3c0002, 0x3c0008 or 0x3c0010 for a timeout value of 60 seconds
if (toshiba->kbd_type == 2)
time |= mode;


Hope this clear things a bit.
Post by Darren Hart
Post by Azael Avalos
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+
toshiba->kbd_mode = mode;
}
@@ -1291,12 +1334,17 @@ static ssize_t toshiba_kbd_bl_mode_show(struct device *dev,
char *buf)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- u32 time;
+ int mode;
- if (toshiba_kbd_illum_status_get(toshiba, &time) < 0)
- return -EIO;
+ if (toshiba->kbd_mode == SCI_KBD_MODE_OFF)
+ mode = 0;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_FNZ ||
+ toshiba->kbd_mode == SCI_KBD_MODE_ON)
+ mode = 1;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_AUTO)
+ mode = 2;
- return sprintf(buf, "%i\n", time & 0x07);
+ return sprintf(buf, "%i %i\n", toshiba->kbd_type, mode);
Why overload the mode==1 to mean two different things? Would it make more sense
to add a user mode value for the new modes and add those?
Sure, its even easier that way, I just wanted to make things to
userspace easier,
but I'll simply add those values and make userspace deal with them.
Post by Darren Hart
By adding the type you are already breaking any API, so I'm confused about why
you didn't just add a mode value and not add the type here.
Ok, I don't want to break any userspace or APIs here.
Post by Darren Hart
Post by Azael Avalos
}
static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
@@ -1304,18 +1352,29 @@ static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int time = -1;
+ int time;
+ int ret;
- if (sscanf(buf, "%i", &time) != 1 && (time < 0 || time > 60))
+ ret = kstrtoint(buf, 0, &time);
+ if (ret)
+ return ret;
+ if (time < 1 || time > 60)
return -EINVAL;
Looks like another (mostly) cleanup block. Perhaps combine with the earlier one
into a patch to remove unecessary assignments and replacing sscanf with
kstrtoint.
Ok
Post by Darren Hart
Please consider the feedback above in the context of the whole patch and with
how this driver is used and prepare an updated patch.
I'll just wait for your comments and send an updated patch.
Post by Darren Hart
Thanks,
--
Darren Hart
Intel Open Source Technology Center
Cheers
Azael
--
-- El mundo apesta y vosotros apestais tambien --
Darren Hart
2014-09-10 18:34:07 UTC
Permalink
Post by Azael Avalos
Hi Darren,
Post by Darren Hart
Hi Azael,
Apologies for the delay. I'm still recovering from a couple weeks of travel and
a nasty conference bug. Thanks for being patient.
Post by Azael Avalos
Newer Toshiba models now come with a new (and different) keyboard
backlight implementation whith three modes of operation: TIMER,
ON and OFF, and the LED is controlled internally by the firmware.
This patch adds support for that type of backlight, changing the
existing code to accomodate the new implementation.
The timeout value range is now 1-60 seconds, and the accepted
modes are now: 0 (OFF), 1 (ON or FN-Z) and 2 (AUTO or TIMER), and
the keyboard_backlight_mode entry now displays two values, the
keyboard backlight type (either 1 or 2) and the current mode.
Wouldn't adding a new entry make more sense than multiplexing an existing one? I
was fairly sure that was contrary to the goals of sys...
Sure, I don't want to break userspace.
Post by Darren Hart
On testing, were you able to verify on new as well as previous models that this
continues to work?
Yes, that was the first thing I did whenever I got this new implementation.
Post by Darren Hart
Post by Azael Avalos
---
drivers/platform/x86/toshiba_acpi.c | 145 ++++++++++++++++++++++++------------
1 file changed, 98 insertions(+), 47 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index ac1503c..1738171 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -142,6 +142,8 @@ MODULE_LICENSE("GPL");
#define HCI_WIRELESS_BT_POWER 0x80
#define SCI_KBD_MODE_FNZ 0x1
#define SCI_KBD_MODE_AUTO 0x2
+#define SCI_KBD_MODE_ON 0x8
+#define SCI_KBD_MODE_OFF 0x10
struct toshiba_acpi_dev {
struct acpi_device *acpi_dev;
@@ -158,6 +160,7 @@ struct toshiba_acpi_dev {
int force_fan;
int last_key_event;
int key_event_valid;
+ int kbd_type;
Consider some defines or enum values for the types?
Makes sense, in case Toshiba decides to change the keyboard backlight
modes again...
Post by Darren Hart
Post by Azael Avalos
int kbd_mode;
int kbd_time;
@@ -499,28 +502,36 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
}
/* KBD Illumination */
-static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
+static int toshiba_kbd_illum_available(struct toshiba_acpi_dev *dev)
{
- u32 result;
+ u32 in[HCI_WORDS] = { SCI_GET, SCI_KBD_ILLUM_STATUS, 0, 0, 0, 0 };
+ u32 out[HCI_WORDS];
acpi_status status;
if (!sci_open(dev))
- return -EIO;
+ return 0;
- status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = hci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to set KBD backlight status failed\n");
- return -EIO;
- } else if (result == HCI_NOT_SUPPORTED) {
- pr_info("Keyboard backlight status not supported\n");
- return -ENODEV;
+ if (ACPI_FAILURE(status) || out[0] == SCI_INPUT_DATA_ERROR) {
+ pr_err("ACPI call to query kbd illumination support failed\n");
+ return 0;
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
+ pr_info("Keyboard illumination not available\n");
+ return 0;
}
- return 0;
+ if (out[3] == 0x3c001a)
Do have any information on what this value means? It would be preferable to use
sensible defines here rather than magic hex codes if at all possible.
That is the max value the backlight method supports, and on the new
implementation, it is different from the previous one.
out[0] holds success or error
out[1] varies depending on method (usually zero)
out[2] holds the actual value
out[3] holds the max value
out[4] varies depending on method (usually zero)
out[5] varies depending on method (usually zero)
Thanks. Please incorporate that into the comments, probably fairly early in the
code.
Post by Azael Avalos
Post by Darren Hart
Post by Azael Avalos
+ dev->kbd_type = 2;
+ else
+ dev->kbd_type = 1;
A couple enum types would be useful here.
Post by Azael Avalos
+ dev->kbd_mode = out[2] & 0x1f;
define TOSHIBA_KBD_MODE_MASK maybe?
Ok
Post by Darren Hart
Post by Azael Avalos
+ dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
+
+ return 1;
}
-static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
+static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
{
u32 result;
acpi_status status;
@@ -528,10 +539,10 @@ static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
if (!sci_open(dev))
return -EIO;
- status = sci_read(dev, SCI_KBD_ILLUM_STATUS, time, &result);
+ status = sci_write(dev, SCI_KBD_ILLUM_STATUS, time, &result);
sci_close(dev);
if (ACPI_FAILURE(status) || result == SCI_INPUT_DATA_ERROR) {
- pr_err("ACPI call to get KBD backlight status failed\n");
+ pr_err("ACPI call to set KBD backlight status failed\n");
return -EIO;
} else if (result == HCI_NOT_SUPPORTED) {
pr_info("Keyboard backlight status not supported\n");
@@ -1264,22 +1275,54 @@ static ssize_t toshiba_kbd_bl_mode_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int mode = -1;
- int time = -1;
+ int mode;
+ int time;
+ int ret;
- if (sscanf(buf, "%i", &mode) != 1 && (mode != 2 || mode != 1))
+ ret = kstrtoint(buf, 0, &mode);
+ if (ret)
+ return ret;
+ if (mode > 2 || mode < 0)
return -EINVAL;
This hunk appears to be unrelated cleanup.
Since it was part of the keyboard backlight mode I thought I could
include it in this patch, I'll send a separete patch later for mode store
and timeout store as well.
Post by Darren Hart
Post by Azael Avalos
- * Mode - Auto (2) | FN-Z (1)
+ * Mode - Auto (2) | FN-Z or ON (1) | OFF (0)
* Auto - KBD backlight turns off automatically in given time
* FN-Z - KBD backlight "toggles" when hotkey pressed
+ * ON - KBD backlight is always on
+ * OFF - KBD backlight is always off
+ */
+
+ /* Convert userspace values to internal ones,
+ * depending on the keyboard backlight type detected
*/
- if (mode != -1 && toshiba->kbd_mode != mode) {
+ if (mode == 0)
+ mode = SCI_KBD_MODE_OFF;
+ else if (mode == 1 && toshiba->kbd_type == 1)
+ mode = SCI_KBD_MODE_FNZ;
+ else if (mode == 1 && toshiba->kbd_type == 2)
The type enums would add some more confidense to this test, as my first thought
was what if kbd_type isn't 1 or 2... which of course it should never be.
Ignore this, I prototyped the enum thing I had in mind, it doesn't really help.

If you add the new mode we discussed below, then you don't need the && testts
here, and you can verify if the mode is valid earlier on in argument validation.
Post by Azael Avalos
Post by Darren Hart
Post by Azael Avalos
+ mode = SCI_KBD_MODE_ON;
+ else if (mode == 2)
+ mode = SCI_KBD_MODE_AUTO;
+
There are a number of if blocks around mode and type now. I wonder if a simple
array might make this more condensed, but of course you'd have to do bounds
checking (especially with user data as the index) which might nullify the gains.
Something to consider, I'm not insisting on it.
I was using a switch before, but for saving a few lines I changed it to
a bunch of if-else, so perhaps I can switch back to a switch :-P
There isn't much value in one or the other that I know of with only a few
entries.

I was suggesting something more like:

if (mode >= 0 && mode < TOSHIBA_KBD_MODE_MAX) // maybe this is checked earlier
mode = SCI_KBD_MODE_MAP[mode]
else
return -EINVAL;
Post by Azael Avalos
Post by Darren Hart
Post by Azael Avalos
+ /* Only make a change if the actual mode has changed */
+ if (toshiba->kbd_mode != mode) {
+ /* KBD backlight type 1 doesn't support SCI_KBD_MODE_OFF,
+ * bailout silently if set to it
+ */
+ if (toshiba->kbd_type == 1 && mode == SCI_KBD_MODE_OFF)
+ return count;
Why a silent return? Would -EINVAL not be more appropriate?
Ok
This probably should be done earlier as part of argument validation.
Post by Azael Avalos
Post by Darren Hart
Post by Azael Avalos
+
time = toshiba->kbd_time << HCI_MISC_SHIFT;
- time = time + toshiba->kbd_mode;
- if (toshiba_kbd_illum_status_set(toshiba, time) < 0)
- return -EIO;
+ if (toshiba->kbd_type == 1)
+ time |= toshiba->kbd_mode;
+ else if (toshiba->kbd_type == 2)
+ time |= mode;
+
What? :)
Welcome to Toshiba's keyboard backlight implementation :-)
Post by Darren Hart
I'm not following the concept of OR'ing the mode in, and am also confused by why
we use user data for type 2 and internal values for type 1...
The first kbd bl implementation has two modes of operation, SCI_KBD_MODE_AUTO
and SCI_KBD_MODE_FNZ, to change modes you need to set the timeout value
to the current mode, either by oring or adding, both yield the same result.
On the second implementation, we now have three modes, SCI_KBD_MODE_AUTO,
SCI_KBD_MODE_ON and SCI_KBD_MODE_OFF, to change modes you need to
set the timeout value to the desired mode you want to change, again by oring
or adding.
Post by Darren Hart
Can you explain? And if an explanation is needed, perhaps this can be cleaned up
to be a bit more readable?
Sure thing, I can add a buch of comments along the way to let know others
what is happening. I'll explain below :-)
- This shifts the current timeout value stored, yielding a value from
0x10000-0x3c0000
for timeout values 1-60 seconds
time = toshiba->kbd_time << HCI_MISC_SHIFT;
- This changes modes depending on kbd bl type, if the type is one (or the first
implementation), OR the value to the current mode, yielding 0x3c0001 or
0x3c0002 for a timeout value of 60 seconds.
if (toshiba->kbd_type == 1)
time |= toshiba->kbd_mode;
- When the type is two (or the second implementation) the value yielded will be
0x3c0002, 0x3c0008 or 0x3c0010 for a timeout value of 60 seconds
if (toshiba->kbd_type == 2)
time |= mode;
Hope this clear things a bit.
It does, thanks. Yuck :-)

Just a couple lines of comments at each block would help clarify.
/* type 1 requires existing mode OR'd in */

/* type 2 requires new mode OR'd in */

Or something like that, it makes it obvious that it was intentional. Otherwise,
it looks like a coding mistake :-)
Post by Azael Avalos
Post by Darren Hart
Post by Azael Avalos
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+
toshiba->kbd_mode = mode;
}
@@ -1291,12 +1334,17 @@ static ssize_t toshiba_kbd_bl_mode_show(struct device *dev,
char *buf)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- u32 time;
+ int mode;
- if (toshiba_kbd_illum_status_get(toshiba, &time) < 0)
- return -EIO;
+ if (toshiba->kbd_mode == SCI_KBD_MODE_OFF)
+ mode = 0;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_FNZ ||
+ toshiba->kbd_mode == SCI_KBD_MODE_ON)
+ mode = 1;
+ else if (toshiba->kbd_mode == SCI_KBD_MODE_AUTO)
+ mode = 2;
- return sprintf(buf, "%i\n", time & 0x07);
+ return sprintf(buf, "%i %i\n", toshiba->kbd_type, mode);
Why overload the mode==1 to mean two different things? Would it make more sense
to add a user mode value for the new modes and add those?
Sure, its even easier that way, I just wanted to make things to
userspace easier,
but I'll simply add those values and make userspace deal with them.
Post by Darren Hart
By adding the type you are already breaking any API, so I'm confused about why
you didn't just add a mode value and not add the type here.
Ok, I don't want to break any userspace or APIs here.
By adding a new mode and a new file, the existing code should just work with
existing hardware. New code can use the new mode. An available_modes entry might
be worth considering as well, which would drop mode 1 and add mode 3 for type 2

$ cat type
1
$ cat available_modes
0 1 2

$ cat type
2
$ cat available_mods
0 2 3

Something like that.
Post by Azael Avalos
Post by Darren Hart
Post by Azael Avalos
}
static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
@@ -1304,18 +1352,29 @@ static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- int time = -1;
+ int time;
+ int ret;
- if (sscanf(buf, "%i", &time) != 1 && (time < 0 || time > 60))
+ ret = kstrtoint(buf, 0, &time);
+ if (ret)
+ return ret;
+ if (time < 1 || time > 60)
return -EINVAL;
Looks like another (mostly) cleanup block. Perhaps combine with the earlier one
into a patch to remove unecessary assignments and replacing sscanf with
kstrtoint.
Ok
Post by Darren Hart
Please consider the feedback above in the context of the whole patch and with
how this driver is used and prepare an updated patch.
I'll just wait for your comments and send an updated patch.
Thanks,
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-05 17:14:04 UTC
Permalink
Some Toshiba models with illumination support set a different
value on the returned codes, thus not allowing the illumination
LED to be registered, where it should be.

This patch removes a check from toshiba_illumination_available
function to allow such models to register the illumination LED.

Signed-off-by: Azael Avalos <***@gmail.com>
---
drivers/platform/x86/toshiba_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index a149bc6..4803e7b 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -436,7 +436,7 @@ static int toshiba_illumination_available(struct toshiba_acpi_dev *dev)
if (ACPI_FAILURE(status) || out[0] == HCI_FAILURE) {
pr_err("ACPI call to query Illumination support failed\n");
return 0;
- } else if (out[0] == HCI_NOT_SUPPORTED || out[1] != 1) {
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
pr_info("Illumination device not available\n");
return 0;
}
--
2.0.0
Darren Hart
2014-09-06 02:35:37 UTC
Permalink
Post by Azael Avalos
Some Toshiba models with illumination support set a different
value on the returned codes, thus not allowing the illumination
LED to be registered, where it should be.
This patch removes a check from toshiba_illumination_available
function to allow such models to register the illumination LED.
---
drivers/platform/x86/toshiba_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index a149bc6..4803e7b 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -436,7 +436,7 @@ static int toshiba_illumination_available(struct toshiba_acpi_dev *dev)
if (ACPI_FAILURE(status) || out[0] == HCI_FAILURE) {
pr_err("ACPI call to query Illumination support failed\n");
return 0;
- } else if (out[0] == HCI_NOT_SUPPORTED || out[1] != 1) {
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
OK, but by eliminating the check, supposedly certain models which do not support
illumination but do not report it via out[0], but instead via out[1], will now
attempt to use illumination - correct?

The end result being user calls to an ACPI function which at best doesn't exist
and at worst.... does, but does something entirely different.

I admit the potential for a problem is slight, but is it possible to check
something explicit for support on the newer models rather than removing an
existing check?
--
Darren Hart
Intel Open Source Technology Center
Azael Avalos
2014-09-06 04:49:09 UTC
Permalink
Hi there
Post by Darren Hart
Post by Azael Avalos
Some Toshiba models with illumination support set a different
value on the returned codes, thus not allowing the illumination
LED to be registered, where it should be.
This patch removes a check from toshiba_illumination_available
function to allow such models to register the illumination LED.
---
drivers/platform/x86/toshiba_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index a149bc6..4803e7b 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -436,7 +436,7 @@ static int toshiba_illumination_available(struct toshiba_acpi_dev *dev)
if (ACPI_FAILURE(status) || out[0] == HCI_FAILURE) {
pr_err("ACPI call to query Illumination support failed\n");
return 0;
- } else if (out[0] == HCI_NOT_SUPPORTED || out[1] != 1) {
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
OK, but by eliminating the check, supposedly certain models which do not support
illumination but do not report it via out[0], but instead via out[1], will now
attempt to use illumination - correct?
Oh no, the main check is out[0], which either hold success if the
feature is supported
or an HCI/SCI error otherwise.
Post by Darren Hart
The end result being user calls to an ACPI function which at best doesn't exist
and at worst.... does, but does something entirely different.
I admit the potential for a problem is slight, but is it possible to check
something explicit for support on the newer models rather than removing an
existing check?
Our only resource right now is the DSDT and actual hardware to test,
as those calls
are not documented anywhere, and everytime the vendor decides to
change something,
we're on the loose end.

All the DSDTs that I previously had all set out[1] to one, so I was
using that as an
extra check to make sure we had illumination support, but now, recent models
set out[1] to zero, and those models, which do happen to have illumination
support (Qosmio X75 for example) were failing to register the LED.
Post by Darren Hart
--
Darren Hart
Intel Open Source Technology Center
--
-- El mundo apesta y vosotros apestais tambien --
Darren Hart
2014-09-09 00:09:08 UTC
Permalink
Post by Azael Avalos
Hi there
Post by Darren Hart
Post by Azael Avalos
Some Toshiba models with illumination support set a different
value on the returned codes, thus not allowing the illumination
LED to be registered, where it should be.
This patch removes a check from toshiba_illumination_available
function to allow such models to register the illumination LED.
---
drivers/platform/x86/toshiba_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index a149bc6..4803e7b 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -436,7 +436,7 @@ static int toshiba_illumination_available(struct toshiba_acpi_dev *dev)
if (ACPI_FAILURE(status) || out[0] == HCI_FAILURE) {
pr_err("ACPI call to query Illumination support failed\n");
return 0;
- } else if (out[0] == HCI_NOT_SUPPORTED || out[1] != 1) {
+ } else if (out[0] == HCI_NOT_SUPPORTED) {
OK, but by eliminating the check, supposedly certain models which do not support
illumination but do not report it via out[0], but instead via out[1], will now
attempt to use illumination - correct?
Oh no, the main check is out[0], which either hold success if the
feature is supported
or an HCI/SCI error otherwise.
Post by Darren Hart
The end result being user calls to an ACPI function which at best doesn't exist
and at worst.... does, but does something entirely different.
I admit the potential for a problem is slight, but is it possible to check
something explicit for support on the newer models rather than removing an
existing check?
Our only resource right now is the DSDT and actual hardware to test,
as those calls
are not documented anywhere, and everytime the vendor decides to
change something,
we're on the loose end.
All the DSDTs that I previously had all set out[1] to one, so I was
using that as an
extra check to make sure we had illumination support, but now, recent models
set out[1] to zero, and those models, which do happen to have illumination
support (Qosmio X75 for example) were failing to register the LED.
OK, I've been warned about taking non-obvious changes here. However, as you were
the original author here and are effectively telling me you want to revert the
out[1] check as it breaks hardware, I'm queueing this patch.

Thanks,
--
Darren Hart
Intel Open Source Technology Center
Loading...