| Message ID | 1479831207-32699-2-git-send-email-benjamin.gaignard@st.com |
|---|---|
| State | New |
| Headers | show |
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"; >> + }; >> + };
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
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
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"; + }; + };
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