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@xxxxxxxxxxxxx>:
> 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@xxxxxxxxx>
>
>
> 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 --
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/