Re: [PATCH 1/2] dtbindings: clock: Add bindings for Renesas PhiClock

[Alex Helms]

On 11/16/2022 1:20 AM, Krzysztof Kozlowski wrote:
> On 15/11/2022 20:26, Alex Helms wrote:
>> Add dt bindings for the Renesas PhiClock clock generator.
>>
> 
> Subject: drop second, redundant "bindings"
> 
>> Signed-off-by: Alex Helms <alexander.helms.jy@xxxxxxxxxxx>
>> ---
>>  .../bindings/clock/renesas,phiclock.yaml      | 81 +++++++++++++++++++
>>  MAINTAINERS                                   |  5 ++
>>  2 files changed, 86 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
>> new file mode 100644
>> index 000000000..2b36534d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/renesas,phiclock.yaml
> 
> Filename based on compatible.
> 

As Geert mentioned in the other thread, this is a family of products but
the others cannot be added now.

>> @@ -0,0 +1,81 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Frenesas%2Cphiclock.yaml%23&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=J6kNqua%2FJf0c8HczRM8gU8%2Fm%2BhX6gSF2fqnf2n3wSbI%3D&amp;reserved=0
>> +$schema: https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ofZbC2sBnTpJR3KKzPVqhsFy28r4JjbJpaVGSuHKx38%3D&amp;reserved=0
>> +
>> +title: Renesas PhiClock Clock Generator Device Tree Bindings
> 
> Drop "Device Tree Bindings"
> 
>> +
>> +maintainers:
>> +  - Alex Helms <alexander.helms.jy@xxxxxxxxxxx>
>> +
>> +description: |
>> +  The Renesas PhiClock is a programmable I2C clock generator that provides
>> +  1 reference output and 2 clock outputs.
>> +
>> +  The driver supports spread spectrum but only if all configurations use the
> 
> Driver as in Linux driver? Drop entire paragraph. Bindings are about
> hardware, not driver.
> 
>> +  same spread spectrum parameters. If your configuration uses spread spectrum,
>> +  you must include renesas,ss-amount-percent, renesas,ss-modulation-hz, and
>> +  renesas,ss-direction in the device tree.
>> +
>> +properties:
> 
> compatible goes always first. Start your schema from example-schema.yaml.
> 
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xin-clkin
> 
> Just "xin" or entirely drop.

The pin name on the datasheet is "xin-clkin" and as the name implies it
can be a crystal or clock input. If the name were different it could be
confusing.

> 
>> +
>> +  clocks:
>> +    const: 1
>> +
>> +  compatible:
>> +    enum:
>> +      - renesas,9fgv1006
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  renesas,ss-amount-percent:
>> +    description: Spread spectrum absolute amount as hundredths of a percent, e.g. 150 is 1.50%.
> 
> What? If this is percent then it cannot be hundreds of percent. Percent
> is percent. Use appropriate units.
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas%2Fproperty-units.yaml&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C9c13a32848f3434e217108dac7ab69f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041836281252737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=6MULpJhPyyjWSo1SvPCrz6KidE1VEtiiNYk1O5wS1vI%3D&amp;reserved=0
> 

Values like 0.5% or 2.5% must be representable which is why this
property is an integer of hundredths of percent. How else would you
represent a non-integer percent?

>> +    minimum: 0
>> +    maximum: 500
>> +
>> +  renesas,ss-modulation-hz:
>> +    description: Spread spectrum modulation rate in Hz
>> +    minimum: 30000
>> +    maximum: 63000
>> +
>> +  renesas,ss-direction:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: Spread spectrum direction
>> +    enum: [ down, center ]
>> +
>> +required:
>> +  - clock-names
>> +  - '#clock-cells'
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    ref25: ref25m {
>> +      compatible = "fixed-clock";
>> +      #clock-cells = <0>;
>> +      clock-frequency = <25000000>;
>> +    };
> 
> Drop, it's obvious, isn't it?
> 

I disagree, this may be obvious to someone familiar with how clocks in
the device tree works but not long ago it was entirely new to me and
examples like these in the dt schemas were very helpful in getting the
device up and running. There are several other bindings that define
external crystals and reference clocks in this way.

>> +
> 
> Best regards,
> Krzysztof
>