Message-ID: <CAMknhBFoRoaXWBL-vDnDrepqw_KJ-VrYeOoGJfjz8q=wDNM6xA@mail.gmail.com>
Date: Mon, 3 Mar 2025 13:28:35 +0100
From: David Lechner <dlechner@...libre.com>
To: David Jander <david@...tonic.nl>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org, 
	Jonathan Corbet <corbet@....net>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org, 
	linux-doc@...r.kernel.org, Nuno Sa <nuno.sa@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [RFC PATCH 6/7] dt-bindings: motion: Add adi,tmc5240 bindings

(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@...tonic.nl> wrote:
>
>
> Dear David,
>
> On Fri, 28 Feb 2025 16:38:51 -0600
> David Lechner <dlechner@...libre.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@...tonic.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@...tonic>
> > > +
> > > +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

Powered by blists - more mailing lists