Hi Guenter,
On Dec 1, 2016, at 7:19 AM, Guenter Roeck <linux@xxxxxxxxxxxx> 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,
>> + &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-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
