Message-ID: <53997544.5090106@denx.de>
Date:	Thu, 12 Jun 2014 11:39:16 +0200
From:	Heiko Schocher <hs@...x.de>
To:	Guenter Roeck <linux@...ck-us.net>
CC:	lm-sensors@...sensors.org, Jean Delvare <khali@...ux-fr.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/13] hwmon: Driver for TI TMP103 temperature sensor

Hello Guenter,

Am 11.06.2014 19:11, schrieb Guenter Roeck:
> On 06/10/2014 11:28 PM, 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 <hs@...x.de>
>> Cc: Jean Delvare <khali@...ux-fr.org>
>> Cc: Guenter Roeck <linux@...ck-us.net>
>> Cc: linux-kernel@...r.kernel.org
>
> Hi Heiko,
>
> You don't need those Cc: in the header.

Ok, I remove them.

> Why patch 09/13 ? I'd expect to see 12 more patches in this series,

Hups, sorry, my mistake, there is just this patch...

> but there is only one (even on kernel.org). If you sent those
> other patches to various other mailing lists, everyone will wonder
> where the remaining patches are. If there is just one patch,
> it is just confusing.
>
>> ---
>> Documentation/devicetree/bindings/hwmon/tmp103 | 30 +++
>> drivers/hwmon/Kconfig | 10 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/tmp103.c | 281 +++++++++++++++++++++++++
>> 4 files changed, 322 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/hwmon/tmp103
>> create mode 100644 drivers/hwmon/tmp103.c
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/tmp103 b/Documentation/devicetree/bindings/hwmon/tmp103
>> new file mode 100644
>> index 0000000..36d7b36
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/tmp103
>
> Should be Documentation/hwmon/tmp103.

Hmm.. I did this first there, but "scripts/checkpatch.pl"
warns (if using a DT based board with compatible = "ti,tmp103"):

WARNING: DT compatible string "ti,tmp103" appears un-documented -- check ./Documentation/devicetree/bindings/
#324: FILE: arch/arm/boot/dts/imx6qdl-aristainetos.dtsi:107:
+               compatible = "ti,tmp103";

>> @@ -0,0 +1,30 @@
>> +Kernel driver tmp103
>> +====================
>> +
>> +Supported chips:
>> + * Texas Instruments TMP103
>> + Prefix: 'tmp103'
>> + Addresses scanned: none
>> + Product info and datasheet: http://www.ti.com/product/tmp103
>> +
>> +Author:
>> + Heiko Schocher <hs@...x.de>
>> +
>> +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°C. The TMP103 is specified for
>> +operation over a temperature range of –40°C to +125°C.
>> +
>> +Resolution: 8 Bits
>> +Accuracy: ±1°C Typ (–10°C to +100°C)
>> +
>> +The driver provides the common sysfs-interface for temperatures (see
>> +Documentation/hwmon/sysfs-interface under Temperatures).
>> +
>> +Required node properties:
>> +- compatible: manufacturer and chip name
>> + "ti,tmp103"
>> +- reg: I2C bus address of the device
>
> You don't need this part, and it is not really correct (there are
> other ways to instantiate the device, devicetree is just one of them).

Ok.

> I would suggest to refer to Documentation/i2c/instantiating-devices
> instead. It might also make sense to update
> Documentation/devicetree/bindings/i2c/trivial-devices.txt.

Ah, ok, good idea, this removes also the checkpatch warning, thanks!
Changed.

> Also, whenever you touch any of the the dt files, you need to copy
> the DT maintainers. scripts/get_maintainer.pl will tell you who needs
> to be copied.

Ok.

[...]
>> 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)+= smsc47m192.o
>> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
>> obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
>> obj-$(CONFIG_SENSORS_TMP102) += tmp102.o
>> +obj-$(CONFIG_SENSORS_TMP103) += tmp103.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/tmp103.c b/drivers/hwmon/tmp103.c
>> new file mode 100644
>> index 0000000..0bd33d6
>> --- /dev/null
>> +++ b/drivers/hwmon/tmp103.c
>> @@ -0,0 +1,281 @@
>> +/*
>> + * Texas Instruments TMP103 SMBus temperature sensor driver
>> + * Copyright (C) 2014 Heiko Schocher <hs@...x.de>
>> + *
>> + * Based on:
>> + * Texas Instruments TMP102 SMBus temperature sensor driver
>> + *
>> + * Copyright (C) 2010 Steven King <sfking@...dc.com>
>> + *
>> + * 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 <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/i2c.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/device.h>
>> +#include <linux/jiffies.h>
>> +
>> +#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)
>> +
>> +struct tmp103 {
>> + struct device *hwmon_dev;
>> + struct mutex lock;
>> + u16 config_orig;
>> + unsigned long last_update;
>> + int temp[3];
>> +};
>> +
>> +static inline int tmp103_reg_to_mC(s8 val)
>> +{
>> + return val * 1000;
>> +}
>> +
>> +static inline u8 tmp103_mC_to_reg(int val)
>> +{
>> + return val / 1000;
>
> DIV_ROUND_CLOSEST() ?
>
> No camelCase, please.

Ok ... hmm, wondering that checkpatch.pl not claimed this.
removed.

>> +}
>> +
>> +static const u8 tmp103_reg[] = {
>> + TMP103_TEMP_REG,
>> + TMP103_TLOW_REG,
>> + TMP103_THIGH_REG,
>> +};
>> +
>> +static struct tmp103 *tmp103_update_device(struct i2c_client *client)
>> +{
>> + struct tmp103 *tmp103 = i2c_get_clientdata(client);
>> +
>> + mutex_lock(&tmp103->lock);
>> + if (time_after(jiffies, tmp103->last_update + HZ / 3)) {
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(tmp103->temp); ++i) {
>> + int status = i2c_smbus_read_byte_data(client,
>> + tmp103_reg[i]);
>> + if (status > -1)
>> + tmp103->temp[i] = tmp103_reg_to_mC(status);
>
> I understand this is from the tmp102 driver, but it is kind of unusual
> because it does not report the error back to the user. Please consider
> doing that, ie return PTR_ERR(status) on error and check/return the error
> from the calling functions. Irrelevant if you use regmap (see below).

you mean ERR_PTR() ?
Ok, add this.

>> + }
>> + tmp103->last_update = jiffies;
>> + }
>> + mutex_unlock(&tmp103->lock);
>> + return tmp103;
>> +}
>
> Overall you might consider dropping the update function and using regmap instead.
> It would be an excellent fit for this driver; while it doesn't do timed caching
> it supports per-register caching which is ultimately more useful anyway.
> Some of the drivers in the hwmon directory use it, so you could use that
> as example. Not mandatory, though; just a suggestion.

Could you give me an example?

>> +
>> +static ssize_t tmp103_show_temp(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
>> + struct tmp103 *tmp103 = tmp103_update_device(to_i2c_client(dev));
>> +
>> + return sprintf(buf, "%d\n", tmp103->temp[sda->index]);
>> +}
>> +
>> +static ssize_t tmp103_set_temp(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct tmp103 *tmp103 = i2c_get_clientdata(client);
>> + long val;
>> + int status;
>> +
>> + if (kstrtol(buf, 10, &val) < 0)
>> + return -EINVAL;
>> + val = clamp_val(val, -55000, 128000);
>
> Max should be 127000.

Ok, change this.

>> +
>> + mutex_lock(&tmp103->lock);
>> + tmp103->temp[sda->index] = val;
>
> You can not do that since it is the non-rounded value. You have to convert to
> the register value, then convert back to the display value.
>
> If you use regmap, the problem goes away, since you would not cache converted
> data but convert in the show function. Alternatively, you could store register
> values and convert in the show function.

I take a look at this.

>> + status = i2c_smbus_write_byte_data(client, tmp103_reg[sda->index],
>> + tmp103_mC_to_reg(val));
>
> Please make sure that continuation lines match the '(' in the line above.
> Alignments are inconsistent, otherwise I would not mention it.
> checkpatch --strict tells you which ones are misaligned.

Ah, did a checkpatch but without the "--strict" option !

I fix this globally.

>> + mutex_unlock(&tmp103->lock);
>> + return status ? : count;
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp103_show_temp, NULL , 0);
>> +
> If you use regmap you can store the register directly here and you would not
> need tmp103_reg[].

Ok.

>> +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, tmp103_show_temp,
>> + tmp103_set_temp, 1);
>> +
> Per the datasheet this should be temp1_min. The register definition is different to TMP102.

changed.

>> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp103_show_temp,
>> + tmp103_set_temp, 2);
>> +
>> +static struct attribute *tmp103_attributes[] = {
>> + &sensor_dev_attr_temp1_input.dev_attr.attr,
>> + &sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
>> + &sensor_dev_attr_temp1_max.dev_attr.attr,
>
> You might also consider adding temp1_min_alarm and temp1_max_alarm to inform
> user space if limits are exceeded. The status is reported with the FL and FH
> bits in the configuration register.

Add this if I find time, or in an another patch...

>> + NULL
>> +};
>> +
>> +static const struct attribute_group tmp103_attr_group = {
>> + .attrs = tmp103_attributes,
>> +};
>> +
> Please use the ATTRIBUTE_GROUPS() macro.

Ok.

>> +#define TMP103_CONFIG (TMP103_CONF_CR1 | TMP103_CONF_M1)
>> +#define TMP103_CONFIG_RD_ONLY (TMP103_CONF_CR1 | TMP103_CONF_M1)
>
> Latter define is not used as far as I can see. Since it is the same
> it is unnecessary anyway.

removed.

>> +
>> +static int tmp103_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct tmp103 *tmp103;
>> + int status;
>> +
>> + if (!i2c_check_functionality(client->adapter,
>> + I2C_FUNC_SMBUS_WORD_DATA)) {
>> + dev_err(&client->dev,
>> + "adapter doesn't support SMBus word transactions\n");
>
> Good that the chip doesn't support any ;-). Check for byte transactions instead.

Heh... changed.

>> + return -ENODEV;
>> + }
>> +
>> + tmp103 = devm_kzalloc(&client->dev, sizeof(*tmp103), GFP_KERNEL);
>> + if (!tmp103)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(client, tmp103);
>> +
>> + status = i2c_smbus_read_byte_data(client, TMP103_CONF_REG);
>> + if (status < 0) {
>> + dev_err(&client->dev, "error reading config register\n");
>> + return status;
>> + }
>> + tmp103->config_orig = status;
>
> You don't use config_orig outside the probe function,
> so storing it in private data doesn't add any value.
> Alternatively, you could restore the original configuration on exit.

I add "restoring it on exit."

>> + status = i2c_smbus_write_byte_data(client, TMP103_CONF_REG,
>> + TMP103_CONFIG);
>> + if (status < 0) {
>> + dev_err(&client->dev, "error writing config register\n");
>> + goto fail_restore_config;
>> + }
>> + status = i2c_smbus_read_byte_data(client, TMP103_CONF_REG);
>> + if (status < 0) {
>> + dev_err(&client->dev, "error reading config register\n");
>> + goto fail_restore_config;
>> + }
>> + if (status != TMP103_CONFIG) {
>> + dev_err(&client->dev, "config settings did not stick\n");
>> + status = -ENODEV;
>> + goto fail_restore_config;
>> + }
>> + tmp103->last_update = jiffies - HZ;
>> + mutex_init(&tmp103->lock);
>> +
>> + status = sysfs_create_group(&client->dev.kobj, &tmp103_attr_group);
>> + if (status) {
>> + dev_dbg(&client->dev, "could not create sysfs files\n");
>> + goto fail_restore_config;
>> + }
>> + tmp103->hwmon_dev = hwmon_device_register(&client->dev);
>> + if (IS_ERR(tmp103->hwmon_dev)) {
>> + dev_dbg(&client->dev, "unable to register hwmon device\n");
>> + status = PTR_ERR(tmp103->hwmon_dev);
>> + goto fail_remove_sysfs;
>> + }
>
> Please use devm_hwmon_device_register_with_groups().

Ok.

>> +
>> + dev_info(&client->dev, "initialized\n");
>
> Please drop this message.

Ok.

>> +
>> + return 0;
>> +
>> +fail_remove_sysfs:
>> + sysfs_remove_group(&client->dev.kobj, &tmp103_attr_group);
>> +fail_restore_config:
>> + i2c_smbus_write_word_swapped(client, TMP103_CONF_REG,
>> + tmp103->config_orig);
>
> i2c_smbus_write_byte_data() ?

Yes, changed!

>> + return status;
>> +}
>> +
>> +static int tmp103_remove(struct i2c_client *client)
>> +{
>> + struct tmp103 *tmp103 = i2c_get_clientdata(client);
>> +
>> + hwmon_device_unregister(tmp103->hwmon_dev);
>> + sysfs_remove_group(&client->dev.kobj, &tmp103_attr_group);
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int tmp103_suspend(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + int config;
>> +
>> + config = i2c_smbus_read_word_swapped(client, TMP103_CONF_REG);
>
> byte
>
>> + if (config < 0)
>> + return config;
>> +
>> + config &= ~TMP103_CONF_SD;
>> + return i2c_smbus_write_word_swapped(client, TMP103_CONF_REG, config);
>
> byte
>
>> +}
>> +
>> +static int tmp103_resume(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + int config;
>> +
>> + config = i2c_smbus_read_word_swapped(client, TMP103_CONF_REG);
>
> byte
>
>> + if (config < 0)
>> + return config;
>> +
>> + config |= TMP103_CONF_SD;
>> + return i2c_smbus_write_word_swapped(client, TMP103_CONF_REG, config);
>
> byte

changed.

>> +}
>> +
>> +static const struct dev_pm_ops tmp103_dev_pm_ops = {
>> + .suspend = tmp103_suspend,
>> + .resume = 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[] = {
>> + { DRIVER_NAME, 0 },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tmp103_id);
>> +
>> +static struct i2c_driver tmp103_driver = {
>> + .driver.name = DRIVER_NAME,
>> + .driver.pm = TMP103_DEV_PM_OPS,
>
> .driver = {
> .name = DRIVER_NAME,
> .pm = TMP103_DEV_PM_OPS,
> },
>
> would be a bit nicer. I won't mandate it, though.

changed.

>> + .probe = tmp103_probe,
>> + .remove = tmp103_remove,
>> + .id_table = tmp103_id,
>> +};
>> +
>> +module_i2c_driver(tmp103_driver);
>> +
>> +MODULE_AUTHOR("Heiko Schocher <hs@...x.de>");
>> +MODULE_DESCRIPTION("Texas Instruments TMP103 temperature sensor driver");
>> +MODULE_LICENSE("GPL");

Thanks for this great review!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists