Re: [PATCH 1/2] hwmon: Add Texas Instruments TMP108 temperature sensor driver.
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-doc" 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.
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-doc" 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.
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,
>> + &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 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.
Hi John, On 11/30/2016 12:36 PM, John Muir wrote: Add support for the TI TMP108 temperature sensor with some device configuration parameters. Overall looks pretty good. Just a couple of nitpicks. Thanks, Guenter Signed-off-by: John Muir --- Documentation/hwmon/tmp108 | 38 drivers/hwmon/Kconfig | 11 + drivers/hwmon/Makefile | 1 + drivers/hwmon/tmp108.c | 495 + 4 files changed, 545 insertions(+) create mode 100644 Documentation/hwmon/tmp108 create mode 100644 drivers/hwmon/tmp108.c diff --git a/Documentation/hwmon/tmp108 b/Documentation/hwmon/tmp108 new file mode 100644 index 000..ef2e9a3 --- /dev/null +++ b/Documentation/hwmon/tmp108 @@ -0,0 +1,38 @@ +Kernel driver tmp108 + + +Supported chips: + * Texas Instruments TMP108 +Prefix: 'tmp108' +Addresses scanned: none +Datasheet: http://www.ti.com/product/tmp108 + +Author: + John Muir + +Description +--- + +The Texas Instruments TMP108 implements one temperature sensor. An alert pin +can be set when temperatures exceed minimum or maximum values plus or minus a +hysteresis value. + +The sensor is accurate to 0.75C over the range of -25 to +85 C, and to 1.0 +degree from -40 to +125 C. Resolution of the sensor is 0.0625 degree. The +operating temperature has a minimum of -55 C and a maximum of +150 C. +Hysteresis values can be set to 0, 1, 2, or 4C. + +The TMP108 has a programmable update rate that can select between 8, 4, 1, and +0.5 Hz. + +By default the TMP108 reads the temperature continuously. To conserve power, +the TMP108 has a one-shot mode where the device is normally shut-down. When a +one shot is requested the temperature is read, the result can be retrieved, +and then the device is shut down automatically. (This driver only supports +continuous mode.) + +The driver provides the common sysfs-interface for temperatures (see +Documentation/hwmon/sysfs-interface under Temperatures). + +See Documentation/devicetree/bindings/hwmon/tmp108.txt for configuration +properties. diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 45cef3d..4c173de 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1591,6 +1591,17 @@ config SENSORS_TMP103 This driver can also be built as a module. If so, the module will be called tmp103. +config SENSORS_TMP108 + tristate "Texas Instruments TMP108" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for Texas Instruments TMP108 + sensor chips. + + This driver can also be built as a module. If so, the module + will be called tmp108. + config SENSORS_TMP401 tristate "Texas Instruments TMP401 and compatibles" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index aecf4ba..51e5256 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -152,6 +152,7 @@ obj-$(CONFIG_SENSORS_TC74) += tc74.o obj-$(CONFIG_SENSORS_THMC50) += thmc50.o obj-$(CONFIG_SENSORS_TMP102) += tmp102.o obj-$(CONFIG_SENSORS_TMP103) += tmp103.o +obj-$(CONFIG_SENSORS_TMP108) += tmp108.o obj-$(CONFIG_SENSORS_TMP401) += tmp401.o obj-$(CONFIG_SENSORS_TMP421) += tmp421.o obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c new file mode 100644 index 000..35d598d --- /dev/null +++ b/drivers/hwmon/tmp108.c @@ -0,0 +1,495 @@ +/* Texas Instruments TMP108 SMBus temperature sensor driver + * + * Copyright (C) 2016 John Muir + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#defineDRIVER_NAME "tmp108" + +#defineTMP108_REG_TEMP 0x00 +#defineTMP108_REG_CONF 0x01 +#defineTMP108_REG_TLOW 0x02 +#defineTMP108_REG_THIGH0x03 + +#define TMP108_TEMP_MIN_MC -5 /* Minimum millicelcius. */ +#define TMP108_TEMP_MAX_MC 127937 /* Maximum millicelcius. */ + +/* Configuration register bits. + * Note: these bit definitions are byte swapped. + */ +#define TMP108_CONF_M0 0x0100 /* Sensor mode. */ +#define TMP108_CONF_M1 0x0200 +#define TMP108_CONF_TM 0x0400 /* Thermostat mode. */ +#define TMP108_CONF_FL 0x0800 /* Watchdog flag - TLOW */ +#define TMP108_CONF_FH 0x1000
