[PATCH 0/5] toshiba_acpi: Various changes plus fixes

[PATCH 0/5] toshiba_acpi: Various changes plus fixes

[Azael Avalos]

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

[PATCH 1/5] toshiba_acpi: Additional hotkey scancodes

[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.

Signed-off-by: Azael Avalos <coproscefalo@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

Re: [PATCH 1/5] toshiba_acpi: Additional hotkey scancodes

[Darren Hart]

On Fri, Sep 05, 2014 at 11:14:03AM -0600, Azael Avalos wrote:
> 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 <coproscefalo@gmail.com>

Queued, thanks.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 2/5] toshiba_acpi: Fix illumination not available on certain models

[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.

Signed-off-by: Azael Avalos <coproscefalo@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

Re: [PATCH 2/5] toshiba_acpi: Fix illumination not available on certain models

[Darren Hart]

On Fri, Sep 05, 2014 at 11:14:04AM -0600, Azael Avalos wrote:
> 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 <coproscefalo@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) {

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

Re: [PATCH 2/5] toshiba_acpi: Fix illumination not available on certain models

[Azael Avalos]

Hi there

2014-09-05 20:35 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Fri, Sep 05, 2014 at 11:14:04AM -0600, Azael Avalos wrote:
>> 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 <coproscefalo@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) {
>
> 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.

>
> 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.

>
> --
> Darren Hart
> Intel Open Source Technology Center



-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 2/5] toshiba_acpi: Fix illumination not available on certain models

[Darren Hart]

On Fri, Sep 05, 2014 at 10:49:09PM -0600, Azael Avalos wrote:
> Hi there
> 
> 2014-09-05 20:35 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> > On Fri, Sep 05, 2014 at 11:14:04AM -0600, Azael Avalos wrote:
> >> 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 <coproscefalo@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) {
> >
> > 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.
> 
> >
> > 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

[PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[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.

Signed-off-by: Azael Avalos <coproscefalo@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

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Darren Hart]

On Fri, Sep 05, 2014 at 11:14:05AM -0600, Azael Avalos wrote:
> 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

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Azael Avalos]

Hi there,

2014-09-05 20:42 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Fri, Sep 05, 2014 at 11:14:05AM -0600, Azael Avalos wrote:
>> 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.

>
> --
> Darren Hart
> Intel Open Source Technology Center


Cheers
Azael


-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Darren Hart]

On Fri, Sep 05, 2014 at 11:04:18PM -0600, Azael Avalos wrote:
> Hi there,
> 
> 2014-09-05 20:42 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> > On Fri, Sep 05, 2014 at 11:14:05AM -0600, Azael Avalos wrote:
> >> 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?

> 
> 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.

> 
> 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

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Greg Kroah-Hartman]

On Mon, Sep 08, 2014 at 05:04:30PM -0700, Darren Hart wrote:
> On Fri, Sep 05, 2014 at 11:04:18PM -0600, Azael Avalos wrote:
> > Hi there,
> > 
> > 2014-09-05 20:42 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> > > On Fri, Sep 05, 2014 at 11:14:05AM -0600, Azael Avalos wrote:
> > >> 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?
> 
> > 
> > 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.
> 
> > 
> > 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

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Darren Hart]

On Mon, Sep 08, 2014 at 06:35:53PM -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 08, 2014 at 05:04:30PM -0700, Darren Hart wrote:
> > On Fri, Sep 05, 2014 at 11:04:18PM -0600, Azael Avalos wrote:
> > > Hi there,
> > > 
> > > 2014-09-05 20:42 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> > > > On Fri, Sep 05, 2014 at 11:14:05AM -0600, Azael Avalos wrote:
> > > >> 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?
> > 
> > > 
> > > 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.
> > 
> > > 
> > > 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

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Azael Avalos]

Hi there,

2014-09-09 21:35 GMT-06:00 Darren Hart <dvhart@infradead.org>:
>
> 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.

>
> --
> Darren Hart
> Intel Open Source Technology Center

Cheers
Azael


-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Greg Kroah-Hartman]

On Wed, Sep 10, 2014 at 09:28:44AM -0600, Azael Avalos wrote:
> Hi there,
> 
> 2014-09-09 21:35 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> >
> > 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.

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

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Jonathan Cameron]

On September 9, 2014 1:04:30 AM GMT+01:00, Darren Hart <dvhart@infradead.org> wrote:
>On Fri, Sep 05, 2014 at 11:04:18PM -0600, Azael Avalos wrote:
>> Hi there,
>> 
>> 2014-09-05 20:42 GMT-06:00 Darren Hart <dvhart@infradead.org>:
>> > On Fri, Sep 05, 2014 at 11:14:05AM -0600, Azael Avalos wrote:
>> >> 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?
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.

>
>> 
>> 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.
>
>> 
>> 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.

Re: [PATCH 3/5] toshiba_acpi: Add accelerometer input polled device

[Darren Hart]

On Wed, Sep 17, 2014 at 05:36:31PM +0100, Jonathan Cameron wrote:
> 
> 
> On September 9, 2014 1:04:30 AM GMT+01:00, Darren Hart <dvhart@infradead.org> wrote:
> >On Fri, Sep 05, 2014 at 11:04:18PM -0600, Azael Avalos wrote:
> >> Hi there,
> >> 
> >> 2014-09-05 20:42 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> >> > On Fri, Sep 05, 2014 at 11:14:05AM -0600, Azael Avalos wrote:
> >> >> 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?
> 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

[PATCH 4/5] toshiba_acpi: Support new keyboard backlight type

[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.

Signed-off-by: Azael Avalos <coproscefalo@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

Re: [PATCH 4/5] toshiba_acpi: Support new keyboard backlight type

[Darren Hart]

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.

> 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...


> 
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>


On testing, were you able to verify on new as well as previous models that this
continues to work?


> ---
>  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?

>  	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.

> +		dev->kbd_type = 2;
> +	else
> +		dev->kbd_type = 1;

A couple enum types would be useful here.

> +	dev->kbd_mode = out[2] & 0x1f;

define TOSHIBA_KBD_MODE_MASK maybe?

> +	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.


>  	/* 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)


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.


> +		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.

> +	/* 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?

> +
>  		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?

> +		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.

>  }
>  
>  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

Re: [PATCH 4/5] toshiba_acpi: Support new keyboard backlight type

[Azael Avalos]

Hi Darren,

2014-09-09 22:11 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> 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.
>
>> 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.

>
>
>>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>
>
> 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.

>
>
>> ---
>>  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...

>
>>       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)

>
>> +             dev->kbd_type = 2;
>> +     else
>> +             dev->kbd_type = 1;
>
> A couple enum types would be useful here.
>
>> +     dev->kbd_mode = out[2] & 0x1f;
>
> define TOSHIBA_KBD_MODE_MASK maybe?

Ok

>
>> +     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.

>
>
>>       /* 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)
>
>
> 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.
>
>
>> +             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

>
>> +     /* 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

>
>> +
>>               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 :-)

>
> 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.

>
> 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.

>
>> +             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.

>
> 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.

>
>>  }
>>
>>  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

>
> 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

Cheers
Azael


-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 4/5] toshiba_acpi: Support new keyboard backlight type

[Darren Hart]

On Wed, Sep 10, 2014 at 10:52:28AM -0600, Azael Avalos wrote:
> Hi Darren,
> 
> 2014-09-09 22:11 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> > 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.
> >
> >> 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.
> 
> >
> >
> >>
> >> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> >
> >
> > 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.
> 
> >
> >
> >> ---
> >>  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...
> 
> >
> >>       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)

Thanks. Please incorporate that into the comments, probably fairly early in the
code.

> 
> >
> >> +             dev->kbd_type = 2;
> >> +     else
> >> +             dev->kbd_type = 1;
> >
> > A couple enum types would be useful here.
> >
> >> +     dev->kbd_mode = out[2] & 0x1f;
> >
> > define TOSHIBA_KBD_MODE_MASK maybe?
> 
> Ok
> 
> >
> >> +     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.
> 
> >
> >
> >>       /* 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)
> >
> >
> > 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.

> >
> >> +             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;

> >
> >> +     /* 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.

> 
> >
> >> +
> >>               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 :-)
> 
> >
> > 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.
> 
> >
> > 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 :-)

> >
> >> +             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.
> 
> >
> > 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.

> >
> >>  }
> >>
> >>  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
> 
> >
> > 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

[PATCH 5/5] toshiba_acpi: Change touchpad store to check for invalid values

[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.

Signed-off-by: Azael Avalos <coproscefalo@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

Re: [PATCH 5/5] toshiba_acpi: Change touchpad store to check for invalid values

[Darren Hart]

On Fri, Sep 05, 2014 at 11:14:07AM -0600, Azael Avalos wrote:
> 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 <coproscefalo@gmail.com>

Queued, thanks.

-- 
Darren Hart
Intel Open Source Technology Center