[PATCH] toshiba_acpi: Adapt kbd_bl_timeout

Discussion:

[PATCH] toshiba_acpi: Adapt kbd_bl_timeout_store to the new kbd type

Azael Avalos

2014-09-30 02:57:04 UTC

With the introduccion of the new keyboard backlight
implementation, the *_timeout_store function is
broken, as it only supports the first kbd_type.

This patch adapt such function for the new kbd_type,
as well as convert from using sscanf to kstrtoint.

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

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 5d509ea..13ee56b 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -1453,18 +1453,35 @@ 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;
+
+ ret = kstrtoint(buf, 0, &time);
+ if (ret)
+ return ret;

- if (sscanf(buf, "%i", &time) != 1 && (time < 0 || time > 60))
+ 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 a change if the actual timeout has changed */
+ if (toshiba->kbd_time != time) {
+ /* Shift the time to "base time" (0x3c0000 == 60 seconds)*/
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;
+ /* OR the "base time" to the actual method format */
+ if (toshiba->kbd_type == 1) {
+ /* Type 1 requires the oposite mode */
+ time |= SCI_KBD_MODE_FNZ;
+ } else if (toshiba->kbd_type == 2) {
+ /* Type 2 requires the actual mode */
+ time |= SCI_KBD_MODE_AUTO;
+ }
+
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+
toshiba->kbd_time = time >> HCI_MISC_SHIFT;
}

--
2.0.0

Darren Hart

2014-10-02 18:32:30 UTC

Permalink

Post by Azael Avalos
With the introduccion of the new keyboard backlight
implementation, the *_timeout_store function is
broken, as it only supports the first kbd_type.
This patch adapt such function for the new kbd_type,
as well as convert from using sscanf to kstrtoint.
---
drivers/platform/x86/toshiba_acpi.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 5d509ea..13ee56b 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -1453,18 +1453,35 @@ 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;
+
+ ret = kstrtoint(buf, 0, &time);
+ if (ret)
+ return ret;
- if (sscanf(buf, "%i", &time) != 1 && (time < 0 || time > 60))
+ if (time < 1 || time > 60)
return -EINVAL;

If I'm parsing this correctly, previously a time==0 was valid, and now it will
return -EINVAL. Is that intentional?

Post by Azael Avalos
- /* Set the Keyboard Backlight Timeout: 0-60 seconds */
- if (time != -1 && toshiba->kbd_time != time) {
+ /* Set the Keyboard Backlight Timeout: 1-60 seconds */

So the time range change appears intentional. Why is that?

Post by Azael Avalos
+
+ /* Only make a change if the actual timeout has changed */
+ if (toshiba->kbd_time != time) {
+ /* Shift the time to "base time" (0x3c0000 == 60 seconds)*/
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;
+ /* OR the "base time" to the actual method format */
+ if (toshiba->kbd_type == 1) {
+ /* Type 1 requires the oposite mode */

opposite

Is it "opposite" or "current"?

Post by Azael Avalos
+ time |= SCI_KBD_MODE_FNZ;
+ } else if (toshiba->kbd_type == 2) {
+ /* Type 2 requires the actual mode */

actual... as in the mode you are changing to or the mode you are changing from?
toshiba_acpi: Support new keyboard backlight type

There are several keyboard modes, why do we have only 2 of them here? Is it
because by setting the timeout we are always changing to _AUTO? Even if that's
the case, shouldn't one of these be OR'ing in the current mode - whatever it is,
instead of a fixed one?

Post by Azael Avalos
+ time |= SCI_KBD_MODE_AUTO;
+ }
+
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+

So here you are changing the sysfs API as you can now return -ENODEV in addition
to -EIO. We *can* do this, but it is a risk, and if a regression is reported, I
will be forced to revert this patch.

--
Darren Hart
Intel Open Source Technology Center

Azael Avalos

2014-10-02 20:02:10 UTC

Permalink

Hi there,

Post by Darren Hart

If I'm parsing this correctly, previously a time==0 was valid, and now it will
return -EINVAL. Is that intentional?

Yes, see below.

Post by Darren Hart

Post by Azael Avalos
- /* Set the Keyboard Backlight Timeout: 0-60 seconds */
- if (time != -1 && toshiba->kbd_time != time) {
+ /* Set the Keyboard Backlight Timeout: 1-60 seconds */

So the time range change appears intentional. Why is that?

The previous implementation (type 1) accepted values 0-60,
but the new one (type 2) just accepts 1-60, so I basically
just changed both to the new range.

Post by Darren Hart

opposite

Typo there, sorry.

Post by Darren Hart
Is it "opposite" or "current"?

Opposite (again, welcome to Toshiba's KBD BL implementation).

For type 1, to change modes you set (OR?) the value to
the current mode, if FNZ, you set it to AUTO, and viceversa.
Now, to change the time (in case we are in AUTO), you set
the timeout to the opposite mode, if AUTO, you set it to FNZ,
and viceversa (of course this will never happen as the sysfs
entry is hidden).

For type 2, to change modes and time you set the value to the desired
mode (ON, OFF or AUTO), again, the time can only be changed when
in AUTO mode (entry is hidden).

Perhaps something like this?

/* Type 1 requires FNZ mode, if set to AUTO, the time
* will change, but the mode will be changed as well
*/

Post by Darren Hart

Post by Azael Avalos
+ time |= SCI_KBD_MODE_FNZ;
+ } else if (toshiba->kbd_type == 2) {
+ /* Type 2 requires the actual mode */

actual... as in the mode you are changing to or the mode you are changing from?

We're not changing modes here, just time. Perhaps that comment
is misleading, I can probaly change it to:

/* Type 2 requires SCI_KBD_MODE_AUTO */

Or leave it blank, or perhaps be more verbose:

/* Type 2 requires SCI_KBD_MODE_AUTO, if set to another
* mode, the time will change but the mode will change as well
*/

Post by Darren Hart
toshiba_acpi: Support new keyboard backlight type
There are several keyboard modes, why do we have only 2 of them here?

Because the timeout entry only takes place (and appears)
whenever the keyboard mode is set to AUTO, on any other
mode is hidden.

Post by Darren Hart
Is it because by setting the timeout we are always changing to _AUTO?

Yes and no.
If the timeout entry exists, that means we are in AUTO, however,
the timeout can be changed even if we are on another mode (without
changing modes).

Post by Darren Hart
Even if that's
the case, shouldn't one of these be OR'ing in the current mode - whatever it is,
instead of a fixed one?

Yes, you can do that, and it will change the time successfuly,
but again, this entry only appears whenever in AUTO mode,
so we only have one mode to deal here with.

Post by Darren Hart

Post by Azael Avalos
+ time |= SCI_KBD_MODE_AUTO;
+ }
+
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+

So here you are changing the sysfs API as you can now return -ENODEV in addition
to -EIO. We *can* do this, but it is a risk, and if a regression is reported, I
will be forced to revert this patch.

I was just propagating the error code from toshiba_kbd_illum_status_set,
as it was already done on toshiba_kbd_bl_mode_store, but I can leave
this part intact if that's a concern, even tho' the kbd_mode file was recently
introduced.

Post by Darren Hart
--
Darren Hart
Intel Open Source Technology Center

Cheers
Azael

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

Darren Hart

2014-10-04 06:57:58 UTC

Permalink

Post by Azael Avalos
Hi there,

Post by Darren Hart

If I'm parsing this correctly, previously a time==0 was valid, and now it will
return -EINVAL. Is that intentional?

Yes, see below.

Post by Darren Hart

Post by Azael Avalos
- /* Set the Keyboard Backlight Timeout: 0-60 seconds */
- if (time != -1 && toshiba->kbd_time != time) {
+ /* Set the Keyboard Backlight Timeout: 1-60 seconds */

So the time range change appears intentional. Why is that?

The previous implementation (type 1) accepted values 0-60,
but the new one (type 2) just accepts 1-60, so I basically
just changed both to the new range.

I see how setting it to 0 is not particularly useful :-) The concern, again, is
with the userspace interface. If anyone complains, this will get reverted. Can
we not work around the 0 value for type 2?

Post by Azael Avalos

Post by Darren Hart

opposite

Typo there, sorry.

Post by Darren Hart
Is it "opposite" or "current"?

Opposite (again, welcome to Toshiba's KBD BL implementation).
For type 1, to change modes you set (OR?) the value to
the current mode, if FNZ, you set it to AUTO, and viceversa.
Now, to change the time (in case we are in AUTO), you set
the timeout to the opposite mode, if AUTO, you set it to FNZ,
and viceversa (of course this will never happen as the sysfs
entry is hidden).
For type 2, to change modes and time you set the value to the desired
mode (ON, OFF or AUTO), again, the time can only be changed when
in AUTO mode (entry is hidden).
Perhaps something like this?
/* Type 1 requires FNZ mode, if set to AUTO, the time
* will change, but the mode will be changed as well
*/

Post by Darren Hart

Post by Azael Avalos
+ time |= SCI_KBD_MODE_FNZ;
+ } else if (toshiba->kbd_type == 2) {
+ /* Type 2 requires the actual mode */

actual... as in the mode you are changing to or the mode you are changing from?

We're not changing modes here, just time. Perhaps that comment
/* Type 2 requires SCI_KBD_MODE_AUTO */>
/* Type 2 requires SCI_KBD_MODE_AUTO, if set to another
* mode, the time will change but the mode will change as well
*/

Just leave it blank for both. As you say, toshiba interface specific stuff. Not
much to say other than "this is required" which is about as helpful as reading
the assignment :-)

Post by Azael Avalos

Post by Darren Hart
toshiba_acpi: Support new keyboard backlight type
There are several keyboard modes, why do we have only 2 of them here?

Because the timeout entry only takes place (and appears)
whenever the keyboard mode is set to AUTO, on any other
mode is hidden.

Got it.

Post by Azael Avalos

Post by Darren Hart
Is it because by setting the timeout we are always changing to _AUTO?

Yes and no.
If the timeout entry exists, that means we are in AUTO, however,
the timeout can be changed even if we are on another mode (without
changing modes).

Post by Darren Hart
Even if that's
the case, shouldn't one of these be OR'ing in the current mode - whatever it is,
instead of a fixed one?

Yes, you can do that, and it will change the time successfuly,
but again, this entry only appears whenever in AUTO mode,
so we only have one mode to deal here with.

nevermind, got it.

Post by Azael Avalos

Post by Darren Hart

Post by Azael Avalos
+ time |= SCI_KBD_MODE_AUTO;
+ }
+
+ ret = toshiba_kbd_illum_status_set(toshiba, time);
+ if (ret)
+ return ret;
+

So here you are changing the sysfs API as you can now return -ENODEV in addition
to -EIO. We *can* do this, but it is a risk, and if a regression is reported, I
will be forced to revert this patch.

Nope, you're OK then I think. Please consider the input value change and update
the comments and resubmit.

Thanks,

--
Darren Hart
Intel Open Source Technology Center