Re: [PATCH v2 1/5] dt-bindings: iio: adc: add xlnx,versal-sysmon binding

[Salih Erim <salih.erim@amd.com>]

Hi Krzysztof,

Thanks for the review, Replies inline.

On 5/3/2026 3:20 PM, Krzysztof Kozlowski wrote:

>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - xlnx,versal-sysmon
>> +      - xlnx,versal-sysmon-i2c
> 
> This is explicitly mentioned in writing bindings. One device, one
> compatible (not two, not three, not four compatibles).

Accepted. Will use single "xlnx,versal-sysmon" compatible. The I2C 
driver will match via the I2C bus, so the bus type provides the 
differentiation.

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  '#io-channel-cells':
>> +    const: 1
>> +
>> +  supply-channels:
>> +    type: object
>> +    description:
>> +      Container for supply voltage measurement channels.
> 
> voltage, not supply. Supply is the source of energy coming to this
> device. But are you measuring that source?

Accepted. Will rename to "voltage-channels".

> 
>> +
>> +    properties:
>> +      '#address-cells':
>> +        const: 1
>> +
>> +      '#size-cells':
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^channel@([0-9a-f]|[1-9][0-9a-f])$":
> 
> Keep consistent quotes, either ' or "

Will use single quotes throughout.

> 
>> +        $ref: adc.yaml
>> +
>> +        description:
>> +          Measures a supply rail voltage. The register index and rail
>> +          name are assigned by the hardware design tool (Vivado).
>> +
>> +        properties:
>> +          reg:
>> +            minimum: 0
>> +            maximum: 159
>> +            description:
>> +              Supply measurement register index assigned by the hardware
>> +              design tool.
>> +
>> +          label:
>> +            description:
>> +              Name of the supply rail being monitored.
> 
> Drop property, it's already in adc.yaml.

Accepted, will drop from both voltage and temperature channel blocks.

> 
>> +
>> +          bipolar: true
> 
> Drop

Accepted, already defined in adc.yaml.

> 
>> +
>> +        required:
>> +          - reg
>> +          - label
>> +
>> +        unevaluatedProperties: false
>> +
>> +    required:
>> +      - '#address-cells'
>> +      - '#size-cells'
>> +
>> +    additionalProperties: false
>> +
>> +  temperature-channels:
>> +    type: object
>> +    description:
>> +      Container for temperature satellite measurement channels.
>> +
>> +    properties:
>> +      '#address-cells':
>> +        const: 1
>> +
>> +      '#size-cells':
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^channel@([1-9a-f]|[1-3][0-9a-f]|40)$":
>> +        $ref: adc.yaml
>> +
>> +        description:
>> +          Reads a temperature satellite sensor. Each satellite monitors
>> +          a specific region of the SoC die.
>> +
>> +        properties:
>> +          reg:
>> +            minimum: 1
>> +            maximum: 64
>> +            description:
>> +              Temperature satellite number (1-based hardware index).
>> +
>> +          label:
>> +            description:
>> +              Name identifying this temperature satellite.
> 
> Drop property
> 
>> +
>> +          xlnx,aie-temp:
>> +            type: boolean
>> +            description:
>> +              Indicates this satellite monitors an AI Engine tile.
> 
> What for? What is on the other side of the channel is not really
> relevant to this device, but rather to that other side (consumer).

Agreed. The driver does not use this property. Will remove.

> 
>> +
>> +        required:
>> +          - reg
>> +          - label
>> +
>> +        unevaluatedProperties: false
>> +
>> +    required:
>> +      - '#address-cells'
>> +      - '#size-cells'
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
> 
> Best regards,
> Krzysztof
> 

I will address all items in v3.

Salih.