From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [PATCH v2] hwmon: Driver for TI TMP103 temperature sensor Date: Fri, 13 Jun 2014 09:03:01 -0700 Message-ID: <539B20B5.7000506@roeck-us.net> References: <1402643421-9102-1-git-send-email-hs@denx.de> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <1402643421-9102-1-git-send-email-hs-ynQEQJNshbs@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Heiko Schocher , lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org Cc: Jean Delvare , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org On 06/13/2014 12:10 AM, Heiko Schocher wrote: > Driver for the TI TMP103. > > The TI TMP103 is similar to the TMP102. It differs from the TMP102 > by having only 8 bit registers. > > Signed-off-by: Heiko Schocher > Hello Heiko, Couple of additional comments. Thanks, Guenter > --- > > Cc: Jean Delvare > Cc: Guenter Roeck > Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > > - change for v2: > - add comments from GuenterRoeck: > - remove Cc from commit subject > - add devicetree mailinglist > - move Documentation to Documentation/hwmon/tmp103 > - remove devicetree bindings from Documentation > - add compatible string to > "Documentation/devicetree/bindings/i2c/trivial-devices.txt" > - remove CamelCase > - fix Codingstyle issues > - use ATTRIBUTE_GROUPS and devm_hwmon_device_register_with_group= s() > - remove unsused define TMP103_CONFIG_RD_ONLY > - restore config register when exit() > - use regmap > > .../devicetree/bindings/i2c/trivial-devices.txt | 1 + > Documentation/hwmon/tmp103 | 28 ++ > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/tmp103.c | 304 ++++++++++= +++++++++++ > 5 files changed, 344 insertions(+) > create mode 100644 Documentation/hwmon/tmp103 > create mode 100644 drivers/hwmon/tmp103.c > > diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.tx= t b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > index bef86e5..fc944e0 100644 > --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt > +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > @@ -83,5 +83,6 @@ stm,m41t80 M41T80 - SERIAL ACCESS RTC WITH ALARMS > taos,tsl2550 Ambient Light Sensor with SMBUS/Two Wire Serial Inter= face > ti,tsc2003 I2C Touch-Screen Controller > ti,tmp102 Low Power Digital Temperature Sensor with SMBUS/Two Wire= Serial Interface > +ti,tmp103 Low Power Digital Temperature Sensor with SMBUS/Two Wire = Serial Interface > ti,tmp275 Digital Temperature Sensor > winbond,wpct301 i2c trusted platform module (TPM) > diff --git a/Documentation/hwmon/tmp103 b/Documentation/hwmon/tmp103 > new file mode 100644 > index 0000000..559fea5 > --- /dev/null > +++ b/Documentation/hwmon/tmp103 > @@ -0,0 +1,28 @@ > +Kernel driver tmp103 > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > + > +Supported chips: > + * Texas Instruments TMP103 > + Prefix: 'tmp103' > + Addresses scanned: none > + Product info and datasheet: http://www.ti.com/product/tmp103 > + > +Author: > + Heiko Schocher > + > +Description > +----------- > + > +The TMP103 is a digital output temperature sensor in a four-ball > +wafer chip-scale package (WCSP). The TMP103 is capable of reading > +temperatures to a resolution of 1=C2=B0C. The TMP103 is specified fo= r > +operation over a temperature range of =E2=80=9340=C2=B0C to +125=C2=B0= C. > + > +Resolution: 8 Bits > +Accuracy: =C2=B11=C2=B0C Typ (=E2=80=9310=C2=B0C to +100=C2=B0C) > + > +The driver provides the common sysfs-interface for temperatures (see > +Documentation/hwmon/sysfs-interface under Temperatures). > + > +please refer how to instantiate this driver: s/please/Please/ > +Documentation/i2c/instantiating-devices > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 0034316..0f44dbb 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1381,6 +1381,16 @@ config SENSORS_TMP102 > This driver can also be built as a module. If so, the module > will be called tmp102. > > +config SENSORS_TMP103 > + tristate "Texas Instruments TMP103" > + depends on I2C > + help > + If you say yes here you get support for Texas Instruments TMP103 > + sensor chips. > + > + This driver can also be built as a module. If so, the module > + will be called tmp103. > + > config SENSORS_TMP401 > tristate "Texas Instruments TMP401 and compatibles" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 11798ad..8e2f6a2 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -134,6 +134,7 @@ obj-$(CONFIG_SENSORS_SMSC47M192)+=3D smsc47m192.o > obj-$(CONFIG_SENSORS_AMC6821) +=3D amc6821.o > obj-$(CONFIG_SENSORS_THMC50) +=3D thmc50.o > obj-$(CONFIG_SENSORS_TMP102) +=3D tmp102.o > +obj-$(CONFIG_SENSORS_TMP103) +=3D tmp103.o > obj-$(CONFIG_SENSORS_TMP401) +=3D tmp401.o > obj-$(CONFIG_SENSORS_TMP421) +=3D tmp421.o > obj-$(CONFIG_SENSORS_TWL4030_MADC)+=3D twl4030-madc-hwmon.o > diff --git a/drivers/hwmon/tmp103.c b/drivers/hwmon/tmp103.c > new file mode 100644 > index 0000000..01119a6 > --- /dev/null > +++ b/drivers/hwmon/tmp103.c > @@ -0,0 +1,304 @@ > +/* > + * Texas Instruments TMP103 SMBus temperature sensor driver > + * Copyright (C) 2014 Heiko Schocher > + * > + * Based on: > + * Texas Instruments TMP102 SMBus temperature sensor driver > + * > + * Copyright (C) 2010 Steven King > + * > + * This program is free software; you can redistribute it and/or mod= ify > + * 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 > + > +#define DRIVER_NAME "tmp103" > + > +#define TMP103_TEMP_REG 0x00 > +#define TMP103_CONF_REG 0x01 > +#define TMP103_TLOW_REG 0x02 > +#define TMP103_THIGH_REG 0x03 > + > +#define TMP103_CONF_M0 0x01 > +#define TMP103_CONF_M1 0x02 > +#define TMP103_CONF_LC 0x04 > +#define TMP103_CONF_FL 0x08 > +#define TMP103_CONF_FH 0x10 > +#define TMP103_CONF_CR0 0x20 > +#define TMP103_CONF_CR1 0x40 > +#define TMP103_CONF_ID 0x80 > +#define TMP103_CONF_SD (TMP103_CONF_M0 | TMP103_CONF_M1) > + No tab between #define and the actual definition, please. > +struct tmp103 { > + struct i2c_client *client; No longer needed. See below. > + struct device *hwmon_dev; > + struct regmap *regmap; > + struct mutex lock; > + unsigned int config_orig; > + unsigned long last_update; > + int temp[3]; Both no longer needed. Actually, the entire structure is not really needed anymore with the ch= anges suggested below. The only value still needed would be regmap, and you could store= that directly in drvdata. > +}; > + > +static inline int tmp103_reg_to_mc(s8 val) > +{ > + return val * 1000; > +} > + > +static inline u8 tmp103_mc_to_reg(int val) > +{ > + return DIV_ROUND_CLOSEST(val, 1000); > +} > + > +static const u8 tmp103_reg[] =3D { > + TMP103_TEMP_REG, > + TMP103_TLOW_REG, > + TMP103_THIGH_REG, > +}; No longer needed; you can put the register directly into sda->index. > + > +#define TMP103_INVALID 0xffff > + > +static ssize_t tmp103_show_temp(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct sensor_device_attribute *sda =3D to_sensor_dev_attr(attr); > + struct i2c_client *client =3D to_i2c_client(dev); > + struct tmp103 *tmp103 =3D i2c_get_clientdata(client); Too complicated. Attributes are now attached to the hwmon device, and t= mp103 can be retrieved from there. struct tmp103 *tmp103 =3D dev_get_drvdata(dev); or, with all the changes I suggested, even simpler struct regmap *regmap =3D dev_get_drvdata(dev); > + unsigned int status; > + int ret; > + > + mutex_lock(&tmp103->lock); > + if (time_after(jiffies, tmp103->last_update + HZ / 3)) { > + int i; > + > + for (i =3D 0; i < ARRAY_SIZE(tmp103->temp); ++i) { > + ret =3D regmap_read(tmp103->regmap, tmp103_reg[i], > + &status); > + if (ret < 0) > + tmp103->temp[i] =3D TMP103_INVALID; Good idea, but not really what we'd want to see. In this case the error should be reported to use space, ie return the value of 'ret', not "inv= alid". Also, you don't need to read all registers here, and just forget about local caching (not worth it). Suggested replacement below. > + else > + tmp103->temp[i] =3D tmp103_reg_to_mc(status); > + } > + tmp103->last_update =3D jiffies; > + } > + mutex_unlock(&tmp103->lock); > + > + if (tmp103->temp[sda->index] !=3D TMP103_INVALID) > + return sprintf(buf, "%d\n", tmp103->temp[sda->index]); > + > + return sprintf(buf, "invalid\n"); > +} static ssize_t tmp103_show_temp(struct device *dev, struct device_attribute *attr, char *buf) { struct sensor_device_attribute *sda =3D to_sensor_dev_attr(attr); struct tmp103 *tmp103 =3D dev_get_drvdata(dev); /* or struct regmap *r= egmap =3D dev_get_drvdata(dev); */ unsigned int regval; int ret; ret =3D regmap_read(tmp103->regmap, sda->index, ®val); if (ret < 0) return ret; return return sprintf(buf, "%d\n", tmp103_reg_to_mc(regval)); } > + > +static ssize_t tmp103_set_temp(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct sensor_device_attribute *sda =3D to_sensor_dev_attr(attr); > + struct i2c_client *client =3D to_i2c_client(dev); > + struct tmp103 *tmp103 =3D i2c_get_clientdata(client); Same as above. struct tmp103 *tmp103 =3D dev_get_drvdata(dev); > + long val; > + int ret; > + > + if (kstrtol(buf, 10, &val) < 0) > + return -EINVAL; > + > + val =3D clamp_val(val, -55000, 127000); > + mutex_lock(&tmp103->lock); > + tmp103->temp[sda->index] =3D val; > + ret =3D regmap_write(tmp103->regmap, tmp103_reg[sda->index], > + tmp103_mc_to_reg(val)); After dropping tmp103->temp[] you don't need the lock anymore, which means it can go away completely. > + mutex_unlock(&tmp103->lock); > + return ret ? : count; > +} > + > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp103_show_temp, NU= LL , 0); > + As suggested above, instead of using an indirect index, use TMP103_TEMP= _REG directly to simplify the code. > +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, tmp103_show_= temp, > + tmp103_set_temp, 1); > + > +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp103_show_= temp, > + tmp103_set_temp, 2); > + > +static struct attribute *tmp103_attrs[] =3D { > + &sensor_dev_attr_temp1_input.dev_attr.attr, > + &sensor_dev_attr_temp1_min.dev_attr.attr, > + &sensor_dev_attr_temp1_max.dev_attr.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(tmp103); > + > +static struct regmap_config tmp103_regmap_config =3D { > + .reg_bits =3D 8, > + .val_bits =3D 8, > + .max_register =3D TMP103_THIGH_REG, You can also add a 'volatile' function to tell regmap which registers a= re volatile. .volatile_reg =3D tmp103_is_volatile, with static bool tmp103_regmap_is_volatile(struct device *dev, unsigned int = reg) { return reg =3D=3D TMP103_TEMP_REG; } With this, only the temperature register will be read after the first i= nitialization. If you add alarm attributes, you'd have to add the configuration regist= er, but not for now. > +}; > + > +#define TMP103_CONFIG (TMP103_CONF_CR1 | TMP103_CONF_M1) > + Can you move this to the other definitions ? TMP103_CONF_SD is already = there, so this would be more consistent. > +static int tmp103_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev =3D &client->dev; > + struct device *hwmon_dev; > + struct tmp103 *tmp103; > + int ret; > + unsigned int status; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_BYTE_DATA)) { > + dev_err(&client->dev, > + "adapter doesn't support SMBus byte transactions\n"); > + return -ENODEV; > + } > + > + tmp103 =3D devm_kzalloc(&client->dev, sizeof(*tmp103), GFP_KERNEL); > + if (!tmp103) > + return -ENOMEM; > + > + i2c_set_clientdata(client, tmp103); > + tmp103->client =3D client; Both no longer needed. > + > + tmp103->regmap =3D devm_regmap_init_i2c(client, &tmp103_regmap_conf= ig); > + if (IS_ERR(tmp103->regmap)) { > + dev_err(dev, "failed to allocate register map\n"); > + return PTR_ERR(tmp103->regmap); > + } > + > + ret =3D regmap_read(tmp103->regmap, TMP103_CONF_REG, &status); > + if (ret < 0) > + goto fail_init_regmap; > + > + tmp103->config_orig =3D status; > + ret =3D regmap_write(tmp103->regmap, TMP103_CONF_REG, TMP103_CONFIG= ); > + if (ret < 0) { > + dev_err(&client->dev, "error writing config register\n"); > + goto fail_restore_config; > + } > + > + ret =3D regmap_read(tmp103->regmap, TMP103_CONF_REG, &status); > + if (ret < 0) { > + dev_err(&client->dev, "error reading config register\n"); > + goto fail_restore_config; > + } > + if (status !=3D TMP103_CONFIG) { > + dev_err(&client->dev, "config settings did not stick\n"); > + ret =3D -ENODEV; > + goto fail_restore_config; > + } The above write/read sequence can be made conditional. If (config_orig & 0x67) =3D=3D TMP103_CONFIG, there is no need to updat= e the configuration register. Also, I just realized that the current code won't work: The FH and FL b= its are not controlled by you, but by the chip. So the verification (as wri= tten) will fail if you have an alarm condition. I would suggest to drop the verification entirely; I don't think it adds much if any value. At the very least, you'll have to mask the result against 0x67. Actually, I would suggest to drop config_orig entirely. Then you can use regmap_update_bits() to set the configuration. Also, you can then u= se devm_hwmon_device_register_with_groups(), and drop the remove function as well. > + tmp103->last_update =3D jiffies - HZ; No longer needed. > + mutex_init(&tmp103->lock); > + > + hwmon_dev =3D hwmon_device_register_with_groups(dev, client->name, > + tmp103, tmp103_groups); > + if (IS_ERR(hwmon_dev)) { > + dev_dbg(dev, "unable to register hwmon device\n"); > + status =3D PTR_ERR(hwmon_dev); > + goto fail_restore_config; > + } > + tmp103->hwmon_dev =3D hwmon_dev; > + > + return 0; > + > +fail_restore_config: > + regmap_write(tmp103->regmap, TMP103_CONF_REG, tmp103->config_orig); > +fail_init_regmap: > + regmap_exit(tmp103->regmap); This is not needed since you used the devm_ initialization function abo= ve. In fact, it is likely wrong. If you drop restoring the configuration register, the entire sequence c= an become hwmon_dev =3D devm_hwmon_device_register_with_groups(dev, client->name= , tmp103, tmp103_groups); return PTR_ERR_OR_ZERO(hwmon_dev); > + return ret; > +} > + > +static int tmp103_remove(struct i2c_client *client) > +{ > + struct tmp103 *tmp103 =3D i2c_get_clientdata(client); > + int ret; > + > + ret =3D regmap_write(tmp103->regmap, TMP103_CONF_REG, > + tmp103->config_orig); Only needed if (config_orig & 0x67) !=3D TMP103_CONFIG. > + if (ret < 0) > + dev_err(&client->dev, "error writing config register\n"); > + I'd suggest to drop the error message. It does not really matter, after all, and is quite unlikely. > + hwmon_device_unregister(tmp103->hwmon_dev); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int tmp103_suspend(struct device *dev) > +{ > + struct i2c_client *client =3D to_i2c_client(dev); > + struct tmp103 *tmp103 =3D i2c_get_clientdata(client); > + unsigned int config; > + int ret; > + > + ret =3D regmap_read(tmp103->regmap, TMP103_CONF_REG, &config); > + if (ret < 0) > + return ret; > + > + config &=3D ~TMP103_CONF_SD; > + return regmap_write(tmp103->regmap, TMP103_CONF_REG, config); You can use regmap_update_bits() here. > +} > + > +static int tmp103_resume(struct device *dev) > +{ > + struct i2c_client *client =3D to_i2c_client(dev); > + struct tmp103 *tmp103 =3D i2c_get_clientdata(client); > + unsigned int config; > + int ret; > + > + ret =3D regmap_read(tmp103->regmap, TMP103_CONF_REG, &config); > + if (ret < 0) > + return ret; > + > + config |=3D TMP103_CONF_SD; > + return regmap_write(tmp103->regmap, TMP103_CONF_REG, config); You can use regmap_update_bits() here. > +} > + > +static const struct dev_pm_ops tmp103_dev_pm_ops =3D { > + .suspend =3D tmp103_suspend, > + .resume =3D tmp103_resume, > +}; > + > +#define TMP103_DEV_PM_OPS (&tmp103_dev_pm_ops) > +#else > +#define TMP103_DEV_PM_OPS NULL > +#endif /* CONFIG_PM */ > + > +static const struct i2c_device_id tmp103_id[] =3D { > + { DRIVER_NAME, 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, tmp103_id); > + > +static struct i2c_driver tmp103_driver =3D { > + .driver =3D { > + .name =3D DRIVER_NAME, > + .pm =3D TMP103_DEV_PM_OPS, > + }, > + .probe =3D tmp103_probe, > + .remove =3D tmp103_remove, > + .id_table =3D tmp103_id, > +}; > + > +module_i2c_driver(tmp103_driver); > + > +MODULE_AUTHOR("Heiko Schocher "); > +MODULE_DESCRIPTION("Texas Instruments TMP103 temperature sensor driv= er"); > +MODULE_LICENSE("GPL"); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html