| Message ID | 20160608092135.21184-21-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>; };
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(-) -- 2.8.3 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html