ibm-acpi-devel — ibm-acpi Linux driver development

Re: [ibm-acpi-devel] [External] Re: [PATCH 2/2] platorm/x86: thinkpad_acpi: sysfs interface to interface to get wwan antenna type

[Nitin J. <nj...@le...>]

Hi

>-----Original Message-----
>From: Barnabás Pőcze <po...@pr...>
>Sent: Sunday, February 14, 2021 5:43 AM
>To: Nitin Joshi <nit...@gm...>
>Cc: hde...@re...; ibm...@li...; platform-
>dri...@vg...; Nitin Joshi1 <nj...@le...>; Mark RH
>Pearson <mar...@le...>
>Subject: [External] Re: [PATCH 2/2] platorm/x86: thinkpad_acpi: sysfs
>interface to interface to get wwan antenna type
>
>Hi
>
>
>2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta:
>
>> [...]
>> +/* sysfs wwan antenna type entry */
>> +static ssize_t wwan_antenna_type_show(struct device *dev,
>> +				struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +	switch (wwan_antennatype) {
>> +	case 1:
>> +		return sysfs_emit(buf, "type a\n");
>> +	case 2:
>> +		return sysfs_emit(buf, "type b\n");
>> +	default:
>> +		return sysfs_emit(buf, "unknown type\n");
>
>I feel like returning -ENODATA would be more appropriate here. Or maybe
>you could choose not to create the attribute if the antenna type is unknown.
>And I'm not sure if the "type" prefix is necessary. I believe the name of the
>attribute 'wwan_antenna_type'
Ack . I will check it. 
Regarding prefix, it's not so necessary but let me keep "type" prefix.
   
>already implies that the content will describe a type. Furthermore, I think you
>could omit the `has_antennatype` variable altogether, storing only
>`wwan_antennatype` is enough.
Ack . I will check it
>
>
>> +	}
>> +}
>> +
>>  static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>>  				struct device_attribute *attr,
>>  				const char *buf, size_t count)
>> @@ -10076,24 +10114,29 @@ static ssize_t
>wlan_tx_strength_reduce_store(struct device *dev,
>>  	}
>>
>>  	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> "wlan_tx_strength_reduce");
>> +
>
>If you want the empty line here, I think you should place it in the previous
>patch.
Ack . I will remove it.
>
>
>>  	return count;
>>  }
>>  static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
>> +static DEVICE_ATTR_RO(wwan_antenna_type);
>>
>>  static int tpacpi_dprc_init(struct ibm_init_struct *iibm)  {
>> -	int wlantx_err, err;
>> +	int wlantx_err, wwanantenna_err, err;
>>
>>  	wlantx_err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> +	wwanantenna_err = get_wwan_antenna(&has_antennatype,
>&wwan_antennatype);
>>  	/*
>>  	 * If support isn't available (ENODEV) for both devices then quit, but
>>  	 * don't return an error.
>>  	 */
>> -	if (wlantx_err == -ENODEV)
>> +	if ((wlantx_err == -ENODEV) && (wwanantenna_err == -ENODEV))
>>  		return 0;
>>  	/* Otherwise, if there was an error return it */
>>  	if (wlantx_err && (wlantx_err != -ENODEV))
>>  		return wlantx_err;
>> +	if (wwanantenna_err && (wwanantenna_err != -ENODEV))
>> +		return wwanantenna_err;
>>
>>  	if (has_wlantxreduce) {
>>  		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> @@ -10101,6 +10144,12 @@ static int tpacpi_dprc_init(struct
>ibm_init_struct *iibm)
>>  		if (err)
>>  			return err;
>>  	}
>> +
>> +	if (has_antennatype) {
>> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>&dev_attr_wwan_antenna_type.attr);
>> +		if (err)
>> +			return err;
>> +	}
>>  	return 0;
>>  }
>> [...]
>
>
>Regards,
>Barnabás Pőcze

Thanks & regards,
Nitin Joshi 

Re: [ibm-acpi-devel] [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Nitin J. <nj...@le...>]

Hi,

>-----Original Message-----
>From: Barnabás Pőcze <po...@pr...>
>Sent: Sunday, February 14, 2021 5:48 AM
>To: Nitin Joshi1 <nj...@le...>
>Cc: Nitin Joshi <nit...@gm...>; hde...@re...; ibm-acpi-
>de...@li...; pla...@vg...; Mark RH
>Pearson <mar...@le...>
>Subject: RE: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs
>interface to reduce wlan tx power
>
>Hi
>
>
>2021. február 12., péntek 11:40 keltezéssel, Nitin Joshi1
><nj...@le...> írta:
>
>> [...]
>> >>
>>
>>+/**************************************************************
>*
>> >*****
>> >> +*****
>> >> + * DPRC(Dynamic Power Reduction Control) subdriver, for the Lenovo
>> >> +WWAN
>> >> + * and WLAN feature.
>> >> + */
>> >> +#define DPRC_GET_WLAN_STATE             0x20000
>> >> +#define DPRC_SET_WLAN_POWER_REDUCE      0x00030010
>> >> +#define DPRC_SET_WLAN_POWER_FULL        0x00030100
>> >> +static int has_wlantxreduce;
>> >
>> >I think `bool` would be better.
>>
>> Ack . I will modify  it in next version.
>>
>> >
>> >
>> >> +static int wlan_txreduce;
>> >> +
>> >> +static int dprc_command(int command, int *output) {
>> >> +	acpi_handle dprc_handle;
>> >> +
>> >> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DPRC",
>> >&dprc_handle))) {
>> >> +		/* Platform doesn't support DPRC */
>> >> +		return -ENODEV;
>> >> +	}
>> >> +
>> >> +	if (!acpi_evalf(dprc_handle, output, NULL, "dd", command))
>> >> +		return -EIO;
>> >> +
>> >> +	/*
>> >> +	 * METHOD_ERR gets returned on devices where few commands are
>> >not supported
>> >> +	 * for example WLAN power reduce command is not supported on
>> >some devices.
>> >> +	 */
>> >> +	if (*output & METHOD_ERR)
>> >> +		return -ENODEV;
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int get_wlan_state(int *has_wlantxreduce, int
>> >> +*wlan_txreduce) {
>> >> +	int output, err;
>> >> +
>> >> +	/* Get current WLAN Power Transmission state */
>> >> +	err = dprc_command(DPRC_GET_WLAN_STATE, &output);
>> >> +	if (err)
>> >> +		return err;
>> >> +
>> >> +	if (output & BIT(4))
>> >
>> >I believe it'd be preferable to name `BIT(4)` and `BIT(8)`. E.g.:
>> >
>> >  #define DPRC_GET_WLAN_STATE_RES_REDUCED BIT(4)
>> >  #define DPRC_GET_WLAN_STATE_RES_FULL    BIT(8)
>> >
>> >(or any name you like).
>> >
>> Ack . I will modify  it in next version.
>>
>> >
>> >> +		*wlan_txreduce = 1;
>> >> +	else if (output & BIT(8))
>> >> +		*wlan_txreduce = 0;
>> >> +	else
>> >> +		*wlan_txreduce = -1;
>> >
>> >Can you elaborate what -1 means here? Unknown/not
>> >available/invalid/error?
>>
>> -1 means "error" .
>> I had found that when "DPRC" method return METHOD_ERR i.e BIT(31) as 0
>then it goes to this condition.
>> So , therefore I had added METHOD_ERR check in dprc_command() and
>now , this doesnot goes here.
>> But, I have still kept it here , just in case if any other error occurred .
>> Can you please suggest , if I should remove it (i.e remove *wlan_txreduce = -
>1; )?  as I still think, there is no harm keeping like this.
>>
>
>If `dprc_command()` handles all error cases (i.e. it is not possible that
>`dprc_command()` returns 0, but there was an error) - which seems to be the
>case - then I think in that branch you should return -ENODATA and not touch
>`wlan_txreduce`. Because if that branch runs, then there was no error, but the
>state cannot be determined, so I think -ENODATA is an appropriate error code.
Ack . Noted, I will check it
>
>
>> >
>> >
>> >> +
>> >> +	*has_wlantxreduce = 1;
>> >
>> >Is it necessary for the getter to set this? Couldn't it be set in
>> >`tpacpi_dprc_init()` once during initialization?
>>
>> Actually, yes we can keep it in init function also but I have not kept
>> it because of other patch (PATCH 2/2) which I had sent . patch 1 (this
>> patch) and patch 2 ( other patch which create sysfs of WWWAN Antenna
>type) both uses "DPRC" method . So , we will need a flag to create sysfs
>because few system will not have this "wlan tx reduce"
>> even if it has "DPRC" method exists and vice versa .
>> So , in this case, we need to explicitly check whether it require to create
>corresponding sysfs  or not.
>>
>
>I was thinking of something like the following:
>
>  static int tpacpi_dprc_init(...) {
>    ...
>    int err = get_wlan_state(&reduced);
>    if (err && err != -ENODEV)
>      return err;
>    else if (!err)
>      has_wlantxreduce = true;
>
>    err = get_wwan_antenna(&antenna);
>    if (err && err != -ENODEV)
>      return err;
>    else if (!err)
>      has_antennatype = true; /* as I've commented on the second patch, this
>                                 variable is probably not needed */
>    ...
>
Ack . I will check it

>If I'm not mistaken this is enough not to need the second argument in either
>`get_wlan_state()` or `get_wwan_antenna()`. Note, that if both functions may
>return -ENODATA, you might also want to add `err != -ENODATA` to the
>conditions.
>
Ack . yes.

>
>> >
>> >
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +/* sysfs wlan entry */
>> >> +static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
>> >> +				struct device_attribute *attr,
>> >> +				char *buf)
>> >
>> >Please align the arguments:
>> >
>> >  ..._show(struct device *dev,
>> >           struct device_attribute ...
>> >           ...);
>> >
>> Ack . I will modify  it in next version.
>> Also , I will correct it in my other patch(PATCH 2/2) also.
>>
>> >
>> >> +{
>> >> +	int err;
>> >> +
>> >> +	err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> >> +	if (err)
>> >> +		return err;
>> >> +
>> >
>> >Wouldn't it be better to return -ENODATA or something similar when
>> >`wlan_txreduce` == -1?
>>
>> Ack . I think EOPNOTSUPP will be better ? reason is that "DPRC" method is
>available but there is error . So , its more likely that command is not
>supported.
>> However, I will modify it after I get feedback about my previous comment.
>>
>
>I think the place to determine whether the operation is supported or not is in
>`tpacpi_dprc_init()` and the attribute should not be created if it's not
>supported.
Ack . I will check it and send next patch version soon.
>
>
>> >
>> >
>> >> +	return sysfs_emit(buf, "%d\n", wlan_txreduce); }
>> >> +
>> >> +static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>> >> +				struct device_attribute *attr,
>> >> +				const char *buf, size_t count)
>> >
>> >Same here.
>> >
>> Ack . I will modify  it in next version.
>> >
>> >> +{
>> >> +	int output, err;
>> >> +	unsigned long t;
>> >> +
>> >> +	if (parse_strtoul(buf, 1, &t))
>> >
>> >Maybe `kstrtobool`?
>>
>> Thank you for your suggestion.
>> I can use 'kstrtobool' and will modify on my next version.
>>
>> >
>> >
>> >> +		return -EINVAL;
>> >> +
>> >> +	tpacpi_disclose_usertask(attr->attr.name,
>> >> +				"WLAN tx strength reduced %lu\n", t);
>> >> +
>> >> +	switch (t) {
>> >> +	case 0:
>> >> +		err = dprc_command(DPRC_SET_WLAN_POWER_FULL,
>> >&output);
>> >> +		break;
>> >> +	case 1:
>> >> +		err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE,
>> >&output);
>> >> +		break;
>> >> +	default:
>> >> +		err = -EINVAL;
>> >> +		dev_err(&tpacpi_pdev->dev, "Unknown operating mode.
>> >Ignoring\n");
>> >> +		break;
>> >> +	}
>> >> +
>> >
>> >If I'm not mistaken, `err` is never returned, so the write() will
>> >always seem to succeed.
>>
>> Yes , its correct . I will use 'kstrtobool' and will drop this : "err = -EINVAL;"
>> Is it Ok ?
>>
>
>If you use `kstrtobool()`, you can even drop the entire switch statement.
Ack . yes , I think so too.
>
>
>> >
>> >
>> >> +	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> >"wlan_tx_strength_reduce");
>> >> +	return count;
>> >> +}
>> >> +static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
>> >> +
>> >> +static int tpacpi_dprc_init(struct ibm_init_struct *iibm) {
>> >> +	int wlantx_err, err;
>> >> +
>> >> +	wlantx_err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> >> +	/*
>> >> +	 * If support isn't available (ENODEV) for both devices then quit, but
>> >> +	 * don't return an error.
>> >> +	 */
>> >> +	if (wlantx_err == -ENODEV)
>> >> +		return 0;
>> >> +	/* Otherwise, if there was an error return it */
>> >> +	if (wlantx_err && (wlantx_err != -ENODEV))
>> >> +		return wlantx_err;
>> >> +
>> >> +	if (has_wlantxreduce) {
>> >> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> >> +				&dev_attr_wlan_tx_strength_reduce.attr);
>> >
>> >I believe `device_create_file()` would be better.
>> >
>> Since, file will be created in /sys/ directory , hence I think its better to use
>sysfs_create_file.
>> Also, by checking in this  file, it looks like sysfs_create_file will be more
>reasonable to do .
>>
>> Please let me know, if its Ok to continue using sysfs_create_file or
>> you still feel. we need to use
>> device_create_file()  also feel free to correct me, if I am wrong.
>>
>
>There's not much difference, so sysfs_{create,remove}_file() would work just
>as fine here. The reason why I believe `device_{create,remove}_file()` is
>possibly preferable is that you don't need to reference the kobj and the
>attribute when calling it, and you're adding an attribute to a *device* (not any
>kobj), so device_*() functions are a better fit syntactically in my opinion. But in
>the end, both will achieve the same effect, so you are free to choose
>whichever you like.
Ack . Noted.
I would like to keep sysfs_{ create,remove}_file() now.

I will recheck code, incorporate comments and send next patch version soon.

>
>> >
>> >> +		if (err)
>> >> +			return err;
>> >> +	}
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static void dprc_exit(void)
>> >> +{
>> >> +	if (has_wlantxreduce)
>> >> +		sysfs_remove_file(&tpacpi_pdev->dev.kobj,
>> >> +&dev_attr_wlan_tx_strength_reduce.attr);
>> >
>> >And similarly, `device_remove_file()`.
>>
>> Same comment as above . I feel, sysfs_remove_file is more reasonable to do.
>> [...]
>
>
>Regards,
>Barnabás Pőcze

Thanks & Regards,
Nitin Joshi

Re: [ibm-acpi-devel] [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Nitin J. <nj...@le...>]

Hi,

Thank you for your comments.

>-----Original Message-----
>From: Barnabás Pőcze <po...@pr...>
>Sent: Friday, February 12, 2021 5:56 PM
>To: Nitin Joshi <nit...@gm...>
>Cc: hde...@re...; ibm...@li...; platform-
>dri...@vg...; Nitin Joshi1 <nj...@le...>; Mark RH
>Pearson <mar...@le...>
>Subject: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs
>interface to reduce wlan tx power
>
>Hi
>
>
>2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta:
>
>> Some newer Thinkpads have the WLAN antenna placed close to the WWAN
>> antenna. In these cases FCC certification requires that when WWAN is
>> active we reduce WLAN transmission power. A new Dynamic Power
>> Reduction Control (DPRC) method is available from the BIOS on these
>> platforms to reduce or restore WLAN Tx power.
>>
>> This patch provides a sysfs interface that userspace can use to adjust
>> the WLAN power appropriately.
>>
>> Reviewed-by: Mark Pearson <mar...@le...>
>> Signed-off-by: Nitin Joshi <nj...@le...>
>> ---
>>  .../admin-guide/laptops/thinkpad-acpi.rst     |  18 +++
>>  drivers/platform/x86/thinkpad_acpi.c          | 136 ++++++++++++++++++
>>  2 files changed, 154 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 5fe1ade88c17..10410811b990 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
>> +	- WLAN transmission power control
>>
>>  A compatibility table by model and feature is maintained on the web
>> site, http://ibm-acpi.sf.net/. I appreciate any success or failure @@
>> -1447,6 +1448,23 @@ they differ between desk and lap mode.
>>  The property is read-only. If the platform doesn't have support the
>> sysfs  class is not created.
>>
>> +WLAN transmission power control
>> +--------------------------------
>> +
>> +sysfs: wlan_tx_strength_reduce
>> +
>> +Some newer Thinkpads have the WLAN antenna placed close to the WWAN
>antenna.
>> +This interface will be used by userspace to reduce the WLAN Tx power
>> +strength when WWAN is active, as is required for FCC certification.
>> +
>> +The available commands are::
>> +
>> +        echo '0' >
>/sys/devices/platform/thinkpad_acpi/wlan_tx_strength_reduce
>> +        echo '1' >
>> + /sys/devices/platform/thinkpad_acpi/wlan_tx_strength_reduce
>> +
>> +The first command restores the wlan transmission power and the latter
>> +one reduces wlan transmission power.
>> +
>>  EXPERIMENTAL: UWB
>>  -----------------
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index f3e8eca8d86d..6dbbd4a14264 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -9983,6 +9983,138 @@ static struct ibm_struct proxsensor_driver_data
>= {
>>  	.exit = proxsensor_exit,
>>  };
>>
>>
>+/***************************************************************
>*****
>> +*****
>> + * DPRC(Dynamic Power Reduction Control) subdriver, for the Lenovo
>> +WWAN
>> + * and WLAN feature.
>> + */
>> +#define DPRC_GET_WLAN_STATE             0x20000
>> +#define DPRC_SET_WLAN_POWER_REDUCE      0x00030010
>> +#define DPRC_SET_WLAN_POWER_FULL        0x00030100
>> +static int has_wlantxreduce;
>
>I think `bool` would be better.

Ack . I will modify  it in next version.

>
>
>> +static int wlan_txreduce;
>> +
>> +static int dprc_command(int command, int *output) {
>> +	acpi_handle dprc_handle;
>> +
>> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DPRC",
>&dprc_handle))) {
>> +		/* Platform doesn't support DPRC */
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!acpi_evalf(dprc_handle, output, NULL, "dd", command))
>> +		return -EIO;
>> +
>> +	/*
>> +	 * METHOD_ERR gets returned on devices where few commands are
>not supported
>> +	 * for example WLAN power reduce command is not supported on
>some devices.
>> +	 */
>> +	if (*output & METHOD_ERR)
>> +		return -ENODEV;
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_wlan_state(int *has_wlantxreduce, int *wlan_txreduce)
>> +{
>> +	int output, err;
>> +
>> +	/* Get current WLAN Power Transmission state */
>> +	err = dprc_command(DPRC_GET_WLAN_STATE, &output);
>> +	if (err)
>> +		return err;
>> +
>> +	if (output & BIT(4))
>
>I believe it'd be preferable to name `BIT(4)` and `BIT(8)`. E.g.:
>
>  #define DPRC_GET_WLAN_STATE_RES_REDUCED BIT(4)
>  #define DPRC_GET_WLAN_STATE_RES_FULL    BIT(8)
>
>(or any name you like).
>
Ack . I will modify  it in next version.

>
>> +		*wlan_txreduce = 1;
>> +	else if (output & BIT(8))
>> +		*wlan_txreduce = 0;
>> +	else
>> +		*wlan_txreduce = -1;
>
>Can you elaborate what -1 means here? Unknown/not
>available/invalid/error?

-1 means "error" .
I had found that when "DPRC" method return METHOD_ERR i.e BIT(31) as 0 then it goes to this condition.
So , therefore I had added METHOD_ERR check in dprc_command() and now , this doesnot goes here.
But, I have still kept it here , just in case if any other error occurred . 
Can you please suggest , if I should remove it (i.e remove *wlan_txreduce = -1; )?  as I still think, there is no harm keeping like this.
 
>
>
>> +
>> +	*has_wlantxreduce = 1;
>
>Is it necessary for the getter to set this? Couldn't it be set in
>`tpacpi_dprc_init()` once during initialization?

Actually, yes we can keep it in init function also but I have not kept it because of other patch (PATCH 2/2)
which I had sent . patch 1 (this patch) and patch 2 ( other patch which create sysfs of WWWAN Antenna type)
both uses "DPRC" method . So , we will need a flag to create sysfs because few system will not have this "wlan tx reduce"
even if it has "DPRC" method exists and vice versa . 
So , in this case, we need to explicitly check whether it require to create corresponding sysfs  or not.

>
>
>> +	return 0;
>> +}
>> +
>> +/* sysfs wlan entry */
>> +static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
>> +				struct device_attribute *attr,
>> +				char *buf)
>
>Please align the arguments:
>
>  ..._show(struct device *dev,
>           struct device_attribute ...
>           ...);
>
Ack . I will modify  it in next version.
Also , I will correct it in my other patch(PATCH 2/2) also.

>
>> +{
>> +	int err;
>> +
>> +	err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> +	if (err)
>> +		return err;
>> +
>
>Wouldn't it be better to return -ENODATA or something similar when
>`wlan_txreduce` == -1?

Ack . I think EOPNOTSUPP will be better ? reason is that "DPRC" method is available but there is error . So , its more likely that command is not supported.
However, I will modify it after I get feedback about my previous comment.

>
>
>> +	return sysfs_emit(buf, "%d\n", wlan_txreduce); }
>> +
>> +static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>> +				struct device_attribute *attr,
>> +				const char *buf, size_t count)
>
>Same here.
>
Ack . I will modify  it in next version.
>
>> +{
>> +	int output, err;
>> +	unsigned long t;
>> +
>> +	if (parse_strtoul(buf, 1, &t))
>
>Maybe `kstrtobool`?

Thank you for your suggestion.
I can use 'kstrtobool' and will modify on my next version.

>
>
>> +		return -EINVAL;
>> +
>> +	tpacpi_disclose_usertask(attr->attr.name,
>> +				"WLAN tx strength reduced %lu\n", t);
>> +
>> +	switch (t) {
>> +	case 0:
>> +		err = dprc_command(DPRC_SET_WLAN_POWER_FULL,
>&output);
>> +		break;
>> +	case 1:
>> +		err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE,
>&output);
>> +		break;
>> +	default:
>> +		err = -EINVAL;
>> +		dev_err(&tpacpi_pdev->dev, "Unknown operating mode.
>Ignoring\n");
>> +		break;
>> +	}
>> +
>
>If I'm not mistaken, `err` is never returned, so the write() will always seem to
>succeed.

Yes , its correct . I will use 'kstrtobool' and will drop this : "err = -EINVAL;"
Is it Ok ?

>
>
>> +	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>"wlan_tx_strength_reduce");
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
>> +
>> +static int tpacpi_dprc_init(struct ibm_init_struct *iibm) {
>> +	int wlantx_err, err;
>> +
>> +	wlantx_err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> +	/*
>> +	 * If support isn't available (ENODEV) for both devices then quit, but
>> +	 * don't return an error.
>> +	 */
>> +	if (wlantx_err == -ENODEV)
>> +		return 0;
>> +	/* Otherwise, if there was an error return it */
>> +	if (wlantx_err && (wlantx_err != -ENODEV))
>> +		return wlantx_err;
>> +
>> +	if (has_wlantxreduce) {
>> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> +				&dev_attr_wlan_tx_strength_reduce.attr);
>
>I believe `device_create_file()` would be better.
>
Since, file will be created in /sys/ directory , hence I think its better to use sysfs_create_file.
Also, by checking in this  file, it looks like sysfs_create_file will be more reasonable to do .

Please let me know, if its Ok to continue using sysfs_create_file or you still feel. we need to use 
device_create_file()  also feel free to correct me, if I am wrong.
 
>
>> +		if (err)
>> +			return err;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void dprc_exit(void)
>> +{
>> +	if (has_wlantxreduce)
>> +		sysfs_remove_file(&tpacpi_pdev->dev.kobj,
>> +&dev_attr_wlan_tx_strength_reduce.attr);
>
>And similarly, `device_remove_file()`.

Same comment as above . I feel, sysfs_remove_file is more reasonable to do.
>
>
>> +}
>> +
>> +static struct ibm_struct dprc_driver_data = {
>> +	.name = "dprc",
>> +	.exit = dprc_exit,
>> +};
>> +
>>
>/****************************************************************
>************
>>
>*****************************************************************
>***********
>>   *
>> @@ -10475,6 +10607,10 @@ static struct ibm_init_struct ibms_init[]
>__initdata = {
>>  		.init = tpacpi_proxsensor_init,
>>  		.data = &proxsensor_driver_data,
>>  	},
>> +	{
>> +		.init = tpacpi_dprc_init,
>> +		.data = &dprc_driver_data,
>> +	},
>>  };
>>
>>  static int __init set_ibm_param(const char *val, const struct
>> kernel_param *kp)
>> --
>> 2.25.1
>
>
>Regards,
>Barnabás Pőcze

Thanks & Regards,
Nitin Joshi 

Re: [ibm-acpi-devel] [PATCH] platform/x86: ideapad-laptop/thinkpad_acpi: mark conflicting symbols static

[Hans de G. <hde...@re...>]

Hi Arnd,

On 2/4/21 4:38 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <ar...@ar...>
> 
> Three of the newly added functions are accidently not marked 'static' which
> causes a warning when building with W=1
> 
> drivers/platform/x86/thinkpad_acpi.c:10081:5: warning: no previous prototype for function 'dytc_profile_get' [-Wmissing-prototypes]
> drivers/platform/x86/thinkpad_acpi.c:10095:5: warning: no previous prototype for function 'dytc_cql_command' [-Wmissing-prototypes]
> drivers/platform/x86/thinkpad_acpi.c:10133:5: warning: no previous prototype for function 'dytc_profile_set' [-Wmissing-prototypes]
> 
> The functions are also present in two files, causing a link error when
> both are built into the kernel:

Thank you for your patch, but the issue has already been fixed in both drivers
in my review-hans branch (which will become for-next soon):

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

(note the ideapad fix is part of the big set of ideapad cleanups)

Regards,

Hans




> 
> ld.lld: error: duplicate symbol: dytc_cql_command
>>>> defined at ideapad-laptop.c
>>>>            platform/x86/ideapad-laptop.o:(dytc_cql_command) in archive drivers/built-in.a
>>>> defined at thinkpad_acpi.c
>>>>            platform/x86/thinkpad_acpi.o:(.text+0x20) in archive drivers/built-in.a
> 
> ld.lld: error: duplicate symbol: dytc_profile_get
>>>> defined at ideapad-laptop.c
>>>>            platform/x86/ideapad-laptop.o:(dytc_profile_get) in archive drivers/built-in.a
>>>> defined at thinkpad_acpi.c
>>>>            platform/x86/thinkpad_acpi.o:(.text+0x0) in archive drivers/built-in.a
> 
> ld.lld: error: duplicate symbol: dytc_profile_set
>>>> defined at ideapad-laptop.c
>>>>            platform/x86/ideapad-laptop.o:(dytc_profile_set) in archive drivers/built-in.a
>>>> defined at thinkpad_acpi.c
>>>>            platform/x86/thinkpad_acpi.o:(.text+0x220) in archive drivers/built-in.a
> 
> Mark these all as static to avoid both problems.
> 
> Fixes: eabe533904cb ("platform/x86: ideapad-laptop: DYTC Platform profile support")
> Fixes: c3bfcd4c6762 ("platform/x86: thinkpad_acpi: Add platform profile support")
> Signed-off-by: Arnd Bergmann <ar...@ar...>
> ---
>  drivers/platform/x86/ideapad-laptop.c | 11 ++++++-----
>  drivers/platform/x86/thinkpad_acpi.c  | 10 +++++-----
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index cc42af2a0a98..6095a4d54881 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -656,8 +656,8 @@ static int convert_profile_to_dytc(enum platform_profile_option profile, int *pe
>   * dytc_profile_get: Function to register with platform_profile
>   * handler. Returns current platform profile.
>   */
> -int dytc_profile_get(struct platform_profile_handler *pprof,
> -			enum platform_profile_option *profile)
> +static int dytc_profile_get(struct platform_profile_handler *pprof,
> +			    enum platform_profile_option *profile)
>  {
>  	struct ideapad_dytc_priv *dytc;
>  
> @@ -673,7 +673,8 @@ int dytc_profile_get(struct platform_profile_handler *pprof,
>   *  - enable CQL
>   *  If not in CQL mode, just run the command
>   */
> -int dytc_cql_command(struct ideapad_private *priv, int command, int *output)
> +static int dytc_cql_command(struct ideapad_private *priv, int command,
> +			    int *output)
>  {
>  	int err, cmd_err, dummy;
>  	int cur_funcmode;
> @@ -710,8 +711,8 @@ int dytc_cql_command(struct ideapad_private *priv, int command, int *output)
>   * dytc_profile_set: Function to register with platform_profile
>   * handler. Sets current platform profile.
>   */
> -int dytc_profile_set(struct platform_profile_handler *pprof,
> -			enum platform_profile_option profile)
> +static int dytc_profile_set(struct platform_profile_handler *pprof,
> +			    enum platform_profile_option profile)
>  {
>  	struct ideapad_dytc_priv *dytc;
>  	struct ideapad_private *priv;
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 18b390153e7f..42e0a497d69e 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10078,8 +10078,8 @@ static int convert_profile_to_dytc(enum platform_profile_option profile, int *pe
>   * dytc_profile_get: Function to register with platform_profile
>   * handler. Returns current platform profile.
>   */
> -int dytc_profile_get(struct platform_profile_handler *pprof,
> -			enum platform_profile_option *profile)
> +static int dytc_profile_get(struct platform_profile_handler *pprof,
> +			    enum platform_profile_option *profile)
>  {
>  	*profile = dytc_current_profile;
>  	return 0;
> @@ -10092,7 +10092,7 @@ int dytc_profile_get(struct platform_profile_handler *pprof,
>   *  - enable CQL
>   *  If not in CQL mode, just run the command
>   */
> -int dytc_cql_command(int command, int *output)
> +static int dytc_cql_command(int command, int *output)
>  {
>  	int err, cmd_err, dummy;
>  	int cur_funcmode;
> @@ -10130,8 +10130,8 @@ int dytc_cql_command(int command, int *output)
>   * dytc_profile_set: Function to register with platform_profile
>   * handler. Sets current platform profile.
>   */
> -int dytc_profile_set(struct platform_profile_handler *pprof,
> -			enum platform_profile_option profile)
> +static int dytc_profile_set(struct platform_profile_handler *pprof,
> +			    enum platform_profile_option profile)
>  {
>  	int output;
>  	int err;
> 

[ibm-acpi-devel] Unhandled HKEY events on Lenovo ThinkPad P15 Gen 1

[<thi...@vo...>]

Hi,

I hope this hasn't already been resolved and I'm reporting a duplicate! 
If so, I apologize, but I could not find anything on this by googling.

Three of the special function keys on my Lenovo P15 Gen1 laptop are not 
working, nor reporting anything with "xinput test" or "evtest". In dmesg 
I get the following messages:

For the "speech bubble" key (on F9):

[27642.269290] thinkpad_acpi: unhandled HKEY event 0x1317
[27642.269291] thinkpad_acpi: please report the conditions when this 
event happened to ibm...@li...

For the "answer telephone" key (on F10):

[27642.585707] thinkpad_acpi: unhandled HKEY event 0x1318
[27642.585711] thinkpad_acpi: please report the conditions when this 
event happened to ibm...@li...

For the "reject telephone call" key (on F11):

[27642.877307] thinkpad_acpi: unhandled HKEY event 0x1319
[27642.877309] thinkpad_acpi: please report the conditions when this 
event happened to ibm...@li...

I have uploaded a picture of what the keys look like (from a Nordic 
keyboard): 
https://www.dropbox.com/s/mnr3t95x0k71fxh/thinkpad_p15_keys.jpg?dl=0

This was tested on Ubuntu 20.04 with the 5.8.0-38-generic kernel.

Best regards,
Arvid Norlander

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: handle HKEY 0x4012, 0x4013 events

[Alexander K. <a-...@a-...>]

Dang, first ever patch and a typo crept in right before the commit... 
:-/  Corrected patch here:


Those events occur when a keyboard cover is attached to a ThinkPad
Tablet device.  Typically, they are used to switch from normal to tablet
mode in userspace; e.g., to offer touch keyboard choices when focus goes
to a text box and no keyboard is attached, or to enable autorotation of
the display according to the builtin orientation sensor.

No attempt is taken to emit an EV_SW event for SW_TABLET_MODE; this is
left to userspace.  So this patch is mainly to avoid warnings about
unknown and unhandled events, which are now reported as:

* Event 0x4012: attached keyboard cover
* Event 0x4013: detached keyboard cover

Tested as working on a ThinkPad X1 Tablet Gen 2, 20JCS00C00, and as
non-interfering with a ThinkPad X1 Carbon 7th, 20QESABM02 (normal
clamshell, so it does not have a keyboard cover).

Signed-off-by: Alexander Kobel <a-...@a-...>
---
  drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index c404706379d9..c16fd8c9d2fa 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -174,6 +174,8 @@ enum tpacpi_hkey_event_t {
  						     or port replicator */
  	TP_HKEY_EV_HOTPLUG_UNDOCK	= 0x4011, /* undocked from hotplug
  						     dock or port replicator */
+	TP_HKEY_EV_KBD_COVER_ATTACH	= 0x4012, /* attached keyboard cover */
+	TP_HKEY_EV_KBD_COVER_DETACH	= 0x4013, /* detached keyboard cover */

  	/* User-interface events */
  	TP_HKEY_EV_LID_CLOSE		= 0x5001, /* laptop lid closed */
@@ -3989,6 +3991,12 @@ static bool hotkey_notify_dockevent(const u32 hkey,
  	case TP_HKEY_EV_HOTPLUG_UNDOCK: /* undocked from port replicator */
  		pr_info("undocked from hotplug port replicator\n");
  		return true;
+	case TP_HKEY_EV_KBD_COVER_ATTACH: /* attached keyboard cover */
+		pr_info("attached keyboard cover\n");
+		return true;
+	case TP_HKEY_EV_KBD_COVER_DETACH: /* detached keyboard cover */
+		pr_info("detached keyboard cover\n");
+		return true;

  	default:
  		return false;
-- 
2.30.0



On 1/18/21 6:26 PM, Alexander Kobel wrote:
> Those events occur when a keyboard cover is attached to a ThinkPad
> Tablet device.  Typically, they are used to switch from normal to tablet
> mode in userspace; e.g., to offer touch keyboard choices when focus goes
> to a text box and no keyboard is attached, or to enable autorotation of
> the display according to the builtin orientation sensor.
> 
> No attempt is taken to emit an EV_SW event for SW_TABLET_MODE; this is
> left to userspace.  So this patch is mainly to avoid warnings about
> unknown and unhandled events, which are now reported as:
> 
> * Event 0x4012: attached keyboard cover
> * Event 0x4013: detached keyboard cover
> 
> Tested as working on a ThinkPad X1 Tablet Gen 2, 20JCS00C00, and as
> non-interfering with a ThinkPad X1 Carbon 7th, 20QESABM02 (normal
> clamshell, so it does not have a keyboard cover).
> 
> Signed-off-by: Alexander Kobel <a-...@a-...>
> ---
>   drivers/platform/x86/thinkpad_acpi.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index c404706379d9..e3b5f02bafa6 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -174,6 +174,8 @@ enum tpacpi_hkey_event_t {
>                                or port replicator */
>       TP_HKEY_EV_HOTPLUG_UNDOCK    = 0x4011, /* undocked from hotplug
>                                dock or port replicator */
> +    TP_HKEY_EV_KBD_COVER_ATTACH    = 0x4012, /* attached keyboard cover */
> +    TP_HKEY_EV_KBD_COVER_DETACH    = 0x4013, /* detached keyboard cover */
> 
>       /* User-interface events */
>       TP_HKEY_EV_LID_CLOSE        = 0x5001, /* laptop lid closed */
> @@ -3976,7 +3978,7 @@ static bool hotkey_notify_dockevent(const u32 hkey,
>       *ignore_acpi_ev = false;
> 
>       switch (hkey) {
> -    case TP_HKEY_EV_UNDOCK_ACK:
> +    case TP_HKEY_EV_UNDOCK_ACK cover:
>           /* ACPI undock operation completed after wakeup */
>           hotkey_autosleep_ack = 1;
>           pr_info("undocked\n");
> @@ -3989,6 +3991,12 @@ static bool hotkey_notify_dockevent(const u32 hkey,
>       case TP_HKEY_EV_HOTPLUG_UNDOCK: /* undocked from port replicator */
>           pr_info("undocked from hotplug port replicator\n");
>           return true;
> +    case TP_HKEY_EV_KBD_COVER_ATTACH: /* attached keyboard cover */
> +        pr_info("attached keyboard cover\n");
> +        return true;
> +    case TP_HKEY_EV_KBD_COVER_DETACH: /* detached keyboard cover */
> +        pr_info("detached keyboard cover\n");
> +        return true;
> 
>       default:
>           return false;
> 
> 
> _______________________________________________
> ibm-acpi-devel mailing list
> ibm...@li...
> https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
> 

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Add P53/73 firmware to fan_quirk_table for dual fan control

[Hans de G. <hde...@re...>]

Hi,

On 1/15/21 5:06 PM, jeanniestevenson wrote:
> This commit enables dual fan control for the new Lenovo P53 and P73 laptop models.
> 
> Signed-off-by: Jeannie Stevenson <jea...@pr...>

I know that Henrique has asked for a better commit message, but I'm preparing a fixes
branch to send out to Linus before the weekend and I want to include this, so I'm
going to accept this patch as is. Jeannie it would still be good if you can send
and email reply with the requested info for the archives.

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


> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c404706379d9..69402758b99c 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8782,6 +8782,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	TPACPI_Q_LNV3('N', '1', 'T', TPACPI_FAN_2CTL),	/* P71 */
>  	TPACPI_Q_LNV3('N', '1', 'U', TPACPI_FAN_2CTL),	/* P51 */
>  	TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2CTL),	/* P52 / P72 */
> +	TPACPI_Q_LNV3('N', '2', 'N', TPACPI_FAN_2CTL),	/* P53 / P73 */
>  	TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (1st gen) */
>  	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (2nd gen) */
>  	TPACPI_Q_LNV3('N', '2', 'V', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (3nd gen) */
> 

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: handle HKEY 0x4012, 0x4013 events

[Alexander K. <a-...@a-...>]

Hi all,



and thanks for all your work on thinkpad_acpi.


This is my very first attempt at submitting a kernel patch, so please 
bear with me if I didn't quite comply with the etiquette.  Like sending 
the patch mail as straight from Git's patch file without introducing me 
first.  ;-)

This one is pretty much trivial and conservative, I assume.  However, 
it's not 100% clear to me whether emitting a EV_SW event for 
SW_TABLET_MODE would be in order, as mentioned in the description.  If 
so, I'd have a deeper look on how to do that, and perhaps require some 
hand-holding.


Besides, the X1 Tablet has a few Fn+smth combos that are not working 
currently, and instead report (in evtest):

Input driver version is 1.0.1
Input device ID: bus 0x3 vendor 0x17ef product 0x60a3 version 0x111
Input device name: "PRIMAX ThinkPad X1 Tablet Thin Keyboard Gen 2 
Consumer Control"
type 4 (EV_MSC), code 4 (MSC_SCAN), value c0001
type 1 (EV_KEY), code 240 (KEY_UNKNOWN), value 1

Keys affected are Fn+Esc (Fn lock) and Fn+F7 - Fn+F12 (various settings 
like radio/bluetooth on/off).

Also, the mute and micmute leds are not working, and Fn lock led and 
function is unavailable.


I'd love to give those minor issues a shot, too, but I'm not sure where 
to start.  Would this go via thinkpad_acpi or hid_lenovo?


Thanks and cheers,
Alexander



On 1/18/21 6:26 PM, Alexander Kobel wrote:
> Those events occur when a keyboard cover is attached to a ThinkPad
> Tablet device.  Typically, they are used to switch from normal to tablet
> mode in userspace; e.g., to offer touch keyboard choices when focus goes
> to a text box and no keyboard is attached, or to enable autorotation of
> the display according to the builtin orientation sensor.
> 
> No attempt is taken to emit an EV_SW event for SW_TABLET_MODE; this is
> left to userspace.  So this patch is mainly to avoid warnings about
> unknown and unhandled events, which are now reported as:
> 
> * Event 0x4012: attached keyboard cover
> * Event 0x4013: detached keyboard cover
> 
> Tested as working on a ThinkPad X1 Tablet Gen 2, 20JCS00C00, and as
> non-interfering with a ThinkPad X1 Carbon 7th, 20QESABM02 (normal
> clamshell, so it does not have a keyboard cover).
> 
> Signed-off-by: Alexander Kobel <a-...@a-...>
> ---
>   drivers/platform/x86/thinkpad_acpi.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index c404706379d9..e3b5f02bafa6 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -174,6 +174,8 @@ enum tpacpi_hkey_event_t {
>                                or port replicator */
>       TP_HKEY_EV_HOTPLUG_UNDOCK    = 0x4011, /* undocked from hotplug
>                                dock or port replicator */
> +    TP_HKEY_EV_KBD_COVER_ATTACH    = 0x4012, /* attached keyboard cover */
> +    TP_HKEY_EV_KBD_COVER_DETACH    = 0x4013, /* detached keyboard cover */
> 
>       /* User-interface events */
>       TP_HKEY_EV_LID_CLOSE        = 0x5001, /* laptop lid closed */
> @@ -3976,7 +3978,7 @@ static bool hotkey_notify_dockevent(const u32 hkey,
>       *ignore_acpi_ev = false;
> 
>       switch (hkey) {
> -    case TP_HKEY_EV_UNDOCK_ACK:
> +    case TP_HKEY_EV_UNDOCK_ACK cover:
>           /* ACPI undock operation completed after wakeup */
>           hotkey_autosleep_ack = 1;
>           pr_info("undocked\n");
> @@ -3989,6 +3991,12 @@ static bool hotkey_notify_dockevent(const u32 hkey,
>       case TP_HKEY_EV_HOTPLUG_UNDOCK: /* undocked from port replicator */
>           pr_info("undocked from hotplug port replicator\n");
>           return true;
> +    case TP_HKEY_EV_KBD_COVER_ATTACH: /* attached keyboard cover */
> +        pr_info("attached keyboard cover\n");
> +        return true;
> +    case TP_HKEY_EV_KBD_COVER_DETACH: /* detached keyboard cover */
> +        pr_info("detached keyboard cover\n");
> +        return true;
> 
>       default:
>           return false;

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: handle HKEY 0x4012, 0x4013 events

[Alexander K. <a-...@a-...>]

Those events occur when a keyboard cover is attached to a ThinkPad
Tablet device.  Typically, they are used to switch from normal to tablet
mode in userspace; e.g., to offer touch keyboard choices when focus goes
to a text box and no keyboard is attached, or to enable autorotation of
the display according to the builtin orientation sensor.

No attempt is taken to emit an EV_SW event for SW_TABLET_MODE; this is
left to userspace.  So this patch is mainly to avoid warnings about
unknown and unhandled events, which are now reported as:

* Event 0x4012: attached keyboard cover
* Event 0x4013: detached keyboard cover

Tested as working on a ThinkPad X1 Tablet Gen 2, 20JCS00C00, and as
non-interfering with a ThinkPad X1 Carbon 7th, 20QESABM02 (normal
clamshell, so it does not have a keyboard cover).

Signed-off-by: Alexander Kobel <a-...@a-...>
---
  drivers/platform/x86/thinkpad_acpi.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index c404706379d9..e3b5f02bafa6 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -174,6 +174,8 @@ enum tpacpi_hkey_event_t {
  						     or port replicator */
  	TP_HKEY_EV_HOTPLUG_UNDOCK	= 0x4011, /* undocked from hotplug
  						     dock or port replicator */
+	TP_HKEY_EV_KBD_COVER_ATTACH	= 0x4012, /* attached keyboard cover */
+	TP_HKEY_EV_KBD_COVER_DETACH	= 0x4013, /* detached keyboard cover */

  	/* User-interface events */
  	TP_HKEY_EV_LID_CLOSE		= 0x5001, /* laptop lid closed */
@@ -3976,7 +3978,7 @@ static bool hotkey_notify_dockevent(const u32 hkey,
  	*ignore_acpi_ev = false;

  	switch (hkey) {
-	case TP_HKEY_EV_UNDOCK_ACK:
+	case TP_HKEY_EV_UNDOCK_ACK cover:
  		/* ACPI undock operation completed after wakeup */
  		hotkey_autosleep_ack = 1;
  		pr_info("undocked\n");
@@ -3989,6 +3991,12 @@ static bool hotkey_notify_dockevent(const u32 hkey,
  	case TP_HKEY_EV_HOTPLUG_UNDOCK: /* undocked from port replicator */
  		pr_info("undocked from hotplug port replicator\n");
  		return true;
+	case TP_HKEY_EV_KBD_COVER_ATTACH: /* attached keyboard cover */
+		pr_info("attached keyboard cover\n");
+		return true;
+	case TP_HKEY_EV_KBD_COVER_DETACH: /* detached keyboard cover */
+		pr_info("detached keyboard cover\n");
+		return true;

  	default:
  		return false;
-- 
2.30.0

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Add P53/73 firmware to fan_quirk_table for dual fan control

[Henrique de M. H. <hm...@hm...>]

On Fri, 15 Jan 2021, jeanniestevenson wrote:
> This commit enables dual fan control for the new Lenovo P53 and P73 laptop models.
> 
> Signed-off-by: Jeannie Stevenson <jea...@pr...>

It has been tested on which thinkpad model (name and numerical model,
please) ?  It is also nice to have that kind of information in the
commit message.

With that information:
Acked-by: Henrique de Moraes Holschuh <hm...@hm...>

> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c404706379d9..69402758b99c 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8782,6 +8782,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	TPACPI_Q_LNV3('N', '1', 'T', TPACPI_FAN_2CTL),	/* P71 */
>  	TPACPI_Q_LNV3('N', '1', 'U', TPACPI_FAN_2CTL),	/* P51 */
>  	TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2CTL),	/* P52 / P72 */
> +	TPACPI_Q_LNV3('N', '2', 'N', TPACPI_FAN_2CTL),	/* P53 / P73 */
>  	TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (1st gen) */
>  	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (2nd gen) */
>  	TPACPI_Q_LNV3('N', '2', 'V', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (3nd gen) */

-- 
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: fix: use scnprintf instead of snprintf.

[Henrique de M. H. <hm...@hm...>]

On Wed, 06 Jan 2021, Joe Perches wrote:
> On Wed, 2021-01-06 at 14:36 +0800, YANG LI wrote:
> > The snprintf() function returns the number of characters which would
> > have been printed if there were enough space, but the scnprintf()
> > returns the number of characters which were actually printed. If the
> > buffer is not large enough, then using snprintf() would result in a
> > read overflow and an information leak. This error was found with the
> > help of coccicheck.
> 
> In all cases, the buffer _is_ large enough.

Thank you for double-checking!

> _show function lengths are OK for all the uses with PAGE_SIZE.
> And it's probably better to use sysfs_emit for all the _show functions

Indeed.

-- 
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: fix: use scnprintf instead of snprintf.

[Hans de G. <hde...@re...>]

Hi,

On 1/6/21 10:01 AM, Joe Perches wrote:
> On Wed, 2021-01-06 at 14:36 +0800, YANG LI wrote:
>> The snprintf() function returns the number of characters which would
>> have been printed if there were enough space, but the scnprintf()
>> returns the number of characters which were actually printed. If the
>> buffer is not large enough, then using snprintf() would result in a
>> read overflow and an information leak. This error was found with the
>> help of coccicheck.
> 
> In all cases, the buffer _is_ large enough.
> 
> tmpi is length 5 and ok.
> include/sound/core.h:   char shortname[32];             /* short name of this soundcard */
> include/sound/core.h:   char longname[80];              /* name of this soundcard */
> include/sound/core.h:   char mixername[80];             /* mixer name */
> 
> _show function lengths are OK for all the uses with PAGE_SIZE.
> And it's probably better to use sysfs_emit for all the _show functions

Yes, please send a v2 with the following changes:

1. Use sysfs_emit in all the sysfs read functions
2. Do not replace snprintf with scnprintf when the return value is not used, that is just needless churn
3. Update the commit message to reflect 1.

Regards,

Hans

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: fix: use scnprintf instead of snprintf.

[Joe P. <jo...@pe...>]

On Wed, 2021-01-06 at 14:36 +0800, YANG LI wrote:
> The snprintf() function returns the number of characters which would
> have been printed if there were enough space, but the scnprintf()
> returns the number of characters which were actually printed. If the
> buffer is not large enough, then using snprintf() would result in a
> read overflow and an information leak. This error was found with the
> help of coccicheck.

In all cases, the buffer _is_ large enough.

tmpi is length 5 and ok.
include/sound/core.h:   char shortname[32];             /* short name of this soundcard */
include/sound/core.h:   char longname[80];              /* name of this soundcard */
include/sound/core.h:   char mixername[80];             /* mixer name */

_show function lengths are OK for all the uses with PAGE_SIZE.
And it's probably better to use sysfs_emit for all the _show functions

Re: [ibm-acpi-devel] [PATCH] Input: cyapa - do not call input_device_enabled from power mode handler

[Dmitry T. <dmi...@gm...>]

On Fri, Dec 11, 2020 at 09:22:44AM +0100, Marek Szyprowski wrote:
> 
> On 11.12.2020 08:09, Dmitry Torokhov wrote:
> > Input device's user counter is supposed to be accessed only while holding
> > input->mutex.  Commit d69f0a43c677 ("Input: use input_device_enabled()")
> > recently switched cyapa to using the dedicated API and it uncovered the
> > fact that cyapa driver violated this constraint.
> >
> > This patch removes checks whether the input device is open when clearing
> > device queues when changing device's power mode as there is no harm in
> > sending input events through closed input device - the events will simply
> > be dropped by the input core.
> >
> > Note that there are more places in cyapa driver that call
> > input_device_enabled() without holding input->mutex, those are left
> > unfixed for now.
> >
> > Reported-by: Marek Szyprowski <m.s...@sa...>
> > Signed-off-by: Dmitry Torokhov <dmi...@gm...>
> > ---
> >
> > Marek, could you please try this one?
> 
> The warning is still there:

Ah, yes, we are hitting another instance right after setting power mode.
I need to think more how to handle that one.

Thanks.

-- 
Dmitry

[ibm-acpi-devel] [PATCH] Input: cyapa - do not call input_device_enabled from power mode handler

[Dmitry T. <dmi...@gm...>]

Input device's user counter is supposed to be accessed only while holding
input->mutex.  Commit d69f0a43c677 ("Input: use input_device_enabled()")
recently switched cyapa to using the dedicated API and it uncovered the
fact that cyapa driver violated this constraint.

This patch removes checks whether the input device is open when clearing
device queues when changing device's power mode as there is no harm in
sending input events through closed input device - the events will simply
be dropped by the input core.

Note that there are more places in cyapa driver that call
input_device_enabled() without holding input->mutex, those are left
unfixed for now.

Reported-by: Marek Szyprowski <m.s...@sa...>
Signed-off-by: Dmitry Torokhov <dmi...@gm...>
---

Marek, could you please try this one?

 drivers/input/mouse/cyapa_gen3.c |    5 +----
 drivers/input/mouse/cyapa_gen5.c |    3 +--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
index a97f4acb6452..4a9022faf945 100644
--- a/drivers/input/mouse/cyapa_gen3.c
+++ b/drivers/input/mouse/cyapa_gen3.c
@@ -907,7 +907,6 @@ static u16 cyapa_get_wait_time_for_pwr_cmd(u8 pwr_mode)
 static int cyapa_gen3_set_power_mode(struct cyapa *cyapa, u8 power_mode,
 		u16 always_unused, enum cyapa_pm_stage pm_stage)
 {
-	struct input_dev *input = cyapa->input;
 	u8 power;
 	int tries;
 	int sleep_time;
@@ -953,7 +952,6 @@ static int cyapa_gen3_set_power_mode(struct cyapa *cyapa, u8 power_mode,
 	 * depending on the command's content.
 	 */
 	if (cyapa->operational &&
-	    input && input_device_enabled(input) &&
 	    (pm_stage == CYAPA_PM_RUNTIME_SUSPEND ||
 	     pm_stage == CYAPA_PM_RUNTIME_RESUME)) {
 		/* Try to polling in 120Hz, read may fail, just ignore it. */
@@ -1223,8 +1221,7 @@ static int cyapa_gen3_try_poll_handler(struct cyapa *cyapa)
 	    (data.finger_btn & OP_DATA_VALID) != OP_DATA_VALID)
 		return -EINVAL;
 
-	return cyapa_gen3_event_process(cyapa, &data);
-
+	return cyapa->input ? cyapa_gen3_event_process(cyapa, &data) : 0;
 }
 
 static int cyapa_gen3_initialize(struct cyapa *cyapa) { return 0; }
diff --git a/drivers/input/mouse/cyapa_gen5.c b/drivers/input/mouse/cyapa_gen5.c
index abf42f77b4c5..afc5aa4dcf47 100644
--- a/drivers/input/mouse/cyapa_gen5.c
+++ b/drivers/input/mouse/cyapa_gen5.c
@@ -518,8 +518,7 @@ int cyapa_empty_pip_output_data(struct cyapa *cyapa,
 			*len = length;
 			/* Response found, success. */
 			return 0;
-		} else if (cyapa->operational &&
-			   input && input_device_enabled(input) &&
+		} else if (cyapa->operational && input &&
 			   (pm_stage == CYAPA_PM_RUNTIME_RESUME ||
 			    pm_stage == CYAPA_PM_RUNTIME_SUSPEND)) {
 			/* Parse the data and report it if it's valid. */

Re: [ibm-acpi-devel] [PATCH v4 2/7] Input: use input_device_enabled()

[Dmitry T. <dmi...@gm...>]

On Tue, Dec 08, 2020 at 11:05:42AM +0100, Marek Szyprowski wrote:
> Hi Andrzej,
> 
> On 07.12.2020 16:50, Andrzej Pietrasiewicz wrote:
> > Hi Marek,
> >
> > W dniu 07.12.2020 o 14:32, Marek Szyprowski pisze:
> >> Hi Andrzej,
> >>
> >> On 08.06.2020 13:22, Andrzej Pietrasiewicz wrote:
> >>> Use the newly added helper in relevant input drivers.
> >>>
> >>> Signed-off-by: Andrzej Pietrasiewicz <and...@co...>
> >>
> >> This patch landed recently in linux-next as commit d69f0a43c677 ("Input:
> >> use input_device_enabled()"). Sadly it causes following warning during
> >> system suspend/resume cycle on ARM 32bit Samsung Exynos5250-based Snow
> >> Chromebook with kernel compiled from exynos_defconfig:
> >>
> >> ------------[ cut here ]------------
> >> WARNING: CPU: 0 PID: 1777 at drivers/input/input.c:2230
> >> input_device_enabled+0x68/0x6c
> >> Modules linked in: cmac bnep mwifiex_sdio mwifiex sha256_generic
> >> libsha256 sha256_arm cfg80211 btmrvl_sdio btmrvl bluetooth s5p_mfc
> >> exynos_gsc v4l2_mem2mem videob
> >> CPU: 0 PID: 1777 Comm: rtcwake Not tainted
> >> 5.10.0-rc6-next-20201207-00001-g49a0dc04c46d-dirty #9902
> >> Hardware name: Samsung Exynos (Flattened Device Tree)
> >> [<c0111718>] (unwind_backtrace) from [<c010d050>] (show_stack+0x10/0x14)
> >> [<c010d050>] (show_stack) from [<c0b32810>] (dump_stack+0xb4/0xd4)
> >> [<c0b32810>] (dump_stack) from [<c0126e24>] (__warn+0xd8/0x11c)
> >> [<c0126e24>] (__warn) from [<c0126f18>] (warn_slowpath_fmt+0xb0/0xb8)
> >> [<c0126f18>] (warn_slowpath_fmt) from [<c07fa2fc>]
> >> (input_device_enabled+0x68/0x6c)
> >> [<c07fa2fc>] (input_device_enabled) from [<c080a0f8>]
> >
> > Apparently you are hitting this line of code in drivers/input/input.c:
> >
> > lockdep_assert_held(&dev->mutex);
> >
> > Inspecting input device's "users" member should happen under dev's lock.
> >
> This check and warning has been introduced by this patch. I assume that 
> the suspend/resume paths are correct, but it looks that they were not 
> tested with this patch thus it has not been noticed that they are not 
> called under the input's lock. This needs a fix. Dmitry: how would you 
> like to handle this issue?

The check is proper and the warning is legit, cyapa should not be
checking this field without holding the lock. I think we can simply
remove this check from the power ops for gen3 and gen5, and this should
shut up the warning on suspend, but there other places in cyapa that do
check 'users', and they also need to be fixed.

Thanks.

-- 
Dmitry

Re: [ibm-acpi-devel] [PATCH v2] Input: document inhibiting

[Dmitry T. <dmi...@gm...>]

On Wed, Jun 17, 2020 at 12:18:22PM +0200, Andrzej Pietrasiewicz wrote:
> Document inhibiting input devices and its relation to being
> a wakeup source.
> 
> Signed-off-by: Andrzej Pietrasiewicz <and...@co...>

Applied, thank you.

-- 
Dmitry

Re: [ibm-acpi-devel] [PATCH v4 7/7] Input: Add "inhibited" property

[Dmitry T. <dmi...@gm...>]

On Tue, Oct 06, 2020 at 06:12:49PM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 06, 2020 at 06:11:02PM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 06, 2020 at 03:04:28PM +0200, Andrzej Pietrasiewicz wrote:
> > > Hi Dmitry,
> > > 
> > > W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze:
> > > > Hi Andrzej,
> > > > 
> > > > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote:
> > > > > @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev,
> > > > >   	case EV_KEY:
> > > > >   		if (is_event_supported(code, dev->keybit, KEY_MAX)) {
> > > > > -			/* auto-repeat bypasses state updates */
> > > > > -			if (value == 2) {
> > > > > +			/*
> > > > > +			 * auto-repeat bypasses state updates but repeat
> > > > > +			 * events are ignored if the key is not pressed
> > > > > +			 */
> > > > > +			if (value == 2 && test_bit(code, dev->key)) {
> > > > >   				disposition = INPUT_PASS_TO_HANDLERS;
> > > > >   				break;
> > > > >   			}
> > > > 
> > > > Is this chunk really part of inhibit support? I'd think we cancel
> > > > autorepeat timer when we are releasing a key, no?
> > > > 
> > > 
> > > When I look at it now it seems to me the chunk might be redundant.
> > > But let me explain what I had in mind when adding it.
> > > 
> > > It is a matter of what we do with input events generated while a
> > > device is inhibited. If ->open()/->close() are not provided by the
> > > driver then inhibiting amounts to merely ignoring input events from
> > > a device while it remains active. What else can you do if the driver
> > > does not provide a method to prepare the device for generating events/
> > > to stop generating events?
> > > 
> > > In this special case a user might trigger a repeated event while the
> > > device is inhibited, then the user keeps holding the key down and the
> > > device is uninhibited. Do we pass anything to handlers then?
> > > 
> > > In my opinion we should not. Such an event is "illegal" in a sense that it
> > > was generated at a time when nobody wanted any events from the device.
> > > Hence the test to let only those auto-repeat events through for which
> > > a key is actually pressed.
> > > 
> > > However, what I see now is that if a device is inhibited, no key
> > > will ever reach neither the "1" nor "2" state because of the "if"
> > > in the very beginning of input_handle_event().
> > 
> > OK, then let's drop it for now. We can revisit if we see that a problem.
> 
> And by that I mean that I will drop it myself, no need to resend. I will
> be applying this shortly.

Well, "shortly" was just a tad optimistic, but I did apply it ;)

Thanks.

-- 
Dmitry

Re: [ibm-acpi-devel] [PATCH v4 2/7] Input: use input_device_enabled()

[Dmitry T. <dmi...@gm...>]

On Mon, Jun 08, 2020 at 01:22:06PM +0200, Andrzej Pietrasiewicz wrote:
> Use the newly added helper in relevant input drivers.
> 
> Signed-off-by: Andrzej Pietrasiewicz <and...@co...>

Applied, thank you.

-- 
Dmitry

Re: [ibm-acpi-devel] [PATCH v4 1/7] Input: add input_device_enabled()

[Dmitry T. <dmi...@gm...>]

On Mon, Jun 08, 2020 at 01:22:05PM +0200, Andrzej Pietrasiewicz wrote:
> A helper function for drivers to decide if the device is used or not.
> A lockdep check is introduced as inspecting ->users should be done under
> input device's mutex.
> 
> Signed-off-by: Andrzej Pietrasiewicz <and...@co...>

Applied, thank you.

-- 
Dmitry

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: remove trailing semicolon in macro definition

[Hans de G. <hde...@re...>]

Hi,

On 11/27/20 7:10 PM, tr...@re... wrote:
> From: Tom Rix <tr...@re...>
> 
> The macro use will already have a semicolon.
> 
> Signed-off-by: Tom Rix <tr...@re...>

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

> ---
>  drivers/platform/x86/thinkpad_acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 36d9594bca7f..5bbb378d1a37 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -1025,7 +1025,7 @@ static struct attribute_set *create_attr_set(unsigned int max_members,
>  }
>  
>  #define destroy_attr_set(_set) \
> -	kfree(_set);
> +	kfree(_set)
>  
>  /* not multi-threaded safe, use it in a single thread per set */
>  static int add_to_attr_set(struct attribute_set *s, struct attribute *attr)
> 

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Whitelist P15 firmware for dual fan control

[Hans de G. <hde...@re...>]

Hi,

On 11/26/20 1:04 AM, tam...@43... wrote:
> From: Matthias Maier <ta...@43...>
> 
> This commit enables dual fan control for the following new Lenovo
> models: P15, P15v.
> 
> Signed-off-by: Matthias Maier <ta...@43...>

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

> ---
>  drivers/platform/x86/thinkpad_acpi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 5ad06fd98515..26b1502f9bbe 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8786,6 +8786,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (1st gen) */
>  	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (2nd gen) */
>  	TPACPI_Q_LNV3('N', '2', 'V', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (3nd gen) */
> +	TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL),	/* P15 (1st gen) / P15v (1st gen) */
>  };
>  
>  static int __init fan_init(struct ibm_init_struct *iibm)
> 

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Whitelist P15 firmware for dual fan control

[<tam...@43...>]

From: Matthias Maier <ta...@43...>

This commit enables dual fan control for the following new Lenovo
models: P15, P15v.

Signed-off-by: Matthias Maier <ta...@43...>
---
 drivers/platform/x86/thinkpad_acpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 5ad06fd98515..26b1502f9bbe 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8786,6 +8786,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
 	TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (1st gen) */
 	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (2nd gen) */
 	TPACPI_Q_LNV3('N', '2', 'V', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (3nd gen) */
+	TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL),	/* P15 (1st gen) / P15v (1st gen) */
 };
 
 static int __init fan_init(struct ibm_init_struct *iibm)
-- 
2.26.2

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Whitelist P15 firmware for dual

[<tam...@43...>]

Dear all,

The new Lenovo Thinkpad P15/P15v series are both equipped with two fans
(with independent fan control).

Simply enabling TPACPI_FAN_2CTL for the respective firmware is enough to
make fan control work.

I have tested the following patch on my P15 (with "N30..." firmware): I can
control the second fan via /proc/acpi/ibm/fan, and the second fan now shows
up in lm-sensors:

    $ sensors
    [...]
    thinkpad-isa-0000
    Adapter: ISA adapter
    fan1:           0 RPM
    fan2:           0 RPM

Best,
Matthias

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Send tablet mode switch at wakeup time

[Hans de G. <hde...@re...>]

Hi,

On 11/23/20 2:21 PM, Benjamin Berg wrote:
> From: Benjamin Berg <bb...@re...>
> 
> The lid state may change while the machine is suspended. As such, we may
> need to re-check the state at wake-up time (at least when waking up from
> hibernation).
> Add the appropriate call to the resume handler in order to sync the
> SW_TABLET_MODE switch state with the hardware state.
> 
> Fixes: dda3ec0aa631 ("platform/x86: thinkpad_acpi: Implement tablet mode using GMMS method")
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210269
> Signed-off-by: Benjamin Berg <bb...@re...>

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

> ---
>  drivers/platform/x86/thinkpad_acpi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e3810675090a..9104e0ee37cd 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4228,6 +4228,7 @@ static void hotkey_resume(void)
>  		pr_err("error while attempting to reset the event firmware interface\n");
>  
>  	tpacpi_send_radiosw_update();
> +	tpacpi_input_send_tabletsw();
>  	hotkey_tablet_mode_notify_change();
>  	hotkey_wakeup_reason_notify_change();
>  	hotkey_wakeup_hotunplug_complete_notify_change();
>