| Message ID | 20191112230944.48716-5-abhishekpandit@chromium.org |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | Bluetooth: hci_bcm: Additional changes for BCM4354 support | expand |
Hi Abhishek, > 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(+) > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > index c749dc297624..42fb2fa8143d 100644 > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > @@ -29,6 +29,11 @@ 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: 0-3 (PCM, Transport, Codec, I2S) > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps) > + - brcm,pcm-frame-type: 0-1 (short, long) > + - brcm,pcm-sync-mode: 0-1 (slave, master) > + - brcm,pcm-clock-mode: 0-1 (slave, master) I think that all of them need to start with brcm,bt- prefix since it is rather Bluetooth specific. > > > Example: > @@ -40,5 +45,11 @@ Example: > bluetooth { > compatible = "brcm,bcm43438-bt"; > max-speed = <921600>; > + > + brcm,bt-sco-routing = [01]; > + brcm,pcm-interface-rate = [02]; > + brcm,pcm-frame-type = [00]; > + brcm,pcm-sync-mode = [01]; > + brcm,pcm-clock-mode = [01]; > }; My personal taste would be to add a comment after each entry that gives the human readable setting. Regards Marcel
Hi, On Tue, Nov 12, 2019 at 3:10 PM Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote: > > 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(+) > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > index c749dc297624..42fb2fa8143d 100644 > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > @@ -29,6 +29,11 @@ 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: 0-3 (PCM, Transport, Codec, I2S) > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps) > + - brcm,pcm-frame-type: 0-1 (short, long) > + - brcm,pcm-sync-mode: 0-1 (slave, master) > + - brcm,pcm-clock-mode: 0-1 (slave, master) 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. > Example: > @@ -40,5 +45,11 @@ Example: > bluetooth { > compatible = "brcm,bcm43438-bt"; > max-speed = <921600>; > + > + brcm,bt-sco-routing = [01]; > + brcm,pcm-interface-rate = [02]; > + brcm,pcm-frame-type = [00]; > + brcm,pcm-sync-mode = [01]; > + brcm,pcm-clock-mode = [01]; 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, > > > 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(+) > > > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > index c749dc297624..42fb2fa8143d 100644 > > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > @@ -29,6 +29,11 @@ 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: 0-3 (PCM, Transport, Codec, I2S) > > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps) > > + - brcm,pcm-frame-type: 0-1 (short, long) > > + - brcm,pcm-sync-mode: 0-1 (slave, master) > > + - brcm,pcm-clock-mode: 0-1 (slave, master) > > I think that all of them need to start with brcm,bt- prefix since it is rather Bluetooth specific. > > > > > > > Example: > > @@ -40,5 +45,11 @@ Example: > > bluetooth { > > compatible = "brcm,bcm43438-bt"; > > max-speed = <921600>; > > + > > + brcm,bt-sco-routing = [01]; > > + brcm,pcm-interface-rate = [02]; > > + brcm,pcm-frame-type = [00]; > > + brcm,pcm-sync-mode = [01]; > > + brcm,pcm-clock-mode = [01]; > > }; > > 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 <dianders@chromium.org> wrote: > > Hi, > > On Tue, Nov 12, 2019 at 3:10 PM Abhishek Pandit-Subedi > <abhishekpandit@chromium.org> wrote: > > > > 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(+) > > > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > index c749dc297624..42fb2fa8143d 100644 > > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > @@ -29,6 +29,11 @@ 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: 0-3 (PCM, Transport, Codec, I2S) > > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps) > > + - brcm,pcm-frame-type: 0-1 (short, long) > > + - brcm,pcm-sync-mode: 0-1 (slave, master) > > + - brcm,pcm-clock-mode: 0-1 (slave, master) > > 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. > > > > Example: > > @@ -40,5 +45,11 @@ Example: > > bluetooth { > > compatible = "brcm,bcm43438-bt"; > > max-speed = <921600>; > > + > > + brcm,bt-sco-routing = [01]; > > + brcm,pcm-interface-rate = [02]; > > + brcm,pcm-frame-type = [00]; > > + brcm,pcm-sync-mode = [01]; > > + brcm,pcm-clock-mode = [01]; > > 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. It's a bytestring with a length of 1. See bytestrings under https://devicetree-specification.readthedocs.io/en/latest/source-language.html#node-and-property-definitions > > -Doug
On Thu, Nov 14, 2019 at 9:58 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Wed, Nov 13, 2019 at 01:21:06AM +0100, Marcel Holtmann wrote: > > Hi Abhishek, > > > > > 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(+) > > > > > > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > > index c749dc297624..42fb2fa8143d 100644 > > > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt > > > @@ -29,6 +29,11 @@ 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: 0-3 (PCM, Transport, Codec, I2S) > > > + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps) > > > + - brcm,pcm-frame-type: 0-1 (short, long) > > > + - brcm,pcm-sync-mode: 0-1 (slave, master) > > > + - brcm,pcm-clock-mode: 0-1 (slave, master) > > > > I think that all of them need to start with brcm,bt- prefix since it is rather Bluetooth specific. > > > > > > > > > > > Example: > > > @@ -40,5 +45,11 @@ Example: > > > bluetooth { > > > compatible = "brcm,bcm43438-bt"; > > > max-speed = <921600>; > > > + > > > + brcm,bt-sco-routing = [01]; > > > + brcm,pcm-interface-rate = [02]; > > > + brcm,pcm-frame-type = [00]; > > > + brcm,pcm-sync-mode = [01]; > > > + brcm,pcm-clock-mode = [01]; > > > }; > > > > 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
Hi, On Thu, Nov 14, 2019 at 11:20 AM Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote: > > > > Example: > > > @@ -40,5 +45,11 @@ Example: > > > bluetooth { > > > compatible = "brcm,bcm43438-bt"; > > > max-speed = <921600>; > > > + > > > + brcm,bt-sco-routing = [01]; > > > + brcm,pcm-interface-rate = [02]; > > > + brcm,pcm-frame-type = [00]; > > > + brcm,pcm-sync-mode = [01]; > > > + brcm,pcm-clock-mode = [01]; > > > > 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. > > It's a bytestring with a length of 1. See bytestrings under > https://devicetree-specification.readthedocs.io/en/latest/source-language.html#node-and-property-definitions 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
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index c749dc297624..42fb2fa8143d 100644 --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt @@ -29,6 +29,11 @@ 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: 0-3 (PCM, Transport, Codec, I2S) + - brcm,pcm-interface-rate: 0-4 (128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps) + - brcm,pcm-frame-type: 0-1 (short, long) + - brcm,pcm-sync-mode: 0-1 (slave, master) + - brcm,pcm-clock-mode: 0-1 (slave, master) Example: @@ -40,5 +45,11 @@ Example: bluetooth { compatible = "brcm,bcm43438-bt"; max-speed = <921600>; + + brcm,bt-sco-routing = [01]; + brcm,pcm-interface-rate = [02]; + brcm,pcm-frame-type = [00]; + brcm,pcm-sync-mode = [01]; + brcm,pcm-clock-mode = [01]; }; };
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(+)