Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Marcel Holtmann <marcel@holtmann.org>]

Hi Balakrishna,

>>>>>> During hci down we are sending reset command to chip, which
>>>>>> is not required for wcn3990, as hdev->shutdown() will turn off the
>>>>>> regulators.
>>>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>>>>> ---
>>>>>> drivers/bluetooth/hci_qca.c | 1 +
>>>>>> 1 file changed, 1 insertion(+)
>>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>>> index 8301663f0004..97b57e0f4725 100644
>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>>> 		 */
>>>>>> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>>>>> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>>>>> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>>>>>> 		hu->hdev->shutdown = qca_power_off;
>>>>>> 		ret = qca_wcn3990_init(hu);
>>>>>> 		if (ret)
>>>>> I am pretty certain that you didn’t want this quirk:
>>>>>       /* When this quirk is set, the HCI Reset command is send when
>>>>>        * closing the transport instead of when opening it.
>>>>> This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior
>>>>> was not clear or for devices that actually misbehave with the initial
>>>>> HCI_Reset.
>>>>> In addition, you commit message is totally misleading. That is not
>>>>> what is happening with this quirk.
>>>>> Regards
>>>>> Marcel
>>>> My intention was reset command is not required when we do an hci down.
>>>> this is because of hdev->shutdown will turn off the regulators.
>>>> It is like turning off the chip. sending reset command after turning off the chip is not required.
>>>> I understand the usage of the quirk, will update the commit text.
>>> you are papering over the issue. Actually
>>> hci_serdev.c:hci_uart_register_device() is the culprit with the legacy
>>> code copied over from hci_ldisc.c:hci_uart_register_dev(). I think
>>> there is no point doing all this legacy line discipline quirk handling
>>> until it is really needed. The serdev drivers are all for recent
>>> hardware.
>>> That said, having moved over to a btuart.c approach and killed the
>>> whole hci_serdev.c thing would have been a lot better here. You will
>>> keep running in weird situations where 18 year old code keeps
>>> surprising you.
>> [Bala]: even i feel the same. they are lot such kind of HACK's we need
>> to do with current arch.
>>        when can we expect btuart.c merged to bt-next. i think having
>> btuart will helps us to have the control of
>>        vendor porto's call's like in btusb.c
>>> Regards
>>> Marcel
> 
> I need some clarification, do you expect some thing like this https://github.com/torvalds/linux/blob/master/drivers/bluetooth/btmtkuart.c for Qualcomm BT chip too.
> it looks it is completely avoided hci_serdev.c interface.

you tell me actually. Are you using the H:4 transport or do you have an extra protocol layer / framing below it. If you do, then use your own driver, but if the transport is H:4 with vendor packets and vendor setup, then btuart.c (which is not yet upstream) should be your target.

For the MTK hardware it was obvious that it was better served as a separate driver. For QCA serial it really depends on how much extra protocol you have to run. So this might be an exercise in trying QCA serial as a separate driver and then go from there.

It is clear that the baggage from hci_ldisc.c etc is in the way for serdev based systems.

Regards

Marcel