| Message ID | 20200617180937.14949-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [RESEND,v2] platform/x86: thinkpad_acpi: lap or desk mode interface | expand |
Hey Mark, ----- Original Message ----- > Newer Lenovo Thinkpad platforms have support to identify whether the > system is on-lap or not using an ACPI DYTC event from the firmware. > > This patch provides the ability to retrieve the current mode via sysfs > entrypoints and will be used by userspace for thermal mode and WWAN > functionality > > Co-developed-by: Nitin Joshi <njoshi1@lenovo.com> > Signed-off-by: Nitin Joshi <njoshi1@lenovo.com> > Reviewed-by: Sugumaran <slacshiminar@lenovo.com> > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > Resend: > - Adding Bastien Nocera to cc list as requested > Changes in v2: > - cleaned up initialisation sequence to be cleaner and avoid spamming > platforms that don't have DYTC with warning message. Tested on P52 > - Adding platform-driver-x86 mailing list for review as requested > > drivers/platform/x86/thinkpad_acpi.c | 113 +++++++++++++++++++++++++++ > 1 file changed, 113 insertions(+) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c > b/drivers/platform/x86/thinkpad_acpi.c > index 0f704484ae1d..8f51bbba21cd 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -4049,6 +4049,7 @@ static bool hotkey_notify_6xxx(const u32 hkey, > pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n"); > /* recommended action: do nothing, we don't have > * Lenovo ATM information */ > + tpacpi_driver_event(hkey); > return true; > case TP_HKEY_EV_THM_TRANSFM_CHANGED: > pr_debug("EC reports: Thermal Transformation changed (GMTS)\n"); > @@ -9811,6 +9812,110 @@ static struct ibm_struct lcdshadow_driver_data = { > .write = lcdshadow_write, > }; > > +/************************************************************************* > + * DYTC subdriver, for the Lenovo performace mode feature > + */ I don't think this should mention the performance mode, as it's a lap/table detection mode. Do we need to call that "DYTC"? "lapmode"? "lap_detection"? Or does the DYTC interface offer more functionality that we'd export in the future? > + > +#define DYTC_CMD_GET 2 /*To get current IC function and mode*/ For this comment and all the ones below, space after "/*" and before "*/" > +#define DYTC_GET_ENABLE_MASK 0x1 /*0 = disabled, 1 = enabled*/ Is that necessary? > +#define DYTC_GET_LAPMODE_SHIFT 17 You'd probably want to call this "bit" rather than shift. We shift it to read the value, but 17 is the bit's position. (See BIT() usage in the driver) Do you want to add a comment here? Is there anything else that could be documented (the other bits, which versions of firmware would have that, if there's a publicly available version, or which hardware if publicly available) > +static int dytc_lapmode; > +static void dytc_lapmode_notify_change(void) > +{ > + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, > + "dytc_lapmode"); > +} > + > +static int dytc_command(int command) > +{ > + acpi_handle dytc_handle; > + int output; > + > + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) { > + /*Platform doesn't support DYTC*/ > + return -ENODEV; > + } > + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command)) > + return -EIO; > + return output; > +} > + > +static int dytc_lapmode_get(void) > +{ > + int output; > + > + output = dytc_command(DYTC_CMD_GET); > + if ((output == -ENODEV) || (output == -EIO)) > + return output; > + > + return ((output >> DYTC_GET_LAPMODE_SHIFT) & > + DYTC_GET_ENABLE_MASK); Use BIT() instead? eg. return (output & BIT(DYTC_GET_LAPMODE_SHIFT)); > +} > + > +static void dytc_lapmode_refresh(void) > +{ > + int new_state; > + > + new_state = dytc_lapmode_get(); > + if ((new_state == -ENODEV) || (new_state == -EIO)) > + return; You could also return early if "dytc_lapmode == new_state". Rest looks good to me.
Hi Bastien On 6/19/2020 9:52 AM, Bastien Nocera wrote: > Hey Mark, > > ----- Original Message ----- <snip> >> >> +/************************************************************************* >> + * DYTC subdriver, for the Lenovo performace mode feature >> + */ > > I don't think this should mention the performance mode, as it's a lap/table > detection mode. Do we need to call that "DYTC"? "lapmode"? "lap_detection"? > Or does the DYTC interface offer more functionality that we'd export in the > future? > I've just noticed that I can't spell performance either which is embarrassing :) Originally I developed this code for a thermal modes feature - but this portion of it is also needed for WWAN so we pulled out just this piece as the first bit to get in. Having WWAN available for users is more important than the thermal mode interface (there are a lot of users who want WWAN working properly on our laptops). So...yes, DYTC does offer more functionality and I'm planning on proposing the thermal patch as soon as this one makes it through, but I agree that in the context of this patch the comment is misleading. I will clean that up for this version. >> + >> +#define DYTC_CMD_GET 2 /*To get current IC function and mode*/ > > For this comment and all the ones below, space after "/*" and before "*/" > Ack >> +#define DYTC_GET_ENABLE_MASK 0x1 /*0 = disabled, 1 = enabled*/ > > Is that necessary? > Another hangover in that the other fields used for the thermal mode have more interesting masks and this fitted in with that. I can remove for now if it's really a problem. >> +#define DYTC_GET_LAPMODE_SHIFT 17 > > You'd probably want to call this "bit" rather than shift. We shift it to read > the value, but 17 is the bit's position. (See BIT() usage in the driver) > Ack > Do you want to add a comment here? Is there anything else that could be > documented (the other bits, which versions of firmware would have that, if > there's a publicly available version, or which hardware if publicly available) > So what is the preference normally? More pieces will definitely be made public when I release the thermal mode updates but I assumed we kept things related only to the code used. I can add more detail here if that helps. Not trying to hide anything :) >> +static int dytc_lapmode; >> +static void dytc_lapmode_notify_change(void) >> +{ >> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, >> + "dytc_lapmode"); >> +} >> + >> +static int dytc_command(int command) >> +{ >> + acpi_handle dytc_handle; >> + int output; >> + >> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) { >> + /*Platform doesn't support DYTC*/ >> + return -ENODEV; >> + } >> + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command)) >> + return -EIO; >> + return output; >> +} >> + >> +static int dytc_lapmode_get(void) >> +{ >> + int output; >> + >> + output = dytc_command(DYTC_CMD_GET); >> + if ((output == -ENODEV) || (output == -EIO)) >> + return output; >> + >> + return ((output >> DYTC_GET_LAPMODE_SHIFT) & >> + DYTC_GET_ENABLE_MASK); > > Use BIT() instead? eg. > return (output & BIT(DYTC_GET_LAPMODE_SHIFT)); > Ack >> +} >> + >> +static void dytc_lapmode_refresh(void) >> +{ >> + int new_state; >> + >> + new_state = dytc_lapmode_get(); >> + if ((new_state == -ENODEV) || (new_state == -EIO)) >> + return; > > You could also return early if "dytc_lapmode == new_state". > Good point. > Rest looks good to me. > Great - thanks for the review. I'll prepare the updates and if there's any feedback on the questions above please let me know Mark
----- Original Message ----- > Hi Bastien > > On 6/19/2020 9:52 AM, Bastien Nocera wrote: > > Hey Mark, > > > > ----- Original Message ----- > <snip> > >> > >> +/************************************************************************* > >> + * DYTC subdriver, for the Lenovo performace mode feature > >> + */ > > > > I don't think this should mention the performance mode, as it's a lap/table > > detection mode. Do we need to call that "DYTC"? "lapmode"? "lap_detection"? > > Or does the DYTC interface offer more functionality that we'd export in the > > future? > > > I've just noticed that I can't spell performance either which is > embarrassing :) > > Originally I developed this code for a thermal modes feature - but > this portion of it is also needed for WWAN so we pulled out just this > piece as the first bit to get in. Having WWAN available for users is > more important than the thermal mode interface (there are a lot of users > who want WWAN working properly on our laptops). > > So...yes, DYTC does offer more functionality and I'm planning on > proposing the thermal patch as soon as this one makes it through, but I > agree that in the context of this patch the comment is misleading. I > will clean that up for this version. If you can only propose this patch right now, maybe change the label, and change it again when the functionality is expanded? I'm just thinking that we want to avoid a case where the comments mention a performance mode, but we're looking at a lap detection instead. > >> + > >> +#define DYTC_CMD_GET 2 /*To get current IC function and mode*/ > > > > For this comment and all the ones below, space after "/*" and before "*/" > > > Ack > > >> +#define DYTC_GET_ENABLE_MASK 0x1 /*0 = disabled, 1 = enabled*/ > > > > Is that necessary? > > > Another hangover in that the other fields used for the thermal mode have > more interesting masks and this fitted in with that. I can remove for > now if it's really a problem. > > >> +#define DYTC_GET_LAPMODE_SHIFT 17 > > > > You'd probably want to call this "bit" rather than shift. We shift it to > > read > > the value, but 17 is the bit's position. (See BIT() usage in the driver) > > > Ack > > Do you want to add a comment here? Is there anything else that could be > > documented (the other bits, which versions of firmware would have that, if > > there's a publicly available version, or which hardware if publicly > > available) > > > So what is the preference normally? More pieces will definitely be made > public when I release the thermal mode updates but I assumed we kept > things related only to the code used. I can add more detail here if that > helps. Not trying to hide anything :) A comment with an explanation of what all the bits correspond to would be very useful, it would allow the community to extend the driver, if the functionality offered is deemed useful. > >> +static int dytc_lapmode; > >> +static void dytc_lapmode_notify_change(void) > >> +{ > >> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, > >> + "dytc_lapmode"); > >> +} > >> + > >> +static int dytc_command(int command) > >> +{ > >> + acpi_handle dytc_handle; > >> + int output; > >> + > >> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) { > >> + /*Platform doesn't support DYTC*/ > >> + return -ENODEV; > >> + } > >> + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command)) > >> + return -EIO; > >> + return output; > >> +} > >> + > >> +static int dytc_lapmode_get(void) > >> +{ > >> + int output; > >> + > >> + output = dytc_command(DYTC_CMD_GET); > >> + if ((output == -ENODEV) || (output == -EIO)) > >> + return output; > >> + > >> + return ((output >> DYTC_GET_LAPMODE_SHIFT) & > >> + DYTC_GET_ENABLE_MASK); > > > > Use BIT() instead? eg. > > return (output & BIT(DYTC_GET_LAPMODE_SHIFT)); > > > Ack > >> +} > >> + > >> +static void dytc_lapmode_refresh(void) > >> +{ > >> + int new_state; > >> + > >> + new_state = dytc_lapmode_get(); > >> + if ((new_state == -ENODEV) || (new_state == -EIO)) > >> + return; > > > > You could also return early if "dytc_lapmode == new_state". > > > Good point. > > > Rest looks good to me. > > > Great - thanks for the review. I'll prepare the updates and if there's > any feedback on the questions above please let me know > > Mark > >
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 0f704484ae1d..8f51bbba21cd 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -4049,6 +4049,7 @@ static bool hotkey_notify_6xxx(const u32 hkey, pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n"); /* recommended action: do nothing, we don't have * Lenovo ATM information */ + tpacpi_driver_event(hkey); return true; case TP_HKEY_EV_THM_TRANSFM_CHANGED: pr_debug("EC reports: Thermal Transformation changed (GMTS)\n"); @@ -9811,6 +9812,110 @@ static struct ibm_struct lcdshadow_driver_data = { .write = lcdshadow_write, }; +/************************************************************************* + * DYTC subdriver, for the Lenovo performace mode feature + */ + +#define DYTC_CMD_GET 2 /*To get current IC function and mode*/ + +#define DYTC_GET_ENABLE_MASK 0x1 /*0 = disabled, 1 = enabled*/ +#define DYTC_GET_LAPMODE_SHIFT 17 + +static int dytc_lapmode; +static void dytc_lapmode_notify_change(void) +{ + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, + "dytc_lapmode"); +} + +static int dytc_command(int command) +{ + acpi_handle dytc_handle; + int output; + + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) { + /*Platform doesn't support DYTC*/ + return -ENODEV; + } + if (!acpi_evalf(dytc_handle, &output, NULL, "dd", command)) + return -EIO; + return output; +} + +static int dytc_lapmode_get(void) +{ + int output; + + output = dytc_command(DYTC_CMD_GET); + if ((output == -ENODEV) || (output == -EIO)) + return output; + + return ((output >> DYTC_GET_LAPMODE_SHIFT) & + DYTC_GET_ENABLE_MASK); +} + +static void dytc_lapmode_refresh(void) +{ + int new_state; + + new_state = dytc_lapmode_get(); + if ((new_state == -ENODEV) || (new_state == -EIO)) + return; + + if (dytc_lapmode != new_state) { + dytc_lapmode = new_state; + dytc_lapmode_notify_change(); + } +} + +/* sysfs lapmode entry */ +static ssize_t dytc_lapmode_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + if (dytc_lapmode < 0) + return dytc_lapmode; + + return snprintf(buf, PAGE_SIZE, "%d\n", dytc_lapmode); +} + +static DEVICE_ATTR_RO(dytc_lapmode); + +static struct attribute *dytc_attributes[] = { + &dev_attr_dytc_lapmode.attr, + NULL +}; + +static const struct attribute_group dytc_attr_group = { + .attrs = dytc_attributes, +}; + +static int tpacpi_dytc_init(struct ibm_init_struct *iibm) +{ + int res; + + dytc_lapmode = dytc_lapmode_get(); + + if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV) + return dytc_lapmode; + + res = sysfs_create_group(&tpacpi_pdev->dev.kobj, + &dytc_attr_group); + + return res; +} + +static void dytc_exit(void) +{ + sysfs_remove_group(&tpacpi_pdev->dev.kobj, + &dytc_attr_group); +} + +static struct ibm_struct dytc_driver_data = { + .name = "dytc", + .exit = dytc_exit +}; + /**************************************************************************** **************************************************************************** * @@ -9858,6 +9963,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event) mutex_unlock(&kbdlight_mutex); } + + if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) + dytc_lapmode_refresh(); + } static void hotkey_driver_event(const unsigned int scancode) @@ -10296,6 +10405,10 @@ static struct ibm_init_struct ibms_init[] __initdata = { .init = tpacpi_lcdshadow_init, .data = &lcdshadow_driver_data, }, + { + .init = tpacpi_dytc_init, + .data = &dytc_driver_data, + }, }; static int __init set_ibm_param(const char *val, const struct kernel_param *kp)