[v3] dt-bindings: mfd: Convert ChromeOS EC bindings to json-schema

Convert the ChromeOS EC bindings to json-schema.

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---
v3: node name changed in rpmsg example
v2: cleanup description, fix typos, remove LPC, add add RPMSG example
---
 .../devicetree/bindings/mfd/cros-ec.txt       |  76 ------------
 .../devicetree/bindings/mfd/cros-ec.yaml      | 111 ++++++++++++++++++
 2 files changed, 111 insertions(+), 76 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.yaml

Hi Ikjoon,

On 21/1/20 8:47, Ikjoon Jang wrote:
> Convert the ChromeOS EC bindings to json-schema.
> 
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> ---
> v3: node name changed in rpmsg example
> v2: cleanup description, fix typos, remove LPC, add add RPMSG example
> ---
>  .../devicetree/bindings/mfd/cros-ec.txt       |  76 ------------
>  .../devicetree/bindings/mfd/cros-ec.yaml      | 111 ++++++++++++++++++
>  2 files changed, 111 insertions(+), 76 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.yaml
> 

Still in mfd directory, this is not a MFD driver, please move to chrome/cros-ec.yaml

Thanks,
 Enric

> diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.txt b/Documentation/devicetree/bindings/mfd/cros-ec.txt
> deleted file mode 100644
> index 4860eabd0f72..000000000000
> --- a/Documentation/devicetree/bindings/mfd/cros-ec.txt
> +++ /dev/null
> @@ -1,76 +0,0 @@
> -ChromeOS Embedded Controller
> -
> -Google's ChromeOS EC is a Cortex-M device which talks to the AP and
> -implements various function such as keyboard and battery charging.
> -
> -The EC can be connect through various means (I2C, SPI, LPC, RPMSG) and the
> -compatible string used depends on the interface. Each connection method has
> -its own driver which connects to the top level interface-agnostic EC driver.
> -Other Linux driver (such as cros-ec-keyb for the matrix keyboard) connect to
> -the top-level driver.
> -
> -Required properties (I2C):
> -- compatible: "google,cros-ec-i2c"
> -- reg: I2C slave address
> -
> -Required properties (SPI):
> -- compatible: "google,cros-ec-spi"
> -- reg: SPI chip select
> -
> -Required properties (RPMSG):
> -- compatible: "google,cros-ec-rpmsg"
> -
> -Optional properties (SPI):
> -- google,cros-ec-spi-pre-delay: Some implementations of the EC need a little
> -  time to wake up from sleep before they can receive SPI transfers at a high
> -  clock rate. This property specifies the delay, in usecs, between the
> -  assertion of the CS to the start of the first clock pulse.
> -- google,cros-ec-spi-msg-delay: Some implementations of the EC require some
> -  additional processing time in order to accept new transactions. If the delay
> -  between transactions is not long enough the EC may not be able to respond
> -  properly to subsequent transactions and cause them to hang. This property
> -  specifies the delay, in usecs, introduced between transactions to account
> -  for the time required by the EC to get back into a state in which new data
> -  can be accepted.
> -
> -Required properties (LPC):
> -- compatible: "google,cros-ec-lpc"
> -- reg: List of (IO address, size) pairs defining the interface uses
> -
> -Optional properties (all):
> -- google,has-vbc-nvram: Some implementations of the EC include a small
> -  nvram space used to store verified boot context data. This boolean flag
> -  is used to specify whether this nvram is present or not.
> -
> -Example for I2C:
> -
> -i2c@12ca0000 {
> -	cros-ec@1e {
> -		reg = <0x1e>;
> -		compatible = "google,cros-ec-i2c";
> -		interrupts = <14 0>;
> -		interrupt-parent = <&wakeup_eint>;
> -		wakeup-source;
> -	};
> -
> -
> -Example for SPI:
> -
> -spi@131b0000 {
> -	ec@0 {
> -		compatible = "google,cros-ec-spi";
> -		reg = <0x0>;
> -		interrupts = <14 0>;
> -		interrupt-parent = <&wakeup_eint>;
> -		wakeup-source;
> -		spi-max-frequency = <5000000>;
> -		controller-data {
> -		cs-gpio = <&gpf0 3 4 3 0>;
> -		samsung,spi-cs;
> -		samsung,spi-feedback-delay = <2>;
> -		};
> -	};
> -};
> -
> -
> -Example for LPC is not supplied as it is not yet implemented.
> diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.yaml b/Documentation/devicetree/bindings/mfd/cros-ec.yaml
> new file mode 100644
> index 000000000000..6a5b87cebcfa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/cros-ec.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/cros-ec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ChromeOS Embedded Controller
> +
> +maintainers:
> +  - Benson Leung <bleung@chromium.org>
> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> +  - Guenter Roeck <groeck@chromium.org>
> +
> +description: |
> +  Google's ChromeOS EC is a microcontroller which talks to the AP and
> +  implements various functions such as keyboard and battery charging.
> +  The EC can be connected through various interfaces (I2C, SPI, and others)
> +  and the compatible string specifies which interface is being used.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description:
> +          For implementations of the EC is connected through I2C.
> +        const: google,cros-ec-i2c
> +      - description:
> +          For implementations of the EC is connected through SPI.
> +        const: google,cros-ec-spi
> +      - description:
> +          For implementations of the EC is connected through RPMSG.
> +        const: google,cros-ec-rpmsg
> +
> +  google,has-vbc-nvram:
> +    description: |
> +      Some implementations of the EC include a small
> +      nvram space used to store verified boot context data.
> +      This boolean flag is used to specify whether this nvram is
> +      present or not.
> +    type: boolean
> +
> +required:
> +  - compatible
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: google,cros-ec-i2c
> +    then:
> +      properties:
> +        reg:
> +          description: I2C slave address
> +          maxItems: 1
> +      required:
> +        - reg
> +  - if:
> +      properties:
> +        compatible:
> +          const: google,cros-ec-spi
> +    then:
> +      properties:
> +        reg:
> +          description: SPI chip select
> +          maxItems: 1
> +        google,cros-ec-spi-pre-delay:
> +          description: |
> +            This property specifies the delay in usecs between the
> +            assertion of the CS and the first clock pulse.
> +        google,cros-ec-spi-msg-delay:
> +          description: |
> +            This property specifies the delay in usecs between messages.
> +      required:
> +        - reg
> +
> +examples:
> +  # Example for I2C
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cros-ec@1e {
> +            compatible = "google,cros-ec-i2c";
> +            reg = <0x1e>;
> +            interrupts = <6 0>;
> +            interrupt-parent = <&gpx1>;
> +        };
> +    };
> +  # Example for SPI
> +  - |
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cros-ec@0 {
> +            compatible = "google,cros-ec-spi";
> +            reg = <0x0>;
> +            google,cros-ec-spi-msg-delay = <30>;
> +            interrupts = <99 0>;
> +            interrupt-parent = <&gpio7>;
> +            spi-max-frequency = <5000000>;
> +        };
> +    };
> +  # Example for RPMSG
> +  - |
> +    scp0 {
> +        cros-ec@0 {
> +            compatible = "google,cros-ec-rpmsg";
> +        };
> +    };
> +...
>

On Tue, Jan 21, 2020 at 03:47:27PM +0800, Ikjoon Jang wrote:
> Convert the ChromeOS EC bindings to json-schema.
> 
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> ---
> v3: node name changed in rpmsg example
> v2: cleanup description, fix typos, remove LPC, add add RPMSG example
> ---
>  .../devicetree/bindings/mfd/cros-ec.txt       |  76 ------------
>  .../devicetree/bindings/mfd/cros-ec.yaml      | 111 ++++++++++++++++++
>  2 files changed, 111 insertions(+), 76 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.yaml

> -Required properties (LPC):
> -- compatible: "google,cros-ec-lpc"
> -- reg: List of (IO address, size) pairs defining the interface uses

Where did this go?


> diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.yaml b/Documentation/devicetree/bindings/mfd/cros-ec.yaml
> new file mode 100644
> index 000000000000..6a5b87cebcfa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/cros-ec.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/cros-ec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ChromeOS Embedded Controller
> +
> +maintainers:
> +  - Benson Leung <bleung@chromium.org>
> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> +  - Guenter Roeck <groeck@chromium.org>
> +
> +description: |
> +  Google's ChromeOS EC is a microcontroller which talks to the AP and
> +  implements various functions such as keyboard and battery charging.
> +  The EC can be connected through various interfaces (I2C, SPI, and others)
> +  and the compatible string specifies which interface is being used.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description:
> +          For implementations of the EC is connected through I2C.
> +        const: google,cros-ec-i2c
> +      - description:
> +          For implementations of the EC is connected through SPI.
> +        const: google,cros-ec-spi
> +      - description:
> +          For implementations of the EC is connected through RPMSG.
> +        const: google,cros-ec-rpmsg
> +
> +  google,has-vbc-nvram:
> +    description: |

You can drop '|' if there's no formatting to maintain. And you should 
reflow this.

> +      Some implementations of the EC include a small
> +      nvram space used to store verified boot context data.
> +      This boolean flag is used to specify whether this nvram is
> +      present or not.
> +    type: boolean
> +
> +required:
> +  - compatible
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: google,cros-ec-i2c
> +    then:
> +      properties:
> +        reg:
> +          description: I2C slave address
> +          maxItems: 1
> +      required:
> +        - reg
> +  - if:
> +      properties:
> +        compatible:
> +          const: google,cros-ec-spi
> +    then:
> +      properties:
> +        reg:
> +          description: SPI chip select
> +          maxItems: 1
> +        google,cros-ec-spi-pre-delay:
> +          description: |
> +            This property specifies the delay in usecs between the
> +            assertion of the CS and the first clock pulse.

Needs a type reference at a minumum and ideally some constraints.

> +        google,cros-ec-spi-msg-delay:
> +          description: |
> +            This property specifies the delay in usecs between messages.

Same here.

> +      required:
> +        - reg
> +

Add:
additionalProperties: false

> +examples:
> +  # Example for I2C
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cros-ec@1e {
> +            compatible = "google,cros-ec-i2c";
> +            reg = <0x1e>;
> +            interrupts = <6 0>;

Not documented.

> +            interrupt-parent = <&gpx1>;
> +        };
> +    };
> +  # Example for SPI
> +  - |
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cros-ec@0 {
> +            compatible = "google,cros-ec-spi";
> +            reg = <0x0>;
> +            google,cros-ec-spi-msg-delay = <30>;
> +            interrupts = <99 0>;
> +            interrupt-parent = <&gpio7>;
> +            spi-max-frequency = <5000000>;
> +        };
> +    };
> +  # Example for RPMSG
> +  - |
> +    scp0 {
> +        cros-ec@0 {
> +            compatible = "google,cros-ec-rpmsg";
> +        };
> +    };
> +...
> -- 
> 2.25.0.341.g760bfbb309-goog
>

On Mon, Jan 27, 2020 at 04:57:13PM +0100, Enric Balletbo i Serra wrote:
> Hi Ikjoon,
> 
> On 21/1/20 8:47, Ikjoon Jang wrote:
> > Convert the ChromeOS EC bindings to json-schema.
> > 
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > ---
> > v3: node name changed in rpmsg example
> > v2: cleanup description, fix typos, remove LPC, add add RPMSG example
> > ---
> >  .../devicetree/bindings/mfd/cros-ec.txt       |  76 ------------
> >  .../devicetree/bindings/mfd/cros-ec.yaml      | 111 ++++++++++++++++++
> >  2 files changed, 111 insertions(+), 76 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
> >  create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.yaml
> > 
> 
> Still in mfd directory, this is not a MFD driver, please move to chrome/cros-ec.yaml

Please don't. Looks to me like the chrome EC has multiple functions and 
is a device.

Bindings directories are often 1:1 with kernel driver directories, but 
not always.

Rob

Hi Rob,

On 27/1/20 17:12, Rob Herring wrote:
> On Mon, Jan 27, 2020 at 04:57:13PM +0100, Enric Balletbo i Serra wrote:
>> Hi Ikjoon,
>>
>> On 21/1/20 8:47, Ikjoon Jang wrote:
>>> Convert the ChromeOS EC bindings to json-schema.
>>>
>>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
>>> ---
>>> v3: node name changed in rpmsg example
>>> v2: cleanup description, fix typos, remove LPC, add add RPMSG example
>>> ---
>>>  .../devicetree/bindings/mfd/cros-ec.txt       |  76 ------------
>>>  .../devicetree/bindings/mfd/cros-ec.yaml      | 111 ++++++++++++++++++
>>>  2 files changed, 111 insertions(+), 76 deletions(-)
>>>  delete mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.yaml
>>>
>>
>> Still in mfd directory, this is not a MFD driver, please move to chrome/cros-ec.yaml
> 
> Please don't. Looks to me like the chrome EC has multiple functions and 
> is a device.
> 

This binding is part of the Chrome EC core and we explicitly moved all this out
from mfd subsytem in the past because wasn't really fit as mfd driver.

We still have one device driver (cros_ec_dev) in mfd, this is the one that
instantiates all the subdevices but the above binding is not related to this
one. All the core/bus part is in platform/chrome, looks to me more coherent have
the binding accordingly.

Anyway I'll let you the final decision, nothing that worries me too much.

Thanks,
 Enric

> Bindings directories are often 1:1 with kernel driver directories, but 
> not always.
> 
> Rob
>

Thanks for the reviews,
I will send v4 soon, sorry for the late reply.

On Tue, Jan 28, 2020 at 12:25 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Rob,
>
> On 27/1/20 17:12, Rob Herring wrote:
> > On Mon, Jan 27, 2020 at 04:57:13PM +0100, Enric Balletbo i Serra wrote:
> >> Hi Ikjoon,
> >>
> >> On 21/1/20 8:47, Ikjoon Jang wrote:
> >>> Convert the ChromeOS EC bindings to json-schema.
> >>>
> >>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> >>> ---
> >>> v3: node name changed in rpmsg example
> >>> v2: cleanup description, fix typos, remove LPC, add add RPMSG example
> >>> ---
> >>>  .../devicetree/bindings/mfd/cros-ec.txt       |  76 ------------
> >>>  .../devicetree/bindings/mfd/cros-ec.yaml      | 111 ++++++++++++++++++
> >>>  2 files changed, 111 insertions(+), 76 deletions(-)
> >>>  delete mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
> >>>  create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.yaml
> >>>
> >>
> >> Still in mfd directory, this is not a MFD driver, please move to chrome/cros-ec.yaml
> >
> > Please don't. Looks to me like the chrome EC has multiple functions and
> > is a device.
> >
>
> This binding is part of the Chrome EC core and we explicitly moved all this out
> from mfd subsytem in the past because wasn't really fit as mfd driver.
>
> We still have one device driver (cros_ec_dev) in mfd, this is the one that
> instantiates all the subdevices but the above binding is not related to this
> one. All the core/bus part is in platform/chrome, looks to me more coherent have
> the binding accordingly.
>
> Anyway I'll let you the final decision, nothing that worries me too much.
>
> Thanks,
>  Enric
>
> > Bindings directories are often 1:1 with kernel driver directories, but
> > not always.

Converting the format at this time. I will keep the same directory in v4.
It seems that cros_ec_dev device driver should be changed before moving.

> >
> > Rob
> >

On Tue, Jan 28, 2020 at 12:05 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Jan 21, 2020 at 03:47:27PM +0800, Ikjoon Jang wrote:
> > Convert the ChromeOS EC bindings to json-schema.
> >
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > ---
> > v3: node name changed in rpmsg example
> > v2: cleanup description, fix typos, remove LPC, add add RPMSG example
> > ---
> >  .../devicetree/bindings/mfd/cros-ec.txt       |  76 ------------
> >  .../devicetree/bindings/mfd/cros-ec.yaml      | 111 ++++++++++++++++++
> >  2 files changed, 111 insertions(+), 76 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
> >  create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.yaml
>
> > -Required properties (LPC):
> > -- compatible: "google,cros-ec-lpc"
> > -- reg: List of (IO address, size) pairs defining the interface uses
>
> Where did this go?

I'm not sure about the details or future plans on LPC interface,
but I guess LPC has been just a future plan without any usages, so removed it.

>
>
> > diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.yaml b/Documentation/devicetree/bindings/mfd/cros-ec.yaml
> > new file mode 100644
> > index 000000000000..6a5b87cebcfa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/cros-ec.yaml
> > @@ -0,0 +1,111 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/cros-ec.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ChromeOS Embedded Controller
> > +
> > +maintainers:
> > +  - Benson Leung <bleung@chromium.org>
> > +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > +  - Guenter Roeck <groeck@chromium.org>
> > +
> > +description: |
> > +  Google's ChromeOS EC is a microcontroller which talks to the AP and
> > +  implements various functions such as keyboard and battery charging.
> > +  The EC can be connected through various interfaces (I2C, SPI, and others)
> > +  and the compatible string specifies which interface is being used.
> > +

I will drop | here also.

> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - description:
> > +          For implementations of the EC is connected through I2C.
> > +        const: google,cros-ec-i2c
> > +      - description:
> > +          For implementations of the EC is connected through SPI.
> > +        const: google,cros-ec-spi
> > +      - description:
> > +          For implementations of the EC is connected through RPMSG.
> > +        const: google,cros-ec-rpmsg
> > +
> > +  google,has-vbc-nvram:
> > +    description: |
>
> You can drop '|' if there's no formatting to maintain. And you should
> reflow this.

Okay,

>
> > +      Some implementations of the EC include a small
> > +      nvram space used to store verified boot context data.
> > +      This boolean flag is used to specify whether this nvram is
> > +      present or not.
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: google,cros-ec-i2c
> > +    then:
> > +      properties:
> > +        reg:
> > +          description: I2C slave address
> > +          maxItems: 1
> > +      required:
> > +        - reg
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: google,cros-ec-spi
> > +    then:
> > +      properties:
> > +        reg:
> > +          description: SPI chip select
> > +          maxItems: 1
> > +        google,cros-ec-spi-pre-delay:
> > +          description: |
> > +            This property specifies the delay in usecs between the
> > +            assertion of the CS and the first clock pulse.
>
> Needs a type reference at a minumum and ideally some constraints.

Got it, I will add a type reference here,
and for the constraints, these spi transaction delay cannot be bound.
I will just add default: 0 here.

>
> > +        google,cros-ec-spi-msg-delay:
> > +          description: |
> > +            This property specifies the delay in usecs between messages.
>
> Same here.
>
> > +      required:
> > +        - reg
> > +
>
> Add:
> additionalProperties: false

Ack.

>
> > +examples:
> > +  # Example for I2C
> > +  - |
> > +    i2c0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        cros-ec@1e {
> > +            compatible = "google,cros-ec-i2c";
> > +            reg = <0x1e>;
> > +            interrupts = <6 0>;
>
> Not documented.

Sorry but I can't understand this part, can you elaborate on here?
Do you mean adding comments here? or
need more documentation on cros-ec-i2c?

>
> > +            interrupt-parent = <&gpx1>;
> > +        };
> > +    };
> > +  # Example for SPI
> > +  - |
> > +    spi0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        cros-ec@0 {
> > +            compatible = "google,cros-ec-spi";
> > +            reg = <0x0>;
> > +            google,cros-ec-spi-msg-delay = <30>;
> > +            interrupts = <99 0>;
> > +            interrupt-parent = <&gpio7>;
> > +            spi-max-frequency = <5000000>;
> > +        };
> > +    };
> > +  # Example for RPMSG
> > +  - |
> > +    scp0 {
> > +        cros-ec@0 {
> > +            compatible = "google,cros-ec-rpmsg";
> > +        };
> > +    };
> > +...
> > --
> > 2.25.0.341.g760bfbb309-goog
> >