On 04/07/2024 10:25, Haylen Chu wrote: > On Tue, Jul 02, 2024 at 05:09:35PM +0200, Krzysztof Kozlowski wrote: >>>> + >>>> +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. > > In clock ticks. Then please mention it in the property description. > > When setting to 1024, a time of sample takes (1024 + 2 + 64) clock > ticks. The clock runs at (25MHz / divider) and the divider is > configurable. > >>> >>>> + 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ >>> >> >> 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. > > Theses values affect accuracy in a minor way (about 1 Celsius degree in > my test) and could be shared between CV18xx/SG20xx SoCs as they have the > same design. > > In the first revision, fixed values are used, and I was asked to add > support for all possible configuration[1]. Now I think this introduces > extra unnecessary complexity and should be avoided, since this is a > simple thermal sensor, tuning seems to be useless. > > I suggest renaming "sample-cycle-us" to "sample-rate-hz" and dropping > other parameters for simplicity. Ack for me. Best regards, Krzysztof
