Date:	Wed, 11 Sep 2013 07:48:03 +0200
From:	"Markezana, William" <William.Markezana@...s-spec.com>
To:	"Guenter Roeck" <linux@...ck-us.net>
Cc:	<khali@...ux-fr.org>, <lm-sensors@...sensors.org>,
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] hwmon: (ms5637) Add Measurement Specialties MS5637 support

Hi Guenter,

Thank you for your feedback, I will rewrite the drivers for IIO then.

Best regards,

William MARKEZANA
Direct : + 33 (0) 582 082 286
http://www.meas-spec.com


-----Message d'origine-----
De : Guenter Roeck [mailto:groeck7@...il.com] De la part de Guenter Roeck
Envoyé : mardi 10 septembre 2013 17:21
À : Markezana, William
Cc : khali@...ux-fr.org; lm-sensors@...sensors.org; linux-kernel@...r.kernel.org
Objet : Re: [PATCH] hwmon: (ms5637) Add Measurement Specialties MS5637 support

On Tue, Sep 10, 2013 at 05:09:57PM +0200, Markezana, William wrote:
> From: William Markezana <william.markezana@...s-spec.com>
> 
> hwmon: (ms5637) Add Measurement Specialties MS5637support
> Signed-off-by: William Markezana <william.markezana@...s-spec.com>

Hi William,

"pressure" is hardly a hardware monitoring attribute. It might make more sense to add your driver to the iio subsystem, which already supports at least one other pressure sensor.

Thanks,
Guenter

> ---
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 
> 55973cd..c4f1c8e 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -954,6 +954,16 @@ config SENSORS_MCP3021
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called mcp3021.
>  
> +config SENSORS_MS5637
> +	tristate "Measurement Specialties MS5637 pressure sensors"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the Measurement Specialties
> +	  MS5637 pressure sensors.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called ms5637.
> +
>  config SENSORS_NCT6775
>  	tristate "Nuvoton NCT6775F and compatibles"
>  	depends on !PPC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 
> ec7cde0..5d8f699 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -110,6 +110,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
>  obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
>  obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>  obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
> +obj-$(CONFIG_SENSORS_MS5637)	+= ms5637.o
>  obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
>  obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
> diff --git a/drivers/hwmon/ms5637.c b/drivers/hwmon/ms5637.c new file 
> mode 100644 index 0000000..fe2c2df
> --- /dev/null
> +++ b/drivers/hwmon/ms5637.c
> @@ -0,0 +1,302 @@
> +/*
> + * Measurement Specialties MS5637 pressure and temperature sensor 
> +driver
> + *
> + * Copyright (C) 2013 William Markezana 
> +<william.markezana@...s-spec.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>
> +#include <linux/delay.h>
> +
> +/* MS5637 Commands */
> +#define MS5637_CONVERT_D1_OSR_4096	(0x48)
> +#define MS5637_CONVERT_D2_OSR_4096	(0x58)
> +#define MS5637_ADC_READ				(0x00)
> +#define MS5637_PROM_READ			(0xA0)
> +
> +struct ms5637 {
> +	struct device *hwmon_dev;
> +	struct mutex lock;
> +	bool valid;
> +	unsigned long last_update;
> +	int temperature;
> +	int pressure;
> +	unsigned short calibration_words[6];
> +	bool got_calibration_words;
> +};
> +
> +static int ms5637_get_calibration_word(struct i2c_client *client,
> +	unsigned char address, unsigned short *word) {
> +	int ret = 0;
> +	ret = i2c_smbus_read_word_swapped(client,
> +		MS5637_PROM_READ + (address << 1));
> +	if (ret < 0)
> +		return ret;
> +	*word = (unsigned short)ret & 0xFFFF;
> +	return 0;
> +}
> +
> +static int ms5637_fill_calibration_words(struct i2c_client *client) {
> +	int i, ret = 0;
> +	struct ms5637 *ms5637 = i2c_get_clientdata(client);
> +
> +	for (i = 0; i < 6; i++) {
> +		ret = ms5637_get_calibration_word(client, i+1,
> +			&ms5637->calibration_words[i]);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"unable to get calibration word at address %d\n",
> +				i+1);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int ms5637_get_adc_value(struct i2c_client *client,
> +	unsigned int *adc_value)
> +{
> +	int ret = 0;
> +	unsigned char buf[3];
> +	ret = i2c_smbus_read_i2c_block_data(client, MS5637_ADC_READ, 3, buf);
> +	if (ret < 0)
> +		return ret;
> +	*adc_value = (buf[0] << 16) + (buf[1] << 8) + buf[2];
> +	return 0;
> +}
> +
> +static int ms5637_get_conversion_result(struct i2c_client *client,
> +	unsigned char command, unsigned int *adc_value) {
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte(client, command);
> +	if (ret < 0)
> +		return ret;
> +	msleep(9);
> +	ret = ms5637_get_adc_value(client, adc_value);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}
> +
> +static int ms5637_update_measurements(struct i2c_client *client) {
> +	int ret = 0;
> +	unsigned int d1, d2;
> +	int dt, temp, p;
> +	long long int off, sens;
> +	struct ms5637 *ms5637 = i2c_get_clientdata(client);
> +
> +	mutex_lock(&ms5637->lock);
> +
> +	if (time_after(jiffies, ms5637->last_update + HZ / 2) ||
> +	    !ms5637->valid) {
> +
> +		if (!ms5637->got_calibration_words)	{
> +			ret = ms5637_fill_calibration_words(client);
> +			if (ret < 0) {
> +				dev_err(&client->dev,
> +					"unable to get calibration words\n");
> +				goto out;
> +			}
> +
> +			ms5637->got_calibration_words = true;
> +		}
> +
> +		ret = ms5637_get_conversion_result(client,
> +			MS5637_CONVERT_D1_OSR_4096, &d1);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"unable to get adc conversion of D1_OSR_4096\n");
> +			goto out;
> +		}
> +
> +		ret = ms5637_get_conversion_result(client,
> +			MS5637_CONVERT_D2_OSR_4096, &d2);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"unable to get adc conversion of D2_OSR_4096\n");
> +			goto out;
> +		}
> +
> +		dt = d2 - (int)ms5637->calibration_words[4] * 256;
> +		temp = 20000 + div_s64((long long int)dt *
> +			(long long int)ms5637->calibration_words[5], 838861);
> +
> +		off = (long long int)ms5637->calibration_words[1] * 131072 +
> +			div_s64((long long int)ms5637->calibration_words[3] *
> +			(long long int)dt, 64);
> +		sens = (long long int)ms5637->calibration_words[0] * 65536 +
> +			div_s64((long long int)ms5637->calibration_words[2] *
> +			(long long int)dt, 128);
> +		p = (int)div_s64((div_s64((long long int)d1 * sens, 2097152) -
> +			off), 3277);
> +
> +		ms5637->temperature = temp;
> +		ms5637->pressure = p;
> +		ms5637->last_update = jiffies;
> +		ms5637->valid = true;
> +	}
> +out:
> +	mutex_unlock(&ms5637->lock);
> +
> +	return ret >= 0 ? 0 : ret;
> +}
> +
> +static ssize_t ms5637_show_temperature(struct device *dev,
> +				      struct device_attribute *attr, char *buf) {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ms5637 *ms5637 = i2c_get_clientdata(client);
> +	int ret = ms5637_update_measurements(client);
> +	if (ret < 0)
> +		return ret;
> +	return sprintf(buf, "%d\n", ms5637->temperature); }
> +
> +static ssize_t ms5637_show_pressure(struct device *dev,
> +				   struct device_attribute *attr, char *buf) {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ms5637 *ms5637 = i2c_get_clientdata(client);
> +	int ret = ms5637_update_measurements(client);
> +	if (ret < 0)
> +		return ret;
> +	return sprintf(buf, "%d\n", ms5637->pressure); }
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
> +			  ms5637_show_temperature, NULL, 0); static 
> +SENSOR_DEVICE_ATTR(pressure1_input, S_IRUGO,
> +			  ms5637_show_pressure, NULL, 0);
> +
> +static struct attribute *ms5637_attributes[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_pressure1_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ms5637_group = {
> +	.attrs = ms5637_attributes,
> +};
> +
> +static int ms5637_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id) {
> +	struct ms5637 *ms5637;
> +	int err;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +					I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> +		dev_err(&client->dev,
> +			"adapter does not support SMBus word transactions\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				    I2C_FUNC_SMBUS_WRITE_BYTE)) {
> +		dev_err(&client->dev,
> +			"adapter does not support SMBus byte transactions\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				    I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_err(&client->dev,
> +			"adapter does not support SMBus block transactions\n");
> +		return -ENODEV;
> +	}
> +
> +	ms5637 = devm_kzalloc(&client->dev, sizeof(*ms5637), GFP_KERNEL);
> +	if (!ms5637)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, ms5637);
> +
> +	mutex_init(&ms5637->lock);
> +
> +	err = sysfs_create_group(&client->dev.kobj, &ms5637_group);
> +	if (err) {
> +		dev_dbg(&client->dev, "could not create sysfs files\n");
> +		return err;
> +	}
> +	ms5637->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(ms5637->hwmon_dev)) {
> +		dev_dbg(&client->dev, "unable to register hwmon device\n");
> +		err = PTR_ERR(ms5637->hwmon_dev);
> +		goto error;
> +	}
> +
> +	mutex_lock(&ms5637->lock);
> +	err = ms5637_fill_calibration_words(client);
> +	if (err >= 0)
> +		ms5637->got_calibration_words = true;
> +	mutex_unlock(&ms5637->lock);
> +
> +	dev_info(&client->dev, "initialized\n");
> +
> +	return 0;
> +
> +error:
> +	sysfs_remove_group(&client->dev.kobj, &ms5637_group);
> +	return err;
> +}
> +
> +static int ms5637_remove(struct i2c_client *client) {
> +	struct ms5637 *ms5637 = i2c_get_clientdata(client);
> +
> +	hwmon_device_unregister(ms5637->hwmon_dev);
> +	sysfs_remove_group(&client->dev.kobj, &ms5637_group);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ms5637_id[] = {
> +	{ "ms5637", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ms5637_id);
> +
> +static struct i2c_driver ms5637_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "ms5637",
> +	},
> +	.probe       = ms5637_probe,
> +	.remove      = ms5637_remove,
> +	.id_table    = ms5637_id,
> +};
> +
> +module_i2c_driver(ms5637_driver);
> +
> +MODULE_AUTHOR("William Markezana <william.markezana@...s-spec.com>");
> +MODULE_DESCRIPTION("MEAS MS5637 pressure and temperature sensor 
> +driver"); MODULE_LICENSE("GPL");
--
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