| Message ID | 20180929013937.29289-1-nicoleotsuka@gmail.com |
|---|---|
| Headers | show |
| Series | Add an initial DT binding doc for ina3221 | expand |
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
On Sun, Sep 30, 2018 at 01:55:18PM -0700, Guenter Roeck wrote: > 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. Hmm..that's true...glad you found and pointed it out. > 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; I would prefer to just add another regmap_read at the end for the reg_config sync, and to use regmap_update_bits() for the bit set/clear. It might not serialize things as the solution above does but at least the reg_config would get the correct value eventually. Since regmap has an internal mutex already, we may get rid of another mutex by doing so. I will send a v9 tomorrow. Thanks for the review Nicolin