Re: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set keyboard language

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 1/26/21 4:58 PM, Nitin Joshi1 wrote:
> Hello Andy ,
> 
>> -----Original Message-----
>> From: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Sent: Tuesday, January 26, 2021 6:31 AM
>> To: Nitin Joshi <nitjoshi@gmail.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>; Benjamin Berg
>> <bberg@redhat.com>; peter.hutterer@redhat.com; Tomoki Maruichi
>> <maruichit@lenovo.com>; Mark Pearson <mpearson@lenovo.com>; Platform
>> Driver <platform-driver-x86@vger.kernel.org>; Nitin Joshi1
>> <njoshi1@lenovo.com>
>> Subject: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set keyboard
>> language
>>
>> On Mon, Jan 25, 2021 at 5:03 AM Nitin Joshi <nitjoshi@gmail.com> wrote:
>>>
>>> From: Nitin Joshi <njoshi1@lenovo.com>
>>>
>>
>> Maybe it's a bit late, but... nevertheless my comments below.
>>
> Thank you for comments . It was my mistake , I should have put mailing list before . 
> 
>>> This patch is to create sysfs entry for setting keyboard language
>>
>> create a sysfs
>>
>>> using ASL method. Some thinkpads models like T580 , T590 , T15 Gen 1
>>> etc. has "=", "(',")" numeric keys, which are not displaying
>>> correctly, when keyboard language is other than "english".
>>> This patch fixes this issue by setting keyboard language to ECFW.
>>
>>> +This feature is used to set keyboard language to ECFW using ASL interface.
>>> +Fewer thinkpads models like T580 , T590 , T15 Gen 1 etc.. has "=",
>>> +"(', ")" numeric keys, which are not displaying correctly, when
>>> +keyboard language is other than "english". This is because of default
>>> +keyboard language in ECFW
>>
>> because the default
> 
> Ack . I will correct it.
> 
>>
>>> +is set as "english". Hence using this sysfs, user can set correct
>>> +keyboard
>>
>> the user can set the correct
> 
> Ack . I will correct it .
> 
>>
>>> +language to ECFW and then these key's will work correctly .
>>> +
>>> +Example of command to set keyboard language is mentioned below::
>>> +
>>> +        echo jp > /sys/devices/platform/thinkpad_acpi/keyboard_lang
>>> +
>>> +Text corresponding to keyboard layout to be set in sysfs are : jp
>>> +(Japan), be(Belgian), cz(Czech), en(English), da(Danish), de(German),
>>> +es(Spain) , et(Estonian),
>>> +fr(French) , fr-ch (French(Switzerland)), pl(Polish), sl(Slovenian),
>>> +hu (Hungarian), nl(Dutch), tr(Turkey), it(Italy), sv(Sweden),
>>> +pt(portugese)
>>
>> Can we keep this sorted?
> Ack . I will make this sorted.
> 
>> Also see below.
>>
>> ...
>>
>>> +struct keyboard_lang_data keyboard_lang_data[] = {
>>> +       {"en", 0},
>>
>> 0x0000 ?
> Ack . yes , I will update it.

While at it can you also please mark the table static const ?

> 
>>
>>> +       {"be", 0x080c},
>>> +       {"cz", 0x0405},
>>> +       {"da", 0x0406},
>>> +       {"de", 0x0c07},
>>> +       {"es", 0x2c0a},
>>> +       {"et", 0x0425},
>>> +       {"fr", 0x040c},
>>> +       {"fr-ch", 0x100c},
>>> +       {"hu", 0x040e},
>>> +       {"it", 0x0410},
>>> +       {"jp", 0x0411},
>>> +       {"nl", 0x0413},
>>> +       {"nn", 0x0414},
>>> +       {"pl", 0x0415},
>>> +       {"pt", 0x0816},
>>> +       {"sl", 0x041b},
>>> +       {"sv", 0x081d},
>>> +       {"tr", 0x041f},
>>> +};
>>
>> So, the above definitely has a meaning of the second byte as bit field. I believe
>> that be is something like be-be and so on.
> 
> Language Id is based on Language and Location . 
> You can refer attached pdf file for example 0x0411 is for Japanese layout but 0x04 is not basic 
> Currently , Lenovo ECFW  can change keyboard layout for only few language id by "SSKL" ASL method to fix 
> "=", "(', ")" numeric keys issue . hence , I have added only few language id (Actually I have received language id supported list from ECFW team).
> 
>> We have 0x04,0x08, 0x0c, 0x2c, and 0x10. 0x04 seems like a basic variant,
> By seeing the attached pdf , it seems not a basic variant.
> 
>> what about the rest?
> As per my understanding , ECFW is either not supporting rest or keyboard layout of others is similar to "English".
> If in future , ECFW supports any other keyboard layout then we can add it .
> 
>>
>> ...
>>
>>> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSKL",
>> &gskl_handle))) {
>>> +               /* Platform doesn't support GSKL */
>>> +               return -ENODEV;
>>
>> Perhaps EOPNOTSUPP is better?
> Ack . 
> How about keeping ENODEV  and changing only text as below :
> /* Platform doesn't have GSKL method */
> As per my understanding , in this case we will get ACPI failue only when GSKL method doesn’t exists in BIOS .
> So , I think its better to keep ENODEV.
> 
> Please correct me , if I am wrong.

I think we should keep -ENODEV here, since this path is hit on devices
which do not have the kbd-lang feature and returning -ENODEV from the
getter in this case is how this is handled in most other getters in
thinkpad_acpi.

>>
>>> +       }
>>
>> ...
>>
>>> +       char select_lang[80] = "";
>>> +       char lang[8] = "";
>>
>> I don't think we need assignments and moreover, we need these variables.
>> See below for the details.
>>
> Ack . 
> 
>>> +
>>> +       err = get_keyboard_lang(&output);
>>> +       if (err)
>>> +               return err;
>>> +
>>> +       for (i = 0; i < ARRAY_SIZE(keyboard_lang_data); i++) {
>>> +               if (i)
>>> +                       strcat(select_lang, " ");
>>> +
>>> +               if (output == keyboard_lang_data[i].lang_code) {
>>> +                       strcat(lang, "[");
>>> +                       strcat(lang, keyboard_lang_data[i].lang_str);
>>> +                       strcat(lang, "]");
>>> +                       strcat(select_lang, lang);
>>> +               } else {
>>> +                       strcat(select_lang, keyboard_lang_data[i].lang_str);
>>> +               }
>>> +       }
>>> +
>>> +       return sysfs_emit(buf, "%s\n", select_lang);
>>
>> We have sysfs_emit_at(), please use it instead of these ugly str*() calls.
> Ack . I will check about sysfs_emit_at() and get back on this .
> Thanks for suggestion.  
> 
>>
>>> +}
>>> +
>>> +static ssize_t keyboard_lang_store(struct device *dev,
>>> +                               struct device_attribute *attr,
>>> +                               const char *buf, size_t count) {
>>> +       int err, i;
>>
>>> +       bool lang_found = false;
>>
>> Redundant variable.
> 
> Ack . I think if we use sysfs_match_string()  then it will be redundant .
> But , I need to check it .

sysfs_match_string() only works on arrays of strings, in this case we have an array
of struct keyboard_lang_data, so using it here will not work.

>   
>>
>>> +       int lang_code = 0;
>>> +
>>> +       for (i = 0; i < ARRAY_SIZE(keyboard_lang_data); i++) {
>>> +               if (sysfs_streq(buf, keyboard_lang_data[i].lang_str)) {
>>> +                       lang_code = keyboard_lang_data[i].lang_code;
>>> +                       lang_found = true;
>>> +                       break;
>>> +               }
>>> +       }
>>
>> Don't we have sysfs_match_string() ?
> Ack . I will check it and get back .
> 
>>
>>> +       if (lang_found) {
>>
>> Use traditional pattern, like
>>
>> ret = sysfs_match_string(...);
>> if (ret < 0)
>>  return ret;
>>
> Noted 
> 
>>> +               lang_code = lang_code | 1 << 24;
>>> +
>>> +               /* Set language code */
>>> +               err = set_keyboard_lang_command(lang_code);
>>> +               if (err)
>>> +                       return err;
>>> +       } else {
>>
>>> +               pr_err("Unknown Keyboard language. Ignoring\n");
>>
>> Why not dev_err() ?
> 
> Ack . I will change it . The reason for using dev_err will only to provide better message to userspace . is it correct ?
>>
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       tpacpi_disclose_usertask(attr->attr.name,
>>> +                       "keyboard language is set to  %s\n", buf);
>>> +
>>> +       sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "keyboard_lang");
>>> +
>>> +       return count;
>>> +}
>>
>>> +
>>
>> Redundant blank line.
>>
> Ack. I will correct it.
> 
>>> +static DEVICE_ATTR_RW(keyboard_lang);
>>
>> ...
>>
>>> +       /*
>>> +        * If support isn't available (ENODEV) then don't return an error
>>> +        * just don't create the sysfs group
>>
>> Missed period.
>>
> Ack . I will correct it.
> 
>>> +        */
>>
>> ...
>>
>>> +       /* Platform supports this feature - create the sysfs file */
>>> +       err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
>>> + &kbdlang_attr_group);
>>> +
>>> +       return err;
>>
>> return sysfs_create_group();
>>
> 
> Ack . I will correct it .
> 
>>> +}
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
> 
> I will update patch on top of my last patch and send it for review  by next week .
> Please correct me , if I am missing something as I have just started pushing patch for upstream. 

Sounds good, thank you.

Regards,

Hans