| Message ID | 20250227162823.3585810-7-david@protonic.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Linux Motion Control subsystem | expand |
On Thu, Feb 27, 2025 at 05:28:22PM +0100, David Jander wrote: > +$id: http://devicetree.org/schemas/motion/adi,tmc5240.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices TMC5240 Stepper Motor controller > + > +maintainers: > + - David Jander <david@protonic> > + > +description: | Do not need '|' unless you need to preserve formatting. > + Stepper motor controller with motion engine and SPI interface. > + > +properties: > + compatible: > + enum: > + - adi,tmc5240 > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + enable-supply: > + description: Optional external enable supply to control SLEEPn pin. Can That's odd. regular pins are not supplies. This must be named after physical supplies. There is vdd18, vcc, vcp but nothing about enable supply in datasheet. > + be shared between several controllers. Second sentence is both redundant and really not relevant to this binding. It's not this binding which decides about sharing. > + > + clocks: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + - $ref: /schemas/motion/common.yaml# > + > +unevaluatedProperties: false > + > +examples: > + - | > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + motor@0 { > + compatible = "adi,tmc5240"; > + reg = <0>; > + interrupts-extended = <&gpiok 7 0>; Include header and use standard defines for flags. > + clocks = <&clock_tmc5240>; > + enable-supply = <&stpsleepn>; > + spi-max-frequency = <1000000>; Where are any other properties from common schema? Best regards, Krzysztof
Dear Krzysztof, Thanks for reviewing... On Fri, 28 Feb 2025 08:11:04 +0100 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Thu, Feb 27, 2025 at 05:28:22PM +0100, David Jander wrote: > [...] > > + > > + enable-supply: > > + description: Optional external enable supply to control SLEEPn pin. Can > > That's odd. regular pins are not supplies. This must be named after > physical supplies. There is vdd18, vcc, vcp but nothing about enable > supply in datasheet. > > > + be shared between several controllers. > > Second sentence is both redundant and really not relevant to this > binding. It's not this binding which decides about sharing. Good point. I think I should drop the whole property, since it is indeed irrelevant. If extra supplies need to be specified, they always can be, right? > > + > > + clocks: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + > > +allOf: > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + - $ref: /schemas/motion/common.yaml# > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + motor@0 { > > + compatible = "adi,tmc5240"; > > + reg = <0>; > > + interrupts-extended = <&gpiok 7 0>; > > Include header and use standard defines for flags. Thanks. I didn't know I could include them here... will fix this. > > > + clocks = <&clock_tmc5240>; > > + enable-supply = <&stpsleepn>; > > + spi-max-frequency = <1000000>; > > Where are any other properties from common schema? The properties in common.yaml are optional. Do I need to explicitly specify this somehow (they are not listed under "required:")? In fact, the tmc5240 driver has its own known constants for these properties that it fills in. One can overrule them here if needed, but I suppose in the case of the tmc5240 the regular use-case is not to do that. Should I maybe add a second example that overrules the defaults? Best regards,
On 28/02/2025 09:48, David Jander wrote: > > Dear Krzysztof, > > Thanks for reviewing... > > On Fri, 28 Feb 2025 08:11:04 +0100 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> On Thu, Feb 27, 2025 at 05:28:22PM +0100, David Jander wrote: >> [...] >>> + >>> + enable-supply: >>> + description: Optional external enable supply to control SLEEPn pin. Can >> >> That's odd. regular pins are not supplies. This must be named after >> physical supplies. There is vdd18, vcc, vcp but nothing about enable >> supply in datasheet. >> >>> + be shared between several controllers. >> >> Second sentence is both redundant and really not relevant to this >> binding. It's not this binding which decides about sharing. > > Good point. I think I should drop the whole property, since it is indeed > irrelevant. If extra supplies need to be specified, they always can be, right? You should specify all supplies now, because hardware should be fully described by binding and DTS. What's more, the necessary supplies (according to datasheet) should be required, not optional. > >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - clocks >>> + >>> +allOf: >>> + - $ref: /schemas/spi/spi-peripheral-props.yaml# >>> + - $ref: /schemas/motion/common.yaml# >>> + >>> +unevaluatedProperties: false >>> + >>> +examples: >>> + - | >>> + spi { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + motor@0 { >>> + compatible = "adi,tmc5240"; >>> + reg = <0>; >>> + interrupts-extended = <&gpiok 7 0>; >> >> Include header and use standard defines for flags. > > Thanks. I didn't know I could include them here... will fix this. > >> >>> + clocks = <&clock_tmc5240>; >>> + enable-supply = <&stpsleepn>; >>> + spi-max-frequency = <1000000>; >> >> Where are any other properties from common schema? > > The properties in common.yaml are optional. Do I need to explicitly specify > this somehow (they are not listed under "required:")? The common says default=1, so it is fine to skip them if that default makes sense here. Anyway, it's fine. > In fact, the tmc5240 driver has its own known constants for these properties > that it fills in. One can overrule them here if needed, but I suppose in the > case of the tmc5240 the regular use-case is not to do that. Should I maybe add > a second example that overrules the defaults? Not necessarily, one real-world, complete example is enough. Best regards, Krzysztof
On Fri, 28 Feb 2025 10:35:38 +0100 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 28/02/2025 09:48, David Jander wrote: > > > > Dear Krzysztof, > > > > Thanks for reviewing... > > > > On Fri, 28 Feb 2025 08:11:04 +0100 > > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > >> On Thu, Feb 27, 2025 at 05:28:22PM +0100, David Jander wrote: > >> [...] > >>> + > >>> + enable-supply: > >>> + description: Optional external enable supply to control SLEEPn pin. Can > >> > >> That's odd. regular pins are not supplies. This must be named after > >> physical supplies. There is vdd18, vcc, vcp but nothing about enable > >> supply in datasheet. > >> > >>> + be shared between several controllers. > >> > >> Second sentence is both redundant and really not relevant to this > >> binding. It's not this binding which decides about sharing. > > > > Good point. I think I should drop the whole property, since it is indeed > > irrelevant. If extra supplies need to be specified, they always can be, right? > > You should specify all supplies now, because hardware should be fully > described by binding and DTS. In the case of the hardware I use for testing all of this, there are several tmc5240 chips which have their "SLEEPN" pin tied together controlled by a single GPIO pin that needs to be pulled high before any of these chips can be talked to. The usual way I know of solving this is by specifying a common "virtual" supply of type "regulator-fixed" with an enable gpio. But this isn't strictly a supply that has to do with this chip or driver, so I don't think it should be specified in the schema. I do need to use it in my particular case though. Is there a better way of doing this? > What's more, the necessary supplies (according to datasheet) should be > required, not optional. Do you mean that they should be in the binding definition as well? I.e. add all of Vs, Vdd1v8 and Vcc_io here? Best regards,
On 28/02/2025 10:51, David Jander wrote: >>>> >>>> Second sentence is both redundant and really not relevant to this >>>> binding. It's not this binding which decides about sharing. >>> >>> Good point. I think I should drop the whole property, since it is indeed >>> irrelevant. If extra supplies need to be specified, they always can be, right? >> >> You should specify all supplies now, because hardware should be fully >> described by binding and DTS. > > In the case of the hardware I use for testing all of this, there are several > tmc5240 chips which have their "SLEEPN" pin tied together controlled by a > single GPIO pin that needs to be pulled high before any of these chips can be > talked to. The usual way I know of solving this is by specifying a common > "virtual" supply of type "regulator-fixed" with an enable gpio. No, that is not usual way. Representing pin as fake supply is hack and not correct hardware description. > But this isn't strictly a supply that has to do with this chip or driver, so I > don't think it should be specified in the schema. I do need to use it in my > particular case though. Is there a better way of doing this? I speak about voltage and current supplies. These you must specify. > >> What's more, the necessary supplies (according to datasheet) should be >> required, not optional. > > Do you mean that they should be in the binding definition as well? I.e. add > all of Vs, Vdd1v8 and Vcc_io here? Yes, all expected supplies must be in the binding. Best regards, Krzysztof
On 2/27/25 10:28 AM, David Jander wrote: > Add device-tree bindings for Analog Devices TMC5240 stepper controllers. > > Signed-off-by: David Jander <david@protonic.nl> > --- > .../bindings/motion/adi,tmc5240.yaml | 60 +++++++++++++++++++ > 1 file changed, 60 insertions(+) > create mode 100644 Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > diff --git a/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > new file mode 100644 > index 000000000000..3364f9dfccb1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > @@ -0,0 +1,60 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/motion/adi,tmc5240.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices TMC5240 Stepper Motor controller > + > +maintainers: > + - David Jander <david@protonic> > + > +description: | > + Stepper motor controller with motion engine and SPI interface. Please include a link to the datasheet. > + > +properties: > + compatible: > + enum: > + - adi,tmc5240 > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 I assume that this is the overvoltage output (OV pin). Would be nice to have a description here saying that. There are also NAO and DIAG0/1 output pins, so it's a bit ambiguous otherwise. > + > + enable-supply: > + description: Optional external enable supply to control SLEEPn pin. Can > + be shared between several controllers. > + This doesn't look like a supply, but krzk already discussed that. But there should be actual power supplies: vs-supply, vdd1v8-supply, vcc-io-supply. And a reference voltage supply: iref-supply And if there are any pins would make sense to connect to a gpio, we can add those even if the driver doesn't use it currently. > + clocks: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + - $ref: /schemas/motion/common.yaml# If we need to know about what is connected to the output of a motor controller I would expect it to be done with child node for each output. That way each output can be unique, if needed. Basically, similar to iio/adc.yaml is used to provide common properties for channel@ child nodes on iio devices. > + > +unevaluatedProperties: false > + > +examples: > + - | > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + motor@0 { motor-controller@ or actuator-controller@ The chip is the controller/driver, it is not a motor. > + compatible = "adi,tmc5240"; > + reg = <0>; > + interrupts-extended = <&gpiok 7 0>; > + clocks = <&clock_tmc5240>; > + enable-supply = <&stpsleepn>; > + spi-max-frequency = <1000000>; > + }; > + }; > +
Dear David, On Fri, 28 Feb 2025 16:38:51 -0600 David Lechner <dlechner@baylibre.com> wrote: > On 2/27/25 10:28 AM, David Jander wrote: > > Add device-tree bindings for Analog Devices TMC5240 stepper controllers. > > > > Signed-off-by: David Jander <david@protonic.nl> > > --- > > .../bindings/motion/adi,tmc5240.yaml | 60 +++++++++++++++++++ > > 1 file changed, 60 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > > > diff --git a/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > new file mode 100644 > > index 000000000000..3364f9dfccb1 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > @@ -0,0 +1,60 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/motion/adi,tmc5240.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices TMC5240 Stepper Motor controller > > + > > +maintainers: > > + - David Jander <david@protonic> > > + > > +description: | > > + Stepper motor controller with motion engine and SPI interface. > > Please include a link to the datasheet. Will do. > > + > > +properties: > > + compatible: > > + enum: > > + - adi,tmc5240 > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > I assume that this is the overvoltage output (OV pin). Would be nice to have > a description here saying that. There are also NAO and DIAG0/1 output pins, so > it's a bit ambiguous otherwise. This is the DIAG0 output pin which on this chip has a dual function as either a STEP output or an interrupt output. The pin name is a bit misleading, but it is the "interrupt" function that is meant here. The datasheet documents all the different events that can trigger this interrupt. I will add a description to clarify this. > > + > > + enable-supply: > > + description: Optional external enable supply to control SLEEPn pin. Can > > + be shared between several controllers. > > + > > This doesn't look like a supply, but krzk already discussed that. But there > should be actual power supplies: vs-supply, vdd1v8-supply, vcc-io-supply. And > a reference voltage supply: iref-supply I have added vs-supply and vcc-io-supply to the binding. These are the only supply pins that can be connected to the outside world or otherwise be of concern to the software. vdd1v8-supply is an internal power rail that must not have a connection to the outside of the chip (besides an external filtering capacitor) and also doesn't have any bearing to the software at all. It cannot be disabled, adjusted or anything, so I don't think it needs to be mentioned. IREF isn't a supply pin. It is merely a pin for connecting an external reference resistor that is used internally for current scaling and it too has no interaction with the software in any way. The resistor connected to the IREF pin (Rref) OTOH does have an implication to the software, as it sets the full-range current of the output stage. How should we specify that? Is it adequate to add an optional DT property "rref" or "rref-ohm" with an int32 value in Ohm? The default value if unspecified is 12000 Ohm. > And if there are any pins would make sense to connect to a gpio, we can add > those even if the driver doesn't use it currently. > > > + clocks: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + > > +allOf: > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + - $ref: /schemas/motion/common.yaml# > > If we need to know about what is connected to the output of a motor controller > I would expect it to be done with child node for each output. That way each > output can be unique, if needed. Basically, similar to iio/adc.yaml is used to > provide common properties for channel@ child nodes on iio devices. This controller chip only has one single output for one stepper motor (4 wires). While technically you could connect something else to those 4 wires, I don't think it is the scope of LMC to support that. The chip itself isn't designed for that purpose and it would clearly go far beyond the intended purpose of this device. That being said, your suggestion of supporting child nodes may actually be a good idea. Right now, we specify the type of motor (basically nominal- and hold current settings) in user-space and set the IRUN/IHOLD parameters from user-space via the sysfs attributes interface. It might make sense to have a DT child node to specify this, although in our current application this is not very practical, since there are many motor controllers on one board, and it is configurable in software (runtime) which motor is connected to which output. But I can imagine a situation where it may be fixed and thus can be described in the DT of a board. Then again I don't know if it would be over-complicating things with something like this: motor-controller@0 { ... motor@0 { compatible = "nanotec,st4118s1006"; irun-ma = <1800>; ihold-ma = <270>; }; }; where we'd possibly have a stepper-motors.c file with a lot of structs and matching tables for the different motor types.... sounds like overkill to me, but maybe not? > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + motor@0 { > > motor-controller@ or actuator-controller@ > > The chip is the controller/driver, it is not a motor. Make sense. Will change this. > > + compatible = "adi,tmc5240"; > > + reg = <0>; > > + interrupts-extended = <&gpiok 7 0>; > > + clocks = <&clock_tmc5240>; > > + enable-supply = <&stpsleepn>; > > + spi-max-frequency = <1000000>; > > + }; > > + }; Best regards,
(Sorry if you get this twice. I don't have my regular computer today and didn't realize I was sending HTML the first time. Resending in plain text so the lists pick it up.) On Mon, Mar 3, 2025 at 12:22 PM David Jander <david@protonic.nl> wrote: > > > Dear David, > > On Fri, 28 Feb 2025 16:38:51 -0600 > David Lechner <dlechner@baylibre.com> wrote: > > > On 2/27/25 10:28 AM, David Jander wrote: > > > Add device-tree bindings for Analog Devices TMC5240 stepper controllers. > > > > > > Signed-off-by: David Jander <david@protonic.nl> > > > --- > > > .../bindings/motion/adi,tmc5240.yaml | 60 +++++++++++++++++++ > > > 1 file changed, 60 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > > new file mode 100644 > > > index 000000000000..3364f9dfccb1 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > > @@ -0,0 +1,60 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/motion/adi,tmc5240.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Analog Devices TMC5240 Stepper Motor controller > > > + > > > +maintainers: > > > + - David Jander <david@protonic> > > > + > > > +description: | > > > + Stepper motor controller with motion engine and SPI interface. > > > > Please include a link to the datasheet. > > Will do. > > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - adi,tmc5240 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > > I assume that this is the overvoltage output (OV pin). Would be nice to have > > a description here saying that. There are also NAO and DIAG0/1 output pins, so > > it's a bit ambiguous otherwise. > > This is the DIAG0 output pin which on this chip has a dual function as either > a STEP output or an interrupt output. The pin name is a bit misleading, but it > is the "interrupt" function that is meant here. The datasheet documents all > the different events that can trigger this interrupt. > I will add a description to clarify this. > If it makes sense that other pins could possibly ever be connected to interrupts then we can add those and also add interrupt-names (but only if there is more than one possible interrupt). > > > + > > > + enable-supply: > > > + description: Optional external enable supply to control SLEEPn pin. Can > > > + be shared between several controllers. > > > + > > > > This doesn't look like a supply, but krzk already discussed that. But there > > should be actual power supplies: vs-supply, vdd1v8-supply, vcc-io-supply. And > > a reference voltage supply: iref-supply > > I have added vs-supply and vcc-io-supply to the binding. These are the only > supply pins that can be connected to the outside world or otherwise be of > concern to the software. > > vdd1v8-supply is an internal power rail that must not have a connection to the > outside of the chip (besides an external filtering capacitor) and also doesn't > have any bearing to the software at all. It cannot be disabled, adjusted or > anything, so I don't think it needs to be mentioned. > > IREF isn't a supply pin. It is merely a pin for connecting an external > reference resistor that is used internally for current scaling and it too has > no interaction with the software in any way. > Ah, I read the datasheet too quickly. > The resistor connected to the IREF pin (Rref) OTOH does have an implication to > the software, as it sets the full-range current of the output stage. > > How should we specify that? Is it adequate to add an optional DT property > "rref" or "rref-ohm" with an int32 value in Ohm? The default value if > unspecified is 12000 Ohm. It looks like there are a few standardized properties, like sense-resistor-ohms if that fits the use case. Otherwise, an vendor-specific ti,rref-ohms would work. FYI, you can find the preferred units at [1]. [1]: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml > > > And if there are any pins would make sense to connect to a gpio, we can add > > those even if the driver doesn't use it currently. > > > > > + clocks: > > > + maxItems: 1 > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - interrupts > > > + - clocks > > > + > > > +allOf: > > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > + - $ref: /schemas/motion/common.yaml# > > > > If we need to know about what is connected to the output of a motor controller > > I would expect it to be done with child node for each output. That way each > > output can be unique, if needed. Basically, similar to iio/adc.yaml is used to > > provide common properties for channel@ child nodes on iio devices. > > This controller chip only has one single output for one stepper motor (4 > wires). While technically you could connect something else to those 4 wires, I > don't think it is the scope of LMC to support that. The chip itself isn't > designed for that purpose and it would clearly go far beyond the intended > purpose of this device. > > That being said, your suggestion of supporting child nodes may actually be a > good idea. Right now, we specify the type of motor (basically nominal- and hold > current settings) in user-space and set the IRUN/IHOLD parameters from > user-space via the sysfs attributes interface. It might make sense to have a DT > child node to specify this, although in our current application this is not > very practical, since there are many motor controllers on one board, and it is > configurable in software (runtime) which motor is connected to which output. > > But I can imagine a situation where it may be fixed and thus can be described > in the DT of a board. > > Then again I don't know if it would be over-complicating things with something > like this: > > motor-controller@0 { > ... > motor@0 { > compatible = "nanotec,st4118s1006"; > irun-ma = <1800>; > ihold-ma = <270>; > }; > }; > > where we'd possibly have a stepper-motors.c file with a lot of structs and > matching tables for the different motor types.... sounds like overkill to me, > but maybe not? A compatible for motors seems too much. I was just thinking along the lines that 1) if we need to so some scaling or something that depends on a motor constant, then it would make sense to put those constants in the DT and 2) if there is a motor controller with more than one output that could be connected to two or more different sizes of motors with different constants, then we either need child nodes or an array to be able to enter the different constants. Either one would work. So maybe simpler to just use an array instead of child nodes now that I'm thinking about it more. > > > > + > > > +unevaluatedProperties: false > > > + > > > +examples: > > > + - | > > > + spi { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + motor@0 { > > > > motor-controller@ or actuator-controller@ > > > > The chip is the controller/driver, it is not a motor. > > Make sense. Will change this. > > > > + compatible = "adi,tmc5240"; > > > + reg = <0>; > > > + interrupts-extended = <&gpiok 7 0>; > > > + clocks = <&clock_tmc5240>; > > > + enable-supply = <&stpsleepn>; > > > + spi-max-frequency = <1000000>; > > > + }; > > > + }; > > Best regards, > > -- > David Jander
Dear David, On Mon, 3 Mar 2025 13:28:35 +0100 David Lechner <dlechner@baylibre.com> wrote: > (Sorry if you get this twice. I don't have my regular computer today > and didn't realize I was sending HTML the first time. Resending in > plain text so the lists pick it up.) > > On Mon, Mar 3, 2025 at 12:22 PM David Jander <david@protonic.nl> wrote: > > > > > > Dear David, > > > > On Fri, 28 Feb 2025 16:38:51 -0600 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > On 2/27/25 10:28 AM, David Jander wrote: > > > > Add device-tree bindings for Analog Devices TMC5240 stepper controllers. > > > > > > > > Signed-off-by: David Jander <david@protonic.nl> > > > > --- > > > > .../bindings/motion/adi,tmc5240.yaml | 60 +++++++++++++++++++ > > > > 1 file changed, 60 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > > > new file mode 100644 > > > > index 000000000000..3364f9dfccb1 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml > > > > @@ -0,0 +1,60 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/motion/adi,tmc5240.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Analog Devices TMC5240 Stepper Motor controller > > > > + > > > > +maintainers: > > > > + - David Jander <david@protonic> > > > > + > > > > +description: | > > > > + Stepper motor controller with motion engine and SPI interface. > > > > > > Please include a link to the datasheet. > > > > Will do. > > > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - adi,tmc5240 > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > > > I assume that this is the overvoltage output (OV pin). Would be nice to have > > > a description here saying that. There are also NAO and DIAG0/1 output pins, so > > > it's a bit ambiguous otherwise. > > > > This is the DIAG0 output pin which on this chip has a dual function as either > > a STEP output or an interrupt output. The pin name is a bit misleading, but it > > is the "interrupt" function that is meant here. The datasheet documents all > > the different events that can trigger this interrupt. > > I will add a description to clarify this. > > > > If it makes sense that other pins could possibly ever be connected to > interrupts then we can add those and also add interrupt-names (but > only if there is more than one possible interrupt). AFAIK, only DIAG1 would potentially make sense to be connected to an interrupt. It can be programmed to go low when the motor position matches the contents of the X_COMPARE/X_COMPARE_REPEAT register setting. I will add that one if you agree. It will not be mandatory of course. In any case, if that pin was connected to an interrupt pin right now, it could already be used as an IIO trigger for example. Just not (yet) via this driver. >[...] > > The resistor connected to the IREF pin (Rref) OTOH does have an implication to > > the software, as it sets the full-range current of the output stage. > > > > How should we specify that? Is it adequate to add an optional DT property > > "rref" or "rref-ohm" with an int32 value in Ohm? The default value if > > unspecified is 12000 Ohm. > > It looks like there are a few standardized properties, like > sense-resistor-ohms if that fits the use case. Otherwise, an > vendor-specific ti,rref-ohms would work. FYI, you can find the > preferred units at [1]. > > [1]: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml Ah, thanks! This is helpful. Will use this for ti,rref-ohms. I guess in this case that would be easier to understand than "sense-resistor-ohms", which is also okay, but would require reading the description to know what exactly is meant in this context. > > > And if there are any pins would make sense to connect to a gpio, we can add > > > those even if the driver doesn't use it currently. > > > > > > > + clocks: > > > > + maxItems: 1 > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - interrupts > > > > + - clocks > > > > + > > > > +allOf: > > > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > > + - $ref: /schemas/motion/common.yaml# > > > > > > If we need to know about what is connected to the output of a motor controller > > > I would expect it to be done with child node for each output. That way each > > > output can be unique, if needed. Basically, similar to iio/adc.yaml is used to > > > provide common properties for channel@ child nodes on iio devices. > > > > This controller chip only has one single output for one stepper motor (4 > > wires). While technically you could connect something else to those 4 wires, I > > don't think it is the scope of LMC to support that. The chip itself isn't > > designed for that purpose and it would clearly go far beyond the intended > > purpose of this device. > > > > That being said, your suggestion of supporting child nodes may actually be a > > good idea. Right now, we specify the type of motor (basically nominal- and hold > > current settings) in user-space and set the IRUN/IHOLD parameters from > > user-space via the sysfs attributes interface. It might make sense to have a DT > > child node to specify this, although in our current application this is not > > very practical, since there are many motor controllers on one board, and it is > > configurable in software (runtime) which motor is connected to which output. > > > > But I can imagine a situation where it may be fixed and thus can be described > > in the DT of a board. > > > > Then again I don't know if it would be over-complicating things with something > > like this: > > > > motor-controller@0 { > > ... > > motor@0 { > > compatible = "nanotec,st4118s1006"; > > irun-ma = <1800>; > > ihold-ma = <270>; > > }; > > }; > > > > where we'd possibly have a stepper-motors.c file with a lot of structs and > > matching tables for the different motor types.... sounds like overkill to me, > > but maybe not? > > A compatible for motors seems too much. I was just thinking along the > lines that 1) if we need to so some scaling or something that depends > on a motor constant, then it would make sense to put those constants > in the DT and 2) if there is a motor controller with more than one > output that could be connected to two or more different sizes of > motors with different constants, then we either need child nodes or an > array to be able to enter the different constants. Either one would > work. So maybe simpler to just use an array instead of child nodes now > that I'm thinking about it more. Well, in the case of the TMC5240 there isn't much more than a single motor with possibly some fixed setting of irun/ihold in some cases, but like I said, in our case it is run-time configurable, so not something fixed to the hardware-description. Apart from that, there are the speed- and acceleration- conversion constants, which per default are the constants stated in the datasheet. In some rare cases one might want to overrule them, but that can already be done. LMC does als support multi-channel controllers, and in that case I intend to make use of child nodes for the different channels, to be able to specify those parameters per motor. So maybe just leave it as it currently is for the tmc5240? > > > > + > > > > +unevaluatedProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + spi { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + motor@0 { > > > > > > motor-controller@ or actuator-controller@ > > > > > > The chip is the controller/driver, it is not a motor. > > > > Make sense. Will change this. > > > > > > + compatible = "adi,tmc5240"; > > > > + reg = <0>; > > > > + interrupts-extended = <&gpiok 7 0>; > > > > + clocks = <&clock_tmc5240>; > > > > + enable-supply = <&stpsleepn>; > > > > + spi-max-frequency = <1000000>; > > > > + }; > > > > + }; Best regards,
diff --git a/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml new file mode 100644 index 000000000000..3364f9dfccb1 --- /dev/null +++ b/Documentation/devicetree/bindings/motion/adi,tmc5240.yaml @@ -0,0 +1,60 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/motion/adi,tmc5240.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices TMC5240 Stepper Motor controller + +maintainers: + - David Jander <david@protonic> + +description: | + Stepper motor controller with motion engine and SPI interface. + +properties: + compatible: + enum: + - adi,tmc5240 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + enable-supply: + description: Optional external enable supply to control SLEEPn pin. Can + be shared between several controllers. + + clocks: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + - clocks + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + - $ref: /schemas/motion/common.yaml# + +unevaluatedProperties: false + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + motor@0 { + compatible = "adi,tmc5240"; + reg = <0>; + interrupts-extended = <&gpiok 7 0>; + clocks = <&clock_tmc5240>; + enable-supply = <&stpsleepn>; + spi-max-frequency = <1000000>; + }; + }; +
Add device-tree bindings for Analog Devices TMC5240 stepper controllers. Signed-off-by: David Jander <david@protonic.nl> --- .../bindings/motion/adi,tmc5240.yaml | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 Documentation/devicetree/bindings/motion/adi,tmc5240.yaml