Re: [PATCH 1/2] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[John Muir]

Hi Guenter,

On Dec 1, 2016, at 2:08 PM, Guenter Roeck  wrote:
> 
>> 
>> 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.
> 
OK. Will do.

> Where did you find regmap_sync() ? It seems to be hiding from me.

Sorry, I meant regcache_sync().

John.
--
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

Re: [PATCH 1/2] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[Guenter Roeck]

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

Re: [PATCH 1/2] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[John Muir]

Hi Guenter,

On Dec 1, 2016, at 7:19 AM, Guenter Roeck  wrote:
> 
>> +/* convert 12-bit TMP108 register value to milliCelsius */
>> +static inline int tmp108_temp_reg_to_mC(s16 val)
>> +{
>> +return (val & ~0x01) * 1000 / 256;
> 
> Why ~0x01 and not ~0x0f ? The lower 4 bits are supposed to be 0.

I can probably change it: I will reevaluate this.

>> +tmp108->orig_config = regval;
>> +config = regval;
>> +
> 
> Nitpick, but a separate 'regval' variable is not really necessary.
> Just make config an u32 and use it directly (this actually makes the code
> more efficient on many architectures, since cpu registers tend to be
> at least 32 bit wide and 16 bit accesses result in extra masking).

OK.

>> +if (device_property_read_bool(dev, "ti,thermostat-mode-comparator"))
>> +config &= ~TMP108_CONF_TM;
>> +else
>> +config |= TMP108_CONF_TM;
>> +
> As suggested separately, I would suggest to drop this and always select 
> comparator mode.

Yep.

>> +hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
>> + tmp108,
>> + _chip_info,
>> + NULL);
>> +if (IS_ERR(hwmon_dev)) {
>> +err = PTR_ERR(hwmon_dev);
>> +dev_err(dev, "unable to register hwmon device: %d", err);
>> +return err;
>> +}
> 
> Overall I prefer a simeple
>   return PTR_ERR_OR_ZERO(hwmon_dev);

Ack.


>> +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?

Thanks for your advice,

John.

--
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