Hi,
>-----Original Message-----
>From: Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx>
>Sent: Sunday, February 14, 2021 5:48 AM
>To: Nitin Joshi1 <njoshi1@xxxxxxxxxx>
>Cc: Nitin Joshi <nitjoshi@xxxxxxxxx>; hdegoede@xxxxxxxxxx; ibm-acpi-
>devel@xxxxxxxxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; Mark RH
>Pearson <markpearson@xxxxxxxxxx>
>Subject: RE: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs
>interface to reduce wlan tx power
>
>Hi
>
>
>2021. február 12., péntek 11:40 keltezéssel, Nitin Joshi1
><njoshi1@xxxxxxxxxx> írta:
>
>> [...]
>> >>
>>
>>+/**************************************************************
>*
>> >*****
>> >> +*****
>> >> + * DPRC(Dynamic Power Reduction Control) subdriver, for the Lenovo
>> >> +WWAN
>> >> + * and WLAN feature.
>> >> + */
>> >> +#define DPRC_GET_WLAN_STATE 0x20000
>> >> +#define DPRC_SET_WLAN_POWER_REDUCE 0x00030010
>> >> +#define DPRC_SET_WLAN_POWER_FULL 0x00030100
>> >> +static int has_wlantxreduce;
>> >
>> >I think `bool` would be better.
>>
>> Ack . I will modify it in next version.
>>
>> >
>> >
>> >> +static int wlan_txreduce;
>> >> +
>> >> +static int dprc_command(int command, int *output) {
>> >> + acpi_handle dprc_handle;
>> >> +
>> >> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DPRC",
>> >&dprc_handle))) {
>> >> + /* Platform doesn't support DPRC */
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + if (!acpi_evalf(dprc_handle, output, NULL, "dd", command))
>> >> + return -EIO;
>> >> +
>> >> + /*
>> >> + * METHOD_ERR gets returned on devices where few commands are
>> >not supported
>> >> + * for example WLAN power reduce command is not supported on
>> >some devices.
>> >> + */
>> >> + if (*output & METHOD_ERR)
>> >> + return -ENODEV;
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int get_wlan_state(int *has_wlantxreduce, int
>> >> +*wlan_txreduce) {
>> >> + int output, err;
>> >> +
>> >> + /* Get current WLAN Power Transmission state */
>> >> + err = dprc_command(DPRC_GET_WLAN_STATE, &output);
>> >> + if (err)
>> >> + return err;
>> >> +
>> >> + if (output & BIT(4))
>> >
>> >I believe it'd be preferable to name `BIT(4)` and `BIT(8)`. E.g.:
>> >
>> > #define DPRC_GET_WLAN_STATE_RES_REDUCED BIT(4)
>> > #define DPRC_GET_WLAN_STATE_RES_FULL BIT(8)
>> >
>> >(or any name you like).
>> >
>> Ack . I will modify it in next version.
>>
>> >
>> >> + *wlan_txreduce = 1;
>> >> + else if (output & BIT(8))
>> >> + *wlan_txreduce = 0;
>> >> + else
>> >> + *wlan_txreduce = -1;
>> >
>> >Can you elaborate what -1 means here? Unknown/not
>> >available/invalid/error?
>>
>> -1 means "error" .
>> I had found that when "DPRC" method return METHOD_ERR i.e BIT(31) as 0
>then it goes to this condition.
>> So , therefore I had added METHOD_ERR check in dprc_command() and
>now , this doesnot goes here.
>> But, I have still kept it here , just in case if any other error occurred .
>> Can you please suggest , if I should remove it (i.e remove *wlan_txreduce = -
>1; )? as I still think, there is no harm keeping like this.
>>
>
>If `dprc_command()` handles all error cases (i.e. it is not possible that
>`dprc_command()` returns 0, but there was an error) - which seems to be the
>case - then I think in that branch you should return -ENODATA and not touch
>`wlan_txreduce`. Because if that branch runs, then there was no error, but the
>state cannot be determined, so I think -ENODATA is an appropriate error code.
Ack . Noted, I will check it
>
>
>> >
>> >
>> >> +
>> >> + *has_wlantxreduce = 1;
>> >
>> >Is it necessary for the getter to set this? Couldn't it be set in
>> >`tpacpi_dprc_init()` once during initialization?
>>
>> Actually, yes we can keep it in init function also but I have not kept
>> it because of other patch (PATCH 2/2) which I had sent . patch 1 (this
>> patch) and patch 2 ( other patch which create sysfs of WWWAN Antenna
>type) both uses "DPRC" method . So , we will need a flag to create sysfs
>because few system will not have this "wlan tx reduce"
>> even if it has "DPRC" method exists and vice versa .
>> So , in this case, we need to explicitly check whether it require to create
>corresponding sysfs or not.
>>
>
>I was thinking of something like the following:
>
> static int tpacpi_dprc_init(...) {
> ...
> int err = get_wlan_state(&reduced);
> if (err && err != -ENODEV)
> return err;
> else if (!err)
> has_wlantxreduce = true;
>
> err = get_wwan_antenna(&antenna);
> if (err && err != -ENODEV)
> return err;
> else if (!err)
> has_antennatype = true; /* as I've commented on the second patch, this
> variable is probably not needed */
> ...
>
Ack . I will check it
>If I'm not mistaken this is enough not to need the second argument in either
>`get_wlan_state()` or `get_wwan_antenna()`. Note, that if both functions may
>return -ENODATA, you might also want to add `err != -ENODATA` to the
>conditions.
>
Ack . yes.
>
>> >
>> >
>> >> + return 0;
>> >> +}
>> >> +
>> >> +/* sysfs wlan entry */
>> >> +static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
>> >> + struct device_attribute *attr,
>> >> + char *buf)
>> >
>> >Please align the arguments:
>> >
>> > ..._show(struct device *dev,
>> > struct device_attribute ...
>> > ...);
>> >
>> Ack . I will modify it in next version.
>> Also , I will correct it in my other patch(PATCH 2/2) also.
>>
>> >
>> >> +{
>> >> + int err;
>> >> +
>> >> + err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> >> + if (err)
>> >> + return err;
>> >> +
>> >
>> >Wouldn't it be better to return -ENODATA or something similar when
>> >`wlan_txreduce` == -1?
>>
>> Ack . I think EOPNOTSUPP will be better ? reason is that "DPRC" method is
>available but there is error . So , its more likely that command is not
>supported.
>> However, I will modify it after I get feedback about my previous comment.
>>
>
>I think the place to determine whether the operation is supported or not is in
>`tpacpi_dprc_init()` and the attribute should not be created if it's not
>supported.
Ack . I will check it and send next patch version soon.
>
>
>> >
>> >
>> >> + return sysfs_emit(buf, "%d\n", wlan_txreduce); }
>> >> +
>> >> +static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>> >> + struct device_attribute *attr,
>> >> + const char *buf, size_t count)
>> >
>> >Same here.
>> >
>> Ack . I will modify it in next version.
>> >
>> >> +{
>> >> + int output, err;
>> >> + unsigned long t;
>> >> +
>> >> + if (parse_strtoul(buf, 1, &t))
>> >
>> >Maybe `kstrtobool`?
>>
>> Thank you for your suggestion.
>> I can use 'kstrtobool' and will modify on my next version.
>>
>> >
>> >
>> >> + return -EINVAL;
>> >> +
>> >> + tpacpi_disclose_usertask(attr->attr.name,
>> >> + "WLAN tx strength reduced %lu\n", t);
>> >> +
>> >> + switch (t) {
>> >> + case 0:
>> >> + err = dprc_command(DPRC_SET_WLAN_POWER_FULL,
>> >&output);
>> >> + break;
>> >> + case 1:
>> >> + err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE,
>> >&output);
>> >> + break;
>> >> + default:
>> >> + err = -EINVAL;
>> >> + dev_err(&tpacpi_pdev->dev, "Unknown operating mode.
>> >Ignoring\n");
>> >> + break;
>> >> + }
>> >> +
>> >
>> >If I'm not mistaken, `err` is never returned, so the write() will
>> >always seem to succeed.
>>
>> Yes , its correct . I will use 'kstrtobool' and will drop this : "err = -EINVAL;"
>> Is it Ok ?
>>
>
>If you use `kstrtobool()`, you can even drop the entire switch statement.
Ack . yes , I think so too.
>
>
>> >
>> >
>> >> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> >"wlan_tx_strength_reduce");
>> >> + return count;
>> >> +}
>> >> +static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
>> >> +
>> >> +static int tpacpi_dprc_init(struct ibm_init_struct *iibm) {
>> >> + int wlantx_err, err;
>> >> +
>> >> + wlantx_err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> >> + /*
>> >> + * If support isn't available (ENODEV) for both devices then quit, but
>> >> + * don't return an error.
>> >> + */
>> >> + if (wlantx_err == -ENODEV)
>> >> + return 0;
>> >> + /* Otherwise, if there was an error return it */
>> >> + if (wlantx_err && (wlantx_err != -ENODEV))
>> >> + return wlantx_err;
>> >> +
>> >> + if (has_wlantxreduce) {
>> >> + err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> >> + &dev_attr_wlan_tx_strength_reduce.attr);
>> >
>> >I believe `device_create_file()` would be better.
>> >
>> Since, file will be created in /sys/ directory , hence I think its better to use
>sysfs_create_file.
>> Also, by checking in this file, it looks like sysfs_create_file will be more
>reasonable to do .
>>
>> Please let me know, if its Ok to continue using sysfs_create_file or
>> you still feel. we need to use
>> device_create_file() also feel free to correct me, if I am wrong.
>>
>
>There's not much difference, so sysfs_{create,remove}_file() would work just
>as fine here. The reason why I believe `device_{create,remove}_file()` is
>possibly preferable is that you don't need to reference the kobj and the
>attribute when calling it, and you're adding an attribute to a *device* (not any
>kobj), so device_*() functions are a better fit syntactically in my opinion. But in
>the end, both will achieve the same effect, so you are free to choose
>whichever you like.
Ack . Noted.
I would like to keep sysfs_{ create,remove}_file() now.
I will recheck code, incorporate comments and send next patch version soon.
>
>> >
>> >> + if (err)
>> >> + return err;
>> >> + }
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static void dprc_exit(void)
>> >> +{
>> >> + if (has_wlantxreduce)
>> >> + sysfs_remove_file(&tpacpi_pdev->dev.kobj,
>> >> +&dev_attr_wlan_tx_strength_reduce.attr);
>> >
>> >And similarly, `device_remove_file()`.
>>
>> Same comment as above . I feel, sysfs_remove_file is more reasonable to do.
>> [...]
>
>
>Regards,
>Barnabás Pőcze
Thanks & Regards,
Nitin Joshi
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
