Hello Hans ,
>-----Original Message-----
>From: Hans de Goede <hdegoede@xxxxxxxxxx>
>Subject: Re: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set
>keyboard language
>
>Hi,
>
>On 1/26/21 4:58 PM, Nitin Joshi1 wrote:
>> Hello Andy ,
>>
>>> -----Original Message-----
>>> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>>> Sent: Tuesday, January 26, 2021 6:31 AM
>>> To: Nitin Joshi <nitjoshi@xxxxxxxxx>
>>> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; Benjamin Berg
>>> <bberg@xxxxxxxxxx>; peter.hutterer@xxxxxxxxxx; Tomoki Maruichi
>>> <maruichit@xxxxxxxxxx>; Mark Pearson <mpearson@xxxxxxxxxx>;
>Platform
>>> Driver <platform-driver-x86@xxxxxxxxxxxxxxx>; Nitin Joshi1
>>> <njoshi1@xxxxxxxxxx>
>>> Subject: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set
>>> keyboard language
>>>
>>> On Mon, Jan 25, 2021 at 5:03 AM Nitin Joshi <nitjoshi@xxxxxxxxx> wrote:
>>>>
>>>> From: Nitin Joshi <njoshi1@xxxxxxxxxx>
>>>>
>>>
>>> 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 ?
Yes , I will modify it .
>
>>
>>>
>>>> + {"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.
>
Noted .
>>>
>>>> + }
>>>
>>> ...
>>>
>>>> + 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.
Noted with thanks .
>
>>
>>>
>>>> + 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
Thanks & Regards,
Nitin Joshi
