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-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
