Re: [PATCH v3 1/3] dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/07/2024 17:00, Conor Dooley wrote:
> Rob/Krzysztof, Haylen,
>> +
>> +maintainers:
>> +  - Haylen Chu <heylenay@outlook.com>
>> +
>> +description: Binding for Sophgo CV1800 on-SoC thermal sensor

Drop "Binding"

>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - sophgo,cv1800-thermal
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    description: The thermal sensor clock
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  accumulation-period:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Accumulation period for a sample
>> +    enum:
>> +      - 512
>> +      - 1024
>> +      - 2048
>> +      - 4096
>> +    default: 2048
>> +
>> +  chop-period:

period in what sort of units? Sounds like time to me, so this would
require proper unit suffix.

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: ADC chop period
>> +    enum:
>> +      - 128
>> +      - 256
>> +      - 512
>> +      - 1024
>> +    default: 1024
>> +
>> +  sample-cycle-us:
> 
> the more common term btw would be "sample-rate" rather than
> "sample-cycle".

yeah, sample-rate-hz

> 
>> +    description: Period between samples. Should be greater than 524us.
> 
> The constraint here should be "minimum: 524". What's the upper limit?
> 
>> +    default: 1000000
> 
> Rob/Krzysztof, could you comment on the suitability of the three custom
> properties here? I know if this was an IIO device, these kinds of things
> would be controllable from userspace, and not in the binding. I
> mentioned this on the previous version, but I'm not really sure if
> thermal devices are somehow different:
> https://lore.kernel.org/all/SEYPR01MB4221A739D0645EF0255336EBD7CE2@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
> 

Why would different boards have different values here? Does it affect
accuracy? If so, how much?

I doubt there are any boards with different values, thus it sounds like
unnecessary tuning parameter.

Best regards,
Krzysztof