Hi Rob, -----Original Message----- From: Rob Herring [mailto:[email protected]] Sent: Thursday, June 18, 2015 11:03 AM To: Ilya Faenson Cc: [email protected]; Arend Van Spriel; [email protected]; [email protected] Subject: Re: FW: [PATCH v4 1/4] Broadcom Bluetooth UART Device Tree bindings
On Wed, Jun 17, 2015 at 6:11 PM, Ilya Faenson <[email protected]> wrote: > + devicetree lists Please use get_maintainers.pl. IF: Will try, thanks. > -----Original Message----- > From: Ilya Faenson [mailto:[email protected]] > Sent: Wednesday, June 17, 2015 5:31 PM > To: Marcel Holtmann > Cc: [email protected]; Arend Van Spriel; Ilya Faenson > Subject: [PATCH v4 1/4] Broadcom Bluetooth UART Device Tree bindings > > Device Tree bindings to configure the Broadcom Bluetooth UART device. > > Signed-off-by: Ilya Faenson <[email protected]> > --- Change history from the last v4 version? IF: The history was basically relevant for the bluetooth specific code changes only. Some of them have been applied by now so the rest of the code is a lot more device tree oriented. I will send the next set of patches to both device tree and bluetooth lists with a complete revision history. > .../devicetree/bindings/net/bluetooth/btbcm.txt | 86 > ++++++++++++++++++++++ > 1 file changed, 86 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/bluetooth/btbcm.txt > > diff --git a/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt > b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt > new file mode 100644 > index 0000000..5dbcd57 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt > @@ -0,0 +1,86 @@ > +btbcm > +------ > + > +Required properties: > + > + - compatible : must be "brcm,brcm-bt-uart". You need to describe the chip, not the interface. IF: This driver is for all Broadcom Bluetooth UART based chips. > + - tty : tty device connected to this Bluetooth device. "tty" is a bit of a Linuxism and "ttyS0" certainly is. Further, there is no guarantee which uart is assigned ttyS0. This should be a phandle to the connected uart if not a sub node of the uart. This is complicated since these chips have multiple host connections. It needs to go under either uart or SDIO host and have a reference back to the one it is not under. Given the SDIO interface is discoverable (although sideband gpios and regulators are not), I would put this under the uart node as that is never discoverable. As I've mentioned previously, there's several cases of bindings for UART slave devices being posted. This all needs to be coordinated to use a common structure. IF: This driver does not really access the UART. If just needs to have a string of some sort to map instances of the BlueZ protocol into platform devices to employ the right GPIOs and interrupts. I could use anything you recommend available through the tty_struct coming to the protocol from the BlueZ line discipline. Moreover, every end user platform I've ever dealt with in 16 years of working with Bluetooth had a single BT UART device. So these are really rare (typically test platforms) cases only. The mapping is not needed for most platforms at all. I suspect the right thing to do would be to make this parameter optional. The mapping would be done only if the parameter is present. I will use anything tty_struct derived you specify. Makes sense? > + > +Optional properties: > + > + - bt-host-wake-gpios : bt-host-wake input GPIO to be used as an interrupt. > + > + - bt-wake-gpios : bt-wake output GPIO to be used to suspend / resume > device. > + > + - bt-reg-on-gpios : reg-on output GPIO to be used to power device on/off. > + > + - oper-speed : Bluetooth device operational baud rate. > + Default: 3000000. > + > + - manual-fc : flow control UART in suspend / resume scenarios. > + Default: 0. Can be boolean? I don't really follow the description. IF: Okay, I will make it boolean. To clarify the description, it controls whether the BlueZ protocol needs to flow control the UART when the BT device is suspended and un-flow control it when the device is resumed. > + > + - configure-sleep : configure suspend / resume flag. > + Default: false. Please describe better what this does. Perhaps "idle-sleep-en" would be better. IF: Okay, I will rename it as you specify and will describe it better. > + > + - idle-timeout : Number of seconds of inactivity before suspending. idle-timeout-sec IF: Okay, I will rename it. Is this only applicable when configure-sleep is set? IF: Yes. You mix the terms sleep and suspend a lot. Can you be more consistent. IF: Broadcom BT device folks use "sleep" while Linux employs "suspend". Okay, I will try using suspend everywhere. > + Default: 5. > + > + - configure-audio : configure platform PCM SCO flag. > + Default: false. So ignore the rest of the settings if not set? Perhaps combine with pcm-routing: <blank> - no audio audio-mode = "pcm"; audio-mode = "hci"; (or "sco-hci"?) IF: That's right: the rest of the parameters are not needed if configure-audio is false. Talking about your suggestions, this driver does nothing if the audio is either sent inbound or not used at all. Would you agree to something like the configure-pcm-audio flag? > + > + - pcm-clockmode : PCM clock mode. 0-slave, 1-master. > + Default: 0. Can be boolean (property present or not): pcm-clock-mode-master IF: Okay. > + > + - pcm-fillmethod : PCM fill method. 0 to 3. pcm-fill-method IF: Okay, will change. > + Default: 2. > + > + - pcm-fillnum : PCM number of fill bits. 0 to 3. > + Default: 0. pcm-fill-bits IF: Okay, will change. > + > + - pcm-fillvalue : PCM fill value. 0 to 7. > + Default: 3. pcm-fill-value IF: Okay, will change. > + > + - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-512Kbps, > + 3-1024Kbps, 4-2048Kbps. pcm-incall-bitclock-hz IF: Okay, will change. Use the actual rate rather than an enumeration. It is a simple div by 128k and log2 to convert in the driver. This makes the property more compatible with other chips. IF: These rates are subject to change in future chips with no guarantees of the pattern holding. I would prefer to use the actual value expected by the firmware if you don't mind to avoid maintaining the extra driver code. What does incall mean? What is the bit rate when not in a call? IF: That's the name given to me by the hardware guys. What do you think about the "pcm-interface-rate" instead? > + Default: 0. > + > + - pcm-lsbfirst : PCM LSB first. 0 or 1. > + Default: 0. Can be boolean IF: Okay. > + > + - pcm-rightjustify : PCM Justify. 0-left, 1-right. > + Default: 0. Can be boolean IF: Okay. > + - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI. > + Default: 0. Can be boolean: pcm-routing-hci IF: Okay. > + > + - pcm-shortframesync : PCM sync. 0-short, 1-long. > + Default: 0. Can be boolean IF: Okay. > + > + - pcm-syncmode : PCM sync mode. 0-slave, 1-master. > + Default: 0. Can be boolean: pcm-sync-mode-master IF: Okay. Rob N�����r��y����b�X��ǧv�^�){.n�+���z��z��z)����w* jg��� �����ݢj/���z�ޖ��2�ޙ����&�)ߡ�a���� �G���h��j:+v���w��٥
