[1/7] add binding for stm32 multifunctions timer driver

Message ID 1479831207-32699-2-git-send-email-benjamin.gaignard@st.com
State New
Headers show

Commit Message

Benjamin Gaignard Nov. 22, 2016, 4:13 p.m.
Add bindings information for stm32 timer MFD

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>

---
 .../devicetree/bindings/mfd/stm32-timer.txt        | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/stm32-timer.txt

-- 
1.9.1

Comments

Benjamin Gaignard Nov. 23, 2016, 8:15 a.m. | #1
2016-11-22 17:52 GMT+01:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 22 Nov 2016, Benjamin Gaignard wrote:

>

>> Add bindings information for stm32 timer MFD

>>

>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>

>> ---

>>  .../devicetree/bindings/mfd/stm32-timer.txt        | 53 ++++++++++++++++++++++

>>  1 file changed, 53 insertions(+)

>>  create mode 100644 Documentation/devicetree/bindings/mfd/stm32-timer.txt

>>

>> diff --git a/Documentation/devicetree/bindings/mfd/stm32-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-timer.txt

>> new file mode 100644

>> index 0000000..3cefce1

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/mfd/stm32-timer.txt

>> @@ -0,0 +1,53 @@

>> +STM32 multifunctions timer driver

>

> "STM32 Multi-Function Timer/PWM device bindings"

>

> Doesn't this shared device have a better name?


In SoC documentation those hardware blocks are named "advanced-control
timers", "general purpose timers" or "basic timers"
"stm32-timer" name is already used for clock source driver, that why I
have prefix it with mfd

>

>> +stm32 timer MFD allow to handle at the same time pwm and IIO timer devices

>

> No need for this sentence.

>

OK

>> +Required parameters:

>> +- compatible: must be one of the follow value:

>> +     "st,stm32-mfd-timer1"

>> +     "st,stm32-mfd-timer2"

>> +     "st,stm32-mfd-timer3"

>> +     "st,stm32-mfd-timer4"

>> +     "st,stm32-mfd-timer5"

>> +     "st,stm32-mfd-timer6"

>> +     "st,stm32-mfd-timer7"

>> +     "st,stm32-mfd-timer8"

>> +     "st,stm32-mfd-timer9"

>> +     "st,stm32-mfd-timer10"

>> +     "st,stm32-mfd-timer11"

>> +     "st,stm32-mfd-timer12"

>> +     "st,stm32-mfd-timer13"

>> +     "st,stm32-mfd-timer14"

>

> We don't normally number devices.

>

> What's stopping you from simply doing:

>

>         pwm1: pwm1@40010000 {

>                 compatible = "st,stm32-pwm";

>         };

>         pwm2: pwm1@40020000 {

>                 compatible = "st,stm32-pwm";

>         };

>         pwm3: pwm1@40030000 {

>                 compatible = "st,stm32-pwm";

>         };

>


Because each instance of the hardware is slightly different: number of
pwm channels, triggers capabilities, etc ..
so I need to distinguish them.
Since it look to be a problem I will follow your suggestion and add a
property this driver to be able to identify each instance.
Do you think that "id" parameter (integer for 1 to 14) is acceptable ?

>> +- reg :                      Physical base address and length of the controller's

>> +                     registers.

>> +- clock-names:               Set to "mfd_timer_clk".

>

Only one but I use devm_regmap_init_mmio_clk() to avoid calling
clk_{enable/disable}
everywhere in the drivers when reading/writing regsister.
devm_regmap_init_mmio_clk() find the clock by it name that why I have
put it here
In the doc this clock in named "clk_int" I will use this name.


> How many clocks are there?

>

> If only 1, you don't need this property.

>

> "mfd_timer_clk" is not the correct name.

>

> What is it called in the datasheet?

>

>> +- clocks:            Phandle of the clock used by the timer module.

>

> "Phandle to the clock ..."

>

>> +                     For Clk properties, please refer to [1].

>> +- interrupts :               Reference to the timer interrupt

>

> Reference to?

>

> See how other binding documents describe this property.

>

>> +Optional parameters:

>> +- resets :           Reference to a reset controller asserting the timer

>

> As above.

>

>> +Optional subnodes:

>

> Either use ":" or " :" or "<tab>:", but keep it consistent.

>

>> +- pwm:                       See Documentation/devicetree/bindings/pwm/pwm-stm32.txt

>> +- iiotimer:          See Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt

>> +

>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt

>

> Use the relative paths "../clock/", "../pwm/", "../iio/".

>

OK

>> +Example:

>> +     mfd_timer1: mfdtimer1@40010000 {

>

> This is not an "MFD timer".  MFD is a Linuxisum.

>

>> +             compatible = "st,stm32-mfd-timer1";

>

> Better description required.

>

>> +             reg = <0x40010000 0x400>;

>> +             clocks = <&rcc 0 160>;

>> +             clock-names = "mfd_timer_clk";

>> +             interrupts = <27>;

>> +

>> +             pwm1: pwm1@40010000 {

>> +                     compatible = "st,stm32-pwm1";

>> +             };

>> +

>> +             iiotimer1: iiotimer1@40010000 {

>> +                     compatible = "st,stm32-iio-timer1";

>> +             };

>> +     };
Lee Jones Nov. 23, 2016, 9:21 a.m. | #2
On Wed, 23 Nov 2016, Benjamin Gaignard wrote:

> 2016-11-22 17:52 GMT+01:00 Lee Jones <lee.jones@linaro.org>:

> > On Tue, 22 Nov 2016, Benjamin Gaignard wrote:

> >

> >> Add bindings information for stm32 timer MFD

> >>

> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>

> >> ---

> >>  .../devicetree/bindings/mfd/stm32-timer.txt        | 53 ++++++++++++++++++++++

> >>  1 file changed, 53 insertions(+)

> >>  create mode 100644 Documentation/devicetree/bindings/mfd/stm32-timer.txt

> >>

> >> diff --git a/Documentation/devicetree/bindings/mfd/stm32-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-timer.txt

> >> new file mode 100644

> >> index 0000000..3cefce1

> >> --- /dev/null

> >> +++ b/Documentation/devicetree/bindings/mfd/stm32-timer.txt

> >> @@ -0,0 +1,53 @@

> >> +STM32 multifunctions timer driver

> >

> > "STM32 Multi-Function Timer/PWM device bindings"

> >

> > Doesn't this shared device have a better name?

> 

> In SoC documentation those hardware blocks are named "advanced-control

> timers", "general purpose timers" or "basic timers"

> "stm32-timer" name is already used for clock source driver, that why I

> have prefix it with mfd


MFD is a Linuxisum and has no place in hardware description.

Please used one of the names you mentioned above.

Hopefully the one that best fits.

> >> +stm32 timer MFD allow to handle at the same time pwm and IIO timer devices

> >

> > No need for this sentence.

> >

> OK

> 

> >> +Required parameters:

> >> +- compatible: must be one of the follow value:

> >> +     "st,stm32-mfd-timer1"

> >> +     "st,stm32-mfd-timer2"

> >> +     "st,stm32-mfd-timer3"

> >> +     "st,stm32-mfd-timer4"

> >> +     "st,stm32-mfd-timer5"

> >> +     "st,stm32-mfd-timer6"

> >> +     "st,stm32-mfd-timer7"

> >> +     "st,stm32-mfd-timer8"

> >> +     "st,stm32-mfd-timer9"

> >> +     "st,stm32-mfd-timer10"

> >> +     "st,stm32-mfd-timer11"

> >> +     "st,stm32-mfd-timer12"

> >> +     "st,stm32-mfd-timer13"

> >> +     "st,stm32-mfd-timer14"

> >

> > We don't normally number devices.

> >

> > What's stopping you from simply doing:

> >

> >         pwm1: pwm1@40010000 {

> >                 compatible = "st,stm32-pwm";

> >         };

> >         pwm2: pwm1@40020000 {

> >                 compatible = "st,stm32-pwm";

> >         };

> >         pwm3: pwm1@40030000 {

> >                 compatible = "st,stm32-pwm";

> >         };

> >

> 

> Because each instance of the hardware is slightly different: number of

> pwm channels, triggers capabilities, etc ..

> so I need to distinguish them.

> Since it look to be a problem I will follow your suggestion and add a

> property this driver to be able to identify each instance.

> Do you think that "id" parameter (integer for 1 to 14) is acceptable ?


Unfortunately not. IDs aren't allowed in DT.

What about "pwm-chans" and "trigger"?

pwm-chans  	       : Number of available channels available
trigger		       : Boolean value specifying whether a timer is present

Why can't you let of_platform_populate() register the devices for you?
Then you can get rid of all of the meaningless numbers all over the place.

> >> +- reg :                      Physical base address and length of the controller's

> >> +                     registers.

> >> +- clock-names:               Set to "mfd_timer_clk".

> >

> Only one but I use devm_regmap_init_mmio_clk() to avoid calling

> clk_{enable/disable}

> everywhere in the drivers when reading/writing regsister.

> devm_regmap_init_mmio_clk() find the clock by it name that why I have

> put it here

> In the doc this clock in named "clk_int" I will use this name.


Please reply *below* the quote.

But okay, "clk_int" sounds like a more suitable name.

> > How many clocks are there?

> >

> > If only 1, you don't need this property.

> >

> > "mfd_timer_clk" is not the correct name.

> >

> > What is it called in the datasheet?

> >

> >> +- clocks:            Phandle of the clock used by the timer module.

> >

> > "Phandle to the clock ..."

> >

> >> +                     For Clk properties, please refer to [1].

> >> +- interrupts :               Reference to the timer interrupt

> >

> > Reference to?

> >

> > See how other binding documents describe this property.

> >

> >> +Optional parameters:

> >> +- resets :           Reference to a reset controller asserting the timer

> >

> > As above.

> >

> >> +Optional subnodes:

> >

> > Either use ":" or " :" or "<tab>:", but keep it consistent.

> >

> >> +- pwm:                       See Documentation/devicetree/bindings/pwm/pwm-stm32.txt

> >> +- iiotimer:          See Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt

> >> +

> >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt

> >

> > Use the relative paths "../clock/", "../pwm/", "../iio/".

> >

> OK

> 

> >> +Example:

> >> +     mfd_timer1: mfdtimer1@40010000 {

> >

> > This is not an "MFD timer".  MFD is a Linuxisum.

> >

> >> +             compatible = "st,stm32-mfd-timer1";

> >

> > Better description required.

> >

> >> +             reg = <0x40010000 0x400>;

> >> +             clocks = <&rcc 0 160>;

> >> +             clock-names = "mfd_timer_clk";

> >> +             interrupts = <27>;

> >> +

> >> +             pwm1: pwm1@40010000 {

> >> +                     compatible = "st,stm32-pwm1";

> >> +             };

> >> +

> >> +             iiotimer1: iiotimer1@40010000 {

> >> +                     compatible = "st,stm32-iio-timer1";

> >> +             };

> >> +     };


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones Nov. 24, 2016, 8:52 a.m. | #3
Rob,

Would you mind casting an eye on this please?

On Wed, 23 Nov 2016, Benjamin Gaignard wrote:
> 2016-11-23 10:21 GMT+01:00 Lee Jones <lee.jones@linaro.org>:

> > On Wed, 23 Nov 2016, Benjamin Gaignard wrote:

> >

> >> 2016-11-22 17:52 GMT+01:00 Lee Jones <lee.jones@linaro.org>:

> >> > On Tue, 22 Nov 2016, Benjamin Gaignard wrote:

> >> >

> >> >> Add bindings information for stm32 timer MFD

> >> >>

> >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>

> >> >> ---

> >> >>  .../devicetree/bindings/mfd/stm32-timer.txt        | 53 ++++++++++++++++++++++

> >> >>  1 file changed, 53 insertions(+)

> >> >>  create mode 100644 Documentation/devicetree/bindings/mfd/stm32-timer.txt

> >> >>

> >> >> diff --git a/Documentation/devicetree/bindings/mfd/stm32-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-timer.txt

> >> >> new file mode 100644

> >> >> index 0000000..3cefce1

> >> >> --- /dev/null

> >> >> +++ b/Documentation/devicetree/bindings/mfd/stm32-timer.txt

> >> >> @@ -0,0 +1,53 @@

> >> >> +STM32 multifunctions timer driver

> >> >

> >> > "STM32 Multi-Function Timer/PWM device bindings"

> >> >

> >> > Doesn't this shared device have a better name?

> >>

> >> In SoC documentation those hardware blocks are named "advanced-control

> >> timers", "general purpose timers" or "basic timers"

> >> "stm32-timer" name is already used for clock source driver, that why I

> >> have prefix it with mfd

> >

> > MFD is a Linuxisum and has no place in hardware description.

> >

> > Please used one of the names you mentioned above.

> 

> I will go for "st,stm32-advanced-timer"

> 

> >

> > Hopefully the one that best fits.

> >

> >> >> +stm32 timer MFD allow to handle at the same time pwm and IIO timer devices

> >> >

> >> > No need for this sentence.

> >> >

> >> OK

> >>

> >> >> +Required parameters:

> >> >> +- compatible: must be one of the follow value:

> >> >> +     "st,stm32-mfd-timer1"

> >> >> +     "st,stm32-mfd-timer2"

> >> >> +     "st,stm32-mfd-timer3"

> >> >> +     "st,stm32-mfd-timer4"

> >> >> +     "st,stm32-mfd-timer5"

> >> >> +     "st,stm32-mfd-timer6"

> >> >> +     "st,stm32-mfd-timer7"

> >> >> +     "st,stm32-mfd-timer8"

> >> >> +     "st,stm32-mfd-timer9"

> >> >> +     "st,stm32-mfd-timer10"

> >> >> +     "st,stm32-mfd-timer11"

> >> >> +     "st,stm32-mfd-timer12"

> >> >> +     "st,stm32-mfd-timer13"

> >> >> +     "st,stm32-mfd-timer14"

> >> >

> >> > We don't normally number devices.

> >> >

> >> > What's stopping you from simply doing:

> >> >

> >> >         pwm1: pwm1@40010000 {

> >> >                 compatible = "st,stm32-pwm";

> >> >         };

> >> >         pwm2: pwm1@40020000 {

> >> >                 compatible = "st,stm32-pwm";

> >> >         };

> >> >         pwm3: pwm1@40030000 {

> >> >                 compatible = "st,stm32-pwm";

> >> >         };

> >> >

> >>

> >> Because each instance of the hardware is slightly different: number of

> >> pwm channels, triggers capabilities, etc ..

> >> so I need to distinguish them.

> >> Since it look to be a problem I will follow your suggestion and add a

> >> property this driver to be able to identify each instance.

> >> Do you think that "id" parameter (integer for 1 to 14) is acceptable ?

> >

> > Unfortunately not. IDs aren't allowed in DT.

> >

> > What about "pwm-chans" and "trigger"?

> >

> > pwm-chans              : Number of available channels available

> 

> For pwm I need those 4 properties:

> st,pwm-number: the number of PWM devices


st,pwm-num-chan is already documented.

Please use that instead of creating new properties.

> st,complementary: if exist have complementary ouput

> st,32bit-counter: if exist have 32 bits counter

> st,breakinput-polarity: if set enable break input feature.

> 

> Is it acceptable from pwm maintainer point of view ?

> 

> > trigger                : Boolean value specifying whether a timer is present

> 

> Following our discussion on IRC I will try to code for your proposal:

> 

> advanced-timer@40010000 {

>         compatible = "st,stm32-advanced-timer";

>         reg = <0x40010000 0x400>;

>         clocks = <&rcc 0 160>;

>         clock-names = "clk_int";

> 

>         pwm@0 {

>                 compatible = "st,stm32-pwm";

>                 st,pwm-number= <4>;

>                 st,complementary;

>                 st,breakinput;

>         };

> 

>         timer@0 {

>                 reg = <1>;

>                 compatible = "st,stm32-iio-timer";

>                 interrupts = <27>;

>                 triggers = <5 2 3 4>;

>         };

> };

> 

> triggers parameter will be used to know which trigger are valid for

> the IIO device


Except for "st,pwm-number" as mentioned above, this looks good to me.

Rob, would what do you think?

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

Patch hide | download patch | download mbox

diff --git a/Documentation/devicetree/bindings/mfd/stm32-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-timer.txt
new file mode 100644
index 0000000..3cefce1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/stm32-timer.txt
@@ -0,0 +1,53 @@ 
+STM32 multifunctions timer driver
+
+stm32 timer MFD allow to handle at the same time pwm and IIO timer devices
+
+Required parameters:
+- compatible: must be one of the follow value:
+	"st,stm32-mfd-timer1"
+	"st,stm32-mfd-timer2"
+	"st,stm32-mfd-timer3"
+	"st,stm32-mfd-timer4"
+	"st,stm32-mfd-timer5"
+	"st,stm32-mfd-timer6"
+	"st,stm32-mfd-timer7"
+	"st,stm32-mfd-timer8"
+	"st,stm32-mfd-timer9"
+	"st,stm32-mfd-timer10"
+	"st,stm32-mfd-timer11"
+	"st,stm32-mfd-timer12"
+	"st,stm32-mfd-timer13"
+	"st,stm32-mfd-timer14"
+
+- reg :			Physical base address and length of the controller's
+			registers.
+- clock-names: 		Set to "mfd_timer_clk".
+- clocks: 		Phandle of the clock used by the timer module.
+			For Clk properties, please refer to [1].
+- interrupts :		Reference to the timer interrupt
+
+Optional parameters:
+- resets :		Reference to a reset controller asserting the timer
+
+Optional subnodes:
+- pwm:			See Documentation/devicetree/bindings/pwm/pwm-stm32.txt
+- iiotimer:		See Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Example:
+	mfd_timer1: mfdtimer1@40010000 {
+		compatible = "st,stm32-mfd-timer1";
+		reg = <0x40010000 0x400>;
+		clocks = <&rcc 0 160>;
+		clock-names = "mfd_timer_clk";
+		interrupts = <27>;
+
+		pwm1: pwm1@40010000 {
+			compatible = "st,stm32-pwm1";
+		};
+
+		iiotimer1: iiotimer1@40010000 {
+			compatible = "st,stm32-iio-timer1";
+		};
+	};