linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Andreas Färber" <afaerber@suse.de>
To: James Tai <james.tai@realtek.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	linux-realtek-soc@lists.infradead.org,
	cylee12 <cylee12@realtek.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	Philipp Zabel <p.zabel@pengutronix.de>
Subject: Re: [PATCH 6/6] dt-bindings: clk: realtek: add rtd1619 clock controller bindings
Date: Tue, 3 Dec 2019 19:55:24 +0100	[thread overview]
Message-ID: <f16bc1ef-8b69-feb4-bf1a-b015d7f8618e@suse.de> (raw)
In-Reply-To: <20191203074513.9416-7-james.tai@realtek.com>

Hi James,

[dropping Palmer, adding Philipp]

Am 03.12.19 um 08:45 schrieb James Tai:
> From: cylee12 <cylee12@realtek.com>

Author.

$subject: clk vs. clock prefix

Lacking a commit message here.

> 
> Signed-off-by: Cheng-Yu Lee <cylee12@realtek.com>
> Signed-off-by: James Tai <james.tai@realtek.com>
> ---
>  .../bindings/clock/realtek,clocks.txt         | 38 +++++++++++++++++++

Please use YAML schema for any new bindings.

>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/realtek,clocks.txt

This patch needs to be ordered before patches using the binding in a
driver or DT. In this case it should've been squashed into 1/6.

> diff --git a/Documentation/devicetree/bindings/clock/realtek,clocks.txt b/Documentation/devicetree/bindings/clock/realtek,clocks.txt
> new file mode 100644
> index 000000000000..db101508ac6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/realtek,clocks.txt
> @@ -0,0 +1,38 @@
> +Realtek Clock/Reset Controller
> +==============================
> +
> +Realtek CRT/ISO controller device-tree binding for Realtek Platforms.
> +
> +This binding uses the common clock binding[1].
> +
> +The controller node should be the child of a syscon node with the required
> +propertise:
> +
> +- compatible :
> +	should contain only one of the following:
> +		"realtek,rtd1619-cc" for RTD1619 CRT clock controller,
> +		"realtek,rtd1619-ic" for RTD1619 ISO clock controller,

-ic does not strike me as the best name, can we go with -iso-something
for consistency?

> +
> +- #clock-cells : should be 1.
> +
> +- #reset-cells : should be 1.
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Example:
> +
> +	crt@98000000 {

crt: syscon@...

Always prefer generic node names when possible.

> +		compatible = "realtek,rtd1619-crt", "simple-mfd", "syscon";

1) You must not use undefined compatible strings in your example! If we
want to use such compatibles (which I agree with in principle), then we
need to post separate bindings patches before you do so. The big issue
there is how to name them to work across SoC families. For that reason
my syscon series did not include dt-bindings, to not hold us up with
them. Drop it here for now?

2) You must retain the valid order, here defined by the syscon binding.
Like I said for the Mjolnir .dts. If we consequently use YAML schemas,
then you can check your .dts files with make dtbs_check and hopefully
notice it yourself before I complain. .dtsi patches are sadly missing in
this series, so you could only run limited make dt_binding_check.

> +		reg = <0x98000000 0x1000>;
> +
> +		cc: cc@98000000 {

cc: clock-controller@...

But you must not give a unit address in absence of reg.

> +			compatible = "realtek,rtd1619-cc";

reg missing. When you add it, you need #address-cells and #size-cells
above, too. Also ranges for completeness. In YAML it gets compile-tested
and should not sprout warnings.

> +			#clock-cells = <1>;
> +			#reset-cells = <1>;

BTW given the complex mappings that you attempt, wouldn't it be easier
to use #reset-cells = <2>? In that case one could again argue that a
per-bank node/driver will be easier.

> +		};
> +	};

Haven't tested this yet, but I wonder whether we could just use
"realtek,rtd1619-crt" for the clock controller directly and still use
the same node as syscon mfd? If not, it might be nice to describe in the
child node's reg what exactly is covered instead of just <0x0 0x1000>.

My point here is that the DT describes the hardware, but that does not
dictate how the Linux drivers bind to DT. clk is no platform_driver, so
you can have clk and reset drivers binding to the same DT compatible.
Did that for STM32 CRT once. However, don't hide the binding under clock
if it's really mfd - someone looking for reset bindings is going to have
a hard time finding them under clock.

> +
> +	consumer {
> +		clocks = <&cc CC_CKE_GSPI>;
> +	};
> +

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

  reply	other threads:[~2019-12-03 18:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20191203074513.9416-1-james.tai@realtek.com>
2019-12-03  7:45 ` [PATCH 3/6] clk: realtek: add common clock support for Realtek SoCs James Tai
2019-12-03 15:28   ` Andreas Färber
2019-12-03  7:45 ` [PATCH 4/6] clk: realtek: add reset controller " James Tai
2019-12-03 17:46   ` Andreas Färber
2019-12-03  7:45 ` [PATCH 5/6] clk: realtek: add rtd1619 controllers James Tai
2019-12-03  7:45 ` [PATCH 6/6] dt-bindings: clk: realtek: add rtd1619 clock controller bindings James Tai
2019-12-03 18:55   ` Andreas Färber [this message]
     [not found] <20191203073540.9321-1-james.tai@realtek.com>
2019-12-03  7:35 ` James Tai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f16bc1ef-8b69-feb4-bf1a-b015d7f8618e@suse.de \
    --to=afaerber@suse.de \
    --cc=cylee12@realtek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=james.tai@realtek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-realtek-soc@lists.infradead.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=matthias.bgg@gmail.com \
    --cc=mturquette@baylibre.com \
    --cc=p.zabel@pengutronix.de \
    --cc=paul.walmsley@sifive.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH 6/6] dt-bindings: clk: realtek: add rtd1619 clock controller bindings' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).