Re: [PATCH v4 1/3] dt-bindings: w1: UART 1-Wire bus

[Krzysztof Kozlowski]

On 13/01/2024 19:04, Christoph Winklhofer wrote:
> 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@xxxxxxxxx>
>>> ---
>>>  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).


Then probably -bps:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml


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

w1 describes entire class of the devices, so it should not have
compatibles. Look how other buses do it.
> 
> oneOf:
>   - $ref: /schemas/power/supply/maxim,ds2760.yaml
> 
> unevaluatedProperties: false


Best regards,
Krzysztof