Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
From: "Andreas Färber" <afaerber@suse.de>
To: James Tai <james.tai@realtek.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, cylee12 <cylee12@realtek.com>,
	linux-realtek-soc@lists.infradead.org,
	Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	linux-mediatek@lists.infradead.org,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-riscv@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 6/6] dt-bindings: clk: realtek: add rtd1619 clock controller bindings
Date: Tue, 3 Dec 2019 19:55:24 +0100
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 index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-03  7:45 [PATCH 0/6] arm64: Realtek RTD1619 clock and reset controllers James Tai
2019-12-03  7:45 ` [PATCH 1/6] dt-bindings: clock: add bindings for RTD1619 clocks James Tai
2019-12-03  9:32   ` Andreas Färber
2019-12-04  4:11     ` James Tai
2019-12-04 11:25       ` Andreas Färber
2019-12-03  7:45 ` [PATCH 2/6] dt-bindings: reset: add bindings for rtd1619 reset controls James Tai
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]
  -- strict thread matches above, loose matches on Subject: below --
2019-12-03  7:35 [PATCH 0/6] arm64: Realtek RTD1619 clock and reset controllers James Tai
2019-12-03  7:35 ` [PATCH 6/6] dt-bindings: clk: realtek: add rtd1619 clock controller bindings James Tai

Reply instructions:

You may reply publically 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 \
    /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

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org
	public-inbox-index linux-riscv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git