On 01/03/2022 13:42, POTIN LAI wrote: > > Krzysztof Kozlowski 於 1/03/2022 7:16 pm 寫道: >> On 01/03/2022 11:39, Potin Lai wrote: >>> Add documentation of new properties for sample averaging in PMON_CONFIG >>> register. >>> >>> New properties: >>> - adi,volt-curr-sample-average >>> - adi,power-sample-average >>> - adi,power-sample-average-enable >>> >>> Signed-off-by: Potin Lai <potin.lai@xxxxxxxxxxxx> >>> --- >>> .../bindings/hwmon/adi,adm1275.yaml | 44 +++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml >>> index 223393d7cafd..1b612dc06992 100644 >>> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml >>> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml >>> @@ -37,6 +37,47 @@ properties: >>> description: >>> Shunt resistor value in micro-Ohm. >>> >>> + adi,volt-curr-sample-average: >>> + description: | >>> + Number of samples to be used to report voltage and current values. >>> + If the configured value is not a power of 2, sample averaging number >>> + will be configured with smaller and closest power of 2. >>> + >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [1, 2, 4, 8, 16, 32, 64, 128] >>> + default: 1 >>> + >>> + adi,power-sample-average: >>> + description: | >>> + Number of samples to be used to report power values. >>> + If the configured value is not a power of 2, sample averaging number >>> + will be configured with smaller and closest power of 2. >>> + >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [1, 2, 4, 8, 16, 32, 64, 128] >>> + default: 1 >>> + >>> + adi,power-sample-average-enable: >>> + description: Enable sample averaging for power reading. >>> + type: boolean >> Why do you need this property? Voltage/current sampling is enabled in >> your driver with presence of adi,volt-curr-sample-average. Why power >> sampling is different? > For "adi,power-sample-average", adm1075, adm1275 & adm127 don't have config reg for power sample average, so I add boolean type property to enable it > But for "adi,power-sample-average-enable", all chips have ability of configuring, so it doesn't need a property to enable or disable. So the reason to add separate property is that this feature can be disabled. Since your driver does not disable it, it seems it is a default state to have it disabled and you have to enable it, right? Where is the enable code? I see you only write the sample averaging value with adm1275_write_pmon_config(). There is no enable... But wait, the power averaging is being disabled by writing 0 to register, which is not allowed by bindings. How one can disable it? I don't see any usage of "adi,power-sample-average-enable", neither in driver nor in hardware. I also do not see the need for it, the purpose. Then second part, you added default value of 1 to adi,volt-curr-sample-average and adi,power-sample-average. If the property is missing, then the default of 1 is applied, right? But datasheet says that default is 128! The bindings neither match hardware nor driver. They look entirely independent. This is wrong. They should instead be strongly related to the hardware, describe the hardware. Then the driver should implement proper logic for it. > Does example means that I can set any type (not just boolean?) of property to false if not allowed? > Could I write as below? > > allOf: > - if: > properties: > compatible: > contains: > enum: > - chips_not_support > then: > properties: > adi,power-sample-average: false > > Sorry, I am not quite understand the example of set property not allowed, if I still get it wrong, please advise more detailed, thank you. I gave you the example: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/example-schema.yaml?#n221 and there is no boolean in this example. You have a compatible which does not allow some property, so you need "then:properties:foobar:false" Best regards, Krzysztof
