While adding support for the BCM4354, I discovered a few more things
that weren't working as they should have.
First, we disallow serdev from setting the baudrate on BCM4354. Serdev
sets the oper_speed first before calling hu->setup() in
hci_uart_setup(). On the BCM4354, this results in bcm_setup() failing
when the hci reset times out.
Next, we add support for setting the PCM parameters, which consists of
a pair of vendor specific opcodes to set the pcm parameters. The
documentation for these params are available in the brcm_patchram_plus
package (i.e. https://github.com/balena-os/brcm_patchram_plus). This is
necessary for PCM to work properly.
All changes were tested with rk3288-veyron-minnie.dts.
Changes in v4:
- Fix incorrect function name in hci_bcm
Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing
Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit
Abhishek Pandit-Subedi (4):
Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354
Bluetooth: btbcm: Support pcm configuration
Bluetooth: hci_bcm: Support pcm params in dts
dt-bindings: net: broadcom-bluetooth: Add pcm config
.../bindings/net/broadcom-bluetooth.txt | 11 +++
drivers/bluetooth/btbcm.c | 18 +++++
drivers/bluetooth/btbcm.h | 8 +++
drivers/bluetooth/hci_bcm.c | 69 ++++++++++++++++++-
4 files changed, 105 insertions(+), 1 deletion(-)
--
2.24.0.rc1.363.gb1bccd3e3d-goog
Add documentation for pcm parameters.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in v4:
- Fix incorrect function name in hci_bcm
Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing
Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit
.../devicetree/bindings/net/broadcom-bluetooth.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
Add documentation for pcm parameters.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in v4:
- Fix incorrect function name in hci_bcm
Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing
Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit
.../devicetree/bindings/net/broadcom-bluetooth.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
From: Doug Anderson <dianders@chromium.org> Date: 2019-11-14 17:29:49
Hi,
On Tue, Nov 12, 2019 at 3:10 PM Abhishek Pandit-Subedi
[off-list ref] wrote:
quoted hunk
Add documentation for pcm parameters.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in v4:
- Fix incorrect function name in hci_bcm
Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing
Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit
.../devicetree/bindings/net/broadcom-bluetooth.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
Since these are optional your patch should describe what happens if
they are not present. I think in patch #3 of the series you guys are
discussing it, but whatever you end up with should be documented here.
That actually made me realize that this is patch #4 in the series. To
be pedantic, bindings are supposed to be _earlier_ in the series than
the code that implements them.
I'm at least marginally curious why your example has a leading 0 for
all numbers. It makes me think you intend them to be represented in
octal, though I don't know offhand if dtc uses that format for octal.
I guess it doesn't matter since all your numbers are between 0 and 5,
but it does seem strange.
-Doug
On Wed, Nov 13, 2019 at 01:21:06AM +0100, Marcel Holtmann wrote:
Hi Abhishek,
quoted
Add documentation for pcm parameters.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in v4:
- Fix incorrect function name in hci_bcm
Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing
Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit
.../devicetree/bindings/net/broadcom-bluetooth.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
My personal taste would be to add a comment after each entry that gives the human readable setting.
I'd suggest to define constants in include/dt-bindings/bluetooth/brcm.h
and use them instead of literals, with this we wouldn't rely on (optional)
comments to make the configuration human readable.
On Thu, Nov 14, 2019 at 9:29 AM Doug Anderson [off-list ref] wrote:
Hi,
On Tue, Nov 12, 2019 at 3:10 PM Abhishek Pandit-Subedi
[off-list ref] wrote:
quoted
Add documentation for pcm parameters.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in v4:
- Fix incorrect function name in hci_bcm
Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing
Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit
.../devicetree/bindings/net/broadcom-bluetooth.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
Since these are optional your patch should describe what happens if
they are not present. I think in patch #3 of the series you guys are
discussing it, but whatever you end up with should be documented here.
Yes, I think I will document the default values here as well.
That actually made me realize that this is patch #4 in the series. To
be pedantic, bindings are supposed to be _earlier_ in the series than
the code that implements them.
I'm at least marginally curious why your example has a leading 0 for
all numbers. It makes me think you intend them to be represented in
octal, though I don't know offhand if dtc uses that format for octal.
I guess it doesn't matter since all your numbers are between 0 and 5,
but it does seem strange.
On Thu, Nov 14, 2019 at 9:58 AM Matthias Kaehlcke [off-list ref] wrote:
On Wed, Nov 13, 2019 at 01:21:06AM +0100, Marcel Holtmann wrote:
quoted
Hi Abhishek,
quoted
Add documentation for pcm parameters.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in v4:
- Fix incorrect function name in hci_bcm
Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing
Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit
.../devicetree/bindings/net/broadcom-bluetooth.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
My personal taste would be to add a comment after each entry that gives the human readable setting.
I'd suggest to define constants in include/dt-bindings/bluetooth/brcm.h
and use them instead of literals, with this we wouldn't rely on (optional)
comments to make the configuration human readable.
:+1: Sounds like a good idea; expect it in next patch revision
I'm at least marginally curious why your example has a leading 0 for
all numbers. It makes me think you intend them to be represented in
octal, though I don't know offhand if dtc uses that format for octal.
I guess it doesn't matter since all your numbers are between 0 and 5,
but it does seem strange.
Oh, right! ...except that now it's just one value and not an array of
values, just make it a normal number. Don't worry about the fact that
it'll take up 4 bytes instead of 1--it's clearer for it to just be a
normal number.
...I would also note that the definition of the properties talks
nothing about them being a bytestring. ;-)
-Doug