Re: PWM...

Re: PWM...

[Olof Johansson]

[Adding devicetree list since we're talking bindings]

On Sun, Jan 19, 2014 at 04:49:57PM +0000, Russell King - ARM Linux wrote:
> So, having looked at what else I can add support for on the cubox-i, I
> decided it would be nice and simple to add support for the front panel
> LED.  What could possibly go wrong with this.
> 
> Well, the hardware is wired such that the LED is connected between the
> PWM output and +3.3v.  So, a constant low turns the LED on full, whereas
> a constant high turns the LED off.
> 
> So, the polarity of the LED is inverted - but this _can't_ be specified
> in the totally and utterly fucked misdesigned crap that DT is:
> 
>         pwmleds {
>                 compatible = "pwm-leds";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_cubox_i_pwm1>;
>                   
>                 front {
>                         label = "imx6:red:front";
>                         pwms = <&pwm1 0 50000>;
>                         max-brightness = <248>;
>                 };
>         };
> 
>                         pwm1: pwm@02080000 {
>                                 #pwm-cells = <2>;
>                                 compatible = "fsl,imx6q-pwm", "fsl,imx27-pwm";
>                                 reg = <0x02080000 0x4000>;
>                                 interrupts = <0 83 0x04>;
>                                 clocks = <&clks 62>, <&clks 145>;
>                                 clock-names = "ipg", "per";
>                         };
> 
> Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
> be specified in the pwms declaration in pwmleds.  So that doesn't work.
> There's no property to tell pwmleds that it should use inverted sense
> either.

Adding a property for active-low to the pwm-leds binding would be
easy, and backwards compatible. I'm surprised the original binding
didn't specify it. The leds-pwm driver already seems to support it for
C-configured instances.

I'm also surprised that the imx pwm driver even has a #pwm-cells of
two, since the driver only supports one output. It'd be nice if they
had allocated the extra cell for flags, but it's hard to change now,
unless you do a new binding/compatible value and deprecate the old one.

> Moreover, there's no way to specify a default brightness for the LED
> in the absence of any trigger, so this results in the LED being fully
> on.

That seems to be missing from the led-pwm driver altogether, not just the DT
bindings? Shouldn't be too bad to add a default-brightness property and plumb
that up through the driver in this case.

> So, something which _should_ be nice and simple is turned into a major
> fuckup because of the total and utter crappiness that DT is and the
> total misdesign that this shite is.

It's not a major fuckup. None of the above is unfixable.


-Olof

Re: PWM...

[Arnd Bergmann]

On Sunday 19 January 2014 11:03:24 Olof Johansson wrote:
> > 
> > Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
> > be specified in the pwms declaration in pwmleds.  So that doesn't work.
> > There's no property to tell pwmleds that it should use inverted sense
> > either.
> 
> Adding a property for active-low to the pwm-leds binding would be
> easy, and backwards compatible. I'm surprised the original binding
> didn't specify it. The leds-pwm driver already seems to support it for
> C-configured instances.
> 
> I'm also surprised that the imx pwm driver even has a #pwm-cells of
> two, since the driver only supports one output. It'd be nice if they
> had allocated the extra cell for flags, but it's hard to change now,
> unless you do a new binding/compatible value and deprecate the old one.

Actually I think it's not that hard to change: The binding can specify
that either #pwm-cells=<2> or #pwm-cells=<3> is supported, and the
driver extended to handle both cases. This would maintain backwards
compatibility for old dtb files, though no forward compatibility for
new dtb files with old kernels.

	Arnd

Re: PWM...

[Olof Johansson]

On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 19 January 2014 11:03:24 Olof Johansson wrote:
>> >
>> > Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
>> > be specified in the pwms declaration in pwmleds.  So that doesn't work.
>> > There's no property to tell pwmleds that it should use inverted sense
>> > either.
>>
>> Adding a property for active-low to the pwm-leds binding would be
>> easy, and backwards compatible. I'm surprised the original binding
>> didn't specify it. The leds-pwm driver already seems to support it for
>> C-configured instances.
>>
>> I'm also surprised that the imx pwm driver even has a #pwm-cells of
>> two, since the driver only supports one output. It'd be nice if they
>> had allocated the extra cell for flags, but it's hard to change now,
>> unless you do a new binding/compatible value and deprecate the old one.
>
> Actually I think it's not that hard to change: The binding can specify
> that either #pwm-cells=<2> or #pwm-cells=<3> is supported, and the
> driver extended to handle both cases. This would maintain backwards
> compatibility for old dtb files, though no forward compatibility for
> new dtb files with old kernels.

Ah, yes, if you add a cell that can be done. There'll still be the
"dead" first cell that will always be 0, but that's alright.


-Olof

Re: PWM...

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 11:11:41AM -0800, Olof Johansson wrote:
> On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sunday 19 January 2014 11:03:24 Olof Johansson wrote:
> >> >
> >> > Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
> >> > be specified in the pwms declaration in pwmleds.  So that doesn't work.
> >> > There's no property to tell pwmleds that it should use inverted sense
> >> > either.
> >>
> >> Adding a property for active-low to the pwm-leds binding would be
> >> easy, and backwards compatible. I'm surprised the original binding
> >> didn't specify it. The leds-pwm driver already seems to support it for
> >> C-configured instances.
> >>
> >> I'm also surprised that the imx pwm driver even has a #pwm-cells of
> >> two, since the driver only supports one output. It'd be nice if they
> >> had allocated the extra cell for flags, but it's hard to change now,
> >> unless you do a new binding/compatible value and deprecate the old one.
> >
> > Actually I think it's not that hard to change: The binding can specify
> > that either #pwm-cells=<2> or #pwm-cells=<3> is supported, and the
> > driver extended to handle both cases. This would maintain backwards
> > compatibility for old dtb files, though no forward compatibility for
> > new dtb files with old kernels.
> 
> Ah, yes, if you add a cell that can be done. There'll still be the
> "dead" first cell that will always be 0, but that's alright.

Does it not mean that PWM specifications of:

	<&pwm1 0 n> <&pwm2 0 n>

would need to be converted to:

	<&pwm1 0 n 0> <&pwm2 0 n 0>

in every DT file referring to these PWMs - because isn't this just
treated in DT as one single array of values?  (If DT knew how many
were in each specification, we wouldn't need the #foo-cells...)

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: PWM...

[Arnd Bergmann]

On Sunday 19 January 2014, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 11:11:41AM -0800, Olof Johansson wrote:
> > On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> > Ah, yes, if you add a cell that can be done. There'll still be the
> > "dead" first cell that will always be 0, but that's alright.
> 
> Does it not mean that PWM specifications of:
> 
>         <&pwm1 0 n> <&pwm2 0 n>
> 
> would need to be converted to:
> 
>         <&pwm1 0 n 0> <&pwm2 0 n 0>
> 
> in every DT file referring to these PWMs - because isn't this just
> treated in DT as one single array of values?  (If DT knew how many
> were in each specification, we wouldn't need the #foo-cells...)

Right: if you change an existing dts file from #pwm-cells=<2> to
#pwm-cells=<3>, that requires changing all references to the pwm
controller at the same time. If both the per-soc .dtsi files
and the per-board .dts files contain references to the same pwm
controller, that can end up in significant work. I have not checked
if this is the case for i.MX though.

	Arnd

Re: PWM...

[Olof Johansson]

On Sun, Jan 19, 2014 at 09:26:40PM +0100, Arnd Bergmann wrote:
> On Sunday 19 January 2014, Russell King - ARM Linux wrote:
> > On Sun, Jan 19, 2014 at 11:11:41AM -0800, Olof Johansson wrote:
> > > On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > > Ah, yes, if you add a cell that can be done. There'll still be the
> > > "dead" first cell that will always be 0, but that's alright.
> > 
> > Does it not mean that PWM specifications of:
> > 
> >         <&pwm1 0 n> <&pwm2 0 n>
> > 
> > would need to be converted to:
> > 
> >         <&pwm1 0 n 0> <&pwm2 0 n 0>
> > 
> > in every DT file referring to these PWMs - because isn't this just
> > treated in DT as one single array of values?  (If DT knew how many
> > were in each specification, we wouldn't need the #foo-cells...)
> 
> Right: if you change an existing dts file from #pwm-cells=<2> to
> #pwm-cells=<3>, that requires changing all references to the pwm
> controller at the same time. If both the per-soc .dtsi files
> and the per-board .dts files contain references to the same pwm
> controller, that can end up in significant work. I have not checked
> if this is the case for i.MX though.

Only imx53-tx53.dtsi and imx6qdl-sabresd.dtsi seem to reference pwm nodes from
a dtsi file, the rest are from dts.

Not all of the dts files have to be converted at the same time either;
the #pwm-cells property can be changed per dts until they've all been
converted, then the shared SoC dtsi can be converted.


-Olof

Re: PWM...

[Simon Horman]

On Sun, Jan 19, 2014 at 09:26:40PM +0100, Arnd Bergmann wrote:
> On Sunday 19 January 2014, Russell King - ARM Linux wrote:
> > On Sun, Jan 19, 2014 at 11:11:41AM -0800, Olof Johansson wrote:
> > > On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > > Ah, yes, if you add a cell that can be done. There'll still be the
> > > "dead" first cell that will always be 0, but that's alright.
> > 
> > Does it not mean that PWM specifications of:
> > 
> >         <&pwm1 0 n> <&pwm2 0 n>
> > 
> > would need to be converted to:
> > 
> >         <&pwm1 0 n 0> <&pwm2 0 n 0>
> > 
> > in every DT file referring to these PWMs - because isn't this just
> > treated in DT as one single array of values?  (If DT knew how many
> > were in each specification, we wouldn't need the #foo-cells...)
> 
> Right: if you change an existing dts file from #pwm-cells=<2> to
> #pwm-cells=<3>, that requires changing all references to the pwm
> controller at the same time. If both the per-soc .dtsi files
> and the per-board .dts files contain references to the same pwm
> controller, that can end up in significant work. I have not checked
> if this is the case for i.MX though.

Would this change imply that old dtbs would no longer work with new kernels?

Re: PWM...

[Sascha Hauer]

On Mon, Jan 20, 2014 at 09:14:46AM +0900, Simon Horman wrote:
> On Sun, Jan 19, 2014 at 09:26:40PM +0100, Arnd Bergmann wrote:
> > On Sunday 19 January 2014, Russell King - ARM Linux wrote:
> > > On Sun, Jan 19, 2014 at 11:11:41AM -0800, Olof Johansson wrote:
> > > > On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > > > Ah, yes, if you add a cell that can be done. There'll still be the
> > > > "dead" first cell that will always be 0, but that's alright.
> > > 
> > > Does it not mean that PWM specifications of:
> > > 
> > >         <&pwm1 0 n> <&pwm2 0 n>
> > > 
> > > would need to be converted to:
> > > 
> > >         <&pwm1 0 n 0> <&pwm2 0 n 0>
> > > 
> > > in every DT file referring to these PWMs - because isn't this just
> > > treated in DT as one single array of values?  (If DT knew how many
> > > were in each specification, we wouldn't need the #foo-cells...)
> > 
> > Right: if you change an existing dts file from #pwm-cells=<2> to
> > #pwm-cells=<3>, that requires changing all references to the pwm
> > controller at the same time. If both the per-soc .dtsi files
> > and the per-board .dts files contain references to the same pwm
> > controller, that can end up in significant work. I have not checked
> > if this is the case for i.MX though.
> 
> Would this change imply that old dtbs would no longer work with new kernels?

Not necessarily. With Lothars patch the driver works with pwm-cells=2
and pwm-cells=3. The only problem is that if you compile an old board
dts with a new SoC dtsi it will silently fail since the pwm-cells
setting doesn't match the atcual cells in the dts.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: PWM...

[Mark Brown]

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

On Mon, Jan 20, 2014 at 09:14:46AM +0900, Simon Horman wrote:
> On Sun, Jan 19, 2014 at 09:26:40PM +0100, Arnd Bergmann wrote:

> > Right: if you change an existing dts file from #pwm-cells=<2> to
> > #pwm-cells=<3>, that requires changing all references to the pwm

> Would this change imply that old dtbs would no longer work with new kernels?

So long as the code can still handle both values for #pwm-cells existing
DTBs should work fine.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: PWM...

[Simon Horman]

On Mon, Jan 20, 2014 at 04:11:47PM +0000, Mark Brown wrote:
> On Mon, Jan 20, 2014 at 09:14:46AM +0900, Simon Horman wrote:
> > On Sun, Jan 19, 2014 at 09:26:40PM +0100, Arnd Bergmann wrote:
> 
> > > Right: if you change an existing dts file from #pwm-cells=<2> to
> > > #pwm-cells=<3>, that requires changing all references to the pwm
> 
> > Would this change imply that old dtbs would no longer work with new kernels?
> 
> So long as the code can still handle both values for #pwm-cells existing
> DTBs should work fine.

Thanks, as long as that is the case I have no objections at this time.

Re: PWM...

[Sascha Hauer]

On Sun, Jan 19, 2014 at 11:03:24AM -0800, Olof Johansson wrote:
> [Adding devicetree list since we're talking bindings]
> 
> On Sun, Jan 19, 2014 at 04:49:57PM +0000, Russell King - ARM Linux wrote:
> > So, having looked at what else I can add support for on the cubox-i, I
> > decided it would be nice and simple to add support for the front panel
> > LED.  What could possibly go wrong with this.
> > 
> > Well, the hardware is wired such that the LED is connected between the
> > PWM output and +3.3v.  So, a constant low turns the LED on full, whereas
> > a constant high turns the LED off.
> > 
> > So, the polarity of the LED is inverted - but this _can't_ be specified
> > in the totally and utterly fucked misdesigned crap that DT is:
> > 
> >         pwmleds {
> >                 compatible = "pwm-leds";
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&pinctrl_cubox_i_pwm1>;
> >                   
> >                 front {
> >                         label = "imx6:red:front";
> >                         pwms = <&pwm1 0 50000>;
> >                         max-brightness = <248>;
> >                 };
> >         };
> > 
> >                         pwm1: pwm@02080000 {
> >                                 #pwm-cells = <2>;
> >                                 compatible = "fsl,imx6q-pwm", "fsl,imx27-pwm";
> >                                 reg = <0x02080000 0x4000>;
> >                                 interrupts = <0 83 0x04>;
> >                                 clocks = <&clks 62>, <&clks 145>;
> >                                 clock-names = "ipg", "per";
> >                         };
> > 
> > Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
> > be specified in the pwms declaration in pwmleds.  So that doesn't work.
> > There's no property to tell pwmleds that it should use inverted sense
> > either.
> 
> Adding a property for active-low to the pwm-leds binding would be
> easy, and backwards compatible. I'm surprised the original binding
> didn't specify it. The leds-pwm driver already seems to support it for
> C-configured instances.
> 
> I'm also surprised that the imx pwm driver even has a #pwm-cells of
> two, since the driver only supports one output. It'd be nice if they
> had allocated the extra cell for flags, but it's hard to change now,
> unless you do a new binding/compatible value and deprecate the old one.
> 
> > Moreover, there's no way to specify a default brightness for the LED
> > in the absence of any trigger, so this results in the LED being fully
> > on.
> 
> That seems to be missing from the led-pwm driver altogether, not just the DT
> bindings? Shouldn't be too bad to add a default-brightness property and plumb
> that up through the driver in this case.
> 
> > So, something which _should_ be nice and simple is turned into a major
> > fuckup because of the total and utter crappiness that DT is and the
> > total misdesign that this shite is.
> 
> It's not a major fuckup. None of the above is unfixable.

Indeed, and we already have a patch for this:

http://comments.gmane.org/gmane.linux.ports.arm.kernel/294823

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |