Re: [PATCH v3 1/2] dt-bindings: input: Add DT bindings for Whiskers switch

From: Ikjoon Jang <ikjn@chromium.org>
To: Rob Herring <robh@kernel.org>
Cc: linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	linux-kernel@vger.kernel.org,
	Nicolas Boitchat <drinkcat@chromium.org>
Subject: Re: [PATCH v3 1/2] dt-bindings: input: Add DT bindings for Whiskers switch
Date: Tue, 29 Oct 2019 15:22:52 +0800	[thread overview]
Message-ID: <CAATdQgDxC_1EH4cBqf7deEqRjtRU1s4o=L8vOztvVZ7NS_q9Mw@mail.gmail.com> (raw)
In-Reply-To: <20191025194101.GA4734@bogus>

On Sat, Oct 26, 2019 at 3:41 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Oct 23, 2019 at 10:43:48AM +0800, Ikjoon Jang wrote:
> > Add the DT binding document for Hammer's TABLET_MODE switch.
>
> This doesn't have any properties. Why does it need to be in DT? Just
> have the EC driver instantiate it.
>
> >
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > ---
> >  .../devicetree/bindings/input/cros-cbas.yaml  | 22 +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/cros-cbas.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/cros-cbas.yaml b/Documentation/devicetree/bindings/input/cros-cbas.yaml
> > new file mode 100644
> > index 000000000000..3bc989c6a295
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/cros-cbas.yaml
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: GPL-2.0
>
> (GPL-2.0-only OR BSD-2-Clause) for new bindings please.

This will be GPL-2.0-only in next patch.

>
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/cros-cbas.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ChromeOS Hammer's Base Attached Switch
> > +
> > +maintainers:
> > +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > +
> > +description:
> > +  This device is used to signal when a detachable base is attached to a
> > +  Chrome OS tablet. The node for this device must be under a cros-ec node
> > +  like google,cros-ec-spi or google,cros-ec-i2c.
>
> This should probably just be part of an EC schema where it can be
> enforced that this is a child node. It could be either embedded into it
> or referenced. I'd lean toward the former given this is only a
> compatible string...

Sorry for basic questions here but I'm a bit confused,
"embedding" means that cros-ec.txt should be converted
into json schema first and embed this child bindings into there?

Many Chrome OS tablets have a switch exposed by 'cros-ec-keyb' device
which is directly controlled by EC. But this 'cros_cbas' switch device is for
other types of tablets which need additional logics on EC and HID.

Currently it doesn't need to have other properties, but maybe it could require
additional properties or device links in the future, plus this device
is not just
a EC subdevice, so I'd prefer this to be a separate binding.

>
> > +
> > +properties:
> > +  compatible:
> > +    const: google,cros-cbas
> > +
> > +required:
> > +  - compatible
>
> Add here:
>
> additionalProperties: false.

Okay, I will add this in a new patch set , thank you!

>
> > --
> > 2.23.0.866.gb869b98d4c-goog
> >