All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Ilya Faenson <ifaenson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Cc: "marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org"
	<marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>,
	Arend Van Spriel <arend-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	"devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: FW: [PATCH v4 1/4] Broadcom Bluetooth UART Device Tree bindings
Date: Thu, 18 Jun 2015 10:02:58 -0500	[thread overview]
Message-ID: <CAL_JsqK9kup3sm3qgDLqtT8ajrN1ee6zfXwOoZqoq9cXQNwE_w@mail.gmail.com> (raw)
In-Reply-To: <E0D3336E15B58B4294723AC879BA5E942634F2-Wwdb2uEOBX8N93KskqRXH71+IgudQmzARxWJa1zDYLQ@public.gmane.org>

On Wed, Jun 17, 2015 at 6:11 PM, Ilya Faenson <ifaenson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> + devicetree lists

Please use get_maintainers.pl.

> -----Original Message-----
> From: Ilya Faenson [mailto:ifaenson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org]
> Sent: Wednesday, June 17, 2015 5:31 PM
> To: Marcel Holtmann
> Cc: linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 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 <ifaenson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---

Change history from the last v4 version?

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

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

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

> +
> +  - configure-sleep : configure suspend / resume flag.
> +    Default: false.

Please describe better what this does. Perhaps "idle-sleep-en" would be better.

> +
> +  - idle-timeout : Number of seconds of inactivity before suspending.

idle-timeout-sec

Is this only applicable when configure-sleep is set?

You mix the terms sleep and suspend a lot. Can you be more consistent.

> +    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"?)

> +
> +  - pcm-clockmode : PCM clock mode. 0-slave, 1-master.
> +    Default: 0.

Can be boolean (property present or not): pcm-clock-mode-master

> +
> +  - pcm-fillmethod : PCM fill method. 0 to 3.

pcm-fill-method

> +    Default: 2.
> +
> +  - pcm-fillnum : PCM number of fill bits. 0 to 3.
> +    Default: 0.

pcm-fill-bits

> +
> +  - pcm-fillvalue : PCM fill value. 0 to 7.
> +    Default: 3.

pcm-fill-value

> +
> +  - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-512Kbps,
> +    3-1024Kbps, 4-2048Kbps.

pcm-incall-bitclock-hz

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.

What does incall mean? What is the bit rate when not in a call?

> +    Default: 0.
> +
> +  - pcm-lsbfirst : PCM LSB first. 0 or 1.
> +    Default: 0.

Can be boolean

> +
> +  - pcm-rightjustify : PCM Justify. 0-left, 1-right.
> +    Default: 0.

Can be boolean

> +  - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI.
> +    Default: 0.

Can be boolean: pcm-routing-hci

> +
> +  - pcm-shortframesync : PCM sync. 0-short, 1-long.
> +    Default: 0.

Can be boolean

> +
> +  - pcm-syncmode : PCM sync mode. 0-slave, 1-master.
> +    Default: 0.

Can be boolean: pcm-sync-mode-master

Rob

WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <robherring2@gmail.com>
To: Ilya Faenson <ifaenson@broadcom.com>
Cc: "marcel@holtmann.org" <marcel@holtmann.org>,
	Arend Van Spriel <arend@broadcom.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-bluetooth@vger.kernel.org"
	<linux-bluetooth@vger.kernel.org>
Subject: Re: FW: [PATCH v4 1/4] Broadcom Bluetooth UART Device Tree bindings
Date: Thu, 18 Jun 2015 10:02:58 -0500	[thread overview]
Message-ID: <CAL_JsqK9kup3sm3qgDLqtT8ajrN1ee6zfXwOoZqoq9cXQNwE_w@mail.gmail.com> (raw)
In-Reply-To: <E0D3336E15B58B4294723AC879BA5E942634F2@IRVEXCHMB15.corp.ad.broadcom.com>

On Wed, Jun 17, 2015 at 6:11 PM, Ilya Faenson <ifaenson@broadcom.com> wrote:
> + devicetree lists

Please use get_maintainers.pl.

> -----Original Message-----
> From: Ilya Faenson [mailto:ifaenson@broadcom.com]
> Sent: Wednesday, June 17, 2015 5:31 PM
> To: Marcel Holtmann
> Cc: linux-bluetooth@vger.kernel.org; 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 <ifaenson@broadcom.com>
> ---

Change history from the last v4 version?

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

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

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

> +
> +  - configure-sleep : configure suspend / resume flag.
> +    Default: false.

Please describe better what this does. Perhaps "idle-sleep-en" would be better.

> +
> +  - idle-timeout : Number of seconds of inactivity before suspending.

idle-timeout-sec

Is this only applicable when configure-sleep is set?

You mix the terms sleep and suspend a lot. Can you be more consistent.

> +    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"?)

> +
> +  - pcm-clockmode : PCM clock mode. 0-slave, 1-master.
> +    Default: 0.

Can be boolean (property present or not): pcm-clock-mode-master

> +
> +  - pcm-fillmethod : PCM fill method. 0 to 3.

pcm-fill-method

> +    Default: 2.
> +
> +  - pcm-fillnum : PCM number of fill bits. 0 to 3.
> +    Default: 0.

pcm-fill-bits

> +
> +  - pcm-fillvalue : PCM fill value. 0 to 7.
> +    Default: 3.

pcm-fill-value

> +
> +  - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-512Kbps,
> +    3-1024Kbps, 4-2048Kbps.

pcm-incall-bitclock-hz

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.

What does incall mean? What is the bit rate when not in a call?

> +    Default: 0.
> +
> +  - pcm-lsbfirst : PCM LSB first. 0 or 1.
> +    Default: 0.

Can be boolean

> +
> +  - pcm-rightjustify : PCM Justify. 0-left, 1-right.
> +    Default: 0.

Can be boolean

> +  - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI.
> +    Default: 0.

Can be boolean: pcm-routing-hci

> +
> +  - pcm-shortframesync : PCM sync. 0-short, 1-long.
> +    Default: 0.

Can be boolean

> +
> +  - pcm-syncmode : PCM sync mode. 0-slave, 1-master.
> +    Default: 0.

Can be boolean: pcm-sync-mode-master

Rob

  parent reply	other threads:[~2015-06-18 15:02 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-17 21:30 [PATCH v4 0/4] Broadcom Bluetooth UART device driver Ilya Faenson
2015-06-17 21:30 ` [PATCH v4 1/4] Broadcom Bluetooth UART Device Tree bindings Ilya Faenson
2015-06-17 21:33   ` Arend van Spriel
     [not found]   ` <1434576658-20730-2-git-send-email-ifaenson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-06-17 23:11     ` FW: " Ilya Faenson
2015-06-17 23:11       ` Ilya Faenson
     [not found]       ` <E0D3336E15B58B4294723AC879BA5E942634F2-Wwdb2uEOBX8N93KskqRXH71+IgudQmzARxWJa1zDYLQ@public.gmane.org>
2015-06-18 15:02         ` Rob Herring [this message]
2015-06-18 15:02           ` Rob Herring
     [not found]           ` <CAL_JsqK9kup3sm3qgDLqtT8ajrN1ee6zfXwOoZqoq9cXQNwE_w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-06-18 18:54             ` Ilya Faenson
2015-06-18 18:54               ` Ilya Faenson
     [not found]               ` <E0D3336E15B58B4294723AC879BA5E94263E58-Wwdb2uEOBX8N93KskqRXH71+IgudQmzARxWJa1zDYLQ@public.gmane.org>
2015-06-18 19:41                 ` Rob Herring
2015-06-18 19:41                   ` Rob Herring
     [not found]                   ` <CAL_JsqJAUTjNehtr_Af93k71mdSREb9pLMoSyDTm1S42VkA2sQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-06-18 20:37                     ` Ilya Faenson
2015-06-18 20:37                       ` Ilya Faenson
     [not found]                       ` <E0D3336E15B58B4294723AC879BA5E942640A3-Wwdb2uEOBX8N93KskqRXH71+IgudQmzARxWJa1zDYLQ@public.gmane.org>
2015-06-19 15:47                         ` Rob Herring
2015-06-19 15:47                           ` Rob Herring
     [not found]                           ` <CAL_JsqJKyj8sDwG82jb_CzXEDBN8aznR7eF5yTOiWruW8o3cng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-06-19 17:06                             ` Arend van Spriel
2015-06-19 17:06                               ` Arend van Spriel
     [not found]                               ` <55844C1E.8020301-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-06-19 18:49                                 ` Rob Herring
2015-06-19 18:49                                   ` Rob Herring
     [not found]                                   ` <CAL_JsqLr7JZ9=i2XuWR0_FZDyxpDRwDS5dkSpzjKA-8V=-mpOw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-06-19 19:20                                     ` Arend van Spriel
2015-06-19 19:20                                       ` Arend van Spriel
     [not found]                                       ` <55846B7D.1060601-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-06-29 18:36                                         ` Arend van Spriel
2015-06-29 18:36                                           ` Arend van Spriel
     [not found]                                           ` <55919023.200-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-07-02 18:26                                             ` Rob Herring
2015-07-02 18:26                                               ` Rob Herring
2015-06-19 20:37                             ` Ilya Faenson
2015-06-19 20:37                               ` Ilya Faenson
2015-06-19 19:23   ` Fabio Estevam
2015-06-19 20:40     ` Ilya Faenson
2015-06-17 21:30 ` [PATCH v4 2/4] hci_uart: line discipline enhancements Ilya Faenson
2015-06-17 21:50   ` Marcel Holtmann
2015-06-17 21:53     ` Ilya Faenson
2015-06-18  9:46       ` Frederic Danis
2015-06-18 10:17         ` Marcel Holtmann
2015-06-17 21:30 ` [PATCH v4 3/4] btbcm_uart: Broadcom UART Platform Driver Ilya Faenson
2015-06-17 21:30 ` [PATCH v4 4/4] hci_bcm: Broadcom UART protocol enhancements Ilya Faenson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAL_JsqK9kup3sm3qgDLqtT8ajrN1ee6zfXwOoZqoq9cXQNwE_w@mail.gmail.com \
    --to=robherring2-re5jqeeqqe8avxtiumwx3w@public.gmane.org \
    --cc=arend-dY08KVG/lbpWk0Htik3J/w@public.gmane.org \
    --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=ifaenson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org \
    --cc=linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.