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

Salih Erim
2026-05-03T22:52:52Z

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.