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

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

[Nitin Joshi]

From: Nitin Joshi <njoshi1@lenovo.com>

This patch is to create sysfs entry for setting keyboard language
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.

Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
---
Changes in v2:
 - used sysfs_streq() API instead of strcmp
 - used ARRAY_SIZE() API instead of strlen
 - addressed typo
---
 .../admin-guide/laptops/thinkpad-acpi.rst     |  24 +++
 drivers/platform/x86/thinkpad_acpi.c          | 182 ++++++++++++++++++
 2 files changed, 206 insertions(+)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 5fe1ade88c17..b1188f05a99a 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -51,6 +51,7 @@ detailed description):
 	- UWB enable and disable
 	- LCD Shadow (PrivacyGuard) enable and disable
 	- Lap mode sensor
+	- Setting keyboard language
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1466,6 +1467,29 @@ Sysfs notes
 	rfkill controller switch "tpacpi_uwb_sw": refer to
 	Documentation/driver-api/rfkill.rst for details.
 
+
+Setting keyboard language
+-------------------
+
+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 .
+
+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)
+
+
 Adaptive keyboard
 -----------------
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index e03df2881dc6..3cfc4a872c2d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9982,6 +9982,183 @@ static struct ibm_struct proxsensor_driver_data = {
 	.exit = proxsensor_exit,
 };
 
+/*************************************************************************
+ * Keyboard language interface
+ */
+
+struct keyboard_lang_data {
+	const char *lang_str;
+	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},
+	{"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},
+};
+
+static int set_keyboard_lang_command(int command)
+{
+	acpi_handle sskl_handle;
+	int output;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "SSKL", &sskl_handle))) {
+		/* Platform doesn't support SSKL */
+		return -ENODEV;
+	}
+
+	if (!acpi_evalf(sskl_handle, &output, NULL, "dd", command))
+		return -EIO;
+
+	return 0;
+}
+
+static int get_keyboard_lang(int *output)
+{
+	acpi_handle gskl_handle;
+	int kbd_lang;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSKL", &gskl_handle))) {
+		/* Platform doesn't support GSKL */
+		return -ENODEV;
+	}
+
+	if (!acpi_evalf(gskl_handle, &kbd_lang, NULL, "dd", 0x02000000))
+		return -EIO;
+
+	*output = kbd_lang;
+
+	return 0;
+}
+
+/* sysfs keyboard language entry */
+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] = "";
+
+	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);
+}
+
+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;
+	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;
+		}
+	}
+
+	if (lang_found) {
+		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");
+		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;
+}
+
+static DEVICE_ATTR_RW(keyboard_lang);
+
+static struct attribute *kbdlang_attributes[] = {
+	&dev_attr_keyboard_lang.attr,
+	NULL
+};
+
+static const struct attribute_group kbdlang_attr_group = {
+	.attrs = kbdlang_attributes,
+};
+
+static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm)
+{
+	int err, output;
+
+	err = get_keyboard_lang(&output);
+	/*
+	 * If support isn't available (ENODEV) then don't return an error
+	 * just don't create the sysfs group
+	 */
+	if (err == -ENODEV)
+		return 0;
+
+	if (err)
+		return err;
+
+	/* Platform supports this feature - create the sysfs file */
+	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group);
+
+	return err;
+}
+
+static void kbdlang_exit(void)
+{
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group);
+}
+
+static struct ibm_struct kbdlang_driver_data = {
+	.name = "kbdlang",
+	.exit = kbdlang_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -10474,6 +10651,11 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_proxsensor_init,
 		.data = &proxsensor_driver_data,
 	},
+	{
+		.init = tpacpi_kbdlang_init,
+		.data = &kbdlang_driver_data,
+	},
+
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.25.1

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

[Hans de Goede]

Hi,

On 1/25/21 3:59 AM, Nitin Joshi wrote:
> From: Nitin Joshi <njoshi1@lenovo.com>
> 
> This patch is to create sysfs entry for setting keyboard language
> 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.
> 
> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
> ---
> Changes in v2:
>  - used sysfs_streq() API instead of strcmp
>  - used ARRAY_SIZE() API instead of strlen
>  - addressed typo

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

Nitn, I did not notice one small issue while testing this on a X1C8
I will send out a follow-up patch addressing that. If you can
test the follow-up patch on one of the affected models
(T580 , T590 , T15 Gen 1, etc.) that would be great.

Note this patch 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


> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     |  24 +++
>  drivers/platform/x86/thinkpad_acpi.c          | 182 ++++++++++++++++++
>  2 files changed, 206 insertions(+)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 5fe1ade88c17..b1188f05a99a 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -51,6 +51,7 @@ detailed description):
>  	- UWB enable and disable
>  	- LCD Shadow (PrivacyGuard) enable and disable
>  	- Lap mode sensor
> +	- Setting keyboard language
>  
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1466,6 +1467,29 @@ Sysfs notes
>  	rfkill controller switch "tpacpi_uwb_sw": refer to
>  	Documentation/driver-api/rfkill.rst for details.
>  
> +
> +Setting keyboard language
> +-------------------
> +
> +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 .
> +
> +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)
> +
> +
>  Adaptive keyboard
>  -----------------
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e03df2881dc6..3cfc4a872c2d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9982,6 +9982,183 @@ static struct ibm_struct proxsensor_driver_data = {
>  	.exit = proxsensor_exit,
>  };
>  
> +/*************************************************************************
> + * Keyboard language interface
> + */
> +
> +struct keyboard_lang_data {
> +	const char *lang_str;
> +	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},
> +	{"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},
> +};
> +
> +static int set_keyboard_lang_command(int command)
> +{
> +	acpi_handle sskl_handle;
> +	int output;
> +
> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "SSKL", &sskl_handle))) {
> +		/* Platform doesn't support SSKL */
> +		return -ENODEV;
> +	}
> +
> +	if (!acpi_evalf(sskl_handle, &output, NULL, "dd", command))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int get_keyboard_lang(int *output)
> +{
> +	acpi_handle gskl_handle;
> +	int kbd_lang;
> +
> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSKL", &gskl_handle))) {
> +		/* Platform doesn't support GSKL */
> +		return -ENODEV;
> +	}
> +
> +	if (!acpi_evalf(gskl_handle, &kbd_lang, NULL, "dd", 0x02000000))
> +		return -EIO;
> +
> +	*output = kbd_lang;
> +
> +	return 0;
> +}
> +
> +/* sysfs keyboard language entry */
> +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] = "";
> +
> +	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);
> +}
> +
> +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;
> +	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;
> +		}
> +	}
> +
> +	if (lang_found) {
> +		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");
> +		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;
> +}
> +
> +static DEVICE_ATTR_RW(keyboard_lang);
> +
> +static struct attribute *kbdlang_attributes[] = {
> +	&dev_attr_keyboard_lang.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group kbdlang_attr_group = {
> +	.attrs = kbdlang_attributes,
> +};
> +
> +static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm)
> +{
> +	int err, output;
> +
> +	err = get_keyboard_lang(&output);
> +	/*
> +	 * If support isn't available (ENODEV) then don't return an error
> +	 * just don't create the sysfs group
> +	 */
> +	if (err == -ENODEV)
> +		return 0;
> +
> +	if (err)
> +		return err;
> +
> +	/* Platform supports this feature - create the sysfs file */
> +	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group);
> +
> +	return err;
> +}
> +
> +static void kbdlang_exit(void)
> +{
> +	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group);
> +}
> +
> +static struct ibm_struct kbdlang_driver_data = {
> +	.name = "kbdlang",
> +	.exit = kbdlang_exit,
> +};
> +
>  /****************************************************************************
>   ****************************************************************************
>   *
> @@ -10474,6 +10651,11 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = tpacpi_proxsensor_init,
>  		.data = &proxsensor_driver_data,
>  	},
> +	{
> +		.init = tpacpi_kbdlang_init,
> +		.data = &kbdlang_driver_data,
> +	},
> +
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> 

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

[Nitin Joshi1]

Hello Hans ,

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Sent: Tuesday, January 26, 2021 5:34 AM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: bberg@redhat.com; peter.hutterer@redhat.com; Tomoki Maruichi
><maruichit@lenovo.com>; Mark Pearson <mpearson@lenovo.com>;
>platform-driver-x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>
>Subject: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set keyboard
>language
>
>Hi,
>
>On 1/25/21 3:59 AM, Nitin Joshi wrote:
>> From: Nitin Joshi <njoshi1@lenovo.com>
>>
>> This patch is to create sysfs entry for setting keyboard language
>> 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.
>>
>> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
>> ---
>> Changes in v2:
>>  - used sysfs_streq() API instead of strcmp
>>  - used ARRAY_SIZE() API instead of strlen
>>  - addressed typo
>
>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
>
>Nitn, I did not notice one small issue while testing this on a X1C8 I will send out
>a follow-up patch addressing that. If you can test the follow-up patch on one
>of the affected models
>(T580 , T590 , T15 Gen 1, etc.) that would be great.

Yes , sure . I will test it  . Thank you 
 
>
>Note this patch 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 

>
>> ---
>>  .../admin-guide/laptops/thinkpad-acpi.rst     |  24 +++
>>  drivers/platform/x86/thinkpad_acpi.c          | 182 ++++++++++++++++++
>>  2 files changed, 206 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 5fe1ade88c17..b1188f05a99a 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -51,6 +51,7 @@ detailed description):
>>  	- UWB enable and disable
>>  	- LCD Shadow (PrivacyGuard) enable and disable
>>  	- Lap mode sensor
>> +	- Setting keyboard language
>>
>>  A compatibility table by model and feature is maintained on the web
>> site, http://ibm-acpi.sf.net/. I appreciate any success or failure @@
>> -1466,6 +1467,29 @@ Sysfs notes
>>  	rfkill controller switch "tpacpi_uwb_sw": refer to
>>  	Documentation/driver-api/rfkill.rst for details.
>>
>> +
>> +Setting keyboard language
>> +-------------------
>> +
>> +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 .
>> +
>> +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)
>> +
>> +
>>  Adaptive keyboard
>>  -----------------
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index e03df2881dc6..3cfc4a872c2d 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -9982,6 +9982,183 @@ static struct ibm_struct proxsensor_driver_data
>= {
>>  	.exit = proxsensor_exit,
>>  };
>>
>>
>+/***************************************************************
>*****
>> +*****
>> + * Keyboard language interface
>> + */
>> +
>> +struct keyboard_lang_data {
>> +	const char *lang_str;
>> +	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},
>> +	{"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},
>> +};
>> +
>> +static int set_keyboard_lang_command(int command) {
>> +	acpi_handle sskl_handle;
>> +	int output;
>> +
>> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "SSKL",
>&sskl_handle))) {
>> +		/* Platform doesn't support SSKL */
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!acpi_evalf(sskl_handle, &output, NULL, "dd", command))
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_keyboard_lang(int *output) {
>> +	acpi_handle gskl_handle;
>> +	int kbd_lang;
>> +
>> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSKL",
>&gskl_handle))) {
>> +		/* Platform doesn't support GSKL */
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!acpi_evalf(gskl_handle, &kbd_lang, NULL, "dd", 0x02000000))
>> +		return -EIO;
>> +
>> +	*output = kbd_lang;
>> +
>> +	return 0;
>> +}
>> +
>> +/* sysfs keyboard language entry */
>> +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] = "";
>> +
>> +	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); }
>> +
>> +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;
>> +	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;
>> +		}
>> +	}
>> +
>> +	if (lang_found) {
>> +		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");
>> +		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;
>> +}
>> +
>> +static DEVICE_ATTR_RW(keyboard_lang);
>> +
>> +static struct attribute *kbdlang_attributes[] = {
>> +	&dev_attr_keyboard_lang.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group kbdlang_attr_group = {
>> +	.attrs = kbdlang_attributes,
>> +};
>> +
>> +static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm) {
>> +	int err, output;
>> +
>> +	err = get_keyboard_lang(&output);
>> +	/*
>> +	 * If support isn't available (ENODEV) then don't return an error
>> +	 * just don't create the sysfs group
>> +	 */
>> +	if (err == -ENODEV)
>> +		return 0;
>> +
>> +	if (err)
>> +		return err;
>> +
>> +	/* Platform supports this feature - create the sysfs file */
>> +	err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
>> +&kbdlang_attr_group);
>> +
>> +	return err;
>> +}
>> +
>> +static void kbdlang_exit(void)
>> +{
>> +	sysfs_remove_group(&tpacpi_pdev->dev.kobj,
>&kbdlang_attr_group); }
>> +
>> +static struct ibm_struct kbdlang_driver_data = {
>> +	.name = "kbdlang",
>> +	.exit = kbdlang_exit,
>> +};
>> +
>>
>/****************************************************************
>************
>>
>*****************************************************************
>***********
>>   *
>> @@ -10474,6 +10651,11 @@ static struct ibm_init_struct ibms_init[]
>__initdata = {
>>  		.init = tpacpi_proxsensor_init,
>>  		.data = &proxsensor_driver_data,
>>  	},
>> +	{
>> +		.init = tpacpi_kbdlang_init,
>> +		.data = &kbdlang_driver_data,
>> +	},
>> +
>>  };
>>
>>  static int __init set_ibm_param(const char *val, const struct
>> kernel_param *kp)
>>

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

On Mon, Jan 25, 2021 at 11:30 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> 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.

Nitin, if you are not going to submit a series of fix ups as per my
comments I would probably do it myself later on. Just tell what you
want on this matter,

-- 
With Best Regards,
Andy Shevchenko

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

[Nitin Joshi1]

Hello Andy ,

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@gmail.com>
>Sent: Tuesday, January 26, 2021 11:22 PM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: Hans de Goede <hdegoede@redhat.com>; Benjamin Berg
><bberg@redhat.com>; peter.hutterer@redhat.com; Tomoki Maruichi
><maruichit@lenovo.com>; Mark Pearson <mpearson@lenovo.com>; Platform
>Driver <platform-driver-x86@vger.kernel.org>; Nitin Joshi1
><njoshi1@lenovo.com>
>Subject: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set keyboard
>language
>
>On Mon, Jan 25, 2021 at 11:30 PM Andy Shevchenko
><andy.shevchenko@gmail.com> wrote:
>>
>> 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.
>
>Nitin, if you are not going to submit a series of fix ups as per my comments I
>would probably do it myself later on. Just tell what you want on this matter,

Thank you for your comments and its helpful for me as I have just started pushing patch for upstream.
Apologize for not replying today morning as I wanted to check it in detail.

I have just replied on your review comment e-mail and will follow-up on that e-mail chain..

>
>--
>With Best Regards,
>Andy Shevchenko

Thanks & Regards,
Nitin Joshi 

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

[Hans de Goede]

Hi,

On 1/26/21 4:58 PM, Nitin Joshi1 wrote:
> Hello Andy ,
> 
>> -----Original Message-----
>> From: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Sent: Tuesday, January 26, 2021 6:31 AM
>> To: Nitin Joshi <nitjoshi@gmail.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>; Benjamin Berg
>> <bberg@redhat.com>; peter.hutterer@redhat.com; Tomoki Maruichi
>> <maruichit@lenovo.com>; Mark Pearson <mpearson@lenovo.com>; Platform
>> Driver <platform-driver-x86@vger.kernel.org>; Nitin Joshi1
>> <njoshi1@lenovo.com>
>> Subject: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set keyboard
>> language
>>
>> 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.
>>
> Thank you for comments . It was my mistake , I should have put mailing list before . 
> 
>>> 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
> 
> Ack . I will correct it.
> 
>>
>>> +is set as "english". Hence using this sysfs, user can set correct
>>> +keyboard
>>
>> the user can set the correct
> 
> Ack . I will correct it .
> 
>>
>>> +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?
> Ack . I will make this sorted.
> 
>> Also see below.
>>
>> ...
>>
>>> +struct keyboard_lang_data keyboard_lang_data[] = {
>>> +       {"en", 0},
>>
>> 0x0000 ?
> Ack . yes , I will update it.

While at it can you also please mark the table static const ?

> 
>>
>>> +       {"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.
> 
> Language Id is based on Language and Location . 
> You can refer attached pdf file for example 0x0411 is for Japanese layout but 0x04 is not basic 
> Currently , Lenovo ECFW  can change keyboard layout for only few language id by "SSKL" ASL method to fix 
> "=", "(', ")" numeric keys issue . hence , I have added only few language id (Actually I have received language id supported list from ECFW team).
> 
>> We have 0x04,0x08, 0x0c, 0x2c, and 0x10. 0x04 seems like a basic variant,
> By seeing the attached pdf , it seems not a basic variant.
> 
>> what about the rest?
> As per my understanding , ECFW is either not supporting rest or keyboard layout of others is similar to "English".
> If in future , ECFW supports any other keyboard layout then we can add it .
> 
>>
>> ...
>>
>>> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSKL",
>> &gskl_handle))) {
>>> +               /* Platform doesn't support GSKL */
>>> +               return -ENODEV;
>>
>> Perhaps EOPNOTSUPP is better?
> Ack . 
> How about keeping ENODEV  and changing only text as below :
> /* Platform doesn't have GSKL method */
> As per my understanding , in this case we will get ACPI failue only when GSKL method doesn’t exists in BIOS .
> So , I think its better to keep ENODEV.
> 
> Please correct me , if I am wrong.

I think we should keep -ENODEV here, since this path is hit on devices
which do not have the kbd-lang feature and returning -ENODEV from the
getter in this case is how this is handled in most other getters in
thinkpad_acpi.

>>
>>> +       }
>>
>> ...
>>
>>> +       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.
>>
> Ack . 
> 
>>> +
>>> +       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.
> Ack . I will check about sysfs_emit_at() and get back on this .
> Thanks for suggestion.  
> 
>>
>>> +}
>>> +
>>> +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.
> 
> Ack . I think if we use sysfs_match_string()  then it will be redundant .
> But , I need to check it .

sysfs_match_string() only works on arrays of strings, in this case we have an array
of struct keyboard_lang_data, so using it here will not work.

>   
>>
>>> +       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() ?
> Ack . I will check it and get back .
> 
>>
>>> +       if (lang_found) {
>>
>> Use traditional pattern, like
>>
>> ret = sysfs_match_string(...);
>> if (ret < 0)
>>  return ret;
>>
> Noted 
> 
>>> +               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() ?
> 
> Ack . I will change it . The reason for using dev_err will only to provide better message to userspace . is it correct ?
>>
>>> +               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.
>>
> Ack. I will correct it.
> 
>>> +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.
>>
> Ack . I will correct it.
> 
>>> +        */
>>
>> ...
>>
>>> +       /* 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();
>>
> 
> Ack . I will correct it .
> 
>>> +}
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
> 
> I will update patch on top of my last patch and send it for review  by next week .
> Please correct me , if I am missing something as I have just started pushing patch for upstream. 

Sounds good, thank you.

Regards,

Hans

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

[Nitin Joshi1]

Hello Hans ,

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Subject: Re: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set
>keyboard language
>
>Hi,
>
>On 1/26/21 4:58 PM, Nitin Joshi1 wrote:
>> Hello Andy ,
>>
>>> -----Original Message-----
>>> From: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Sent: Tuesday, January 26, 2021 6:31 AM
>>> To: Nitin Joshi <nitjoshi@gmail.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>; Benjamin Berg
>>> <bberg@redhat.com>; peter.hutterer@redhat.com; Tomoki Maruichi
>>> <maruichit@lenovo.com>; Mark Pearson <mpearson@lenovo.com>;
>Platform
>>> Driver <platform-driver-x86@vger.kernel.org>; Nitin Joshi1
>>> <njoshi1@lenovo.com>
>>> Subject: [External] Re: [PATCH] [v2] platform/x86: thinkpad_acpi: set
>>> keyboard language
>>>
>>> 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.
>>>
>> Thank you for comments . It was my mistake , I should have put mailing list
>before .
>>
>>>> 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
>>
>> Ack . I will correct it.
>>
>>>
>>>> +is set as "english". Hence using this sysfs, user can set correct
>>>> +keyboard
>>>
>>> the user can set the correct
>>
>> Ack . I will correct it .
>>
>>>
>>>> +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?
>> Ack . I will make this sorted.
>>
>>> Also see below.
>>>
>>> ...
>>>
>>>> +struct keyboard_lang_data keyboard_lang_data[] = {
>>>> +       {"en", 0},
>>>
>>> 0x0000 ?
>> Ack . yes , I will update it.
>
>While at it can you also please mark the table static const ?
Yes  , I will modify it .

>
>>
>>>
>>>> +       {"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.
>>
>> Language Id is based on Language and Location .
>> You can refer attached pdf file for example 0x0411 is for Japanese
>> layout but 0x04 is not basic Currently , Lenovo ECFW  can change
>> keyboard layout for only few language id by "SSKL" ASL method to fix "=", "(',
>")" numeric keys issue . hence , I have added only few language id (Actually I
>have received language id supported list from ECFW team).
>>
>>> We have 0x04,0x08, 0x0c, 0x2c, and 0x10. 0x04 seems like a basic
>>> variant,
>> By seeing the attached pdf , it seems not a basic variant.
>>
>>> what about the rest?
>> As per my understanding , ECFW is either not supporting rest or keyboard
>layout of others is similar to "English".
>> If in future , ECFW supports any other keyboard layout then we can add it .
>>
>>>
>>> ...
>>>
>>>> +       if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSKL",
>>> &gskl_handle))) {
>>>> +               /* Platform doesn't support GSKL */
>>>> +               return -ENODEV;
>>>
>>> Perhaps EOPNOTSUPP is better?
>> Ack .
>> How about keeping ENODEV  and changing only text as below :
>> /* Platform doesn't have GSKL method */ As per my understanding , in
>> this case we will get ACPI failue only when GSKL method doesn’t exists in
>BIOS .
>> So , I think its better to keep ENODEV.
>>
>> Please correct me , if I am wrong.
>
>I think we should keep -ENODEV here, since this path is hit on devices which
>do not have the kbd-lang feature and returning -ENODEV from the getter in
>this case is how this is handled in most other getters in thinkpad_acpi.
>
Noted .

>>>
>>>> +       }
>>>
>>> ...
>>>
>>>> +       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.
>>>
>> Ack .
>>
>>>> +
>>>> +       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.
>> Ack . I will check about sysfs_emit_at() and get back on this .
>> Thanks for suggestion.
>>
>>>
>>>> +}
>>>> +
>>>> +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.
>>
>> Ack . I think if we use sysfs_match_string()  then it will be redundant .
>> But , I need to check it .
>
>sysfs_match_string() only works on arrays of strings, in this case we have an
>array of struct keyboard_lang_data, so using it here will not work.

Noted with thanks .

>
>>
>>>
>>>> +       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() ?
>> Ack . I will check it and get back .
>>
>>>
>>>> +       if (lang_found) {
>>>
>>> Use traditional pattern, like
>>>
>>> ret = sysfs_match_string(...);
>>> if (ret < 0)
>>>  return ret;
>>>
>> Noted
>>
>>>> +               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() ?
>>
>> Ack . I will change it . The reason for using dev_err will only to provide better
>message to userspace . is it correct ?
>>>
>>>> +               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.
>>>
>> Ack. I will correct it.
>>
>>>> +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.
>>>
>> Ack . I will correct it.
>>
>>>> +        */
>>>
>>> ...
>>>
>>>> +       /* 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();
>>>
>>
>> Ack . I will correct it .
>>
>>>> +}
>>>
>>> --
>>> With Best Regards,
>>> Andy Shevchenko
>>
>> I will update patch on top of my last patch and send it for review  by next
>week .
>> Please correct me , if I am missing something as I have just started pushing
>patch for upstream.
>
>Sounds good, thank you.
>
>Regards,
>
>Hans

Thanks & Regards,
Nitin Joshi