Received-SPF: pass (google.com: domain of linux-kernel+bounces-25346-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sat, 13 Jan 2024 19:04:52 +0100
From: Christoph Winklhofer <cj.winklhofer@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH v4 1/3] dt-bindings: w1: UART 1-Wire bus
Message-ID: <ZaLQxGjjmA_iKOv2@cjw-notebook>
References: <20240106-w1-uart-v4-0-7fe1378a8b3e@gmail.com>
 <20240106-w1-uart-v4-1-7fe1378a8b3e@gmail.com>
 <20240113013917.GA3795949-robh@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240113013917.GA3795949-robh@kernel.org>

On Fri, Jan 12, 2024 at 07:39:17PM -0600, Rob Herring wrote:
> On Sat, Jan 06, 2024 at 05:02:24PM +0100, Christoph Winklhofer wrote:
> > Add device tree binding for UART 1-Wire bus.
> > 
> > Signed-off-by: Christoph Winklhofer <cj.winklhofer@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/w1/w1-uart.yaml | 62 +++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/w1/w1-uart.yaml b/Documentation/devicetree/bindings/w1/w1-uart.yaml
> > new file mode 100644
> > index 000000000000..6b90693b2ca0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/w1/w1-uart.yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/w1/w1-uart.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
..
> > +properties:
> > +  compatible:
> > +    const: w1-uart
> > +
> > +  reset-speed:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 9600
> > +    description: |
> 
> Don't need '|' if no formatting
> 

Ok.

> > +      The baud rate for the 1-Wire reset and presence detect.
> > +
> > +  touch_0-speed:
> 
> Don't use '_' in property names.
> 
> I'm somewhat familar with 1-wire, but I don't get what 'touch' means 
> here. I assume these are low and high times which are a function of the 
> baudrate.
> 

I change the name to 'write-0-speed' and 'write-1-speed'. The function
in the w1-framework is named 'touch_bit' - therefore the previous
naming. 

It is the baud-rate used in the 1-Wire cycle to write a 0-Bit
(write-0-speed) and to perform a 1-Wire cycle that writes a 1-Bit and
reads a 0-Bit or 1-Bit (write-1-speed).

> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 115200
> > +    description: |
> > +      The baud rate for the 1-Wire write-0 cycle (touch bit 0).
> > +
> > +  touch_1-speed:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 115200
> > +    description: |
> > +      The baud rate for the 1-Wire write-1 and read cycle (touch bit 1).
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> 
> w1.txt says there can be a child node for the 1-wire device. You need 
> 'type: object'. Or w1.txt needs to be converted to schema and referenced 
> here (along with using unevaluatedProperties here instead).
> 
> Rob

Will change it to type object. However, I also tried to convert w1.txt
to w1.yaml - I am not quite sure, so I add an excerpt of my attempt.
The example in w1.txt builds fine. Is oneOf allowed as a top-level
property, the documentation only mentions allOf?
Only one child should be allowed, but I did not find a way to restrict
this.

# w1-uart.yaml:
unevaluatedProperties:
  $ref: w1.yaml

# w1.yaml:
properties:
  compatible:
    enum:
      - maxim,ds2760

oneOf:
  - $ref: /schemas/power/supply/maxim,ds2760.yaml

unevaluatedProperties: false
#

Thank you for the review!

Kind regards,
Christoph