Hello Hans ,
>-----Original Message-----
>From: Hans de Goede <hdegoede@xxxxxxxxxx>
>Sent: Tuesday, February 2, 2021 11:14 PM
>To: Nitin Joshi <nitjoshi@xxxxxxxxx>
>Cc: andy.shevchenko@xxxxxxxxx; Mark Pearson <mpearson@xxxxxxxxxx>;
>platform-driver-x86@xxxxxxxxxxxxxxx; Nitin Joshi1 <njoshi1@xxxxxxxxxx>;
>kernel test robot <lkp@xxxxxxxxx>
>Subject: [External] Re: [PATCH] platform/x86: thinkpad_acpi: fixed warning
>and incorporated review comments
>
>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.
>
Thank you for correcting it and Apologize for any inconvenience caused.
I have noted comments.
>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);
Yes , Thank you for simplifying it.
>
>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
Thanks & Regards,
Nitin Joshi
>
>
>
>
>> } 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)
>>
