| Message ID | 20200629191748.3859-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v4] platform/x86: thinkpad_acpi: lap or desk mode interface | expand |
----- 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> You can add my: Reviewed-by: Bastien Nocera <bnocera@redhat.com> Cheers
On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson@lenovo.com> wrote: Thanks for the patch, my comments below. > 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 Please use proper indentation (no need to have spaces) and punctuation (like period at the end of sentences). ... > drivers/platform/x86/thinkpad_acpi.c | 111 ++++++++++++++++++++++++++- > 1 file changed, 109 insertions(+), 2 deletions(-) You specifically added a new ABI, where is documentation? It's a show stopper. ... > + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, > + "dytc_lapmode"); One line? ... > + int output; > + > + output = dytc_command(DYTC_CMD_GET); > + if ((output == -ENODEV) || (output == -EIO)) > + return output; Why not simple if (output < 0) return output; Btw, how will this survive the 31st bit (if some method would like to use it)? I think your prototype should be int foo(cmd, *output); ... > + new_state = dytc_lapmode_get(); > + if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode)) > + return; What about other errors? What about MSB being set? ... > + 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; return ...(...); So, we create a group for all possible error cases but ENODEV. Why? > +} ... > + sysfs_remove_group(&tpacpi_pdev->dev.kobj, > + &dytc_attr_group); One line? ... > +static struct ibm_struct dytc_driver_data = { > + .name = "dytc", > + .exit = dytc_exit Comma is missed. > +};
Thanks Andy On 7/2/2020 5:29 AM, Andy Shevchenko wrote: > On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson@lenovo.com> wrote: > > Thanks for the patch, my comments below. > >> 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 > > Please use proper indentation (no need to have spaces) and punctuation > (like period at the end of sentences). Ack > > ... > >> drivers/platform/x86/thinkpad_acpi.c | 111 ++++++++++++++++++++++++++- >> 1 file changed, 109 insertions(+), 2 deletions(-) > > You specifically added a new ABI, where is documentation? It's a show stopper. Ah - my apologies I didn't know that was a requirement. Any pointers on where to add it? I looked in Documentation/ABI and I couldn't find anything around thinkpad_acpi to add this to. Should there be a sysfs-devices-platform-thinkpad_acpi file? If that's the case I'm happy to look at creating that but as a first time kernel contributor would you object if I took that on as a separate exercise rather than as part of this patch. I'm guessing it would need more time, care and reviewers from other contributors to the thinkpad_acpi.c driver > > ... > >> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, >> + "dytc_lapmode"); > > One line? Ack > > ... > >> + int output; >> + >> + output = dytc_command(DYTC_CMD_GET); > >> + if ((output == -ENODEV) || (output == -EIO)) >> + return output; > > Why not simple > > if (output < 0) > return output; Agreed. I'll fix > > Btw, how will this survive the 31st bit (if some method would like to use it)? Hmmm - good point. I'll revisit this. > > I think your prototype should be > > int foo(cmd, *output); Looking at it again - I agree. > > ... > >> + new_state = dytc_lapmode_get(); >> + if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode)) >> + return; > > What about other errors? > What about MSB being set? Ack - this needs fixing > > ... > >> + 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; > > return ...(...); > > So, we create a group for all possible error cases but ENODEV. Why? There seemed a good reason when I originally wrote it - but now I'm wondering why too. I specifically was catching the ENODEV because of concerns around creating the group on platforms that didn't have the support for this feature - but I think in doing that I missed the obvious of all errors. I'll revisit and fix. > >> +} > > ... > >> + sysfs_remove_group(&tpacpi_pdev->dev.kobj, >> + &dytc_attr_group); > > One line? Ack. As a minor note I think these all arose because of getting checkpatch to run cleanly. I prefer one line too and if that's your preference it works for me. > > ... > >> +static struct ibm_struct dytc_driver_data = { >> + .name = "dytc", >> + .exit = dytc_exit > > Comma is missed. Ack > >> +}; > I'll work on these and get an updated version out for review. Thank you for your time looking at these. Mark
On Thu, Jul 2, 2020 at 1:45 PM Mark Pearson <markpearson@lenovo.com> wrote: > On 7/2/2020 5:29 AM, Andy Shevchenko wrote: > > On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson@lenovo.com> wrote: ... > > You specifically added a new ABI, where is documentation? It's a show stopper. > Ah - my apologies I didn't know that was a requirement. > > Any pointers on where to add it? I looked in Documentation/ABI and I > couldn't find anything around thinkpad_acpi to add this to. > Should there be a sysfs-devices-platform-thinkpad_acpi file? > > If that's the case I'm happy to look at creating that but as a first > time kernel contributor would you object if I took that on as a separate > exercise rather than as part of this patch. I'm guessing it would need > more time, care and reviewers from other contributors to the > thinkpad_acpi.c driver Since it's an old driver its ABI is listed here https://elixir.bootlin.com/linux/latest/source/Documentation/admin-guide/laptops/thinkpad-acpi.rst ... > > Why not simple > > > > if (output < 0) > > return output; > Agreed. I'll fix > > I think your prototype should be > > > > int foo(cmd, *output); > Looking at it again - I agree. And after returning only error codes, you may do above as simple as int ret; ret = ...(.., &output); if (ret) return ret; ... return 0; ... > As a minor note I think these all arose because of getting checkpatch to > run cleanly. I prefer one line too and if that's your preference it > works for me. Checkpatch shouldn't complain (update it if it does).
Hello Bastien >-----Original Message----- >From: Bastien Nocera <bnocera@redhat.com> >----- 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> > > >You can add my: >Reviewed-by: Bastien Nocera <bnocera@redhat.com> It's already added in latest patch and currently in "for-next" http://git.infradead.org/linux-platform-drivers-x86.git/commit/acf7f4a59114471c3964f118564fe8e7a6f34bb8 Thanks
----- Original Message ----- > Hello Bastien > > >-----Original Message----- > >From: Bastien Nocera <bnocera@redhat.com> > > >----- 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> > > > > > >You can add my: > >Reviewed-by: Bastien Nocera <bnocera@redhat.com> > > It's already added in latest patch and currently in "for-next" > http://git.infradead.org/linux-platform-drivers-x86.git/commit/acf7f4a59114471c3964f118564fe8e7a6f34bb8 I sent my message nearly a month ago, 2 days before the authoring date of the patch that was merged, so I'm not sure what you're trying to tell me here :)
>-----Original Message----- >From: Bastien Nocera <bnocera@redhat.com> >Sent: Monday, July 27, 2020 8:04 PM >To: Nitin Joshi1 <njoshi1@lenovo.com> >> >> >> >> 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> >> > >> > >> >You can add my: >> >Reviewed-by: Bastien Nocera <bnocera@redhat.com> >> >> It's already added in latest patch and currently in "for-next" >> http://git.infradead.org/linux-platform-drivers-x86.git/commit/acf7f4a >> 59114471c3964f118564fe8e7a6f34bb8 > >I sent my message nearly a month ago, 2 days before the authoring date of >the patch that was merged, so I'm not sure what you're trying to tell me >here :) Sorry , please ignore this e-mail . Don’t know why it came up on my mailbox today . But its my mistake , I would have seen date when this e-mail was sent :) Apologize for inconvenience caused. Thanks & Regards, Nitin Joshi
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 0f704484ae1d..859b40c7113e 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -4047,8 +4047,8 @@ static bool hotkey_notify_6xxx(const u32 hkey, return true; case TP_HKEY_EV_THM_CSM_COMPLETED: pr_debug("EC reports: Thermal Control Command set completed (DYTC)\n"); - /* recommended action: do nothing, we don't have - * Lenovo ATM information */ + /* Thermal event - pass on to event handler */ + tpacpi_driver_event(hkey); return true; case TP_HKEY_EV_THM_TRANSFM_CHANGED: pr_debug("EC reports: Thermal Transformation changed (GMTS)\n"); @@ -9811,6 +9811,105 @@ static struct ibm_struct lcdshadow_driver_data = { .write = lcdshadow_write, }; +/************************************************************************* + * DYTC subdriver, for the Lenovo lapmode feature + */ + +#define DYTC_CMD_GET 2 /* To get current IC function and mode */ +#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */ + +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 & BIT(DYTC_GET_LAPMODE_BIT) ? 1 : 0); +} + +static void dytc_lapmode_refresh(void) +{ + int new_state; + + new_state = dytc_lapmode_get(); + if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state == dytc_lapmode)) + return; + + 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 +9957,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 +10399,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)