On Wed, 23 Nov 2016, Benjamin Gaignard wrote:
> 2016-11-22 17:52 GMT+01:00 Lee Jones <lee.jones@xxxxxxxxxx>:
> > On Tue, 22 Nov 2016, Benjamin Gaignard wrote:
> >
> >> Add bindings information for stm32 timer MFD
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxx>
> >> ---
> >> .../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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
