| Message ID | 20230929103650.86074-4-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| 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: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
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c index 5ab944056ec0..5ffdc94db436 100644 --- a/drivers/hwmon/ina3221.c +++ b/drivers/hwmon/ina3221.c @@ -6,6 +6,7 @@ * Andrew F. Davis <afd@ti.com> */ +#include <linux/debugfs.h> #include <linux/hwmon.h> #include <linux/hwmon-sysfs.h> #include <linux/i2c.h> @@ -99,11 +100,13 @@ enum ina3221_channels { * @label: label of channel input source * @shunt_resistor: shunt resistor value of channel input source * @disconnected: connection status of channel input source + * @summation_disable: channel summation status of input source */ struct ina3221_input { const char *label; int shunt_resistor; bool disconnected; + bool summation_disable; }; /** @@ -113,8 +116,10 @@ struct ina3221_input { * @fields: Register fields of the device * @inputs: Array of channel input source specific structures * @lock: mutex lock to serialize sysfs attribute accesses + * @debugfs: Pointer to debugfs entry for device * @reg_config: Register value of INA3221_CONFIG * @summation_shunt_resistor: equivalent shunt resistor value for summation + * @summation_channel_control: Value written to SCC field in INA3221_MASK_ENABLE * @single_shot: running in single-shot operating mode */ struct ina3221_data { @@ -123,8 +128,10 @@ struct ina3221_data { struct regmap_field *fields[F_MAX_FIELDS]; struct ina3221_input inputs[INA3221_NUM_CHANNELS]; struct mutex lock; + struct dentry *debugfs; u32 reg_config; int summation_shunt_resistor; + u32 summation_channel_control; bool single_shot; }; @@ -154,7 +161,8 @@ static inline int ina3221_summation_shunt_resistor(struct ina3221_data *ina) int i, shunt_resistor = 0; for (i = 0; i < INA3221_NUM_CHANNELS; i++) { - if (input[i].disconnected || !input[i].shunt_resistor) + if (input[i].disconnected || !input[i].shunt_resistor || + input[i].summation_disable) continue; if (!shunt_resistor) { /* Found the reference shunt resistor value */ @@ -786,6 +794,9 @@ static int ina3221_probe_child_from_dt(struct device *dev, /* Save the connected input label if available */ of_property_read_string(child, "label", &input->label); + /* summation channel control */ + input->summation_disable = of_property_read_bool(child, "ti,summation-disable"); + /* Overwrite default shunt resistor value optionally */ if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &val)) { if (val < 1 || val > INT_MAX) { @@ -827,6 +838,7 @@ static int ina3221_probe(struct i2c_client *client) struct device *dev = &client->dev; struct ina3221_data *ina; struct device *hwmon_dev; + char name[32]; int i, ret; ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL); @@ -873,6 +885,10 @@ static int ina3221_probe(struct i2c_client *client) /* Initialize summation_shunt_resistor for summation channel control */ ina->summation_shunt_resistor = ina3221_summation_shunt_resistor(ina); + for (i = 0; i < INA3221_NUM_CHANNELS; i++) { + if (!ina->inputs[i].summation_disable) + ina->summation_channel_control |= BIT(14 - i); + } ina->pm_dev = dev; mutex_init(&ina->lock); @@ -900,6 +916,15 @@ static int ina3221_probe(struct i2c_client *client) goto fail; } + scnprintf(name, sizeof(name), "%s-%s", INA3221_DRIVER_NAME, dev_name(dev)); + ina->debugfs = debugfs_create_dir(name, NULL); + + for (i = 0; i < INA3221_NUM_CHANNELS; i++) { + scnprintf(name, sizeof(name), "in%d_summation_disable", i); + debugfs_create_bool(name, 0400, ina->debugfs, + &ina->inputs[i].summation_disable); + } + return 0; fail: @@ -918,6 +943,8 @@ static void ina3221_remove(struct i2c_client *client) struct ina3221_data *ina = dev_get_drvdata(&client->dev); int i; + debugfs_remove_recursive(ina->debugfs); + pm_runtime_disable(ina->pm_dev); pm_runtime_set_suspended(ina->pm_dev); @@ -978,13 +1005,13 @@ static int ina3221_resume(struct device *dev) /* Initialize summation channel control */ if (ina->summation_shunt_resistor) { /* - * Take all three channels into summation by default + * Sum only channels that are not disabled for summation. * Shunt measurements of disconnected channels should * be 0, so it does not matter for summation. */ ret = regmap_update_bits(ina->regmap, INA3221_MASK_ENABLE, INA3221_MASK_ENABLE_SCC_MASK, - INA3221_MASK_ENABLE_SCC_MASK); + ina->summation_channel_control); if (ret) { dev_err(dev, "Unable to control summation channel\n"); return ret;