Message ID | 20191118110335.v6.3.I18b06235e381accea1c73aa2f9db358645d9f201@changeid |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | Bluetooth: hci_bcm: Additional changes for BCM4354 support | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success |
Hi Abhishek, > Add documentation for pcm parameters. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > > Changes in v6: None > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: None > > .../bindings/net/broadcom-bluetooth.txt | 16 ++++++++++ > include/dt-bindings/bluetooth/brcm.h | 32 +++++++++++++++++++ > 2 files changed, 48 insertions(+) > create mode 100644 include/dt-bindings/bluetooth/brcm.h > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > index c749dc297624..8561e4684378 100644 > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > @@ -29,10 +29,20 @@ Optional properties: > - "lpo": external low power 32.768 kHz clock > - vbat-supply: phandle to regulator supply for VBAT > - vddio-supply: phandle to regulator supply for VDDIO > + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S > + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps > + - brcm,bt-pcm-frame-type: short, long > + - brcm,bt-pcm-sync-mode: slave, master > + - brcm,bt-pcm-clock-mode: slave, master > > +See include/dt-bindings/bluetooth/brcm.h for SCO/PCM parameters. The default > +value for all these values are 0 (except for brcm,bt-sco-routing which requires > +a value) if you choose to leave it out. > > Example: > > +#include <dt-bindings/bluetooth/brcm.h> > + > &uart2 { > pinctrl-names = "default"; > pinctrl-0 = <&uart2_pins>; > @@ -40,5 +50,11 @@ Example: > bluetooth { > compatible = "brcm,bcm43438-bt"; > max-speed = <921600>; > + > + brcm,bt-sco-routing = <BRCM_SCO_ROUTING_TRANSPORT>; in case you use transport which means HCI, you would not have values below. It is rather PCM here in the example. > + brcm,bt-pcm-interface-rate = <BRCM_PCM_IF_RATE_512KBPS>; > + brcm,bt-pcm-frame-type = <BRCM_PCM_FRAME_TYPE_SHORT>; > + brcm,bt-pcm-sync-mode = <BRCM_PCM_SYNC_MODE_MASTER>; > + brcm,bt-pcm-clock-mode = <BRCM_PCM_CLOCK_MODE_MASTER>; > }; > }; And I am asking this again. Is this adding any value to use an extra include file? Inside the driver we are not really needing these values since they are handed to the hardware. Regards Marcel
Hi, On Mon, Nov 18, 2019 at 9:39 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Abhishek, > > > Add documentation for pcm parameters. > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > --- > > > > Changes in v6: None > > Changes in v5: None > > Changes in v4: None > > Changes in v3: None > > Changes in v2: None > > > > .../bindings/net/broadcom-bluetooth.txt | 16 ++++++++++ > > include/dt-bindings/bluetooth/brcm.h | 32 +++++++++++++++++++ > > 2 files changed, 48 insertions(+) > > create mode 100644 include/dt-bindings/bluetooth/brcm.h > > > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > index c749dc297624..8561e4684378 100644 > > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > @@ -29,10 +29,20 @@ Optional properties: > > - "lpo": external low power 32.768 kHz clock > > - vbat-supply: phandle to regulator supply for VBAT > > - vddio-supply: phandle to regulator supply for VDDIO > > + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S > > + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps > > + - brcm,bt-pcm-frame-type: short, long > > + - brcm,bt-pcm-sync-mode: slave, master > > + - brcm,bt-pcm-clock-mode: slave, master > > > > +See include/dt-bindings/bluetooth/brcm.h for SCO/PCM parameters. The default > > +value for all these values are 0 (except for brcm,bt-sco-routing which requires > > +a value) if you choose to leave it out. > > > > Example: > > > > +#include <dt-bindings/bluetooth/brcm.h> > > + > > &uart2 { > > pinctrl-names = "default"; > > pinctrl-0 = <&uart2_pins>; > > @@ -40,5 +50,11 @@ Example: > > bluetooth { > > compatible = "brcm,bcm43438-bt"; > > max-speed = <921600>; > > + > > + brcm,bt-sco-routing = <BRCM_SCO_ROUTING_TRANSPORT>; > > in case you use transport which means HCI, you would not have values below. It is rather PCM here in the example. > > > + brcm,bt-pcm-interface-rate = <BRCM_PCM_IF_RATE_512KBPS>; > > + brcm,bt-pcm-frame-type = <BRCM_PCM_FRAME_TYPE_SHORT>; > > + brcm,bt-pcm-sync-mode = <BRCM_PCM_SYNC_MODE_MASTER>; > > + brcm,bt-pcm-clock-mode = <BRCM_PCM_CLOCK_MODE_MASTER>; > > }; > > }; > > And I am asking this again. Is this adding any value to use an extra include file? Inside the driver we are not really needing these values since they are handed to the hardware. Personally I find that they add value in that it makes it easier for someone tweaking the device tree to know what the expected valid values are and what they mean. I think Matthias also found value in them since he suggested them in: https://lore.kernel.org/r/20191114175836.GI27773@google.com There, he said: > 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. ...which seems to make sense to me. -Doug
On Mon, Nov 18, 2019 at 11:21:22AM -0800, Abhishek Pandit-Subedi wrote: > Add documentation for pcm parameters. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > > Changes in v6: None > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: None Really? I'm staring at v2 that looks a bit different. > .../bindings/net/broadcom-bluetooth.txt | 16 ++++++++++ > include/dt-bindings/bluetooth/brcm.h | 32 +++++++++++++++++++ > 2 files changed, 48 insertions(+) > create mode 100644 include/dt-bindings/bluetooth/brcm.h > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > index c749dc297624..8561e4684378 100644 > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > @@ -29,10 +29,20 @@ Optional properties: > - "lpo": external low power 32.768 kHz clock > - vbat-supply: phandle to regulator supply for VBAT > - vddio-supply: phandle to regulator supply for VDDIO > + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S > + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps > + - brcm,bt-pcm-frame-type: short, long > + - brcm,bt-pcm-sync-mode: slave, master > + - brcm,bt-pcm-clock-mode: slave, master Little of this seems unique to Broadcom. We already have some standard audio related properties for audio interfaces such as 'format', 'frame-master' and 'bitclock-master'. Ultimately, this would be tied into the audio complex of SoCs and need to work with the audio bindings. We also have HDMI audio bindings. Maybe sco-routing is unique to BT and still needed in some form though if you describe the connection to the SoC audio complex, then maybe not? I'd assume every BT chip has some audio routing configuration. Rob
Hi Rob, >> Add documentation for pcm parameters. >> >> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> >> --- >> >> Changes in v6: None >> Changes in v5: None >> Changes in v4: None >> Changes in v3: None >> Changes in v2: None > > Really? I'm staring at v2 that looks a bit different. > >> .../bindings/net/broadcom-bluetooth.txt | 16 ++++++++++ >> include/dt-bindings/bluetooth/brcm.h | 32 +++++++++++++++++++ >> 2 files changed, 48 insertions(+) >> create mode 100644 include/dt-bindings/bluetooth/brcm.h >> >> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt >> index c749dc297624..8561e4684378 100644 >> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt >> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt >> @@ -29,10 +29,20 @@ Optional properties: >> - "lpo": external low power 32.768 kHz clock >> - vbat-supply: phandle to regulator supply for VBAT >> - vddio-supply: phandle to regulator supply for VDDIO >> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S >> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps >> + - brcm,bt-pcm-frame-type: short, long >> + - brcm,bt-pcm-sync-mode: slave, master >> + - brcm,bt-pcm-clock-mode: slave, master > > Little of this seems unique to Broadcom. We already have some standard > audio related properties for audio interfaces such as 'format', > 'frame-master' and 'bitclock-master'. Ultimately, this would be tied > into the audio complex of SoCs and need to work with the audio > bindings. We also have HDMI audio bindings. > > Maybe sco-routing is unique to BT and still needed in some form though > if you describe the connection to the SoC audio complex, then maybe > not? I'd assume every BT chip has some audio routing configuration. so we tried to generalize this some time before and failed to get a proper consensus. In general I am with you that we should just expose generic properties from the attached audio codec, but nobody has come up with anything like that. And I think aligning all chip manufacturers will take some time. Maybe in the interim we just use brcm,bt-pcm-int-params = [00 00 ..] as initially proposed. Regards Marcel
On Fri, Nov 22, 2019 at 6:34 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Rob, > > >> Add documentation for pcm parameters. > >> > >> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > >> --- > >> > >> Changes in v6: None > >> Changes in v5: None > >> Changes in v4: None > >> Changes in v3: None > >> Changes in v2: None > > > > Really? I'm staring at v2 that looks a bit different. > > > >> .../bindings/net/broadcom-bluetooth.txt | 16 ++++++++++ > >> include/dt-bindings/bluetooth/brcm.h | 32 +++++++++++++++++++ > >> 2 files changed, 48 insertions(+) > >> create mode 100644 include/dt-bindings/bluetooth/brcm.h > >> > >> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > >> index c749dc297624..8561e4684378 100644 > >> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > >> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > >> @@ -29,10 +29,20 @@ Optional properties: > >> - "lpo": external low power 32.768 kHz clock > >> - vbat-supply: phandle to regulator supply for VBAT > >> - vddio-supply: phandle to regulator supply for VDDIO > >> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S > >> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps > >> + - brcm,bt-pcm-frame-type: short, long > >> + - brcm,bt-pcm-sync-mode: slave, master > >> + - brcm,bt-pcm-clock-mode: slave, master > > > > Little of this seems unique to Broadcom. We already have some standard > > audio related properties for audio interfaces such as 'format', > > 'frame-master' and 'bitclock-master'. Ultimately, this would be tied > > into the audio complex of SoCs and need to work with the audio > > bindings. We also have HDMI audio bindings. > > > > Maybe sco-routing is unique to BT and still needed in some form though > > if you describe the connection to the SoC audio complex, then maybe > > not? I'd assume every BT chip has some audio routing configuration. > > so we tried to generalize this some time before and failed to get a proper consensus. > > In general I am with you that we should just expose generic properties from the attached audio codec, but nobody has come up with anything like that. And I think aligning all chip manufacturers will take some time. > That shouldn't be hard. It's a solved problem for codecs and HDMI. I don't think BT is any more complicated (ignoring phones). I suspect it's not solved simply because no one wants to do the work beyond their 1 BT device they care about ATM. > Maybe in the interim we just use brcm,bt-pcm-int-params = [00 00 ..] as initially proposed. What's the device using this? Some chromebook I suppose. I think it would be better to first see how this fits in with the rest of the audio subsystem. Until then, the driver should probably just default to "transport" mode which I assume is audio routed over the UART interface. That should work on any platform at least, but may not be optimal. Rob
Hi Rob, >>>> Add documentation for pcm parameters. >>>> >>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> >>>> --- >>>> >>>> Changes in v6: None >>>> Changes in v5: None >>>> Changes in v4: None >>>> Changes in v3: None >>>> Changes in v2: None >>> >>> Really? I'm staring at v2 that looks a bit different. >>> >>>> .../bindings/net/broadcom-bluetooth.txt | 16 ++++++++++ >>>> include/dt-bindings/bluetooth/brcm.h | 32 +++++++++++++++++++ >>>> 2 files changed, 48 insertions(+) >>>> create mode 100644 include/dt-bindings/bluetooth/brcm.h >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt >>>> index c749dc297624..8561e4684378 100644 >>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt >>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt >>>> @@ -29,10 +29,20 @@ Optional properties: >>>> - "lpo": external low power 32.768 kHz clock >>>> - vbat-supply: phandle to regulator supply for VBAT >>>> - vddio-supply: phandle to regulator supply for VDDIO >>>> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S >>>> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps >>>> + - brcm,bt-pcm-frame-type: short, long >>>> + - brcm,bt-pcm-sync-mode: slave, master >>>> + - brcm,bt-pcm-clock-mode: slave, master >>> >>> Little of this seems unique to Broadcom. We already have some standard >>> audio related properties for audio interfaces such as 'format', >>> 'frame-master' and 'bitclock-master'. Ultimately, this would be tied >>> into the audio complex of SoCs and need to work with the audio >>> bindings. We also have HDMI audio bindings. >>> >>> Maybe sco-routing is unique to BT and still needed in some form though >>> if you describe the connection to the SoC audio complex, then maybe >>> not? I'd assume every BT chip has some audio routing configuration. >> >> so we tried to generalize this some time before and failed to get a proper consensus. >> >> In general I am with you that we should just expose generic properties from the attached audio codec, but nobody has come up with anything like that. And I think aligning all chip manufacturers will take some time. >> > > That shouldn't be hard. It's a solved problem for codecs and HDMI. I > don't think BT is any more complicated (ignoring phones). I suspect > it's not solved simply because no one wants to do the work beyond > their 1 BT device they care about ATM. we tried, but nobody can agree on these right now. I would be happy if others come forward and tell us how they wired up their hardware, but it hasn’t happened yet. >> Maybe in the interim we just use brcm,bt-pcm-int-params = [00 00 ..] as initially proposed. > > What's the device using this? Some chromebook I suppose. I think it > would be better to first see how this fits in with the rest of the > audio subsystem. Until then, the driver should probably just default > to "transport" mode which I assume is audio routed over the UART > interface. That should work on any platform at least, but may not be > optimal. SCO over UART doesn’t really work. Long time ago, some car kits might have done it, but in the Chromebook cases this will just not work. We need to configure the PCM settings of the Bluetooth chip. If we don’t do it via DT, then this gets hardcoded in the driver source and that is not helping either. So until we get anything better, lets use brcm,bt-pcm-int-params = [00 00 ..] and get this supported upstream. Regards Marcel
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index c749dc297624..8561e4684378 100644 --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt @@ -29,10 +29,20 @@ Optional properties: - "lpo": external low power 32.768 kHz clock - vbat-supply: phandle to regulator supply for VBAT - vddio-supply: phandle to regulator supply for VDDIO + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps + - brcm,bt-pcm-frame-type: short, long + - brcm,bt-pcm-sync-mode: slave, master + - brcm,bt-pcm-clock-mode: slave, master +See include/dt-bindings/bluetooth/brcm.h for SCO/PCM parameters. The default +value for all these values are 0 (except for brcm,bt-sco-routing which requires +a value) if you choose to leave it out. Example: +#include <dt-bindings/bluetooth/brcm.h> + &uart2 { pinctrl-names = "default"; pinctrl-0 = <&uart2_pins>; @@ -40,5 +50,11 @@ Example: bluetooth { compatible = "brcm,bcm43438-bt"; max-speed = <921600>; + + brcm,bt-sco-routing = <BRCM_SCO_ROUTING_TRANSPORT>; + brcm,bt-pcm-interface-rate = <BRCM_PCM_IF_RATE_512KBPS>; + brcm,bt-pcm-frame-type = <BRCM_PCM_FRAME_TYPE_SHORT>; + brcm,bt-pcm-sync-mode = <BRCM_PCM_SYNC_MODE_MASTER>; + brcm,bt-pcm-clock-mode = <BRCM_PCM_CLOCK_MODE_MASTER>; }; }; diff --git a/include/dt-bindings/bluetooth/brcm.h b/include/dt-bindings/bluetooth/brcm.h new file mode 100644 index 000000000000..8b86f90d7dd2 --- /dev/null +++ b/include/dt-bindings/bluetooth/brcm.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ +/* + * This header provides constants for Broadcom bluetooth dt-bindings. + */ +#ifndef _DT_BINDINGS_BLUETOOTH_BRCM_H +#define _DT_BINDINGS_BLUETOOTH_BRCM_H + +#define BRCM_BT_SCO_ROUTING_PCM 0 +#define BRCM_BT_SCO_ROUTING_TRANSPORT 1 +#define BRCM_BT_SCO_ROUTING_CODEC 2 +#define BRCM_BT_SCO_ROUTING_I2S 3 + +/* Default is 128KBPs */ +#define BRCM_BT_PCM_INTERFACE_RATE_128KBPS 0 +#define BRCM_BT_PCM_INTERFACE_RATE_256KBPS 1 +#define BRCM_BT_PCM_INTERFACE_RATE_512KBPS 2 +#define BRCM_BT_PCM_INTERFACE_RATE_1024KBPS 3 +#define BRCM_BT_PCM_INTERFACE_RATE_2048KBPS 4 + +/* Default should be short */ +#define BRCM_BT_PCM_FRAME_TYPE_SHORT 0 +#define BRCM_BT_PCM_FRAME_TYPE_LONG 1 + +/* Default should be master */ +#define BRCM_BT_PCM_SYNC_MODE_SLAVE 0 +#define BRCM_BT_PCM_SYNC_MODE_MASTER 1 + +/* Default should be master */ +#define BRCM_BT_PCM_CLOCK_MODE_SLAVE 0 +#define BRCM_BT_PCM_CLOCK_MODE_MASTER 1 + +#endif /* _DT_BINDINGS_BLUETOOTH_BRCM_H */
Add documentation for pcm parameters. Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> --- Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None .../bindings/net/broadcom-bluetooth.txt | 16 ++++++++++ include/dt-bindings/bluetooth/brcm.h | 32 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 include/dt-bindings/bluetooth/brcm.h