Dear David, On Mon, 3 Mar 2025 13:28:35 +0100 David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxx> wrote: > > > > > > Dear David, > > > > On Fri, 28 Feb 2025 16:38:51 -0600 > > David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxx> > > > > --- > > > > .../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, -- David Jander
