Thanks Andy
On 7/2/2020 5:29 AM, Andy Shevchenko wrote:
On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson <markpearson@xxxxxxxxxx> 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
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
