RE: [PATCH] hwmon: (ms5637) Add Measurement Specialties MS5637 support

From: Markezana, William
Date: Wed Sep 11 2013 - 01:48:32 EST


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@xxxxxxxxx] De la part de Guenter Roeck
Envoyé : mardi 10 septembre 2013 17:21
À : Markezana, William
Cc : khali@xxxxxxxxxxxx; lm-sensors@xxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxx>
>
> hwmon: (ms5637) Add Measurement Specialties MS5637support
> Signed-off-by: William Markezana <william.markezana@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxxxxx>");
> +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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/