Re: [PATCH v7 1/8] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990

[Balakrishna Godavarthi <bgodavar@codeaurora.org>]

Hi Rob,

please find my comments inline.

On 2018-06-18 18:59, Rob Herring wrote:
> On Sat, Jun 16, 2018 at 12:27 AM, Balakrishna Godavarthi
> <bgodavar@codeaurora.org> wrote:
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> 
> Commit messag missing.
> 
[Bala]: my bad, i some how missed. will update.

> Please use get_maintainers.pl and send patches to the right lists. DT
> list in this case.
> 
[Bala]: will find the correct POC.


>> ---
>> Changes in v7:
>>         * no change.
>> 
>> Changes in v6:
>> 
>>         * Changed the oper-speed to max-speed.
>> 
>> Changes in v5:
>> 
>>         * Added entry for oper-speed and init-speed.
>> 
>> ---
>>  .../bindings/net/qualcomm-bluetooth.txt       | 31 
>> +++++++++++++++++--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt 
>> b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> index 0ea18a53cc29..c4590bc87390 100644
>> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> @@ -10,12 +10,24 @@ device the slave device is attached to.
>>  Required properties:
>>   - compatible: should contain one of the following:
>>     * "qcom,qca6174-bt"
>> +   * "qcom,wcn3990-bt"
>> 
>>  Optional properties:
>>   - enable-gpios: gpio specifier used to enable chip
>>   - clocks: clock provided to the controller (SUSCLK_32KHZ)
>> -
>> -Example:
>> + - vddpa-supply: Bluetooth VDD PA regulator handle
>> + - vddio-supply: Bluetooth VDD IO regulator handle
>> + - vddldo-supply: Bluetooth VDD LDO regulator handle. Kept under 
>> optional
>> +                 parameters as some of the chipsets doesn't require 
>> ldo or
>> +                 it may use from same vddio.
> 
> You need to be explicit as to which properties apply or don't apply to
> specific compatible strings.
> 

[Bala]: will update.

> Supplies should be based on inputs to the chip, so if 1 regulator
> supplies 2 inputs you still need both.
> 
[Bala]:  sorry i did't get you, all these regulators are single output 
line regulators.
          i.e. <supply-name>- supply, need single regulator input as a 
handle.
          and these line regulators generate fixed voltage output.
          please let me know, if i have cleared your query

>> + - vddxtal-supply: Bluetooth VDD XTAL regulator handle
>> + - vddcore-supply: Bluetooth VDD CORE regulator handle
>> + - vddpwd-supply: Chip power down gpio is required when bluetooth 
>> module
>> +                 and other modules like wifi co-exist in a singe chip 
>> and
>> +                 shares a common gpio to bring chip out of reset.
>> + - max-speed: operating speed of the chip.
>> +
>> +Examples:
>> 
>>  serial@7570000 {
>>         label = "BT-UART";
>> @@ -28,3 +40,18 @@ serial@7570000 {
>>                 clocks = <&divclk4>;
>>         };
>>  };
>> +
>> +serial@898000 {
>> +       label = "BT-UART";
>> +       status = "okay";
>> +
>> +       bluetooth: bt_wcn3990 {
> 
> Reverse these. The node name should be "bluetooth".
> 
>> +               compatible = "qcom,wcn3990-bt";
>> +               vddio-supply = <&pm8998_s3>;
>> +               vddxtal-supply = <&pm8998_s5>;
>> +               vddcore-supply = <&pm8998_l7>;
>> +               vddpa-supply = <&pm8998_l17>;
>> +               vddldo-supply = <&pm8998_l25>;
>> +               max-speed = <3200000>;
>> +       };
>> +};
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Thanks for reviewing, will send a incremental patch.

-- 
Regards
Balakrishna.