Messages in this thread | | | Subject | Re: [PATCH v8 2/2] hwmon: ina3221: Read channel input source info from DT | From | Guenter Roeck <> | Date | Sun, 30 Sep 2018 13:55:18 -0700 |
| |
Hi Nicolin,
On 09/28/2018 06:39 PM, Nicolin Chen wrote: > An ina3221 chip has three input ports. Each port is used > to measure the voltage and current of its input source. > > The DT binding now has defined bindings for their input > sources, so the driver should read these information and > handle accordingly. > > This patch adds a new structure of input source specific > information including input source label, shunt resistor > value and its connection status. It exposes these labels > via in[123]_label sysfs nodes upon available, and also > disables those channels where there are no input source > being connected. Meanwhile, it also adds in[123]_enable > sysfs nodes for users to get control of three channels, > and returns -ENODATA code for any sensor read according > to hwmon ABI. > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
[ ... ]
> + > +static ssize_t ina3221_set_enable(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + 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; > + u16 config = ina->reg_config; > + bool enable; > + int ret; > + > + ret = kstrtobool(buf, &enable); > + if (ret) > + return ret; > + > + if (enable) > + config |= INA3221_CONFIG_CHx_EN(channel); > + else > + config &= ~INA3221_CONFIG_CHx_EN(channel); > + > + ret = regmap_write(ina->regmap, INA3221_CONFIG, config); > + if (ret) > + return ret; > + > + ina->reg_config = config; > +
Sorry it too me so long to realize this: The code above is racy. There could be multiple threads enabling and disabling a channel. Without a mutex, there is no guarantee that the final value of reg_config matches the value written into the chip.
You'll have to introduce a mutex and implement something like
... ret = kstrtobool(buf, &enable); if (ret) return ret;
mutex_lock(&ina->update_lock); config = ina->reg_config; ... ret = regmap_write(ina->regmap, INA3221_CONFIG, config); if (ret) { count = ret; goto error; } ina->reg_config = config; error: mutex_unlock(&ina->update_lock); return count;
Guenter
| |