Hi Abhishek,
>>> Add BCM vendor specific command to configure PCM parameters. The new
>>> vendor opcode allows us to set the sco routing, the pcm interface rate,
>>> and a few other pcm specific options (frame sync, sync mode, and clock
>>> mode). See broadcom-bluetooth.txt in Documentation for more information
>>> about valid values for those settings.
>>>
>>> Here is an example trace where this opcode was used to configure
>>> a BCM4354:
>>>
>>> < HCI Command: Vendor (0x3f|0x001c) plen 5
>>> 01 02 00 01 01
>>>> HCI Event: Command Complete (0x0e) plen 4
>>> Vendor (0x3f|0x001c) ncmd 1
>>> Status: Success (0x00)
>>>
>>> We can read back the values as well with ocf 0x001d to confirm the
>>> values that were set:
>>> $ hcitool cmd 0x3f 0x001d
>>> < HCI Command: ogf 0x3f, ocf 0x001d, plen 0
>>>> HCI Event: 0x0e plen 9
>>> 01 1D FC 00 01 02 00 01 01
>>>
>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
>>> ---
>>>
>>> Changes in v6: None
>>> Changes in v5: None
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>> drivers/bluetooth/btbcm.c | 47 +++++++++++++++++++++++++++++++++++++++
>>> drivers/bluetooth/btbcm.h | 16 +++++++++++++
>>> 2 files changed, 63 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
>>> index 2d2e6d862068..df90841d29c5 100644
>>> --- a/drivers/bluetooth/btbcm.c
>>> +++ b/drivers/bluetooth/btbcm.c
>>> @@ -105,6 +105,53 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>> }
>>> EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
>>>
>>> +int btbcm_read_pcm_int_params(struct hci_dev *hdev,
>>> + struct bcm_set_pcm_int_params *int_params)
>>> +{
>>
>> the name should be _param and not _params since if I remember correctly that is how Broadcom specified it. Also just use param as variable name.
>
> Technically, you are configuring multiple PCM params :)
I know and maybe they renamed the command internally by now. It is just when I read the Broadcom HCI vendor commands, it was named that way. Anyway, I am fine if you want to use _params and params argument variable name. Might make sense since we somehow named the struct that way as well and it is pre-existing.
>>> + struct sk_buff *skb;
>>> + int err = 0;
>>> +
>>> + skb = __hci_cmd_sync(hdev, 0xfc1d, 5, int_params, HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + err = PTR_ERR(skb);
>>> + bt_dev_err(hdev, "BCM: Read PCM int params failed (%d)", err);
>>> + return err;
>>> + }
>>> +
>>> + if (!skb->data[0] && skb->len == sizeof(*int_params) + 1) {
>>> + memcpy(int_params, &skb->data[1], sizeof(*int_params));
>>> + } else {
>>> + bt_dev_err(hdev,
>>> + "BCM: Read PCM int params failed (%d), Length (%d)",
>>> + skb->data[0], skb->len);
>>> + err = -EINVAL;
>>> + }
>>> +
>>> + kfree_skb(skb);
>>
>> I find these harder to read actually and it can be still fault at data[0] access.
>>
>> if (skb->len != sizeof(*param) || skb->data[0]) {
>> bt_dev_err(hdev, "BCM: Read SCO PCM int parameter failure");
>> kfree_skb(skb);
>> return -EIO;
>> }
>>
>> memcpy(param, skb->data + 1, sizeof(*param));
>> kfree_skb(skb);
>> return 0;
>> }
>>
>
> Sure. skb->len should be sizeof(*param) + 1 because there's an extra
> byte for the status as well.
Good point. I forgot about the status octet.
>
>>> +
>>> + return err;
>>> +}
>>> +EXPORT_SYMBOL_GPL(btbcm_read_pcm_int_params);
>>> +
>>> +int btbcm_write_pcm_int_params(struct hci_dev *hdev,
>>> + const struct bcm_set_pcm_int_params *int_params)
>>> +{
>>> + struct sk_buff *skb;
>>> + int err;
>>> +
>>> + /* Vendor ocf 0x001c sets the pcm parameters and 0x001d reads it */
>>
>> Scrap this comment.
>>
>>> + skb = __hci_cmd_sync(hdev, 0xfc1c, 5, int_params, HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + err = PTR_ERR(skb);
>>> + bt_dev_err(hdev, "BCM: Write PCM int params failed (%d)", err);
>>> + return err;
>>> + }
>>> + kfree_skb(skb);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(btbcm_write_pcm_int_params);
>>> +
>>> int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
>>> {
>>
>> Otherwise this looks good.
>>
>> Regards
>>
>> Marcel
>>
>
> So generally, I've done a whole new patch series with every change.
> Would you prefer to see singular updates on the same email thread or
> should I keep doing new patch series?
That is fine by me. I will start applying individual patches if possible and we get the tested-by or ACKs for it where I need them.
Regards
Marcel
