On Thu, Sep 27, 2018 at 03:26:14PM -0700, Nicolin Chen wrote: > On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote: > > > +static inline bool ina3221_is_enable(struct ina3221_data *ina, int channel) > > > > s/is_enable/is_enabled/, maybe ? > > Fixing. > > > > + return (config & INA3221_CONFIG_CHx_EN(channel)) > 0; > > > > The "> 0" is unnecessary. Conversion to bool is automatic. If you want to make > > it explicit, please use > > > > return !!(config & INA3221_CONFIG_CHx_EN(channel)); > > Removing "> 0". > > > It should not be necessary to re-read the value from the chip all the time. > > I would suggest to cache the value in the 'disabled' variable. > > Regarding this part, I added a cache before sending this one. But > I realized if the chip got powered off and rebooted during system > suspend/resume, the cache would not reflect the actual status. As > I mentioned earlier, this was enlightened by your comments about > the BIOS. So I feel it'd be safer to read the register every time > at this point, until I add the suspend/resume feature by syncing > with regcache. What do you think about it? > The proper fix for this problem would be to add support for suspend / resume to the driver. At resume time, all channels will have been re-enabled if the chip was powered off, even if they were explicitly disabled by devicetree (or via explicit configuration). This means the driver just behaves badly across suspend/resume, period. Displaying a raw value instead of a cached one doesn't solve that problem. By using a cached value, at least the user would not notice that the chip no longer does what it is supposed to be doing. I guess we just have different priorities. If I think suspend/resume is a problem for my use case, I would just go ahead and fix it. I would not try to write code that doesn't fix the problem causing it, much less argue for it. Having said that, I didn't mention that part in my other reply, meaning I'll accept the code as is. Thanks, Guenter
