| Message ID | E1sBsxx-00E8vi-Gf@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dd265a7415f8b99fa9b4b055f1ad1015c1e6f250 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: TI wilink8 updates | expand |
On 5/28/2024 12:17 PM, Russell King (Oracle) wrote: > wlcore_fw_status() is passed a pointer to the struct wl_fw_status to > decode the status into, which is always wl->fw_status. Rather than > referencing wl->fw_status within wlcore_fw_status(), use the supplied > argument so that we access this member in a consistent manner. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/wireless/ti/wlcore/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c > index a98b26dc3cb8..3defe49c5120 100644 > --- a/drivers/net/wireless/ti/wlcore/main.c > +++ b/drivers/net/wireless/ti/wlcore/main.c > @@ -392,7 +392,7 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status) > if (ret < 0) > return ret; > > - wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, wl->fw_status); > + wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, status); > > wl1271_debug(DEBUG_IRQ, "intr: 0x%x (fw_rx_counter = %d, " > "drv_rx_counter = %d, tx_results_counter = %d)", > -- > 2.30.2 Agree this is more consistent. Maybe *status shouldn't be an argument to wlcore_fw_status at all? It's called only in one place with wl->fw_status anyway. Michael.
On Wed, May 29, 2024 at 04:15:13PM +0300, Nemanov, Michael wrote: > On 5/28/2024 12:17 PM, Russell King (Oracle) wrote: > > @@ -392,7 +392,7 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status) > > if (ret < 0) > > return ret; > > - wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, wl->fw_status); > > + wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, status); > > wl1271_debug(DEBUG_IRQ, "intr: 0x%x (fw_rx_counter = %d, " > > "drv_rx_counter = %d, tx_results_counter = %d)", > > -- > > 2.30.2 > > Agree this is more consistent. Maybe *status shouldn't be an argument to > wlcore_fw_status at all? It's called only in one place with wl->fw_status > anyway. I did consider that, and if we removed the argument, it would make sense to add a local "status" variable at the top of this function anyway, otherwise endlessly referring to wl->fw_status.foo instead of status->foo becomes quite tiring and needlessly verbose (which means less readable.) That's something which could be done as a separate patch.
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c index a98b26dc3cb8..3defe49c5120 100644 --- a/drivers/net/wireless/ti/wlcore/main.c +++ b/drivers/net/wireless/ti/wlcore/main.c @@ -392,7 +392,7 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status) if (ret < 0) return ret; - wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, wl->fw_status); + wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, status); wl1271_debug(DEBUG_IRQ, "intr: 0x%x (fw_rx_counter = %d, " "drv_rx_counter = %d, tx_results_counter = %d)",
wlcore_fw_status() is passed a pointer to the struct wl_fw_status to decode the status into, which is always wl->fw_status. Rather than referencing wl->fw_status within wlcore_fw_status(), use the supplied argument so that we access this member in a consistent manner. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/wireless/ti/wlcore/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)