Hi Alain,
>>> This change intruduces a wide band speech setting which allows higher
>>> level clients to query the local controller support for wide band speech
>>> as well as set the setting state when the radio is powered off.
>>> Internally, this setting controls if erroneous data reporting is enabled
>>> on the controller.
>>>
>>> Signed-off-by: Alain Michaud <alainm@xxxxxxxxxxxx>
>>> ---
>>>
>>> include/net/bluetooth/hci.h | 14 ++++++++
>>> include/net/bluetooth/hci_core.h | 1 +
>>> include/net/bluetooth/mgmt.h | 2 ++
>>> net/bluetooth/hci_core.c | 21 +++++++++++
>>> net/bluetooth/hci_event.c | 39 ++++++++++++++++++++
>>> net/bluetooth/mgmt.c | 62 ++++++++++++++++++++++++++++++++
>>> 6 files changed, 139 insertions(+)
>>>
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index 0b3ebd35681d..d66648e9ff13 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -286,6 +286,7 @@ enum {
>>> HCI_FAST_CONNECTABLE,
>>> HCI_BREDR_ENABLED,
>>> HCI_LE_SCAN_INTERRUPTED,
>>> + HCI_WIDE_BAND_SPEECH_ENABLED,
>>
>> lets use HCI_WIDEBAND_SPEECH_ENABLED. It might be just me, but when I look at WIDE_BAND compared to WIDEBAND, I prefer the latter.
> Ack.
>>
>>>
>>> HCI_DUT_MODE,
>>> HCI_VENDOR_DIAG,
>>> @@ -1095,6 +1096,19 @@ struct hci_rp_read_inq_rsp_tx_power {
>>> __s8 tx_power;
>>> } __packed;
>>>
>>> +#define HCI_OP_READ_DEF_ERR_DATA_REPORTING 0x0c5a
>>> + #define ERR_DATA_REPORTING_DISABLED 0x00
>>> + #define ERR_DATA_REPORTING_ENABLED 0x01
>>> +struct hci_rp_read_def_err_data_reporting {
>>> + __u8 status;
>>> + __u8 err_data_reporting;
>>> +} __packed;
>>> +
>>> +#define HCI_OP_WRITE_DEF_ERR_DATA_REPORTING 0x0c5b
>>> +struct hci_cp_write_def_err_data_reporting {
>>> + __u8 err_data_reporting;
>>> +} __packed;
>>> +
>>> #define HCI_OP_SET_EVENT_MASK_PAGE_2 0x0c63
>>>
>>> #define HCI_OP_READ_LOCATION_DATA 0x0c64
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index dcc0dc6e2624..c498ac113930 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -260,6 +260,7 @@ struct hci_dev {
>>> __u8 stored_num_keys;
>>> __u8 io_capability;
>>> __s8 inq_tx_power;
>>> + __u8 err_data_reporting;
>>> __u16 page_scan_interval;
>>> __u16 page_scan_window;
>>> __u8 page_scan_type;
>>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>>> index f69f88e8e109..b203df07e7fc 100644
>>> --- a/include/net/bluetooth/mgmt.h
>>> +++ b/include/net/bluetooth/mgmt.h
>>> @@ -672,6 +672,8 @@ struct mgmt_cp_set_blocked_keys {
>>> } __packed;
>>> #define MGMT_OP_SET_BLOCKED_KEYS_SIZE 2
>>>
>>> +#define MGMT_OP_SET_WIDE_BAND_SPEECH 0x0047
>>> +
>>> #define MGMT_EV_CMD_COMPLETE 0x0001
>>> struct mgmt_ev_cmd_complete {
>>> __le16 opcode;
>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> index 4e6d61a95b20..d526d7568396 100644
>>> --- a/net/bluetooth/hci_core.c
>>> +++ b/net/bluetooth/hci_core.c
>>> @@ -603,6 +603,9 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt)
>>> if (hdev->commands[8] & 0x01)
>>> hci_req_add(req, HCI_OP_READ_PAGE_SCAN_ACTIVITY, 0, NULL);
>>>
>>> + if (hdev->commands[18] & 0x02)
>>> + hci_req_add(req, HCI_OP_READ_DEF_ERR_DATA_REPORTING, 0, NULL);
>>> +
>>> /* Some older Broadcom based Bluetooth 1.2 controllers do not
>>> * support the Read Page Scan Type command. Check support for
>>> * this command in the bit mask of supported commands.
>>> @@ -838,6 +841,24 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
>>> sizeof(support), &support);
>>> }
>>>
>>> + /* Set erroneous data reporting if supported to the wideband speech
>>> + * setting value
>>> + */
>>> + if (test_bit(HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED, &hdev->quirks) &&
>>> + (hdev->commands[18] & 0x04) &&
>>> + hci_dev_test_flag(hdev, HCI_WIDE_BAND_SPEECH_ENABLED) !=
>>> + (hdev->err_data_reporting != ERR_DATA_REPORTING_ENABLED)) {
>>
>> So this statement is creating a knot in my brain ;)
>>
>>
>> if (hdev->commands[18] & 0x04) {
>> bool enabled = hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED);
>>
>> ..
>>
>> cp.err_data_reporting = enabled ? ERR_DATA_REPORTING_ENABLED :
>> ERR_DATA_REPORTING_DISABLED;
>>
>> ..
>> }
>>
>> I would not bother checking the quirk here. I would just configure that setting based on the HCI_WIDEBAND_SPEECH_ENABLED flag. Unless we have controllers that report this command and don’t allow setting the value, then we have to deal with it, but right now I assume that they get the basics of HCI right.
>>
>> Btw. I am debating a coding style here.
>>
>> cp.err_data_reporting = enabled ? ERR_DATA_REPORTING_ENABLED
>> : ERR_DATA_REPORTING_DISABLED;
>>
>> But I am not even sure that is clearer or cleaner.
>
> I agree the QUIRK check could be avoided, but I'd argue the following
> condition is still required to avoid sending a command that otherwise
> is not necessary to the controller:
> hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED) !=
> (hdev->err_data_reporting != ERR_DATA_REPORTING_ENABLED). I can
> however simplify it by using a bool enabled flag as you suggested.
ok, then do that.
Regards
Marcel
