Messages in this thread | | | Date | Tue, 13 Sep 2022 09:47:47 +0200 | Subject | Re: [PATCH 1/4] Bluetooth: btusb: mediatek: use readx_poll_timeout instead of open coding | From | AngeloGioacchino Del Regno <> |
| |
Il 13/09/22 00:18, sean.wang@mediatek.com ha scritto: > From: Sean Wang <sean.wang@mediatek.com> > > Use readx_poll_timeout instead of open coding to poll the hardware reset > status until it is done. > > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Hello Sean, thanks for the patch! However, there's something to improve...
> --- > drivers/bluetooth/btusb.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index c3daba17de7f..4dc9cae3e937 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c
..snip..
> @@ -2910,18 +2918,14 @@ static void btusb_mtk_cmd_timeout(struct hci_dev *hdev) > btusb_mtk_uhw_reg_write(data, MTK_BT_SUBSYS_RST, 0); > btusb_mtk_uhw_reg_read(data, MTK_BT_SUBSYS_RST, &val); > > - /* Poll the register until reset is completed */ > - do { > - btusb_mtk_uhw_reg_read(data, MTK_BT_MISC, &val); > - if (val & MTK_BT_RST_DONE) { > - bt_dev_dbg(hdev, "Bluetooth Reset Successfully"); > - break; > - } > + err = readx_poll_timeout(btusb_mtk_reset_done, hdev, val, > + val & MTK_BT_RST_DONE, > + 100000, 1000000);
I agree with using readx_poll_timeout() instead of open coding the same, but there's a catch: this macro uses usleep_range(), which is meant to be used for sleeping less than ~20ms.
Even the kerneldoc at include/linux/iopoll.h advertises that:
* @sleep_us: Maximum time to sleep between reads in us (0 * tight-loops). Should be less than ~20ms since usleep_range * is used (see Documentation/timers/timers-howto.rst).
So, if there's any reason for which you can't sleep for less than 100ms per iteration, I'm afraid that you can't use readx_poll_timeout()... ...otherwise, please change sleep_us to 20000 and keep the timeout at 1 sec.
Regards, Angelo
> + if (err < 0) > + bt_dev_err(hdev, "Reset timeout"); > > - bt_dev_dbg(hdev, "Polling Bluetooth Reset CR"); > - retry++; > - msleep(MTK_BT_RESET_WAIT_MS); > - } while (retry < MTK_BT_RESET_NUM_TRIES); > + if (val & MTK_BT_RST_DONE) > + bt_dev_dbg(hdev, "Bluetooth Reset Successfully"); > > btusb_mtk_id_get(data, 0x70010200, &val); > if (!val)
| |