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

Groups > linux.kernel > #1527647 > unrolled thread

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

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 1/7] add binding for stm32 multifunctions timer driver Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2016-11-22 17:20 +0100
    Re: [PATCH 1/7] add binding for stm32 multifunctions timer driver Lee Jones <lee.jones@linaro.org> - 2016-11-22 18:00 +0100
      Re: [PATCH 1/7] add binding for stm32 multifunctions timer driver Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2016-11-23 09:20 +0100
        Re: [PATCH 1/7] add binding for stm32 multifunctions timer driver Lee Jones <lee.jones@linaro.org> - 2016-11-23 10:20 +0100
          Re: [PATCH 1/7] add binding for stm32 multifunctions timer driver Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2016-11-23 18:10 +0100
            Re: [PATCH 1/7] add binding for stm32 multifunctions timer driver Lee Jones <lee.jones@linaro.org> - 2016-11-24 10:00 +0100

#1527647 — [PATCH 1/7] add binding for stm32 multifunctions timer driver

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 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";
+		};
+	};
-- 
1.9.1

[toc] | [next] | [standalone]

#1527704

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?

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

No need for this sentence.

> +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";
	};

> +- reg :			Physical base address and length of the controller's
> +			registers.
> +- clock-names: 		Set to "mfd_timer_clk".

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/".

> +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

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

#1528181

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";
>> +             };
>> +     };

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

#1528232

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

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

#1528606

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,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

[snip]

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

#1529073

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web