| Message ID | 20230929103650.86074-1-jonathanh@nvidia.com |
|---|---|
| Headers | show |
| Series | hwmon: ina3221: Add selective summation support | expand |
Hi Jean, Guenter, On 29/09/2023 11:36, Jon Hunter wrote: > From: Ninad Malwade <nmalwade@nvidia.com> > > The INA3221 allows the Critical alert pin to 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, we have to use the > same shunt-resistor value on all included channels. Unless equal > shunt-resistor values are used for each channel, the summation control > function cannot be used and it is not enabled by the driver. > > To address this, add support to disable the summation of specific > channels via device tree property "ti,summation-disable". The channel > which has this property would be excluded from the calculation of > summation control function. > > For example, summation control function calculates Shunt-Voltage Sum as: > > - input_shunt_voltage_summation = input_shunt_voltage_channel1 > + input_shunt_voltage_channel2 > + input_shunt_voltage_channel3 > > If we want the summation to only use channel1 and channel3, we can add > 'ti,summation-disable' property in device tree node for channel2. Then > the calculation will skip channel2. > > - input_shunt_voltage_summation = input_shunt_voltage_channel1 > + input_shunt_voltage_channel3 > > Note that we only want the channel to be skipped for summation control > function rather than completely disabled. Therefore, even if we add the > property 'ti,summation-disable', the channel is still enabled and > functional. > > Finally, create debugfs entries that display if summation is disabled > for each of the channels. > > Signed-off-by: Rajkumar Kasirajan <rkasirajan@nvidia.com> > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> Any feedback on this? Thanks Jon
On Fri, Sep 29, 2023 at 11:36:46AM +0100, Jon Hunter wrote: > The current INA3221 driver always sums the shunt voltage for all enabled > channels regardless of the shunt-resistor used for each channel. Summing > the shunt-voltage for channels is only meaningful if the shunt resistor > is the same for each channel. This series adds device-tree support to > allow which channels are summed in device-tree. > > Changes since V4: > - Moved dt-binding comment added in V4 from patch #2 to patch #1. > > Changes since V3: > - Added missing descriptions for new structure members that was reported > by the kernel-test-bot. > - Added comment in the ina3221 dt-binding doc example to explain why we > need to explicitly disable channels. > - Added more commentary in the commit message for the new DT property > to explain that this property does not change the behaviour of the > driver unless it is populated. > > Changes since V2: > - Added note to binding-doc to indicate that input channels must be > explicitly disabled. > - Corrected ordering of properties in the binding-doc > - Updated license for the binding-doc to be dual licensed. > - Changed newly added property from 'summation-bypass' to > summation-disable'. > - Documented type for the new 'summation-disable' property. > - Corrected spelling and comments as per the feedback received. > - Used debugfs instead of sysfs for exposing the 'summation-disable' > status for each input channel. > - Populated missing instances for the ina3221 device for Tegra234 > boards. > - Populated ina219 device for the NVIDIA IGX board (not strictly > related to this series but related to populating all > power-sensors for Tegra234 boards) > > Changes since V1: > - Added yaml conversion patch for binding-doc > - Added binding-doc documentation patch for new property > - Added patch to populate ina3221 devices for Tegra234. > > Jon Hunter (2): > dt-bindings: hwmon: ina3221: Add ti,summation-disable > arm64: tegra: Add power-sensors for Tegra234 boards > > Ninad Malwade (2): > dt-bindings: hwmon: ina3221: Convert to json-schema > hwmon: ina3221: Add support for channel summation disable Jean, Guenter, do you mind if I pick up patches 1, 2 and 4 into the Tegra tree? It's usually convenient to keep the DT bindings and DT additions in the same tree for validation. Thanks, Thierry
From: Thierry Reding <treding@nvidia.com> On Fri, 29 Sep 2023 11:36:46 +0100, Jon Hunter wrote: > The current INA3221 driver always sums the shunt voltage for all enabled > channels regardless of the shunt-resistor used for each channel. Summing > the shunt-voltage for channels is only meaningful if the shunt resistor > is the same for each channel. This series adds device-tree support to > allow which channels are summed in device-tree. > > Changes since V4: > - Moved dt-binding comment added in V4 from patch #2 to patch #1. > > [...] Applied, thanks! [4/4] arm64: tegra: Add power-sensors for Tegra234 boards commit: 9152ed09309de1a876680e6309c8eccb509b44b0 Best regards,
Hi Jean, Guenter, On 11/10/2023 21:28, Thierry Reding wrote: > On Fri, Sep 29, 2023 at 11:36:46AM +0100, Jon Hunter wrote: >> The current INA3221 driver always sums the shunt voltage for all enabled >> channels regardless of the shunt-resistor used for each channel. Summing >> the shunt-voltage for channels is only meaningful if the shunt resistor >> is the same for each channel. This series adds device-tree support to >> allow which channels are summed in device-tree. >> >> Changes since V4: >> - Moved dt-binding comment added in V4 from patch #2 to patch #1. >> >> Changes since V3: >> - Added missing descriptions for new structure members that was reported >> by the kernel-test-bot. >> - Added comment in the ina3221 dt-binding doc example to explain why we >> need to explicitly disable channels. >> - Added more commentary in the commit message for the new DT property >> to explain that this property does not change the behaviour of the >> driver unless it is populated. >> >> Changes since V2: >> - Added note to binding-doc to indicate that input channels must be >> explicitly disabled. >> - Corrected ordering of properties in the binding-doc >> - Updated license for the binding-doc to be dual licensed. >> - Changed newly added property from 'summation-bypass' to >> summation-disable'. >> - Documented type for the new 'summation-disable' property. >> - Corrected spelling and comments as per the feedback received. >> - Used debugfs instead of sysfs for exposing the 'summation-disable' >> status for each input channel. >> - Populated missing instances for the ina3221 device for Tegra234 >> boards. >> - Populated ina219 device for the NVIDIA IGX board (not strictly >> related to this series but related to populating all >> power-sensors for Tegra234 boards) >> >> Changes since V1: >> - Added yaml conversion patch for binding-doc >> - Added binding-doc documentation patch for new property >> - Added patch to populate ina3221 devices for Tegra234. >> >> Jon Hunter (2): >> dt-bindings: hwmon: ina3221: Add ti,summation-disable >> arm64: tegra: Add power-sensors for Tegra234 boards >> >> Ninad Malwade (2): >> dt-bindings: hwmon: ina3221: Convert to json-schema >> hwmon: ina3221: Add support for channel summation disable > > Jean, Guenter, > > do you mind if I pick up patches 1, 2 and 4 into the Tegra tree? It's > usually convenient to keep the DT bindings and DT additions in the same > tree for validation. I have not seen any feedback on this from you? Please let me know if this version is now OK with you? Thanks Jon
On Fri, Sep 29, 2023 at 11:36:49AM +0100, Jon Hunter wrote: > From: Ninad Malwade <nmalwade@nvidia.com> > > The INA3221 allows the Critical alert pin to 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, we have to use the > same shunt-resistor value on all included channels. Unless equal > shunt-resistor values are used for each channel, the summation control > function cannot be used and it is not enabled by the driver. > > To address this, add support to disable the summation of specific > channels via device tree property "ti,summation-disable". The channel > which has this property would be excluded from the calculation of > summation control function. > > For example, summation control function calculates Shunt-Voltage Sum as: > > - input_shunt_voltage_summation = input_shunt_voltage_channel1 > + input_shunt_voltage_channel2 > + input_shunt_voltage_channel3 > > If we want the summation to only use channel1 and channel3, we can add > 'ti,summation-disable' property in device tree node for channel2. Then > the calculation will skip channel2. > > - input_shunt_voltage_summation = input_shunt_voltage_channel1 > + input_shunt_voltage_channel3 > > Note that we only want the channel to be skipped for summation control > function rather than completely disabled. Therefore, even if we add the > property 'ti,summation-disable', the channel is still enabled and > functional. > > Finally, create debugfs entries that display if summation is disabled > for each of the channels. > > Signed-off-by: Rajkumar Kasirajan <rkasirajan@nvidia.com> > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> Applied. Thanks, Guenter