[v4,2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

Message ID 20180314155514.3374-3-thierry.escande@linaro.org
State New
Headers show
Series
  • Untitled series #10060
Related show

Commit Message

Thierry Escande March 14, 2018, 3:55 p.m.
Add binding document for serial bluetooth chips using Qualcomm protocol.

Signed-off-by: Thierry Escande <thierry.escande@linaro.org>

---

v4:
- Move bt-disable-n-gpios to required properties section
- Add clocks and pinctrl-0 as required properties

v3: no change
v2: no change

 .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Thierry Escande March 15, 2018, 11:07 a.m. | #1
Hi Marcel,

On 14/03/2018 20:51, Marcel Holtmann wrote:
> Hi Bjorn,

> 

>>>>>> +		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;

>>>>>

>>>>> can we use a common name here. I think that Nokia and Broadcom drivers

>>>>> define one. And if this is the enable/shutdown GPIO, we should name it

>>>>> consistently across all manufacturers. It essentially does the same on

>>>>> Bluetooth UART chips no matter what chip is behind them.

>>>>>

>>>>

>>>> Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios.

>>>> It might be that these behave in the same way, but from the description

>>>> they only trigger the wakeup.

>>>

>>> that is something that we might need to start fixing. I really prefer

>>> if we name the GPIOs a bit more consistent.

>>>

>>>> The reason for the proposed naming here is that the pin is named

>>>> "BT_DISABLE_N" in the datasheet.

>>>

>>> That is not a reason I buy. So the next board comes around that labels

>>> it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a

>>> patch to the driver to look for that name. If the GPIO does the same

>>> thing, I couldn’t care less what the data sheet says. That might be

>>> a comment in the DT file, but it should not pollute the driver code.

>>>

>>

>> BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip,

>> not on the board, so this name is the same regardless of what you name

>> the line or gpio your board connect it to.

> 

> and QCA chip v1 and QCA chip v2 will use the same driver and same firmware loading mechanism. So why do we have to add a new GPIO naming if they decide to change the name in the data sheet. With Bluetooth it is pretty much all the same. Every UART chip has a shutdown/reset GPIO to enable/disable the chip behind the UART.

> 

>>> A new board should not require driver changes, you just ship a new DT

>>> for that board and an existing driver hopefully just does the job. No

>>> matter how someone named a GPIO in a piece of paper.

>>>

>>

>> I totally agree with the fact that the board should not affect the

>> naming of the gpio in the driver. But I do enjoy when we refer to pins

>> by their real name - instead of having to guess which pin in the _chip_

>> specification the driver actually refer to.

>>

>>

>> That said, what name would you prefer for this?

>>

>> Afaict this is not "wakeup" and there are a few extra steps between the

>> disabled state and "bluetooth is enabled", so "enable" feels slightly

>> wrong. And it probably should be "bluetooth" and not just "device" as

>> this refers to a pin on a WiFi/BT combo chip.

> 

> The Broadcom side called it shutdown GPIO, it is essentially the shutdown/reset GPIO or power on/off GPIO. Personally I do not care what it is named, but it will be all the same for all Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and Qualcomm and you can pick a reasonable common name.


The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has 
"device-wakeup" and "shutdown". The "shutdown" gpio is set to its active 
state to power on the chip which sounds reversed logic. Same for the 
"bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, 
and which is set to 1 to enable it...

So for consistency, naming it as "shutdown" to stick to the bcm driver 
but it should be configured as ACTIVE_LOW in the dts so we actually do a 
gpiod_set_value(0) to un-shutdown it. Does that sound ok?

Regards,
Thierry

> For the wakeup GPIOs, these are different and depend on if there is some low-power mode provided. You would need to check the data sheet to see if they provide more advanced low-power state handling.

> 

> Regards

> 

> Marcel

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
new file mode 100644
index 000000000000..cdb14b96c229
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -0,0 +1,34 @@ 
+Qualcomm Bluetooth Chips
+---------------------
+
+This documents the binding structure and common properties for serial
+attached Qualcomm devices.
+
+Serial attached Qualcomm devices shall be a child node of the host UART
+device the slave device is attached to.
+
+Required properties:
+ - compatible: should contain one of the following:
+   * "qcom,qca6174-bt"
+ - bt-disable-n-gpios: gpio specifier, used to enable chip during probe
+ - pinctrl-0: pin phandle for bt_en gpio
+ - clocks: clock phandle for SUSCLK_32KHZ
+
+Example:
+
+serial@7570000 {
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&blsp1_uart1_default>;
+	pinctrl-1 = <&blsp1_uart1_sleep>;
+
+	bluetooth {
+		compatible = "qcom,qca6174-bt";
+
+		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_en_pin_a>;
+
+		clocks = <&divclk4>;
+	};
+};