|
From: Mark P. <mar...@le...> - 2020-08-07 20:41:12
|
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 <and...@gm...>
>> Sent: Monday, July 27, 2020 7:35 PM
>> On Thu, Jul 16, 2020 at 2:53 AM Mark Pearson <mar...@le...>
>> 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
|
