Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

[Nicolin Chen]

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