| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20221117144709.GD664755@roeck-us.net>
Date: Thu, 17 Nov 2022 06:47:09 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Ninad Malwade <nmalwade@...dia.com>
Cc: treding@...dia.com, jonathanh@...dia.com, jdelvare@...e.com,
nicolinc@...dia.com, rkasirajan@...dia.com,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH] [WAR] hwmon: (ina3221) Apply software WAR to offset
shunt voltage
On Thu, Nov 17, 2022 at 04:32:26PM +0800, Ninad Malwade wrote:
> This is used as a software WAR to offset shunt voltage reading
What is a software WAR ? Or a WAR in the first place ?
What is the relevance of the "[WAR]" tag in the subject ?
None of the definitions at https://acronyms.thefreedictionary.com/WAR
seem to apply. I am sure it means something for you, and it seems to be
important enough to use the term repeatedly, but please do not assume
that others know what it means.
> from INA3221 to increase its accuracy. This patch implements a
> previous downstream feature by reading the offset information
> from DT and apply it to current readings.
>
Where is the devicetree documentation ?
> Signed-off-by: Ninad Malwade <nmalwade@...dia.com>
> ---
> drivers/hwmon/ina3221.c | 141 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 137 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index e06186986444..726c8b99b8cd 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -94,13 +94,39 @@ enum ina3221_channels {
> INA3221_NUM_CHANNELS
> };
>
> +/**
> + * struct shuntv_offset_range - [WAR] shunt voltage offset sub-range
Still no explanation what WAR actually stands for.
> + * @start: range start (uV)
> + * @end: range end (uV)
> + * @offset: offset for the current sub-range
> + */
> +struct shuntv_offset_range {
> + s32 start;
> + s32 end;
> + s32 offset;
> +};
> +
> +/**
> + * struct shuntv_offset - [WAR] shunt voltage offset information
> + * @offset: general offset
> + * @range: pointer to a sub-range of shunt voltage offset (uV)
> + * @num_range: number of sub-ranges of shunt voltage offset
> + */
> +struct shuntv_offset {
> + s32 offset;
> + struct shuntv_offset_range *range;
> + s32 num_range;
> +};
> +
> /**
> * struct ina3221_input - channel input source specific information
> + * @shuntv_offset: [WAR] shunt voltage offset information
> * @label: label of channel input source
> * @shunt_resistor: shunt resistor value of channel input source
> * @disconnected: connection status of channel input source
> */
> struct ina3221_input {
> + struct shuntv_offset *shuntv_offset;
> const char *label;
> int shunt_resistor;
> bool disconnected;
> @@ -329,7 +355,7 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
> struct ina3221_data *ina = dev_get_drvdata(dev);
> struct ina3221_input *input = ina->inputs;
> u8 reg = ina3221_curr_reg[attr][channel];
> - int resistance_uo, voltage_nv;
> + int resistance_uo, voltage_uv;
> int regval, ret;
>
> if (channel > INA3221_CHANNEL3)
> @@ -362,10 +388,34 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
> if (ret)
> return ret;
>
> - /* Scale of shunt voltage: LSB is 40uV (40000nV) */
> - voltage_nv = regval * 40000;
> + /* Scale of shunt voltage: LSB is 40uV */
> + voltage_uv = regval * 40;
> +
> + /* Apply software WAR to offset shunt voltage for accuracy */
> + if (input->shuntv_offset) {
> + struct shuntv_offset_range *range =
> + input->shuntv_offset->range;
> + int num_range = input->shuntv_offset->num_range;
> + int offset = input->shuntv_offset->offset;
> +
> + while (num_range--) {
> + if (voltage_uv >= range->start &&
> + voltage_uv <= range->end) {
> + /* Use range offset instead */
> + offset = range->offset;
> + break;
> + }
> + range++;
> + }
> +
> + if (voltage_uv < 0)
> + voltage_uv += offset;
> + else
> + voltage_uv -= offset;
> + }
> +
> /* Return current in mA */
> - *val = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
> + *val = DIV_ROUND_CLOSEST(voltage_uv * 1000, resistance_uo);
> return 0;
> case hwmon_curr_crit_alarm:
> case hwmon_curr_max_alarm:
> @@ -758,6 +808,84 @@ static const struct regmap_config ina3221_regmap_config = {
> .volatile_table = &ina3221_volatile_table,
> };
>
> +static struct shuntv_offset *
> +ina3221_probe_shuntv_offset_from_dt(struct device *dev,
> + struct device_node *child)
> +{
> + struct device_node *np, *range_np;
> + struct shuntv_offset *shuntv_offset;
> + struct shuntv_offset_range *range;
> + s32 start, end, offset;
> + const __be32 *prop;
> + int ret, num_range;
> +
> + prop = of_get_property(child, "shunt-volt-offset-uv", NULL);
> + /* Silently return for devices with no need of an offset WAR */
> + if (!prop)
> + return NULL;
> +
> + np = of_find_node_by_phandle(be32_to_cpup(prop));
> + if (!np) {
> + dev_err(dev, "corrupted phandle for shunt-volt-offset-uv\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + ret = of_property_read_s32(np, "offset", &offset);
> + if (ret) {
> + dev_err(dev, "failed to read general shuntv offset\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + shuntv_offset = devm_kzalloc(dev, sizeof(*shuntv_offset), GFP_KERNEL);
> + if (!shuntv_offset)
> + return ERR_PTR(-ENOMEM);
> +
> + shuntv_offset->offset = offset;
> +
> + num_range = of_get_child_count(np);
> +
> + /* Return upon no sub-range found */
> + if (!num_range)
> + return shuntv_offset;
> +
> + range = devm_kzalloc(dev, sizeof(*range) * num_range, GFP_KERNEL);
> + if (!range)
> + return ERR_PTR(-ENOMEM);
> +
> + shuntv_offset->range = range;
> + shuntv_offset->num_range = num_range;
> +
> + for_each_child_of_node(np, range_np) {
> + ret = of_property_read_s32(range_np, "start", &start);
> + if (ret) {
> + dev_warn(dev, "missing start in range node\n");
> + range++;
> + continue;
> + }
> +
> + ret = of_property_read_s32(range_np, "end", &end);
> + if (ret) {
> + dev_warn(dev, "missing end in range node\n");
> + range++;
> + continue;
> + }
> +
> + ret = of_property_read_s32(range_np, "offset", &offset);
> + if (ret) {
> + dev_warn(dev, "missing offset in range node\n");
> + range++;
> + continue;
> + }
Not sure if that would be an acceptable devicetree binding. The bindings
will need to be submitted, reviewed, and accepted by a devicetree maintainer.
> +
> + range->start = start;
> + range->end = end;
> + range->offset = offset;
> + range++;
> + }
> +
> + return shuntv_offset;
> +}
> +
> static int ina3221_probe_child_from_dt(struct device *dev,
> struct device_node *child,
> struct ina3221_data *ina)
> @@ -796,6 +924,11 @@ static int ina3221_probe_child_from_dt(struct device *dev,
> input->shunt_resistor = val;
> }
>
> + /* Apply software WAR to offset shunt voltage for accuracy */
> + input->shuntv_offset = ina3221_probe_shuntv_offset_from_dt(dev, child);
> + if (IS_ERR(input->shuntv_offset))
> + return PTR_ERR(input->shuntv_offset);
> +
> return 0;
> }
>
> --
> 2.17.1
>
Powered by blists - more mailing lists