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

[Bjorn Andersson]

On Wed 14 Mar 11:42 PDT 2018, Marcel Holtmann wrote:

> Hi Bjoern,
> 
> >>> +		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.

> 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.

Regards,
Bjorn