Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Nov 2022 16:19:13 -0600
From:   Rob Herring <robh@kernel.org>
To:     Alex Helms <alexander.helms.jy@renesas.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-clk@vger.kernel.org, krzysztof.kozlowski+dt@linaro.org,
        sboyd@kernel.org, mturquette@baylibre.com, geert+renesas@glider.be
Subject: Re: [PATCH v3 1/2] dt-bindings: clock: Add bindings for Renesas ProXO
Message-ID: <20221116221913.GA1122997-robh@kernel.org>
References: <20221115233749.10161-1-alexander.helms.jy@renesas.com>
 <20221115233749.10161-2-alexander.helms.jy@renesas.com>
 <83492f7f-1217-69aa-8b38-ec1f08995832@linaro.org>
 <abc55598-8833-c4b2-aadc-c4e589aa775a@renesas.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <abc55598-8833-c4b2-aadc-c4e589aa775a@renesas.com>
Precedence: bulk

On Wed, Nov 16, 2022 at 01:17:54PM -0700, Alex Helms wrote:
> On 11/16/2022 1:50 AM, Krzysztof Kozlowski wrote:
> > On 16/11/2022 00:37, Alex Helms wrote:
> >> Add dt bindings for the Renesas ProXO oscillator.
> >>
> >> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> >> ---
> >>  .../bindings/clock/renesas,proxo.yaml         | 51 +++++++++++++++++++
> >>  MAINTAINERS                                   |  5 ++
> >>  2 files changed, 56 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/renesas,proxo.yaml b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> >> new file mode 100644
> >> index 000000000..ff960196d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/renesas,proxo.yaml
> >> @@ -0,0 +1,51 @@
> >> +# 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%2Cproxo.yaml%23&amp;data=05%7C01%7Calexander.helms.jy%40renesas.com%7C248dc84dbca44a4013d408dac7af9cf1%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041854305996374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=iGbtWJLjV%2FM%2Fps0lPk7f40bMzX8qdt8VZBtH9J4LdOw%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%7C248dc84dbca44a4013d408dac7af9cf1%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638041854305996374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=zYh4aHuw6G6A35rXBD7FTKeFrC7Hfcxag60ghkKUaGA%3D&amp;reserved=0
> >> +
> >> +title: Renesas ProXO Oscillator Device Tree Bindings
> > 
> > Same comments as for your other patch. All the same...
> > 
> >> +
> >> +maintainers:
> >> +  - Alex Helms <alexander.helms.jy@renesas.com>
> >> +
> >> +description:
> >> +  Renesas ProXO is a family of programmable ultra-low phase noise
> >> +  quartz-based oscillators.
> >> +
> >> +properties:
> >> +  '#clock-cells':
> >> +    const: 0
> >> +
> >> +  compatible:
> >> +    enum:
> >> +      - renesas,proxo-xp
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clock-output-names:
> >> +    maxItems: 1
> >> +
> >> +  renesas,crystal-frequency-hz:
> >> +    description: Internal crystal frequency, default is 50000000 (50MHz)
> > 
> > If it is internal, then it is fixed, right? Embedded in the chip, always
> > the same. Why do you need to specify it?
> > 
> 
> Yes, it is embedded in the package but there are different values
> depending on what chip is ordered and therefore must be specified for
> some configurations.
> 
> I'm also not sure what you mean by me ignoring Rob's comment. I
> explained my case for calling it "crystal-frequency-hz" and moved
> forward. I can call it "clock-frequency" if you want but I find that
> more confusing. Yes it is a built-in name in the schema but it seems to
> be used in a variety of ways. Some devices use it as a crystal input,
> but most seem to use it as the desired output frequency of the device
> which is not how it is used here. Therefore I chose a more clear name
> that better reflects what it is doing.

I think it is fine as-is. But you should have 'default: 50000000' 
instead of prose.

Rob