Hi Andy,
First off apologies for my delay in replying and thanks to Nitin for
covering for me. I took a week of PTO and then suffered the consequences
of that for the week after - it's taken me a bit to catch-up.
On 7/27/2020 11:51 PM, Nitin Joshi1 wrote:
Hi Andy ,
-----Original Message-----
From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Sent: Monday, July 27, 2020 7:35 PM
On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <markpearson@xxxxxxxxxx>
wrote:
case TP_HKEY_EV_PALM_DETECTED:
case TP_HKEY_EV_PALM_UNDETECTED:
- /* palm detected hovering the keyboard, forward to user-space
- * via netlink for consumption */
+ /* palm detected - pass on to event handler */
+ tpacpi_driver_event(hkey);
return true;
Comment here tells something about the netlink interface to user space.
Can you elaborate why we need sysfs now and how it's all supposed to
work?
Using netlink , we were getting proximity events like '0x60b0' and '0x60b1' but for our WWAN requirement, we need default and current
p-sensor state too .
Some tools like "acpi-listen" uses netlink to display events but we need default and current p-sensor state also as per our requirement.
hence , we have added new sysfs to get current p-sensor state using 'GPSS' method from BIOS .
This will be used for implementing "Dynamic power reduction" app which is used to control Body SAR value as per FCC requirement .
When Body or any object is near or away from p-sensor location on thinkpad system , then sysfs will be set .
I think Nitin has covered it. Let us know if any follow on questions or
concerns here.
...
+static int psensor_get(bool *state)
+{
+ acpi_handle psensor_handle;
+ int output;
+
+ if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GPSS",
&psensor_handle)))
+ return -ENODEV;
+
+ if (!acpi_evalf(psensor_handle, &output, NULL, "d"))
+ return -EIO;
+
+ /* Check if sensor has a Psensor */
+ if (!(output & BIT(PSENSOR_PRESENT_BIT)))
+ return -ENODEV;
+
+ /* Return if psensor is set or not */
+ *state = output & BIT(PSENSOR_ON_BIT) ? true : false;
+ return 0;
+}
It reminds me of a function you created in one of the previous changes. Can
you rather create a parameterized helper which will serve for both?
Ack , we will check it .
I've been looking at this and I understand where you're coming from but
the benefits of combining them aren't working for me.
The previous change was for the lapmode sensor which is a separate piece
of hardware. The ACPI handle and access format is different and the
lapmode sensor has some extra handling logic needed. The code has enough
differences too that I don't think combining it makes a lot of sense.
Let me know if I'm missing something obvious or if you disagree.
...
+/* sysfs psensor entry */
+static ssize_t psensor_state_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf) {
+ return snprintf(buf, PAGE_SIZE, "%d\n", psensor_state);
We know that %d takes much less than PAGE_SIZE, use sprintf().
+}
+
No blank line here.
Ack
+static DEVICE_ATTR_RO(psensor_state);
...
+static struct attribute *psensor_attributes[] = {
+ &dev_attr_psensor_state.attr,
+ NULL,
No comma for terminator line(s).
Ack
+};
...
+ /* If support isn't available (ENODEV) then don't return an error
+ * but just don't create the sysfs group
+ */
/*
* Consider to use a proper multi-line comment style.
* Like here. (It's applicable to the entire patch) */
...
+ err = sysfs_create_group(&tpacpi_pdev->dev.kobj,
&psensor_attr_group);
+ return err;
return sysfs...
Ack
I'll push a patch soon with these other adjustments.
Thanks for the review
Mark
