| Message ID | 20230825164249.22860-3-nmalwade@nvidia.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | hwmon: ina3221: Add selective summation support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 25/08/2023 18:42, Ninad Malwade wrote: > The INA3221 has a critical alert pin that can be controlled by the > summation control function. This function adds the single > shunt-voltage conversions for the desired channels in order to > compare the combined sum to the programmed limit. The Shunt-Voltage > Sum Limit register contains the programmed value that is compared > to the value in the Shunt-Voltage Sum register in order to > determine if the total summed limit is exceeded. If the > shunt-voltage sum limit value is exceeded, the critical alert pin > pulls low. > > For the summation limit to have a meaningful value, it is necessary > to use the same shunt-resistor value on all included channels. Add a > new property, 'summation-bypass', to allow specific channels to be > excluded from the summation control function if the shunt resistor > is different to other channels. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> > --- > .../devicetree/bindings/hwmon/ti,ina3221.yaml | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml > index 0c6d41423d8c..20c23febf575 100644 > --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml > +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml > @@ -55,6 +55,24 @@ patternProperties: > shunt-resistor-micro-ohms: > description: shunt resistor value in micro-Ohm > > + summation-bypass: What is the type? There is no vendor prefix here, so you added it as a generic property. Which other devices use or can use it? > + description: | > + The INA3221 has a critical alert pin that can be controlled by the > + summation control function. This function adds the single > + shunt-voltage conversions for the desired channels in order to > + compare the combined sum to the programmed limit. The Shunt-Voltage > + Sum Limit register contains the programmed value that is compared > + to the value in the Shunt-Voltage Sum register in order to > + determine if the total summed limit is exceeded. If the > + shunt-voltage sum limit value is exceeded, the critical alert pin > + pulls low. > + > + For the summation limit to have a meaningful value, it is necessary > + to use the same shunt-resistor value on all included channels. If > + this is not the case for specific channels, then the > + 'summation-bypass' can be populated for a specific channel to > + exclude from the summation control function. I don't understand what this property does. You described feature in the device, that's good, but how does it map to the property? Bypass means disable? > + > additionalProperties: false > > required: Best regards, Krzysztof
On 26/08/2023 09:56, Krzysztof Kozlowski wrote: > On 25/08/2023 18:42, Ninad Malwade wrote: >> The INA3221 has a critical alert pin that can be controlled by the >> summation control function. This function adds the single >> shunt-voltage conversions for the desired channels in order to >> compare the combined sum to the programmed limit. The Shunt-Voltage >> Sum Limit register contains the programmed value that is compared >> to the value in the Shunt-Voltage Sum register in order to >> determine if the total summed limit is exceeded. If the >> shunt-voltage sum limit value is exceeded, the critical alert pin >> pulls low. >> >> For the summation limit to have a meaningful value, it is necessary >> to use the same shunt-resistor value on all included channels. Add a >> new property, 'summation-bypass', to allow specific channels to be >> excluded from the summation control function if the shunt resistor >> is different to other channels. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> >> --- >> .../devicetree/bindings/hwmon/ti,ina3221.yaml | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml >> index 0c6d41423d8c..20c23febf575 100644 >> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml >> @@ -55,6 +55,24 @@ patternProperties: >> shunt-resistor-micro-ohms: >> description: shunt resistor value in micro-Ohm >> >> + summation-bypass: > > What is the type? There is no vendor prefix here, so you added it as a > generic property. Which other devices use or can use it? > >> + description: | >> + The INA3221 has a critical alert pin that can be controlled by the >> + summation control function. This function adds the single >> + shunt-voltage conversions for the desired channels in order to >> + compare the combined sum to the programmed limit. The Shunt-Voltage >> + Sum Limit register contains the programmed value that is compared >> + to the value in the Shunt-Voltage Sum register in order to >> + determine if the total summed limit is exceeded. If the >> + shunt-voltage sum limit value is exceeded, the critical alert pin >> + pulls low. >> + >> + For the summation limit to have a meaningful value, it is necessary >> + to use the same shunt-resistor value on all included channels. If >> + this is not the case for specific channels, then the >> + 'summation-bypass' can be populated for a specific channel to >> + exclude from the summation control function. > > I don't understand what this property does. You described feature in the > device, that's good, but how does it map to the property? Bypass means > disable? Yes it means 'disable'. I kept as 'bypass' to align with the original patch [0], but if it is clearer, we could change this to be 'summation-disable'. Jon [0] https://lore.kernel.org/lkml/20221108045243.24143-1-nmalwade@nvidia.com/
On 29/08/2023 14:48, Jon Hunter wrote: > > > On 26/08/2023 09:56, Krzysztof Kozlowski wrote: >> On 25/08/2023 18:42, Ninad Malwade wrote: >>> The INA3221 has a critical alert pin that can be controlled by the >>> summation control function. This function adds the single >>> shunt-voltage conversions for the desired channels in order to >>> compare the combined sum to the programmed limit. The Shunt-Voltage >>> Sum Limit register contains the programmed value that is compared >>> to the value in the Shunt-Voltage Sum register in order to >>> determine if the total summed limit is exceeded. If the >>> shunt-voltage sum limit value is exceeded, the critical alert pin >>> pulls low. >>> >>> For the summation limit to have a meaningful value, it is necessary >>> to use the same shunt-resistor value on all included channels. Add a >>> new property, 'summation-bypass', to allow specific channels to be >>> excluded from the summation control function if the shunt resistor >>> is different to other channels. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> >>> --- >>> .../devicetree/bindings/hwmon/ti,ina3221.yaml | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml >>> index 0c6d41423d8c..20c23febf575 100644 >>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml >>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml >>> @@ -55,6 +55,24 @@ patternProperties: >>> shunt-resistor-micro-ohms: >>> description: shunt resistor value in micro-Ohm >>> >>> + summation-bypass: >> >> What is the type? There is no vendor prefix here, so you added it as a >> generic property. Which other devices use or can use it? >> >>> + description: | >>> + The INA3221 has a critical alert pin that can be controlled by the >>> + summation control function. This function adds the single >>> + shunt-voltage conversions for the desired channels in order to >>> + compare the combined sum to the programmed limit. The Shunt-Voltage >>> + Sum Limit register contains the programmed value that is compared >>> + to the value in the Shunt-Voltage Sum register in order to >>> + determine if the total summed limit is exceeded. If the >>> + shunt-voltage sum limit value is exceeded, the critical alert pin >>> + pulls low. >>> + >>> + For the summation limit to have a meaningful value, it is necessary >>> + to use the same shunt-resistor value on all included channels. If >>> + this is not the case for specific channels, then the >>> + 'summation-bypass' can be populated for a specific channel to "populated" is confusing here. I guess you wanted "can be used"? >>> + exclude from the summation control function. >> >> I don't understand what this property does. You described feature in the >> device, that's good, but how does it map to the property? Bypass means >> disable? > > > Yes it means 'disable'. I kept as 'bypass' to align with the original > patch [0], but if it is clearer, we could change this to be > 'summation-disable'. Sounds better, but the description could also start with it, e.g. "Disable the summation on specific channel. .... " Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml index 0c6d41423d8c..20c23febf575 100644 --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml @@ -55,6 +55,24 @@ patternProperties: shunt-resistor-micro-ohms: description: shunt resistor value in micro-Ohm + summation-bypass: + description: | + The INA3221 has a critical alert pin that can be controlled by the + summation control function. This function adds the single + shunt-voltage conversions for the desired channels in order to + compare the combined sum to the programmed limit. The Shunt-Voltage + Sum Limit register contains the programmed value that is compared + to the value in the Shunt-Voltage Sum register in order to + determine if the total summed limit is exceeded. If the + shunt-voltage sum limit value is exceeded, the critical alert pin + pulls low. + + For the summation limit to have a meaningful value, it is necessary + to use the same shunt-resistor value on all included channels. If + this is not the case for specific channels, then the + 'summation-bypass' can be populated for a specific channel to + exclude from the summation control function. + additionalProperties: false required: