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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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.

> 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

> +is set as "english". Hence using this sysfs, user can set correct keyboard

the user can set the correct

> +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?
Also see below.

...

> +struct keyboard_lang_data keyboard_lang_data[] = {
> +       {"en", 0},

0x0000 ?

> +       {"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.
We have 0x04,0x08, 0x0c, 0x2c, and 0x10. 0x04 seems like a basic
variant, what about the rest?

...

> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSKL", &gskl_handle))) {
> +               /* Platform doesn't support GSKL */
> +               return -ENODEV;

Perhaps EOPNOTSUPP is better?

> +       }

...

> +       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.

> +
> +       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.

> +}
> +
> +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.

> +       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() ?

> +       if (lang_found) {

Use traditional pattern, like

ret = sysfs_match_string(...);
if (ret < 0)
  return ret;

> +               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() ?

> +               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.

> +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.

> +        */

...

> +       /* 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();

> +}

-- 
With Best Regards,
Andy Shevchenko