Hi,
On 3/11/21 6:48 PM, Mark Pearson wrote:
> Lenovo platforms with DYTC versions earlier than version 5 don't set
> the lapmode interface correctly, causing issues with thermald on
> older platforms.
>
> Add checking to only create the dytc_lapmode interface for version
> 5 and later.
>
> Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx>
I've added a:
Fixes: 1ac09656bded ("platform/x86: thinkpad_acpi: Add palm sensor support")
Tag, this will help with the patch automatically getting selected for
stable kernel-series which contain the patch pointed to by the Fixes: tag.
In this case this won't work since this patch seems to depend on code
introduced in:
commit c3bfcd4c676238 ("platform/x86: thinkpad_acpi: lap or desk mode interface")
So you will need to manually backport this and submit it to stable@xxxxxxxxxxxxxxx
if you want it to be included in any of the stable kernel series.
Still it is always good to have the Fixes: tag present when a commit is actually
fixing a previous commit.
###
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 | 91 ++++++++++++++++++++--------
> 1 file changed, 65 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index b881044b31b0..f7de90a47e28 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9845,6 +9845,11 @@ static struct ibm_struct lcdshadow_driver_data = {
> * Thinkpad sensor interfaces
> */
>
> +#define DYTC_CMD_QUERY 0 /* To get DYTC status - enable/revision */
> +#define DYTC_QUERY_ENABLE_BIT 8 /* Bit 8 - 0 = disabled, 1 = enabled */
> +#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> +#define DYTC_QUERY_REV_BIT 28 /* Bits 28 - 31 - revision */
> +
> #define DYTC_CMD_GET 2 /* To get current IC function and mode */
> #define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
>
> @@ -9855,6 +9860,7 @@ static bool has_palmsensor;
> static bool has_lapsensor;
> static bool palm_state;
> static bool lap_state;
> +static int dytc_version;
>
> static int dytc_command(int command, int *output)
> {
> @@ -9869,6 +9875,33 @@ static int dytc_command(int command, int *output)
> return 0;
> }
>
> +static int dytc_get_version(void)
> +{
> + int err, output;
> +
> + /* Check if we've been called before - and just return cached value */
> + if (dytc_version)
> + return dytc_version;
> +
> + /* Otherwise query DYTC and extract version information */
> + err = dytc_command(DYTC_CMD_QUERY, &output);
> + /*
> + * If support isn't available (ENODEV) then don't return an error
> + * and don't create the sysfs group
> + */
> + if (err == -ENODEV)
> + return 0;
> + /* For all other errors we can flag the failure */
> + if (err)
> + return err;
> +
> + /* Check DYTC is enabled and supports mode setting */
> + if (output & BIT(DYTC_QUERY_ENABLE_BIT))
> + dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
> +
> + return 0;
> +}
> +
> static int lapsensor_get(bool *present, bool *state)
> {
> int output, err;
> @@ -9974,7 +10007,18 @@ static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm)
> if (err)
> return err;
> }
> - if (has_lapsensor) {
> +
> + /* Check if we know the DYTC version, if we don't then get it */
> + if (!dytc_version) {
> + err = dytc_get_version();
> + if (err)
> + return err;
> + }
> + /*
> + * Platforms before DYTC version 5 claim to have a lap sensor, but it doesn't work, so we
> + * ignore them
> + */
> + if (has_lapsensor && (dytc_version >= 5)) {
> err = sysfs_create_file(&tpacpi_pdev->dev.kobj, &dev_attr_dytc_lapmode.attr);
> if (err)
> return err;
> @@ -9999,14 +10043,9 @@ static struct ibm_struct proxsensor_driver_data = {
> * DYTC Platform Profile interface
> */
>
> -#define DYTC_CMD_QUERY 0 /* To get DYTC status - enable/revision */
> #define DYTC_CMD_SET 1 /* To enable/disable IC function mode */
> #define DYTC_CMD_RESET 0x1ff /* To reset back to default */
>
> -#define DYTC_QUERY_ENABLE_BIT 8 /* Bit 8 - 0 = disabled, 1 = enabled */
> -#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> -#define DYTC_QUERY_REV_BIT 28 /* Bits 28 - 31 - revision */
> -
> #define DYTC_GET_FUNCTION_BIT 8 /* Bits 8-11 - function setting */
> #define DYTC_GET_MODE_BIT 12 /* Bits 12-15 - mode setting */
>
> @@ -10211,28 +10250,28 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
> if (err)
> return err;
>
> + /* Check if we know the DYTC version, if we don't then get it */
> + if (!dytc_version) {
> + err = dytc_get_version();
> + if (err)
> + return err;
> + }
> /* Check DYTC is enabled and supports mode setting */
> - if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
> - /* Only DYTC v5.0 and later has this feature. */
> - int dytc_version;
> -
> - dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
> - if (dytc_version >= 5) {
> - dbg_printk(TPACPI_DBG_INIT,
> - "DYTC version %d: thermal mode available\n", dytc_version);
> - /* Create platform_profile structure and register */
> - err = platform_profile_register(&dytc_profile);
> - /*
> - * If for some reason platform_profiles aren't enabled
> - * don't quit terminally.
> - */
> - if (err)
> - return 0;
> + if (dytc_version >= 5) {
> + dbg_printk(TPACPI_DBG_INIT,
> + "DYTC version %d: thermal mode available\n", dytc_version);
> + /* Create platform_profile structure and register */
> + err = platform_profile_register(&dytc_profile);
> + /*
> + * If for some reason platform_profiles aren't enabled
> + * don't quit terminally.
> + */
> + if (err)
> + return 0;
>
> - dytc_profile_available = true;
> - /* Ensure initial values are correct */
> - dytc_profile_refresh();
> - }
> + dytc_profile_available = true;
> + /* Ensure initial values are correct */
> + dytc_profile_refresh();
> }
> return 0;
> }
>
