On Wed, Apr 17, 2019 at 07:04:09AM -0700, Guenter Roeck wrote: > > > I am not quite sure if this update_interval is the best way to > > > implement the conversion time settings but I can't find another > > > common approach. This implementation certainly has drawbacks: > > > 1) It can't configure Bus and Shunt conversion times separately > > > (Not crucial for me at this point as I set them equally) > > > 2) Users need to calculate for the settings of conversion time > > > 3) The ABI defines update_interval in msec while the hardware > > > and datasheet does in usec, and that generates rounding diff > > > 4) The update_interval value would be spontaneously modified > > > everytime number of samples or number of enabled channels > > > gets changed. This might confuses users who tries to have > > > a fixed update_interval other than really merely setting > > > conversion time. > > > > > > I see IIO subsystem have something like IIO_CHAN_INFO_INT_TIME > > > for conversion time setting exclusively. Do we have something > > > similar under hwmon? > > > > > > > No. I think what you have should be good enough for now. > > Please see comments below. OK. I will go ahead with this approach then. > > > + /* Update Bus-voltage conversion time */ > > > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, > > > + INA3221_CONFIG_VBUS_CT_MASK, > > > + idx << INA3221_CONFIG_VBUS_CT_SHIFT); > > > + if (ret) > > > + return ret; > > > + > > > + /* Update Shunt-voltage conversion time */ > > > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, > > > + INA3221_CONFIG_VSH_CT_MASK, > > > + idx << INA3221_CONFIG_VSH_CT_SHIFT); > > > + if (ret) > > > + return ret; > > > > It should be possible to update both conversion times with a single call, > > since both calls touch only one register. Something like > > > > ret = regmap_update_bits(ina->regmap, INA3221_CONFIG > > INA3221_CONFIG_VBUS_CT_MASK | INA3221_CONFIG_VSH_CT_MASK, > > (idx << INA3221_CONFIG_VBUS_CT_SHIFT) | (idx << INA3221_CONFIG_VSH_CT_SHIFT)); > > > > Granted, that is a bit long, but it saves an extra i2c write operation. Will merge them. > Thinking about it ... does it even make sense to cache reg_config twice, > or would it be better to just update the local copy and use regmap_write() > to send it to the chip ? I remember the reason of adding the read-back was to prevent race condition. But now we have mutex protections for all sysfs nodes, maybe it's not necessary anymore. I will read the code carefully and see if it's safe to remove it -- will do in a separate patch. Thanks Nicolin
