lkml.org 
[lkml]   [2010]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] hmc6352: Add driver for the HMC6352 compass
    Hi Alan,

    On Thu, 17 Jun 2010 14:14:40 +0100, Alan Cox wrote:
    > Ok - cleaning up the sysfs API to match the docs also cleans up the code
    > a tiny bit more so do it that way
    >
    > --
    > hmc6352: Add driver for the HMC6352 compass
    >
    > From: Kalhan Trisal <kalhan.trisal@intel.com>
    >
    > This driver will report the heading values in degrees to the sysfs interface.
    > The values returned are headings . e.g. 245.6
    >
    > Alan: Cleanups requested now all folded in and a sysfs description to keep
    > Andrew happy. The sysfs description now resembles hwmon.
    >
    > Signed-off-by: Kalhan Trisal <kalhan.trisal@intel.com>
    > Signed-off-by: Alan Cox <alan@linux.intel.com>

    Code looks nicer, however I fear you introduced a bug in the process...

    > ---
    >
    > .../ABI/testing/sysfs-bus-i2c-devices-hm6352 | 21 +++
    > drivers/misc/Kconfig | 7 +
    > drivers/misc/Makefile | 1
    > drivers/misc/hmc6352.c | 165 ++++++++++++++++++++
    > 4 files changed, 194 insertions(+), 0 deletions(-)
    > create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
    > create mode 100644 drivers/misc/hmc6352.c
    >
    >
    > diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
    > new file mode 100644
    > index 0000000..feb2e4a
    > --- /dev/null
    > +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
    > @@ -0,0 +1,21 @@
    > +Where: /sys/bus/i2c/devices/.../heading0_input
    > +Date: April 2010
    > +Kernel Version: 2.6.36?
    > +Contact: alan.cox@intel.com
    > +Description: Reports the current heading from the compass as a floating
    > + point value in degrees.
    > +
    > +Where: /sys/bus/i2c/devices/.../power_state
    > +Date: April 2010
    > +Kernel Version: 2.6.36?
    > +Contact: alan.cox@intel.com
    > +Description: Sets the power state of the device. 0 sets the device into
    > + sleep mode, 1 wakes it up.
    > +
    > +Where: /sys/bus/i2c/devices/.../calibration
    > +Date: April 2010
    > +Kernel Version: 2.6.36?
    > +Contact: alan.cox@intel.com
    > +Description: Sets the calibration on or off (1 = on, 0 = off). See the
    > + chip data sheet.
    > +
    > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
    > index 26386a9..9e825cb 100644
    > --- a/drivers/misc/Kconfig
    > +++ b/drivers/misc/Kconfig
    > @@ -304,6 +304,13 @@ config SENSORS_TSL2550
    > This driver can also be built as a module. If so, the module
    > will be called tsl2550.
    >
    > +config HMC6352
    > + tristate "Honeywell HMC6352 compass"
    > + depends on I2C
    > + help
    > + This driver provides support for the Honeywell HMC6352 compass,
    > + providing configuration and heading data via sysfs.
    > +
    > config EP93XX_PWM
    > tristate "EP93xx PWM support"
    > depends on ARCH_EP93XX
    > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
    > index 6ed06a1..48597df 100644
    > --- a/drivers/misc/Makefile
    > +++ b/drivers/misc/Makefile
    > @@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682) += ds1682.o
    > obj-$(CONFIG_TI_DAC7512) += ti_dac7512.o
    > obj-$(CONFIG_C2PORT) += c2port/
    > obj-$(CONFIG_IWMC3200TOP) += iwmc3200top/
    > +obj-$(CONFIG_HMC6352) += hmc6352.o
    > obj-y += eeprom/
    > obj-y += cb710/
    > obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
    > diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
    > new file mode 100644
    > index 0000000..63d9519
    > --- /dev/null
    > +++ b/drivers/misc/hmc6352.c
    > @@ -0,0 +1,165 @@
    > +/*
    > + * hmc6352.c - Honeywell Compass Driver
    > + *
    > + * Copyright (C) 2009 Intel Corp
    > + *
    > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    > + *
    > + * 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; version 2 of the License.
    > + *
    > + * 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.
    > + *
    > + * You should have received a copy of the GNU General Public License along
    > + * with this program; if not, write to the Free Software Foundation, Inc.,
    > + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
    > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    > + *
    > + */
    > +
    > +#include <linux/module.h>
    > +#include <linux/init.h>
    > +#include <linux/slab.h>
    > +#include <linux/i2c.h>
    > +#include <linux/err.h>
    > +#include <linux/delay.h>
    > +#include <linux/sysfs.h>
    > +
    > +static DEFINE_MUTEX(compass_mutex);
    > +
    > +static int compass_command(struct i2c_client *c, u8 cmd)
    > +{
    > + int ret = i2c_master_send(c, &cmd, 1);
    > + if (ret < 0)
    > + dev_warn(&c->dev, "command '%c' failed.\n", cmd);
    > + return ret;
    > +}
    > +
    > +static int compass_store(struct device *dev, const char *buf, size_t count,
    > + const char *map)
    > +{
    > + struct i2c_client *c = to_i2c_client(dev);
    > + int ret;
    > + unsigned long val;
    > +
    > + if (strict_strtoul(buf, 10, &val))
    > + return -EINVAL;
    > + if (val >= strlen(map))
    > + return -EINVAL;
    > + mutex_lock(&compass_mutex);
    > + ret = compass_command(c, map[val]);
    > + mutex_unlock(&compass_mutex);
    > + return ret;
    > +}
    > +
    > +static ssize_t compass_calibration_store(struct device *dev,
    > + struct device_attribute *attr, const char *buf, size_t count)
    > +{
    > + return compass_store(dev, buf, count, "EC");
    > +}
    > +
    > +static ssize_t compass_power_mode_store(struct device *dev,
    > + struct device_attribute *attr, const char *buf, size_t count)
    > +{
    > + return compass_store(dev, buf, count, "SW");
    > +}

    These functions are supposed to return count on success. Instead, you
    are returning 1.

    > +
    > +static ssize_t compass_heading_data_show(struct device *dev,
    > + struct device_attribute *attr, char *buf)
    > +{
    > +

    Extra blank line.

    > + struct i2c_client *client = to_i2c_client(dev);
    > + unsigned char i2c_data[2];
    > + unsigned int ret;
    > +
    > + mutex_lock(&compass_mutex);
    > + ret = compass_command(client, 'A');
    > + if (ret != 1) {
    > + mutex_unlock(&compass_mutex);
    > + return ret;
    > + }
    > + msleep(10); /* sending 'A' cmd we need to wait for 7-10 millisecs */
    > + ret = i2c_master_recv(client, i2c_data, 2);
    > + mutex_unlock(&compass_mutex);
    > + if (ret != 1) {
    > + dev_warn(dev, "i2c read data cmd failed\n");
    > + return ret;
    > + }
    > + ret = (i2c_data[0] << 8) | i2c_data[1];
    > + return sprintf(buf, "%d.%d\n", ret/10, ret%10);
    > +}
    > +
    > +
    > +static DEVICE_ATTR(heading0_input, S_IRUGO, compass_heading_data_show, NULL);
    > +static DEVICE_ATTR(calibration, S_IWUSR, NULL, compass_calibration_store);
    > +static DEVICE_ATTR(power_state, S_IWUSR, NULL, compass_power_mode_store);
    > +
    > +static struct attribute *mid_att_compass[] = {
    > + &dev_attr_heading0_input.attr,
    > + &dev_attr_calibration.attr,
    > + &dev_attr_power_state.attr,
    > + NULL
    > +};
    > +
    > +static const struct attribute_group m_compass_gr = {
    > + .name = "hmc6352",
    > + .attrs = mid_att_compass
    > +};
    > +
    > +static int hmc6352_probe(struct i2c_client *client,
    > + const struct i2c_device_id *id)
    > +{
    > + int res;
    > +
    > + res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
    > + if (res) {
    > + dev_err(&client->dev, "device_create_file failed\n");
    > + return res;
    > + }
    > + dev_info(&client->dev, "%s HMC6352 compass chip found\n",
    > + client->name);
    > + return 0;
    > +}
    > +
    > +static int hmc6352_remove(struct i2c_client *client)
    > +{
    > + sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
    > + return 0;
    > +}
    > +
    > +static struct i2c_device_id hmc6352_id[] = {
    > + { "hmc6352", 0 },
    > + { }
    > +};
    > +
    > +MODULE_DEVICE_TABLE(i2c, hmc6352_id);
    > +
    > +static struct i2c_driver hmc6352_driver = {
    > + .driver = {
    > + .name = "hmc6352",
    > + },
    > + .probe = hmc6352_probe,
    > + .remove = hmc6352_remove,
    > + .id_table = hmc6352_id,
    > +};
    > +
    > +static int __init sensor_hmc6352_init(void)
    > +{
    > + return i2c_add_driver(&hmc6352_driver);
    > +}
    > +
    > +static void __exit sensor_hmc6352_exit(void)
    > +{
    > + i2c_del_driver(&hmc6352_driver);
    > +}
    > +
    > +module_init(sensor_hmc6352_init);
    > +module_exit(sensor_hmc6352_exit);
    > +
    > +MODULE_AUTHOR("Kalhan Trisal <kalhan.trisal@intel.com");
    > +MODULE_DESCRIPTION("hmc6352 Compass Driver");
    > +MODULE_LICENSE("GPL v2");

    All the rest looks OK, feel free to add:

    Reviewed-by: Jean Delvare <khali@linux-fr.org>

    --
    Jean Delvare


    \
     
     \ /
      Last update: 2010-06-17 16:55    [W:0.071 / U:0.036 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site