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