linux-kernel - Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Mar 2018 20:51:02 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Thierry Escande <thierry.escande@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <andy.gross@...aro.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        David Brown <david.brown@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Loic Poulain <loic.poulain@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Linux Bluetooth mailing list 
        <linux-bluetooth@...r.kernel.org>, linux-arm-msm@...r.kernel.org,
        devicetree <devicetree@...r.kernel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add
 qualcomm-bluetooth

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.

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