Hi,Hi,
On 2/2/21 1:32 AM, Nitin Joshi wrote:
> The previous commit adding new sysfs for keyboard language has warning and
> few code correction has to be done as per new review comments.
>
> Below changes has been addressed in this version:
> - corrected warning. Many thanks to kernel test robot <lkp@xxxxxxxxx> for
> reporting and determining this warning.
> - used sysfs_emit_at() API instead of strcat.
> - sorted keyboard language array.
> - removed unwanted space and corrected sentences.
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Erm, this patch is the result of some extra reviewing after I merged
the original patch, but the changes themselves have not been reviewed,
so these are wrong.
So I've dropped the 2 Reviewed-by tags.
In the future please only add Reviewed-by tags if you are:
1. Posting a new (revised) version of an existing patch
2. Where people have responded to a previous version with their Reviewed-by.
3. The changes in the new revision are small. Large changes invalidate
previous reviews.
Thanks & Regards,
Hans
> Signed-off-by: Nitin Joshi <njoshi1@xxxxxxxxxx>
> ---
> .../admin-guide/laptops/thinkpad-acpi.rst | 15 ++++----
> drivers/platform/x86/thinkpad_acpi.c | 34 +++++++------------
> 2 files changed, 20 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index b1188f05a99a..3b225ae47f1a 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -1476,18 +1476,19 @@ sysfs: keyboard_lang
> 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
> -is set as "english". Hence using this sysfs, user can set correct keyboard
> -language to ECFW and then these key's will work correctly .
> +is other than "english". This is because the default keyboard language in ECFW
> +is set as "english". Hence using this sysfs, user can set the correct keyboard
> +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)
> +Text corresponding to keyboard layout to be set in sysfs are: be(Belgian),
> +cz(Czech), da(Danish), de(German), en(English), es(Spain), et(Estonian),
> +fr(French), fr-ch(French(Switzerland)), hu(Hungarian), it(Italy), jp (Japan),
> +nl(Dutch), nn(Norway), pl(Polish), pt(portugese), sl(Slovenian), sv(Sweden),
> +tr(Turkey)
>
>
> Adaptive keyboard
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 3cfc4a872c2d..a7f1b4ee5457 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9991,16 +9991,12 @@ struct keyboard_lang_data {
> int lang_code;
> };
>
> -/*
> - * When adding new entries to keyboard_lang_data, please check that
> - * the select_lang[] buffer in keyboard_lang_show() is still large enough.
> - */
> -struct keyboard_lang_data keyboard_lang_data[] = {
> - {"en", 0},
> +static const struct keyboard_lang_data keyboard_lang_data[] = {
> {"be", 0x080c},
> {"cz", 0x0405},
> {"da", 0x0406},
> {"de", 0x0c07},
> + {"en", 0x0000},
> {"es", 0x2c0a},
> {"et", 0x0425},
> {"fr", 0x040c},
> @@ -10056,9 +10052,7 @@ static ssize_t keyboard_lang_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - int output, err, i;
> - char select_lang[80] = "";
> - char lang[8] = "";
> + int output, err, i, len = 0;
>
> err = get_keyboard_lang(&output);
> if (err)
> @@ -10066,19 +10060,18 @@ static ssize_t keyboard_lang_show(struct device *dev,
>
> for (i = 0; i < ARRAY_SIZE(keyboard_lang_data); i++) {
> if (i)
> - strcat(select_lang, " ");
> + len += sysfs_emit_at(buf, len, "%s", " ");
>
> if (output == keyboard_lang_data[i].lang_code) {
> - strcat(lang, "[");
> - strcat(lang, keyboard_lang_data[i].lang_str);
> - strcat(lang, "]");
> - strcat(select_lang, lang);
> + len += sysfs_emit_at(buf, len, "%s%s%s", "[",
> + keyboard_lang_data[i].lang_str, "]");
This can be simplified to:
len += sysfs_emit_at(buf, len, "[%s]", keyboard_lang_data[i].lang_str);
I've applied the patch with this fixed up.
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> } else {
> - strcat(select_lang, keyboard_lang_data[i].lang_str);
> + len += sysfs_emit_at(buf, len, "%s", keyboard_lang_data[i].lang_str);
> }
> }
> + len += sysfs_emit_at(buf, len, "\n");
>
> - return sysfs_emit(buf, "%s\n", select_lang);
> + return len;
> }
>
> static ssize_t keyboard_lang_store(struct device *dev,
> @@ -10105,7 +10098,7 @@ static ssize_t keyboard_lang_store(struct device *dev,
> if (err)
> return err;
> } else {
> - pr_err("Unknown Keyboard language. Ignoring\n");
> + dev_err(&tpacpi_pdev->dev, "Unknown Keyboard language. Ignoring\n");
> return -EINVAL;
> }
>
> @@ -10116,7 +10109,6 @@ static ssize_t keyboard_lang_store(struct device *dev,
>
> return count;
> }
> -
> static DEVICE_ATTR_RW(keyboard_lang);
>
> static struct attribute *kbdlang_attributes[] = {
> @@ -10135,7 +10127,7 @@ static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm)
> err = get_keyboard_lang(&output);
> /*
> * If support isn't available (ENODEV) then don't return an error
> - * just don't create the sysfs group
> + * just don't create the sysfs group.
> */
> if (err == -ENODEV)
> return 0;
> @@ -10144,9 +10136,7 @@ static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm)
> return err;
>
> /* 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(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group);
> }
>
> static void kbdlang_exit(void)
>
