| Message ID | 20191103203334.10539-2-peron.clem@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for H6 PWM | expand |
On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote: > From: Jernej Skrabec <jernej.skrabec@siol.net> > > H6 PWM block is basically the same as A20 PWM, except that it also has > bus clock and reset line which needs to be handled accordingly. > > Expand Allwinner PWM binding with H6 PWM specifics. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > index 0ac52f83a58c..bf36ea509f31 100644 > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > @@ -30,13 +30,46 @@ properties: > - items: > - const: allwinner,sun50i-h5-pwm > - const: allwinner,sun5i-a13-pwm > + - const: allwinner,sun50i-h6-pwm > > reg: > maxItems: 1 > > - clocks: > + # Even though it only applies to subschemas under the conditionals, > + # not listing them here will trigger a warning because of the > + # additionalsProperties set to false. > + clocks: true > + clock-names: true > + resets: > maxItems: 1 > > + if: > + properties: > + compatible: > + contains: > + const: allwinner,sun50i-h6-pwm > + > + then: > + properties: > + clocks: > + items: > + - description: Module Clock > + - description: Bus Clock > + > + clock-names: > + items: > + - const: mod > + - const: bus > + > + required: > + - clock-names > + - resets > + > + else: > + properties: > + clocks: > + maxItems: 1 > + I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and bus clock is required.", right? I wonder if it is sensible to require a clock-names property in the else branch, too. This would make it obvious if the clock there corresponds to the "mod" or the "bus" clock on H6. (I guess it's "mod".) Best regards Uwe
On Mon, 4 Nov 2019 at 09:04, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > H6 PWM block is basically the same as A20 PWM, except that it also has > > bus clock and reset line which needs to be handled accordingly. > > > > Expand Allwinner PWM binding with H6 PWM specifics. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++- > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > index 0ac52f83a58c..bf36ea509f31 100644 > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > @@ -30,13 +30,46 @@ properties: > > - items: > > - const: allwinner,sun50i-h5-pwm > > - const: allwinner,sun5i-a13-pwm > > + - const: allwinner,sun50i-h6-pwm > > > > reg: > > maxItems: 1 > > > > - clocks: > > + # Even though it only applies to subschemas under the conditionals, > > + # not listing them here will trigger a warning because of the > > + # additionalsProperties set to false. > > + clocks: true > > + clock-names: true > > + resets: > > maxItems: 1 > > > > + if: > > + properties: > > + compatible: > > + contains: > > + const: allwinner,sun50i-h6-pwm > > + > > + then: > > + properties: > > + clocks: > > + items: > > + - description: Module Clock > > + - description: Bus Clock > > + > > + clock-names: > > + items: > > + - const: mod > > + - const: bus > > + > > + required: > > + - clock-names > > + - resets > > + > > + else: > > + properties: > > + clocks: > > + maxItems: 1 > > + > > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and > bus clock is required.", right? Correct. > > > I wonder if it is sensible to require a clock-names property in the else > branch, too. This would make it obvious if the clock there corresponds > to the "mod" or the "bus" clock on H6. (I guess it's "mod".) This will also require to change example and all the current allwinner device-tree that have a PWM declared. Regards, Clément > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Clement, Uwe, On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote: > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > H6 PWM block is basically the same as A20 PWM, except that it also has > > bus clock and reset line which needs to be handled accordingly. > > > > Expand Allwinner PWM binding with H6 PWM specifics. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++- > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > index 0ac52f83a58c..bf36ea509f31 100644 > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > @@ -30,13 +30,46 @@ properties: > > - items: > > - const: allwinner,sun50i-h5-pwm > > - const: allwinner,sun5i-a13-pwm > > + - const: allwinner,sun50i-h6-pwm > > > > reg: > > maxItems: 1 > > > > - clocks: > > + # Even though it only applies to subschemas under the conditionals, > > + # not listing them here will trigger a warning because of the > > + # additionalsProperties set to false. > > + clocks: true > > + clock-names: true > > + resets: > > maxItems: 1 > > > > + if: > > + properties: > > + compatible: > > + contains: > > + const: allwinner,sun50i-h6-pwm > > + > > + then: > > + properties: > > + clocks: > > + items: > > + - description: Module Clock > > + - description: Bus Clock > > + > > + clock-names: > > + items: > > + - const: mod > > + - const: bus > > + > > + required: > > + - clock-names > > + - resets > > + > > + else: > > + properties: > > + clocks: > > + maxItems: 1 > > + > > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and > bus clock is required.", right? > > I wonder if it is sensible to require a clock-names property in the else > branch, too. This would make it obvious if the clock there corresponds > to the "mod" or the "bus" clock on H6. (I guess it's "mod".) This can be done a bit differently and could address your concerns Something like properties: ... clocks: minItems: 1 maxItems: 2 items: - description: Bus Clock - description: Module Clock required: - clocks if: ... then: properties: clocks: maxItems: 2 clocks-names: items: - const: mod - const: bus required: - clock-names else: properties: clocks: maxItems: 1 That way, the definition of the order and which clock is which is pretty obvious in both cases, and we don't get any weird warnings. Maxime
Hi Maxime, On Tue, 5 Nov 2019 at 12:11, Maxime Ripard <mripard@kernel.org> wrote: > > Hi Clement, Uwe, > > On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote: > > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote: > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > H6 PWM block is basically the same as A20 PWM, except that it also has > > > bus clock and reset line which needs to be handled accordingly. > > > > > > Expand Allwinner PWM binding with H6 PWM specifics. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++- > > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > index 0ac52f83a58c..bf36ea509f31 100644 > > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > @@ -30,13 +30,46 @@ properties: > > > - items: > > > - const: allwinner,sun50i-h5-pwm > > > - const: allwinner,sun5i-a13-pwm > > > + - const: allwinner,sun50i-h6-pwm > > > > > > reg: > > > maxItems: 1 > > > > > > - clocks: > > > + # Even though it only applies to subschemas under the conditionals, > > > + # not listing them here will trigger a warning because of the > > > + # additionalsProperties set to false. > > > + clocks: true > > > + clock-names: true > > > + resets: > > > maxItems: 1 > > > > > > + if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: allwinner,sun50i-h6-pwm > > > + > > > + then: > > > + properties: > > > + clocks: > > > + items: > > > + - description: Module Clock > > > + - description: Bus Clock > > > + > > > + clock-names: > > > + items: > > > + - const: mod > > > + - const: bus > > > + > > > + required: > > > + - clock-names > > > + - resets > > > + > > > + else: > > > + properties: > > > + clocks: > > > + maxItems: 1 > > > + > > > > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and > > bus clock is required.", right? > > > > I wonder if it is sensible to require a clock-names property in the else > > branch, too. This would make it obvious if the clock there corresponds > > to the "mod" or the "bus" clock on H6. (I guess it's "mod".) > > This can be done a bit differently and could address your concerns > > Something like > > properties: > ... > > clocks: > minItems: 1 > maxItems: 2 > items: > - description: Bus Clock > - description: Module Clock > > required: > - clocks > > if: > ... > > then: > properties: > clocks: > maxItems: 2 Here we should set minItems to 2 right ? so Max = Min = 2 Regards, Clément > > clocks-names: > items: > - const: mod > - const: bus > > required: > - clock-names > > else: > properties: > clocks: > maxItems: 1 > > That way, the definition of the order and which clock is which is > pretty obvious in both cases, and we don't get any weird warnings. > > Maxime
On Tue, Nov 05, 2019 at 01:34:37PM +0100, Clément Péron wrote: > On Tue, 5 Nov 2019 at 12:11, Maxime Ripard <mripard@kernel.org> wrote: > > > > Hi Clement, Uwe, > > > > On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote: > > > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote: > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > H6 PWM block is basically the same as A20 PWM, except that it also has > > > > bus clock and reset line which needs to be handled accordingly. > > > > > > > > Expand Allwinner PWM binding with H6 PWM specifics. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > --- > > > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++- > > > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > > index 0ac52f83a58c..bf36ea509f31 100644 > > > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > > @@ -30,13 +30,46 @@ properties: > > > > - items: > > > > - const: allwinner,sun50i-h5-pwm > > > > - const: allwinner,sun5i-a13-pwm > > > > + - const: allwinner,sun50i-h6-pwm > > > > > > > > reg: > > > > maxItems: 1 > > > > > > > > - clocks: > > > > + # Even though it only applies to subschemas under the conditionals, > > > > + # not listing them here will trigger a warning because of the > > > > + # additionalsProperties set to false. > > > > + clocks: true > > > > + clock-names: true > > > > + resets: > > > > maxItems: 1 > > > > > > > > + if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + const: allwinner,sun50i-h6-pwm > > > > + > > > > + then: > > > > + properties: > > > > + clocks: > > > > + items: > > > > + - description: Module Clock > > > > + - description: Bus Clock > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: mod > > > > + - const: bus > > > > + > > > > + required: > > > > + - clock-names > > > > + - resets > > > > + > > > > + else: > > > > + properties: > > > > + clocks: > > > > + maxItems: 1 > > > > + > > > > > > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and > > > bus clock is required.", right? > > > > > > I wonder if it is sensible to require a clock-names property in the else > > > branch, too. This would make it obvious if the clock there corresponds > > > to the "mod" or the "bus" clock on H6. (I guess it's "mod".) > > > > This can be done a bit differently and could address your concerns > > > > Something like > > > > properties: > > ... > > > > clocks: > > minItems: 1 > > maxItems: 2 > > items: > > - description: Bus Clock > > - description: Module Clock > > > > required: > > - clocks > > > > if: > > ... > > > > then: > > properties: > > clocks: > > maxItems: 2 > > Here we should set minItems to 2 right ? > so Max = Min = 2 It's done automatically by the tooling when the other is missing. Maxime
Hi, On Tue, 5 Nov 2019 at 18:32, Maxime Ripard <mripard@kernel.org> wrote: > > On Tue, Nov 05, 2019 at 01:34:37PM +0100, Clément Péron wrote: > > On Tue, 5 Nov 2019 at 12:11, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > Hi Clement, Uwe, > > > > > > On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote: > > > > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote: > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > H6 PWM block is basically the same as A20 PWM, except that it also has > > > > > bus clock and reset line which needs to be handled accordingly. > > > > > > > > > > Expand Allwinner PWM binding with H6 PWM specifics. > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > --- > > > > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++- > > > > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > > > index 0ac52f83a58c..bf36ea509f31 100644 > > > > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml > > > > > @@ -30,13 +30,46 @@ properties: > > > > > - items: > > > > > - const: allwinner,sun50i-h5-pwm > > > > > - const: allwinner,sun5i-a13-pwm > > > > > + - const: allwinner,sun50i-h6-pwm > > > > > > > > > > reg: > > > > > maxItems: 1 > > > > > > > > > > - clocks: > > > > > + # Even though it only applies to subschemas under the conditionals, > > > > > + # not listing them here will trigger a warning because of the > > > > > + # additionalsProperties set to false. > > > > > + clocks: true > > > > > + clock-names: true > > > > > + resets: > > > > > maxItems: 1 > > > > > > > > > > + if: > > > > > + properties: > > > > > + compatible: > > > > > + contains: > > > > > + const: allwinner,sun50i-h6-pwm > > > > > + > > > > > + then: > > > > > + properties: > > > > > + clocks: > > > > > + items: > > > > > + - description: Module Clock > > > > > + - description: Bus Clock > > > > > + > > > > > + clock-names: > > > > > + items: > > > > > + - const: mod > > > > > + - const: bus > > > > > + > > > > > + required: > > > > > + - clock-names > > > > > + - resets > > > > > + > > > > > + else: > > > > > + properties: > > > > > + clocks: > > > > > + maxItems: 1 > > > > > + > > > > > > > > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and > > > > bus clock is required.", right? > > > > > > > > I wonder if it is sensible to require a clock-names property in the else > > > > branch, too. This would make it obvious if the clock there corresponds > > > > to the "mod" or the "bus" clock on H6. (I guess it's "mod".) > > > > > > This can be done a bit differently and could address your concerns > > > > > > Something like > > > > > > properties: > > > ... > > > > > > clocks: > > > minItems: 1 > > > maxItems: 2 > > > items: > > > - description: Bus Clock > > > - description: Module Clock > > > > > > required: > > > - clocks > > > > > > if: > > > ... > > > > > > then: > > > properties: > > > clocks: > > > maxItems: 2 > > > > Here we should set minItems to 2 right ? > > so Max = Min = 2 > > It's done automatically by the tooling when the other is missing. Ok thanks, I will update in v4. Regards, Clément > > Maxime
diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml index 0ac52f83a58c..bf36ea509f31 100644 --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml @@ -30,13 +30,46 @@ properties: - items: - const: allwinner,sun50i-h5-pwm - const: allwinner,sun5i-a13-pwm + - const: allwinner,sun50i-h6-pwm reg: maxItems: 1 - clocks: + # Even though it only applies to subschemas under the conditionals, + # not listing them here will trigger a warning because of the + # additionalsProperties set to false. + clocks: true + clock-names: true + resets: maxItems: 1 + if: + properties: + compatible: + contains: + const: allwinner,sun50i-h6-pwm + + then: + properties: + clocks: + items: + - description: Module Clock + - description: Bus Clock + + clock-names: + items: + - const: mod + - const: bus + + required: + - clock-names + - resets + + else: + properties: + clocks: + maxItems: 1 + required: - "#pwm-cells" - compatible @@ -54,4 +87,14 @@ examples: #pwm-cells = <3>; }; + - | + pwm@300a000 { + compatible = "allwinner,sun50i-h6-pwm"; + reg = <0x0300a000 0x400>; + clocks = <&osc24M>, <&ccu CLK_BUS_PWM>; + clock-names = "mod", "bus"; + resets = <&ccu RST_BUS_PWM>; + #pwm-cells = <3>; + }; + ...