Re: [PATCH 6/6] dt-bindings: clk: realtek: add rtd1619 clock controller bindings

["Andreas Färber" <afaerber@suse.de>]

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)