[PATCH] platform/x86: thinkpad_acpi: fixed warning and incorporated review comments

[PATCH] platform/x86: thinkpad_acpi: fixed warning and incorporated review comments

[Nitin Joshi]

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@intel.com> 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@intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
---
 .../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, "]");
 		} 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)
-- 
2.25.1

Re: [PATCH] platform/x86: thinkpad_acpi: fixed warning and incorporated review comments

[Hans de Goede]

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@intel.com> 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@intel.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
> ---
>  .../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)
> 

Re: [PATCH] platform/x86: thinkpad_acpi: fixed warning and incorporated review comments

[Hans de Goede]

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@intel.com> 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@intel.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

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@lenovo.com>
> ---
>  .../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)
> 

RE: [External] Re: [PATCH] platform/x86: thinkpad_acpi: fixed warning and incorporated review comments

[Nitin Joshi1]

Hello Hans ,

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Sent: Tuesday, February 2, 2021 11:14 PM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: andy.shevchenko@gmail.com; Mark Pearson <mpearson@lenovo.com>;
>platform-driver-x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>;
>kernel test robot <lkp@intel.com>
>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@intel.com> 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@intel.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
>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@lenovo.com>
>> ---
>>  .../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)
>>