Re: [RFC 2/2] staging: jnx-connector: add device tree binding

From: Rob Herring <robh+dt@kernel.org>
To: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Frank Rowand <frowand.list@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alon Ronen <aronen@juniper.net>,
	Debjit Ghosh <dghosh@juniper.net>,
	Dhruva Diveneni <ddevineni@juniper.net>,
	Georgi Vlaev <gvlaev@juniper.net>,
	Guenter Roeck <linux@roeck-us.net>, Yu-Lin Lu <ylu@juniper.net>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	devel@driverdev.osuosl.org
Subject: Re: [RFC 2/2] staging: jnx-connector: add device tree binding
Date: Sat, 8 Oct 2016 11:05:41 -0500	[thread overview]
Message-ID: <CAL_JsqJoErfiaHKFnWUiswajTvYbmrRM5NOH8YFJ=bRjHQxK+w@mail.gmail.com> (raw)
In-Reply-To: <1475853372-21997-3-git-send-email-pantelis.antoniou@konsulko.com>

On Fri, Oct 7, 2016 at 10:16 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> From: Guenter Roeck <groeck@juniper.net>
>
> Add documentation for the Juniper connector driver.
>
> Signed-off-by: Alon Ronen <aronen@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> [Ported from Juniper kernel]
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  .../devicetree/bindings/jnx/jnx-connector          | 59 ++++++++++++++++++++++

Don't put vendor directories at top level. I assume this is not
stable, stick it in staging with the driver.

>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/jnx/jnx-connector
>
> diff --git a/Documentation/devicetree/bindings/jnx/jnx-connector b/Documentation/devicetree/bindings/jnx/jnx-connector
> new file mode 100644
> index 0000000..9262752
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/jnx/jnx-connector
> @@ -0,0 +1,59 @@
> +Device-Tree bindings for Juniper connector driver
> +
> +Required properties:
> +       - compatible - Must be one of
> +         - "jnx,pic-connector"
> +         - "jnx,sib-connector"
> +         - "jnx,fpc-connector"
> +         - "jnx,cb-connector"
> +         - "jnx,psm-connector"
> +         - "jnx,fan-connector"
> +         - "jnx,mp-connector"
> +         - "jnx,fpm-connector"
> +         - "jnx,spmb-connector"

I imagine all these have different signals. You need to be explicit
which properties below apply to which compatible.

> +         "simple-bus" must also be specified to facilitate instantiation.

No. This looks like abuse of simple-bus.

> +       - slot - slot number

Does reg work here instead?

> +
> +Optional properties:
> +       - assembly-id - Static assembly ID to be used for all adapters.
> +                       If specified, it is assumed that no ID eeprom is
> +                       present, and the provided assembly ID will always be
> +                       used to identify the adapter and the devicetree overlay
> +                       to load.
> +       - presence-detect-gpios - presence detect gpio pin
> +                                 If not provided, card is assumed
> +                                 to be always present.
> +       - attention-button-gpios - request button gpio pin
> +       - attention-button-ignore - set if kernel shall ignore
> +                                   attention button events
> +       - attention-button-holdtime - button hold time in milli-seconds
> +                                     (3s default)
> +       - activation-timeout - activation timeout in milli-seconds (10s default)
> +       - debounce-interval - debounce interval in milli-seconds
> +                               (applies to presence detect pin).

unit suffixes on these. You should know this.

> +       - power-enable-gpios - power enable gpio pin
> +       - power-status-gpios - power status gpio pin (mandatory if
> +                              power-enable-gpios is provided)
> +       - power-enable-timeout - power enable timeout in ms (1s default)
> +       - poweron-reset-delay - delay to be applied after poweron in
> +                               milli-seconds (default none)
> +       - reset-gpios - board reset gpio pin
> +       - led-green - phandle pointing to green LED
> +       - led-red - phandle pointing to red LED

Are all these gpios endpoints? i.e don't need some gpio translation
for downstream card devices? If so, then they mostly look fine.

> +       - sysfs-linkname - name to be user for the symbolic link under jnx/card

Err, NAK.

> +
> +Example node:
> +
> +       pic0 {
> +               compatible = "jnx,pic-connector", "simple-bus";
> +               slot = <0>;
> +               presence-detect-gpios = <&gpio1 148 0x10013>;
> +               attention-button-gpios = <&gpio20 150 0x10013>;
> +               power-enable-gpios = <&gpio1 154 0x0>;
> +               power-status-gpios = <&gpio1 151 0x3>;
> +               reset-gpios = <&gpio1 153 0x10000>;
> +               power-enable-timeout = <2000>;
> +               debounce-interval = <1>;
> +               led-green = <&pic0_green>;
> +               led-red = <&pic0_red>;
> +       };
> --
> 1.9.1
>