[PATCH v2 1/4] dt-bindings: pwm: binding allwinner sun8i.

[PATCH v2 1/4] dt-bindings: pwm: binding allwinner sun8i.

[hao_zhang]

This patch adds allwinner sun8i pwm binding documents.

Signed-off-by: hao_zhang <hao5781286-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
new file mode 100644
index 0000000..e8c48be
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
@@ -0,0 +1,18 @@
+Allwinner sun8i R40/V40/T3 SoC PWM controller
+
+Required properties:
+  - compatible: should be one of:
+    - "allwinner,sun8i-r40-pwm"
+  - reg: physical base address and length of the controller's registers
+  - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
+    the cells format.
+  - clocks: From common clock binding, handle to the parent clock.
+
+Example:
+
+pwm: pwm@1c23400 {
+	     compatible = "allwinner,sun8i-pwm";
+	     reg = <0x01c23400 0x154>;
+	     clocks = <&osc24M>;
+	     #pwm-cells = <3>;
+};
-- 
2.7.4

Re: [PATCH v2 1/4] dt-bindings: pwm: binding allwinner sun8i.

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

Hi,

On Sun, Feb 25, 2018 at 09:50:45PM +0800, hao_zhang wrote:
> This patch adds allwinner sun8i pwm binding documents.
> 
> Signed-off-by: hao_zhang <hao5781286@gmail.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-sun8i.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> new file mode 100644
> index 0000000..e8c48be
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> @@ -0,0 +1,18 @@
> +Allwinner sun8i R40/V40/T3 SoC PWM controller
> +
> +Required properties:
> +  - compatible: should be one of:
> +    - "allwinner,sun8i-r40-pwm"
> +  - reg: physical base address and length of the controller's registers
> +  - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
> +    the cells format.
> +  - clocks: From common clock binding, handle to the parent clock.
> +
> +Example:
> +
> +pwm: pwm@1c23400 {
> +	     compatible = "allwinner,sun8i-pwm";

The compatible you have here isn't the one documented.

> +	     reg = <0x01c23400 0x154>;

And the size isn't correct, even though that doesn't really make any
difference here.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] dt-bindings: pwm: binding allwinner sun8i.

[André Przywara]

Hi,

On 25/02/18 13:50, hao_zhang wrote:
> This patch adds allwinner sun8i pwm binding documents.
> 
> Signed-off-by: hao_zhang <hao5781286-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-sun8i.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> new file mode 100644
> index 0000000..e8c48be
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> @@ -0,0 +1,18 @@
> +Allwinner sun8i R40/V40/T3 SoC PWM controller
> +
> +Required properties:
> +  - compatible: should be one of:
> +    - "allwinner,sun8i-r40-pwm"
> +  - reg: physical base address and length of the controller's registers
> +  - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
> +    the cells format.
> +  - clocks: From common clock binding, handle to the parent clock.

The manual tells me that there are two possible clock sources (24 MHz
OSC and APB1), with actually two bits for encoding the mux source,
allowing for two more potential clock sources.
So can we extend this description to provide up to four clocks, with a
clock-names property telling the driver how this maps to the mux value?
Either we use clock names matching the clocks mentioned in the manual:
	clocks = <&osc24M>, <&ccu CLK_APB1>;
	clock-names = "osc", "apb1";
or we encode the mux values in the clock-names:
	clock-names = "mux-0", "mux-1";
Don't know what's more widely used in those cases, the latter seems to
be more future-proof.
Each channel pair can be configured to use one of the two (or four)
clocks, so we can be more flexible with multiple clocks.


Also, can you please add an "interrupts" property here? You don't need
to use it in the driver right now, but since there are multiple
registers dealing with the interrupts we should have it in the binding.
If in need, we can then add support later.

> +
> +Example:
> +
> +pwm: pwm@1c23400 {
> +	     compatible = "allwinner,sun8i-pwm";

That should match what you wrote above: allwinner,sun8i-r40-pwm.
And I believe this is the right compatible name, since there are
numerous other sun8i SoCs with the sun4i PWM IP, so just sun8i-pwm would
be misleading and confusing.

> +	     reg = <0x01c23400 0x154>;
> +	     clocks = <&osc24M>;

This should then be amended to what I sketched above.

> +	     #pwm-cells = <3>;

And then here please add:
	     interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;

Cheers,
Andre

Re: [linux-sunxi] [PATCH v2 1/4] dt-bindings: pwm: binding allwinner sun8i.

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

Hi,

On Wed, Feb 28, 2018 at 01:51:59AM +0000, André Przywara wrote:
> On 25/02/18 13:50, hao_zhang wrote:
> > This patch adds allwinner sun8i pwm binding documents.
> > 
> > Signed-off-by: hao_zhang <hao5781286@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-sun8i.txt | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> > new file mode 100644
> > index 0000000..e8c48be
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> > @@ -0,0 +1,18 @@
> > +Allwinner sun8i R40/V40/T3 SoC PWM controller
> > +
> > +Required properties:
> > +  - compatible: should be one of:
> > +    - "allwinner,sun8i-r40-pwm"
> > +  - reg: physical base address and length of the controller's registers
> > +  - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
> > +    the cells format.
> > +  - clocks: From common clock binding, handle to the parent clock.
> 
> The manual tells me that there are two possible clock sources (24 MHz
> OSC and APB1), with actually two bits for encoding the mux source,
> allowing for two more potential clock sources.
> So can we extend this description to provide up to four clocks, with a
> clock-names property telling the driver how this maps to the mux value?
> Either we use clock names matching the clocks mentioned in the manual:
> 	clocks = <&osc24M>, <&ccu CLK_APB1>;
> 	clock-names = "osc", "apb1";
> or we encode the mux values in the clock-names:
> 	clock-names = "mux-0", "mux-1";

I'd prefer the former.

> Don't know what's more widely used in those cases, the latter seems to
> be more future-proof.

Not really, nothing prevents the next generation to have three bits,
or even 32 bits to do the muxing :)

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]