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.
> >
> >
> >> +
> >> + *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 */
...
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.
> >
> >
> >> + 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.
> >
> >
> >> + 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.
> >
> >
> >> + 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.
> >
> >> + 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
