Re: [RFC PATCH 6/7] dt-bindings: motion: Add adi,tmc5240 bindings

[David Jander]

Dear Krzysztof,

Thanks for reviewing...

On Fri, 28 Feb 2025 08:11:04 +0100
Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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,

-- 
David Jander