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