Hi Greg,
On 6/6/22 08:48, Greg KH wrote:
- rtw_read8(padapter, REG_FMETHR);
+ /* FIXME: should this read be removed? */
+ res = rtw_read8(padapter, REG_FMETHR, ®);
+ (void)res;
What is that? We don't do "empty" lines like this in the kernel for no
good reason. If the return value must be checked, then that's fine, but
don't do it this way. Shouldn't the function itself return an error?
And reading a value is sometimes required by hardware in order to have
the write call go through. But that's for PCI devices, not normally USB
devices, but we could be wrong. I wouldn't put a FIXME in here unless
you have some plan for how to eventually solve it, otherwise someone
might just drop it without knowing why the FIXME was ever added.
Ok, I will just make this function return an error. Thanks
[snip]
diff --git a/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c b/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c
index b944c8071a3b..a5b7980dfcee 100644
--- a/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c
+++ b/drivers/staging/r8188eu/hal/HalPhyRf_8188e.c
@@ -463,6 +463,7 @@ void _PHY_SaveADDARegisters(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup
}
}
+/* FIXME: return an error to caller */
When are these FIXME going to be resolved? I don't like adding them for
no good reason.
After this series will go in. I really don't want to make this series
huge, since ideally read errors should be passed up to callers. This
driver has a lot of very deep call-chains, so fixing them in one series
is just unreal
I have a plan to address these FIXMEs, that's why I've planted them.
thanks for review,
With regards,
Pavel Skripkin
