Hi Luiz,
On Thu, 2021-12-16 at 16:47 -0800, Luiz Augusto von Dentz wrote:
> Hi Tedd,
>
> On Thu, Dec 16, 2021 at 4:04 PM Tedd Ho-Jeong An <hj.tedd.an@xxxxxxxxx> wrote:
> >
> > From: Tedd Ho-Jeong An <tedd.an@xxxxxxxxx>
> >
> > This patch fixes the broken LED quirk for Intel legacy ROM devices.
> > To fix the LED issue that doesn't turn off immediately, the host sends
> > the SW RFKILL command while shutting down the interface and it puts the
> > devices in an asserted state.
> >
> > Once the device is in SW RFKILL state, it can only accept HCI_Reset to
> > exit from the SW RFKILL state. This patch checks the quirk and sends the
> > HCI_Reset before sending the HCI_Intel_Read_Version command.
> >
> > The affected legacy ROM devices are
> > - 8087:0a2a
> > - 8087:0aa7
> >
> > fixes: ffcba827c0a1d ("Bluetooth: btintel: Fix the LED is not turning off immediately")
> >
> > Signed-off-by: Tedd Ho-Jeong An <tedd.an@xxxxxxxxx>
> > ---
> > drivers/bluetooth/btintel.c | 13 ++++++-------
> > drivers/bluetooth/btusb.c | 10 ++++++++--
> > 2 files changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index e1f96df847b8..75f8d7aceb35 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -2355,8 +2355,13 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> > * As a workaround, send HCI Reset command first which will reset the
> > * number of completed commands and allow normal command processing
> > * from now on.
> > + *
> > + * For INTEL_BROKEN_LED, these devices have an issue with LED which
> > + * doesn't go off immediately during shutdown. Set the flag here to send
> > + * the LED OFF command during shutdown.
> > */
> > - if (btintel_test_flag(hdev, INTEL_BROKEN_INITIAL_NCMD)) {
> > + if (btintel_test_flag(hdev, INTEL_BROKEN_INITIAL_NCMD) ||
> > + btintel_test_flag(hdev, INTEL_BROKEN_LED)) {
> > skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL,
> > HCI_INIT_TIMEOUT);
> > if (IS_ERR(skb)) {
> > @@ -2428,12 +2433,6 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> > set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
> > &hdev->quirks);
> >
> > - /* These devices have an issue with LED which doesn't
> > - * go off immediately during shutdown. Set the flag
> > - * here to send the LED OFF command during shutdown.
> > - */
> > - btintel_set_flag(hdev, INTEL_BROKEN_LED);
> > -
> > err = btintel_legacy_rom_setup(hdev, &ver);
> > break;
> > case 0x0b: /* SfP */
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index d1bd9ee0a6ab..c6a070d5284f 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -60,6 +60,7 @@ static struct usb_driver btusb_driver;
> > #define BTUSB_WIDEBAND_SPEECH 0x400000
> > #define BTUSB_VALID_LE_STATES 0x800000
> > #define BTUSB_QCA_WCN6855 0x1000000
> > +#define BTUSB_INTEL_BROKEN_LED 0x2000000
>
> I wonder why we haven't been using BIT macro here and did we make a
> mistake and leave one bit behind? Or something else was at this bit
> position?
There used to be a flag BTUSB_INTEL_NEWGEN, and it was removed while
refactoring the btintel, and the NCMD was added before removing the
NEWGEN flag.
>
> > #define BTUSB_INTEL_BROKEN_INITIAL_NCMD 0x4000000
> >
> > static const struct usb_device_id btusb_table[] = {
> > @@ -382,9 +383,11 @@ static const struct usb_device_id blacklist_table[] = {
> > { USB_DEVICE(0x8087, 0x07da), .driver_info = BTUSB_CSR },
> > { USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL_COMBINED |
> > BTUSB_INTEL_BROKEN_INITIAL_NCMD },
> > - { USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED },
> > + { USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED |
> > + BTUSB_INTEL_BROKEN_LED },
> > { USB_DEVICE(0x8087, 0x0a2b), .driver_info = BTUSB_INTEL_COMBINED },
> > - { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL_COMBINED },
> > + { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL_COMBINED |
> > + BTUSB_INTEL_BROKEN_LED },
> > { USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_COMBINED },
> >
> > /* Other Intel Bluetooth devices */
> > @@ -3724,6 +3727,9 @@ static int btusb_probe(struct usb_interface *intf,
> >
> > if (id->driver_info & BTUSB_INTEL_BROKEN_INITIAL_NCMD)
> > btintel_set_flag(hdev, INTEL_BROKEN_INITIAL_NCMD);
> > +
> > + if (id->driver_info & BTUSB_INTEL_BROKEN_LED)
> > + btintel_set_flag(hdev, INTEL_BROKEN_LED);
>
> I wonder if wouldn't be simples to have a flag e.g. INTEL_NEED_RESET
> instead of replicating the quirks as flags since in either case the
> actual outcome is to send a reset.
The result is same though, I think it is better to be more specific.
I am fine with it also.
>
> > }
> >
> > if (id->driver_info & BTUSB_MARVELL)
> > --
> > 2.25.1
> >
>
>
