Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1417160 > unrolled thread

[PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes Lee Jones <lee.jones@linaro.org> - 2016-06-08 11:30 +0200
    Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with  recent changes Rob Herring <robh@kernel.org> - 2016-06-08 23:00 +0200
      Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with  recent changes Lee Jones <lee.jones@linaro.org> - 2016-06-09 13:50 +0200
        Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with  recent changes Thierry Reding <thierry.reding@gmail.com> - 2016-06-10 15:20 +0200
          Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with  recent changes Lee Jones <lee.jones@linaro.org> - 2016-06-10 16:10 +0200
            Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with  recent changes Thierry Reding <thierry.reding@gmail.com> - 2016-06-10 17:00 +0200
              Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with  recent changes Lee Jones <lee.jones@linaro.org> - 2016-06-10 17:20 +0200
      Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with  recent changes Thierry Reding <thierry.reding@gmail.com> - 2016-06-10 15:20 +0200

#1417160 — [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
be more inline with the naming conventions of the subsystem.  Where
we used to treat each line as a channel, the PWM convention is to
describe them as devices.

The second documentation adaption entails adding support for PWM
capture devices.  A new clock is required as well as an IRQ line.
We're also adding a new property similar to the one described
above, but for capture channels.  Typically, there will be less
capture channels than PWM-out, since all channels have the latter
capability, but only some have capture support.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/pwm/pwm-st.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
index 84d2fb8..b09f3a4 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-st.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
@@ -13,13 +13,14 @@ Required parameters:
 - pinctrl-0: 		List of phandles pointing to pin configuration nodes
 			for PWM module.
 			For Pinctrl properties, please refer to [1].
-- clock-names: 		Set to "pwm".
+- clock-names: 		Valid entries are "pwm" and/or "capture".
 - clocks: 		phandle of the clock used by the PWM module.
 			For Clk properties, please refer to [2].
+- interrupts:		IRQ for the Capture device
 
 Optional properties:
-- st,pwm-num-chan:	Number of available channels. If not passed, the driver
-			will consider single channel by default.
+- st,pwm-num-devs:	Number of available PWM channels.  Default is 0.
+- st,capture-num-devs:	Number of available Capture channels.  Default is 0.
 
 [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
 [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -37,5 +38,6 @@ pwm1: pwm@fe510000 {
 		     &pinctrl_pwm1_chan3_default>;
 	clocks = <&clk_sysin>;
 	clock-names = "pwm";
-	st,pwm-num-chan = <4>;
+	st,pwm-num-devs = <4>;
+	st,capture-num-devs = <2>;
 };
-- 
2.8.3

[toc] | [next] | [standalone]

#1417866 — Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> be more inline with the naming conventions of the subsystem.  Where
> we used to treat each line as a channel, the PWM convention is to
> describe them as devices.

That's all linux implementation details and you are breaking 
compatibility.

> The second documentation adaption entails adding support for PWM
> capture devices.  A new clock is required as well as an IRQ line.
> We're also adding a new property similar to the one described
> above, but for capture channels.  Typically, there will be less
> capture channels than PWM-out, since all channels have the latter
> capability, but only some have capture support.

Humm, sounds like all of this should be implied from compatible strings.

> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-st.txt | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> index 84d2fb8..b09f3a4 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-st.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> @@ -13,13 +13,14 @@ Required parameters:
>  - pinctrl-0: 		List of phandles pointing to pin configuration nodes
>  			for PWM module.
>  			For Pinctrl properties, please refer to [1].
> -- clock-names: 		Set to "pwm".
> +- clock-names: 		Valid entries are "pwm" and/or "capture".
>  - clocks: 		phandle of the clock used by the PWM module.
>  			For Clk properties, please refer to [2].
> +- interrupts:		IRQ for the Capture device
>  
>  Optional properties:
> -- st,pwm-num-chan:	Number of available channels. If not passed, the driver
> -			will consider single channel by default.
> +- st,pwm-num-devs:	Number of available PWM channels.  Default is 0.
> +- st,capture-num-devs:	Number of available Capture channels.  Default is 0.
>  
>  [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>  [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -37,5 +38,6 @@ pwm1: pwm@fe510000 {
>  		     &pinctrl_pwm1_chan3_default>;
>  	clocks = <&clk_sysin>;
>  	clock-names = "pwm";
> -	st,pwm-num-chan = <4>;
> +	st,pwm-num-devs = <4>;
> +	st,capture-num-devs = <2>;
>  };
> -- 
> 2.8.3
> 

[toc] | [prev] | [next] | [standalone]

#1418253 — Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

On Wed, 08 Jun 2016, Rob Herring wrote:

> On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > be more inline with the naming conventions of the subsystem.  Where
> > we used to treat each line as a channel, the PWM convention is to
> > describe them as devices.
> 
> That's all linux implementation details and you are breaking 
> compatibility.

Normally I'd agree with you, but I happen to know that a) this IP is
currently unused and b) up until this point (and probably beyond), ST
always ship the DTB with the kernel, so there will be no breakage.

> > The second documentation adaption entails adding support for PWM
> > capture devices.  A new clock is required as well as an IRQ line.
> > We're also adding a new property similar to the one described
> > above, but for capture channels.  Typically, there will be less
> > capture channels than PWM-out, since all channels have the latter
> > capability, but only some have capture support.
> 
> Humm, sounds like all of this should be implied from compatible strings.

You mean have a bunch of of_machine_is_compatibles() scattered around?

> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-st.txt | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > index 84d2fb8..b09f3a4 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > @@ -13,13 +13,14 @@ Required parameters:
> >  - pinctrl-0: 		List of phandles pointing to pin configuration nodes
> >  			for PWM module.
> >  			For Pinctrl properties, please refer to [1].
> > -- clock-names: 		Set to "pwm".
> > +- clock-names: 		Valid entries are "pwm" and/or "capture".
> >  - clocks: 		phandle of the clock used by the PWM module.
> >  			For Clk properties, please refer to [2].
> > +- interrupts:		IRQ for the Capture device
> >  
> >  Optional properties:
> > -- st,pwm-num-chan:	Number of available channels. If not passed, the driver
> > -			will consider single channel by default.
> > +- st,pwm-num-devs:	Number of available PWM channels.  Default is 0.
> > +- st,capture-num-devs:	Number of available Capture channels.  Default is 0.
> >  
> >  [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> >  [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -37,5 +38,6 @@ pwm1: pwm@fe510000 {
> >  		     &pinctrl_pwm1_chan3_default>;
> >  	clocks = <&clk_sysin>;
> >  	clock-names = "pwm";
> > -	st,pwm-num-chan = <4>;
> > +	st,pwm-num-devs = <4>;
> > +	st,capture-num-devs = <2>;
> >  };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1419403 — Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> On Wed, 08 Jun 2016, Rob Herring wrote:
> 
> > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > be more inline with the naming conventions of the subsystem.  Where
> > > we used to treat each line as a channel, the PWM convention is to
> > > describe them as devices.
> > 
> > That's all linux implementation details and you are breaking 
> > compatibility.
> 
> Normally I'd agree with you, but I happen to know that a) this IP is
> currently unused and b) up until this point (and probably beyond), ST
> always ship the DTB with the kernel, so there will be no breakage.

Heh... I've long given up on trying to make that argument go anywhere.
The general rule is that once we support a binding in a kernel release
we have to support it indefinitely. If you really want to go ahead with
this change (I don't think you should), you'd at least have to document
both properties and support st,pwm-num-chan in the driver for backwards
compatibility.

> > > The second documentation adaption entails adding support for PWM
> > > capture devices.  A new clock is required as well as an IRQ line.
> > > We're also adding a new property similar to the one described
> > > above, but for capture channels.  Typically, there will be less
> > > capture channels than PWM-out, since all channels have the latter
> > > capability, but only some have capture support.
> > 
> > Humm, sounds like all of this should be implied from compatible strings.
> 
> You mean have a bunch of of_machine_is_compatibles() scattered around?

I don't understand why you need this at all. Quite frankly I don't even
know why st,pwm-num-devs exists. I probably missed it back at the time.
Usually, like Rob suggests, this should be inferred from the compatible
string. One commonly used way to avoid scattering explicit checks for
the compatible string is to add this information to the of_device_id
table. See a bunch of existing drivers for reference.

Also, why make a separation of output vs. capture channels at this
point? Could you not simply obtain the total number of PWM channels,
preferably from SoC data associated with the compatible string, and
check at ->capture() time whether or not the particular PWM supports
this?

As-is, you imply that you have n (output) + m (capture) channels, and
that 0..n-1 are output and n..n+m-1 are capture channels. What if that
no longer holds true, but 0 and 2 are the only ones that support
capture?

If you check for this at runtime you can avoid complicated DT parsing
code, but still get the safety check which should be enough to encourage
people to use the right channels in DT.

Thierry

[toc] | [prev] | [next] | [standalone]

#1419457 — Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

On Fri, 10 Jun 2016, Thierry Reding wrote:

> On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> > On Wed, 08 Jun 2016, Rob Herring wrote:
> > 
> > > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > > be more inline with the naming conventions of the subsystem.  Where
> > > > we used to treat each line as a channel, the PWM convention is to
> > > > describe them as devices.
> > > 
> > > That's all linux implementation details and you are breaking 
> > > compatibility.
> > 
> > Normally I'd agree with you, but I happen to know that a) this IP is
> > currently unused and b) up until this point (and probably beyond), ST
> > always ship the DTB with the kernel, so there will be no breakage.
> 
> Heh... I've long given up on trying to make that argument go anywhere.
> The general rule is that once we support a binding in a kernel release
> we have to support it indefinitely. If you really want to go ahead with
> this change (I don't think you should), you'd at least have to document
> both properties and support st,pwm-num-chan in the driver for backwards
> compatibility.

I understand what the *general* rule is, but we have to remember why
this rule was put into place and apply some common sense.  In some
Enterprise user-cases where DTBs are written into ROM or where they
are difficult /impossible to update, I can completely understand the
requirement to support previous incarnations.  However in this, the
real world, DTBs are shipped with their corresponding kernels.  We
would lack a great deal of functionality if they weren't.  It is
therefor, foolhardy and inappropriate to stick to this rule just
'cos.

> > > > The second documentation adaption entails adding support for PWM
> > > > capture devices.  A new clock is required as well as an IRQ line.
> > > > We're also adding a new property similar to the one described
> > > > above, but for capture channels.  Typically, there will be less
> > > > capture channels than PWM-out, since all channels have the latter
> > > > capability, but only some have capture support.
> > > 
> > > Humm, sounds like all of this should be implied from compatible strings.
> > 
> > You mean have a bunch of of_machine_is_compatibles() scattered around?
> 
> I don't understand why you need this at all. Quite frankly I don't even
> know why st,pwm-num-devs exists. I probably missed it back at the time.
> Usually, like Rob suggests, this should be inferred from the compatible
> string. One commonly used way to avoid scattering explicit checks for
> the compatible string is to add this information to the of_device_id
> table. See a bunch of existing drivers for reference.

Yes, I am aware of the strategy, and happy to oblige if this is your
suggestion.  I'll move all platform data into the driver and eradicate
the DT properties.

> Also, why make a separation of output vs. capture channels at this
> point? Could you not simply obtain the total number of PWM channels,
> preferably from SoC data associated with the compatible string, and
> check at ->capture() time whether or not the particular PWM supports
> this?
> 
> As-is, you imply that you have n (output) + m (capture) channels, and
> that 0..n-1 are output and n..n+m-1 are capture channels. What if that
> no longer holds true, but 0 and 2 are the only ones that support
> capture?

We do?  What makes you think that?

> If you check for this at runtime you can avoid complicated DT parsing
> code, but still get the safety check which should be enough to encourage
> people to use the right channels in DT.

I'm pretty sure I can move all the data into the driver.  I did want
to avoid having lots of different compatible strings, but if that's
what you're suggesting, I can introduce one per supported platform.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1419500 — Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 10, 2016 at 03:06:35PM +0100, Lee Jones wrote:
> On Fri, 10 Jun 2016, Thierry Reding wrote:
> 
> > On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> > > On Wed, 08 Jun 2016, Rob Herring wrote:
> > > 
> > > > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > > > be more inline with the naming conventions of the subsystem.  Where
> > > > > we used to treat each line as a channel, the PWM convention is to
> > > > > describe them as devices.
> > > > 
> > > > That's all linux implementation details and you are breaking 
> > > > compatibility.
> > > 
> > > Normally I'd agree with you, but I happen to know that a) this IP is
> > > currently unused and b) up until this point (and probably beyond), ST
> > > always ship the DTB with the kernel, so there will be no breakage.
> > 
> > Heh... I've long given up on trying to make that argument go anywhere.
> > The general rule is that once we support a binding in a kernel release
> > we have to support it indefinitely. If you really want to go ahead with
> > this change (I don't think you should), you'd at least have to document
> > both properties and support st,pwm-num-chan in the driver for backwards
> > compatibility.
> 
> I understand what the *general* rule is, but we have to remember why
> this rule was put into place and apply some common sense.  In some
> Enterprise user-cases where DTBs are written into ROM or where they
> are difficult /impossible to update, I can completely understand the
> requirement to support previous incarnations.  However in this, the
> real world, DTBs are shipped with their corresponding kernels.  We
> would lack a great deal of functionality if they weren't.  It is
> therefor, foolhardy and inappropriate to stick to this rule just
> 'cos.

I used to advocate the very same point of view a couple of years ago but
was repeatedly told that it's irrelevant, and everybody was to be held
to the same standards, irrespective of how easy it is to update the DTB
in lockstep with the kernel.

Part of me still wishes that the rules were a little less strict, but a
decision was made on this years ago, so I'm just repeating this here in
an attempt to save you from wasting your time arguing.

Then again, there have been occasions where decisions were undone, so
you might have better luck nowadays if you're willing to take this to
the DT bindings and ARM-SoC maintainers.

Bottom line: NAK on the backwards-incompatible DT binding change from me
based on earlier decisions made on this topic. But I may be swayed if
everyone else thinks ABI stability is no longer something we consider
important for DT.

> > > > > The second documentation adaption entails adding support for PWM
> > > > > capture devices.  A new clock is required as well as an IRQ line.
> > > > > We're also adding a new property similar to the one described
> > > > > above, but for capture channels.  Typically, there will be less
> > > > > capture channels than PWM-out, since all channels have the latter
> > > > > capability, but only some have capture support.
> > > > 
> > > > Humm, sounds like all of this should be implied from compatible strings.
> > > 
> > > You mean have a bunch of of_machine_is_compatibles() scattered around?
> > 
> > I don't understand why you need this at all. Quite frankly I don't even
> > know why st,pwm-num-devs exists. I probably missed it back at the time.
> > Usually, like Rob suggests, this should be inferred from the compatible
> > string. One commonly used way to avoid scattering explicit checks for
> > the compatible string is to add this information to the of_device_id
> > table. See a bunch of existing drivers for reference.
> 
> Yes, I am aware of the strategy, and happy to oblige if this is your
> suggestion.  I'll move all platform data into the driver and eradicate
> the DT properties.

Great!

> > Also, why make a separation of output vs. capture channels at this
> > point? Could you not simply obtain the total number of PWM channels,
> > preferably from SoC data associated with the compatible string, and
> > check at ->capture() time whether or not the particular PWM supports
> > this?
> > 
> > As-is, you imply that you have n (output) + m (capture) channels, and
> > that 0..n-1 are output and n..n+m-1 are capture channels. What if that
> > no longer holds true, but 0 and 2 are the only ones that support
> > capture?
> 
> We do?  What makes you think that?

Because there's nothing saying otherwise? The DT binding is completely
silent on the matter, and the above is the most logical interpretation
that I came up with.

Looking at the driver it seems like it's actually the other way around
and you have the first m channels that support both output and capture
functionality. But the issue remains that this isn't documented in the
DT binding documentation. And the current properties are also not very
flexible because they allow for only a single scheme of assigning the
capability.

If you move all of that knowledge into the driver, things become a lot
easier, in my opinion.

> > If you check for this at runtime you can avoid complicated DT parsing
> > code, but still get the safety check which should be enough to encourage
> > people to use the right channels in DT.
> 
> I'm pretty sure I can move all the data into the driver.  I did want
> to avoid having lots of different compatible strings, but if that's
> what you're suggesting, I can introduce one per supported platform.

That sounds like the simplest solution to me.

Thierry

[toc] | [prev] | [next] | [standalone]

#1419510 — Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

On Fri, 10 Jun 2016, Thierry Reding wrote:
> On Fri, Jun 10, 2016 at 03:06:35PM +0100, Lee Jones wrote:
> > On Fri, 10 Jun 2016, Thierry Reding wrote:
> > 
> > > On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> > > > On Wed, 08 Jun 2016, Rob Herring wrote:
> > > > 
> > > > > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > > > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > > > > be more inline with the naming conventions of the subsystem.  Where
> > > > > > we used to treat each line as a channel, the PWM convention is to
> > > > > > describe them as devices.
> > > > > 
> > > > > That's all linux implementation details and you are breaking 
> > > > > compatibility.
> > > > 
> > > > Normally I'd agree with you, but I happen to know that a) this IP is
> > > > currently unused and b) up until this point (and probably beyond), ST
> > > > always ship the DTB with the kernel, so there will be no breakage.
> > > 
> > > Heh... I've long given up on trying to make that argument go anywhere.
> > > The general rule is that once we support a binding in a kernel release
> > > we have to support it indefinitely. If you really want to go ahead with
> > > this change (I don't think you should), you'd at least have to document
> > > both properties and support st,pwm-num-chan in the driver for backwards
> > > compatibility.
> > 
> > I understand what the *general* rule is, but we have to remember why
> > this rule was put into place and apply some common sense.  In some
> > Enterprise user-cases where DTBs are written into ROM or where they
> > are difficult /impossible to update, I can completely understand the
> > requirement to support previous incarnations.  However in this, the
> > real world, DTBs are shipped with their corresponding kernels.  We
> > would lack a great deal of functionality if they weren't.  It is
> > therefor, foolhardy and inappropriate to stick to this rule just
> > 'cos.
> 
> I used to advocate the very same point of view a couple of years ago but
> was repeatedly told that it's irrelevant, and everybody was to be held
> to the same standards, irrespective of how easy it is to update the DTB
> in lockstep with the kernel.
> 
> Part of me still wishes that the rules were a little less strict, but a
> decision was made on this years ago, so I'm just repeating this here in
> an attempt to save you from wasting your time arguing.
> 
> Then again, there have been occasions where decisions were undone, so
> you might have better luck nowadays if you're willing to take this to
> the DT bindings and ARM-SoC maintainers.
> 
> Bottom line: NAK on the backwards-incompatible DT binding change from me
> based on earlier decisions made on this topic. But I may be swayed if
> everyone else thinks ABI stability is no longer something we consider
> important for DT.

I've seen properties come and go.  Both using the deprecation process
and otherwise.  By NAKing these types of changes, you're just
exacerbating the situation.  If you know it's the right
(non-foolhardy) thing to do, just do it.

Although, I feel the point is moot for this particular driver now,
what with all of the pdata being moved into the driver.

> > > > > > The second documentation adaption entails adding support for PWM
> > > > > > capture devices.  A new clock is required as well as an IRQ line.
> > > > > > We're also adding a new property similar to the one described
> > > > > > above, but for capture channels.  Typically, there will be less
> > > > > > capture channels than PWM-out, since all channels have the latter
> > > > > > capability, but only some have capture support.
> > > > > 
> > > > > Humm, sounds like all of this should be implied from compatible strings.
> > > > 
> > > > You mean have a bunch of of_machine_is_compatibles() scattered around?
> > > 
> > > I don't understand why you need this at all. Quite frankly I don't even
> > > know why st,pwm-num-devs exists. I probably missed it back at the time.
> > > Usually, like Rob suggests, this should be inferred from the compatible
> > > string. One commonly used way to avoid scattering explicit checks for
> > > the compatible string is to add this information to the of_device_id
> > > table. See a bunch of existing drivers for reference.
> > 
> > Yes, I am aware of the strategy, and happy to oblige if this is your
> > suggestion.  I'll move all platform data into the driver and eradicate
> > the DT properties.
> 
> Great!
> 
> > > Also, why make a separation of output vs. capture channels at this
> > > point? Could you not simply obtain the total number of PWM channels,
> > > preferably from SoC data associated with the compatible string, and
> > > check at ->capture() time whether or not the particular PWM supports
> > > this?
> > > 
> > > As-is, you imply that you have n (output) + m (capture) channels, and
> > > that 0..n-1 are output and n..n+m-1 are capture channels. What if that
> > > no longer holds true, but 0 and 2 are the only ones that support
> > > capture?
> > 
> > We do?  What makes you think that?
> 
> Because there's nothing saying otherwise? The DT binding is completely
> silent on the matter, and the above is the most logical interpretation
> that I came up with.

But you made it up. :)

It's not documented because it's irrelevant how channels are numbered.

Actually, looking at the docs, the IP doesn't change from one channel
to another, so in theory they all support capture.  The only issue is
whether they are plumbed in or not.  I'll take a  closer look at all
the platforms we support when I move the pdata.

> Looking at the driver it seems like it's actually the other way around
> and you have the first m channels that support both output and capture
> functionality. But the issue remains that this isn't documented in the
> DT binding documentation. And the current properties are also not very
> flexible because they allow for only a single scheme of assigning the
> capability.
> 
> If you move all of that knowledge into the driver, things become a lot
> easier, in my opinion.

Sure.

> > > If you check for this at runtime you can avoid complicated DT parsing
> > > code, but still get the safety check which should be enough to encourage
> > > people to use the right channels in DT.
> > 
> > I'm pretty sure I can move all the data into the driver.  I did want
> > to avoid having lots of different compatible strings, but if that's
> > what you're suggesting, I can introduce one per supported platform.
> 
> That sounds like the simplest solution to me.

Okay.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1419396 — Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jun 08, 2016 at 03:51:52PM -0500, Rob Herring wrote:
> On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > be more inline with the naming conventions of the subsystem.  Where
> > we used to treat each line as a channel, the PWM convention is to
> > describe them as devices.
> 
> That's all linux implementation details and you are breaking 
> compatibility.

While it's true that the API calls channels struct pwm_device, that's
primarily for historical reasons. It's completely fine, and in fact most
drivers already do exactly that, to internally refer to channels as
channels.

Thierry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web