Hi John,
On Thu, Dec 01, 2016 at 01:50:22PM -0800, John Muir wrote:
> Hi Guenter,
>
[ ... ]
>
> >> +static int __maybe_unused tmp108_resume(struct device *dev)
> >> +{
> >> + struct tmp108 *tmp108 = dev_get_drvdata(dev);
> >> + int err;
> >> +
> >> + err = regmap_write(tmp108->regmap, TMP108_REG_CONF,
> >> + tmp108->curr_config);
> >> +
> >> + tmp108->ready_time = jiffies;
> >> + if (tmp108->curr_config & TMP108_MODE_CONTINUOUS)
> >
> > Since only continuous mode is supported, and it is somewhat unlikely
> > that we'll ever support one-shot mode, I think it would be better to
> > unconditionally set continuous mode and the delay here and drop
> > curr_config entirely. curr_config adds quite some complexity to the
> > driver with no real gain.
>
> OK. Due to my ignorance I was assuming that the could would need to restore
> the current configuration during resume, and also the previous versions (that
> you did not see) of this code was not using regmap. In fact I see that there
> is also a function called ‘regmap_sync’ which is used (mainly by audio
> codecs) to do this (i.e.; as part of device reset but there are examples in
> resume()). The configuration register is now marked volatile so it would be
> skipped by regmap_sync anyway.
>
> Should we be concerned about restoring the configuration here?
>
Interesting question. If the chip was really powered off, you would
have to restore the entire configuration, not just the configuration
register. Given that, I think it is sufficient to just restore the
operational mode, ie the value changed when entering suspend.
Where did you find regmap_sync() ? It seems to be hiding from me.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
