| Message ID | 20230825164249.22860-4-nmalwade@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: ina3221: Add selective summation support | expand |
Hi Ninad, kernel test robot noticed the following build warnings: [auto build test WARNING on next-20230825] [also build test WARNING on linus/master v6.5-rc7] [cannot apply to groeck-staging/hwmon-next robh/for-next v6.5-rc7 v6.5-rc6 v6.5-rc5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Ninad-Malwade/dt-bindings-hwmon-ina3221-Convert-to-json-schema/20230826-004606 base: next-20230825 patch link: https://lore.kernel.org/r/20230825164249.22860-4-nmalwade%40nvidia.com patch subject: [PATCH V2 3/4] hwmon: ina3221: add support for summation channel control config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230826/202308260438.7OwsGAd8-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260438.7OwsGAd8-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308260438.7OwsGAd8-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/ina3221.c:108: warning: Function parameter or member 'summation_bypass' not described in 'ina3221_input' >> drivers/hwmon/ina3221.c:132: warning: Function parameter or member 'summation_channel_control' not described in 'ina3221_data' vim +108 drivers/hwmon/ina3221.c 7cb6dcff1956ec Andrew F. Davis 2016-06-10 96 a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 97 /** a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 98 * struct ina3221_input - channel input source specific information a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 99 * @label: label of channel input source a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 100 * @shunt_resistor: shunt resistor value of channel input source a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 101 * @disconnected: connection status of channel input source a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 102 */ a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 103 struct ina3221_input { a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 104 const char *label; a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 105 int shunt_resistor; a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 106 bool disconnected; 3aab5d0b835881 Ninad Malwade 2023-08-26 107 bool summation_bypass; a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 @108 }; a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 109 7cb6dcff1956ec Andrew F. Davis 2016-06-10 110 /** 7cb6dcff1956ec Andrew F. Davis 2016-06-10 111 * struct ina3221_data - device specific information 323aeb0eb5d9a6 Nicolin Chen 2018-11-05 112 * @pm_dev: Device pointer for pm runtime 7cb6dcff1956ec Andrew F. Davis 2016-06-10 113 * @regmap: Register map of the device 7cb6dcff1956ec Andrew F. Davis 2016-06-10 114 * @fields: Register fields of the device a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 115 * @inputs: Array of channel input source specific structures 87625b24986bc2 Nicolin Chen 2018-11-05 116 * @lock: mutex lock to serialize sysfs attribute accesses 59d608e152e582 Nicolin Chen 2018-09-29 117 * @reg_config: Register value of INA3221_CONFIG 2057bdfb7184e9 Nicolin Chen 2019-10-16 118 * @summation_shunt_resistor: equivalent shunt resistor value for summation 43dece162de047 Nicolin Chen 2019-01-17 119 * @single_shot: running in single-shot operating mode 7cb6dcff1956ec Andrew F. Davis 2016-06-10 120 */ 7cb6dcff1956ec Andrew F. Davis 2016-06-10 121 struct ina3221_data { 323aeb0eb5d9a6 Nicolin Chen 2018-11-05 122 struct device *pm_dev; 7cb6dcff1956ec Andrew F. Davis 2016-06-10 123 struct regmap *regmap; 7cb6dcff1956ec Andrew F. Davis 2016-06-10 124 struct regmap_field *fields[F_MAX_FIELDS]; a9e9dd9c6de5d8 Nicolin Chen 2018-10-01 125 struct ina3221_input inputs[INA3221_NUM_CHANNELS]; 87625b24986bc2 Nicolin Chen 2018-11-05 126 struct mutex lock; 59d608e152e582 Nicolin Chen 2018-09-29 127 u32 reg_config; 2057bdfb7184e9 Nicolin Chen 2019-10-16 128 int summation_shunt_resistor; 3aab5d0b835881 Ninad Malwade 2023-08-26 129 u32 summation_channel_control; 43dece162de047 Nicolin Chen 2019-01-17 130 43dece162de047 Nicolin Chen 2019-01-17 131 bool single_shot; 7cb6dcff1956ec Andrew F. Davis 2016-06-10 @132 }; 7cb6dcff1956ec Andrew F. Davis 2016-06-10 133
On Sat, Aug 26, 2023 at 12:42:48AM +0800, Ninad Malwade wrote: > 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, we can't use summation control function to add > the individual conversion values directly together in the > Shunt-Voltage Sum register to report the total current. > > To address this we add support to BYPASS channels > via kernel device tree property "summation-bypass". > > The channel which has this property would be excluded from > the calculation of summation control function, so we can easily > exclude the one which uses different shunt-resistor value or > bus voltage. > > For example, summation control function calculates > Shunt-Voltage Sum like > - input_shunt_voltage_summaion = input_shunt_voltage_channel1 summation > + input_shunt_voltage_channel2 > + input_shunt_voltage_channel3 > > But if we want the summation to consider only channel1 > and channel3, we can add 'summation-bypass' property > in device tree node of channel2. > > Then the calculation will skip channel2. > - input_shunt_voltage_summaion = input_shunt_voltage_channel1 > + input_shunt_voltage_channel3 > summation > Please note that we only want the channel to be skipped > for summation control function rather than completely disabled. > Therefore, even if we add the device tree node, the functionality > of the single channel would still be retained. > > The below sysfs nodes are added to check if the channel is included > or excluded from the summation control function. > > in*_sum_bypass = 0 --> channel voltage is included for sum of > shunt voltages. > > in*_sum_bypass = 1 --> channel voltage is excluded for sum > of shunt voltages. > This is not an acceptable sysfs attribute. Use debugfs. > Signed-off-by: Rajkumar Kasirajan <rkasirajan@nvidia.com> > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> > --- > drivers/hwmon/ina3221.c | 39 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c > index 5ab944056ec0..093ebf9f1f8d 100644 > --- a/drivers/hwmon/ina3221.c > +++ b/drivers/hwmon/ina3221.c > @@ -104,6 +104,7 @@ struct ina3221_input { > const char *label; > int shunt_resistor; > bool disconnected; > + bool summation_bypass; > }; > > /** > @@ -125,6 +126,7 @@ struct ina3221_data { > struct mutex lock; > u32 reg_config; > int summation_shunt_resistor; > + u32 summation_channel_control; > > bool single_shot; > }; > @@ -154,7 +156,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_bypass) > continue; > if (!shunt_resistor) { > /* Found the reference shunt resistor value */ > @@ -731,10 +734,29 @@ static SENSOR_DEVICE_ATTR_RW(shunt1_resistor, ina3221_shunt, INA3221_CHANNEL1); > static SENSOR_DEVICE_ATTR_RW(shunt2_resistor, ina3221_shunt, INA3221_CHANNEL2); > static SENSOR_DEVICE_ATTR_RW(shunt3_resistor, ina3221_shunt, INA3221_CHANNEL3); > > +static ssize_t ina3221_summation_bypass_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > + struct ina3221_data *ina = dev_get_drvdata(dev); > + unsigned int channel = sd_attr->index; > + struct ina3221_input *input = &ina->inputs[channel]; > + > + return sysfs_emit(buf, "%d\n", input->summation_bypass); > +} > + > +/* summation bypass */ > +static SENSOR_DEVICE_ATTR_RO(in1_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL1); > +static SENSOR_DEVICE_ATTR_RO(in2_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL2); > +static SENSOR_DEVICE_ATTR_RO(in3_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL3); > + As mentioned, use debugfs to display this information. > static struct attribute *ina3221_attrs[] = { > &sensor_dev_attr_shunt1_resistor.dev_attr.attr, > &sensor_dev_attr_shunt2_resistor.dev_attr.attr, > &sensor_dev_attr_shunt3_resistor.dev_attr.attr, > + &sensor_dev_attr_in1_sum_bypass.dev_attr.attr, > + &sensor_dev_attr_in2_sum_bypass.dev_attr.attr, > + &sensor_dev_attr_in3_sum_bypass.dev_attr.attr, > NULL, > }; > ATTRIBUTE_GROUPS(ina3221); > @@ -786,6 +808,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_bypass = of_property_read_bool(child, "summation-bypass"); > + > /* Overwrite default shunt resistor value optionally */ > if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &val)) { > if (val < 1 || val > INT_MAX) { > @@ -873,6 +898,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_bypass) > + ina->summation_channel_control |= (BIT(14 - i)); Unnecessary ( ) around BIT(). > + } > > ina->pm_dev = dev; > mutex_init(&ina->lock); > @@ -978,13 +1007,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 > - * Shunt measurements of disconnected channels should > - * be 0, so it does not matter for summation. > + * Sum only channels that are not 'bypassed' for summation > + * by default. Shunt measurements of disconnected channels Drop "by default". > + * 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; > -- > 2.17.1 >
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c index 5ab944056ec0..093ebf9f1f8d 100644 --- a/drivers/hwmon/ina3221.c +++ b/drivers/hwmon/ina3221.c @@ -104,6 +104,7 @@ struct ina3221_input { const char *label; int shunt_resistor; bool disconnected; + bool summation_bypass; }; /** @@ -125,6 +126,7 @@ struct ina3221_data { struct mutex lock; u32 reg_config; int summation_shunt_resistor; + u32 summation_channel_control; bool single_shot; }; @@ -154,7 +156,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_bypass) continue; if (!shunt_resistor) { /* Found the reference shunt resistor value */ @@ -731,10 +734,29 @@ static SENSOR_DEVICE_ATTR_RW(shunt1_resistor, ina3221_shunt, INA3221_CHANNEL1); static SENSOR_DEVICE_ATTR_RW(shunt2_resistor, ina3221_shunt, INA3221_CHANNEL2); static SENSOR_DEVICE_ATTR_RW(shunt3_resistor, ina3221_shunt, INA3221_CHANNEL3); +static ssize_t ina3221_summation_bypass_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); + struct ina3221_data *ina = dev_get_drvdata(dev); + unsigned int channel = sd_attr->index; + struct ina3221_input *input = &ina->inputs[channel]; + + return sysfs_emit(buf, "%d\n", input->summation_bypass); +} + +/* summation bypass */ +static SENSOR_DEVICE_ATTR_RO(in1_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL1); +static SENSOR_DEVICE_ATTR_RO(in2_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL2); +static SENSOR_DEVICE_ATTR_RO(in3_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL3); + static struct attribute *ina3221_attrs[] = { &sensor_dev_attr_shunt1_resistor.dev_attr.attr, &sensor_dev_attr_shunt2_resistor.dev_attr.attr, &sensor_dev_attr_shunt3_resistor.dev_attr.attr, + &sensor_dev_attr_in1_sum_bypass.dev_attr.attr, + &sensor_dev_attr_in2_sum_bypass.dev_attr.attr, + &sensor_dev_attr_in3_sum_bypass.dev_attr.attr, NULL, }; ATTRIBUTE_GROUPS(ina3221); @@ -786,6 +808,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_bypass = of_property_read_bool(child, "summation-bypass"); + /* Overwrite default shunt resistor value optionally */ if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &val)) { if (val < 1 || val > INT_MAX) { @@ -873,6 +898,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_bypass) + ina->summation_channel_control |= (BIT(14 - i)); + } ina->pm_dev = dev; mutex_init(&ina->lock); @@ -978,13 +1007,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 - * Shunt measurements of disconnected channels should - * be 0, so it does not matter for summation. + * Sum only channels that are not 'bypassed' for summation + * by default. 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;
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, we can't use summation control function to add the individual conversion values directly together in the Shunt-Voltage Sum register to report the total current. To address this we add support to BYPASS channels via kernel device tree property "summation-bypass". The channel which has this property would be excluded from the calculation of summation control function, so we can easily exclude the one which uses different shunt-resistor value or bus voltage. For example, summation control function calculates Shunt-Voltage Sum like - input_shunt_voltage_summaion = input_shunt_voltage_channel1 + input_shunt_voltage_channel2 + input_shunt_voltage_channel3 But if we want the summation to consider only channel1 and channel3, we can add 'summation-bypass' property in device tree node of channel2. Then the calculation will skip channel2. - input_shunt_voltage_summaion = input_shunt_voltage_channel1 + input_shunt_voltage_channel3 Please note that we only want the channel to be skipped for summation control function rather than completely disabled. Therefore, even if we add the device tree node, the functionality of the single channel would still be retained. The below sysfs nodes are added to check if the channel is included or excluded from the summation control function. in*_sum_bypass = 0 --> channel voltage is included for sum of shunt voltages. in*_sum_bypass = 1 --> channel voltage is excluded for sum of shunt voltages. Signed-off-by: Rajkumar Kasirajan <rkasirajan@nvidia.com> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> --- drivers/hwmon/ina3221.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)