Hi Andrew, Please note: [email protected] doesn't exist.
Here's my review, as a complement to Andrew's. On Thu, 26 Aug 2010 18:31:57 -0700, [email protected], [email protected] wrote: > From: Andrew Chew <[email protected]> > > This is for the Asahi Kasei's I2C compass sensor AK8975. > > Signed-off-by: Andrew Chew <[email protected]> > --- > drivers/misc/Kconfig | 8 + > drivers/misc/Makefile | 1 + > drivers/misc/akm8975.c | 670 > +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/akm8975.h | 87 ++++++ > 4 files changed, 766 insertions(+), 0 deletions(-) > create mode 100644 drivers/misc/akm8975.c > create mode 100644 include/linux/akm8975.h > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 26386a9..9bb3d03 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -304,6 +304,14 @@ config SENSORS_TSL2550 > This driver can also be built as a module. If so, the module > will be called tsl2550. > > +config SENSORS_AK8975 > + tristate "AK8975 compass support" > + default n > + depends on I2C > + help > + If you say yes here you get support for Asahi Kasei's > + orientation sensor AK8975. > + > config EP93XX_PWM > tristate "EP93xx PWM support" > depends on ARCH_EP93XX > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 6ed06a1..366791b 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP) += iwmc3200top/ > obj-y += eeprom/ > obj-y += cb710/ > obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o > +obj-$(CONFIG_SENSORS_AK8975) += akm8975.o CONFIG_COMPASS_AK8975 would be a better name IMHO, in anticipation of a compass subsystem being created someday. > diff --git a/drivers/misc/akm8975.c b/drivers/misc/akm8975.c > new file mode 100644 > index 0000000..17a096e > --- /dev/null > +++ b/drivers/misc/akm8975.c > @@ -0,0 +1,670 @@ > +/* drivers/misc/akm8975.c - akm8975 compass driver > + * > + * Copyright (C) 2007-2008 HTC Corporation. > + * Author: Hou-Kun Chen <[email protected]> > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * 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. > + * > + */ > + > +/* > + * Revised by AKM 2009/04/02 > + * Revised by Motorola 2010/05/27 > + * > + */ > + > +#include <linux/interrupt.h> > +#include <linux/i2c.h> > +#include <linux/slab.h> > +#include <linux/irq.h> Do you really need this one? > +#include <linux/miscdevice.h> > +#include <linux/gpio.h> You don't seem to need this. > +#include <linux/uaccess.h> > +#include <linux/delay.h> You don't need this? > +#include <linux/input.h> > +#include <linux/workqueue.h> > +#include <linux/freezer.h> What is this one being used for? > +#include <linux/akm8975.h> > + > +#define AK8975DRV_CALL_DBG 0 > +#if AK8975DRV_CALL_DBG > +#define FUNCDBG(msg) pr_err("%s:%s\n", __func__, msg); > +#else > +#define FUNCDBG(msg) > +#endif This tracing can probably go away once this driver is considered good for merging. > + > +#define AK8975DRV_DATA_DBG 0 > +#define MAX_FAILURE_COUNT 10 Not used anywhere? > + > +struct akm8975_data { > + struct i2c_client *this_client; "this_" makes the name longer without adding any value.. > + struct akm8975_platform_data *pdata; > + struct input_dev *input_dev; > + struct work_struct work; > + struct mutex flags_lock; > +}; > + > +/* > +* Because misc devices can not carry a pointer from driver register to > +* open, we keep this global. This limits the driver to a single instance. > +*/ > +struct akm8975_data *akmd_data; Should be static. > + > +static DECLARE_WAIT_QUEUE_HEAD(open_wq); > + > +static atomic_t open_flag; > + > +static short m_flag; > +static short a_flag; > +static short t_flag; > +static short mv_flag; > + > +static short akmd_delay; > + > +static ssize_t akm8975_show(struct device *dev, struct device_attribute > *attr, > + char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + return sprintf(buf, "%u\n", i2c_smbus_read_byte_data(client, > + AK8975_REG_CNTL)); Missing error handling. i2c_smbus_read_byte_data can return an error, which you want to pass to user-space as a proper error code, not a random negative value. > +} > +static ssize_t akm8975_store(struct device *dev, struct device_attribute > *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + unsigned long val; > + strict_strtoul(buf, 10, &val); > + if (val > 0xff) > + return -EINVAL; > + i2c_smbus_write_byte_data(client, AK8975_REG_CNTL, val); Here again, error code is ignored. > + return count; > +} > +static DEVICE_ATTR(akm_ms1, S_IWUSR | S_IRUGO, akm8975_show, akm8975_store); > + > +static int akm8975_i2c_rxdata(struct akm8975_data *akm, char *buf, int > length) > +{ > + struct i2c_msg msgs[] = { > + { > + .addr = akm->this_client->addr, > + .flags = 0, > + .len = 1, > + .buf = buf, > + }, > + { > + .addr = akm->this_client->addr, > + .flags = I2C_M_RD, > + .len = length, > + .buf = buf, Using the same buffer for writing and reading is unsupported. I think it should work, but there's no guarantee. > + }, > + }; > + > + FUNCDBG("called"); > + > + if (i2c_transfer(akm->this_client->adapter, msgs, 2) < 0) { What's the largest data block you need to handle? If 32 or less, you should be using i2c_smbus_read_i2c_block_data(), which is more portable. > + pr_err("akm8975_i2c_rxdata: transfer error\n"); > + return EIO; Should be -EIO, otherwise error handling won't trigger. > + } else > + return 0; > +} > + > +static int akm8975_i2c_txdata(struct akm8975_data *akm, char *buf, int > length) > +{ > + struct i2c_msg msgs[] = { > + { > + .addr = akm->this_client->addr, > + .flags = 0, > + .len = length, > + .buf = buf, > + }, > + }; > + > + FUNCDBG("called"); > + > + if (i2c_transfer(akm->this_client->adapter, msgs, 1) < 0) { What's the largest data block you need to handle? If 32 or less, you should be using i2c_smbus_write_i2c_block_data(), which is more portable. > + pr_err("akm8975_i2c_txdata: transfer error\n"); > + return -EIO; > + } else > + return 0; > +} > + > +static void akm8975_ecs_report_value(struct akm8975_data *akm, short *rbuf) > +{ > + struct akm8975_data *data = i2c_get_clientdata(akm->this_client); > + > + FUNCDBG("called"); > + > +#if AK8975DRV_DATA_DBG > + pr_info("akm8975_ecs_report_value: yaw = %d, pitch = %d, roll = %d\n", > + rbuf[0], rbuf[1], rbuf[2]); > + pr_info("tmp = %d, m_stat= %d, g_stat=%d\n", rbuf[3], rbuf[4], rbuf[5]); > + pr_info("Acceleration: x = %d LSB, y = %d LSB, z = %d LSB\n", > + rbuf[6], rbuf[7], rbuf[8]); > + pr_info("Magnetic: x = %d LSB, y = %d LSB, z = %d LSB\n\n", > + rbuf[9], rbuf[10], rbuf[11]); > +#endif > + mutex_lock(&akm->flags_lock); > + /* Report magnetic sensor information */ > + if (m_flag) { > + input_report_abs(data->input_dev, ABS_RX, rbuf[0]); > + input_report_abs(data->input_dev, ABS_RY, rbuf[1]); > + input_report_abs(data->input_dev, ABS_RZ, rbuf[2]); > + input_report_abs(data->input_dev, ABS_RUDDER, rbuf[4]); > + } > + > + /* Report acceleration sensor information */ > + if (a_flag) { > + input_report_abs(data->input_dev, ABS_X, rbuf[6]); > + input_report_abs(data->input_dev, ABS_Y, rbuf[7]); > + input_report_abs(data->input_dev, ABS_Z, rbuf[8]); > + input_report_abs(data->input_dev, ABS_WHEEL, rbuf[5]); > + } > + > + /* Report temperature information */ > + if (t_flag) > + input_report_abs(data->input_dev, ABS_THROTTLE, rbuf[3]); > + > + if (mv_flag) { > + input_report_abs(data->input_dev, ABS_HAT0X, rbuf[9]); > + input_report_abs(data->input_dev, ABS_HAT0Y, rbuf[10]); > + input_report_abs(data->input_dev, ABS_BRAKE, rbuf[11]); > + } > + mutex_unlock(&akm->flags_lock); > + > + input_sync(data->input_dev); > +} > + > +static void akm8975_ecs_close_done(struct akm8975_data *akm) > +{ > + FUNCDBG("called"); > + mutex_lock(&akm->flags_lock); > + m_flag = 1; > + a_flag = 1; > + t_flag = 1; > + mv_flag = 1; > + mutex_unlock(&akm->flags_lock); > +} > + > +static int akm_aot_open(struct inode *inode, struct file *file) > +{ > + int ret = -1; Useless and dangerous initialization. > + > + FUNCDBG("called"); > + if (atomic_cmpxchg(&open_flag, 0, 1) == 0) { > + wake_up(&open_wq); > + ret = 0; > + } > + > + ret = nonseekable_open(inode, file); > + if (ret) > + return ret; > + > + file->private_data = akmd_data; > + > + return ret; > +} > + > +static int akm_aot_release(struct inode *inode, struct file *file) > +{ > + FUNCDBG("called"); > + atomic_set(&open_flag, 0); > + wake_up(&open_wq); > + return 0; > +} > + > +static int akm_aot_ioctl(struct inode *inode, struct file *file, > + unsigned int cmd, unsigned long arg) > +{ > + void __user *argp = (void __user *) arg; > + short flag; > + struct akm8975_data *akm = file->private_data; > + > + FUNCDBG("called"); > + > + switch (cmd) { > + case ECS_IOCTL_APP_SET_MFLAG: > + case ECS_IOCTL_APP_SET_AFLAG: > + case ECS_IOCTL_APP_SET_MVFLAG: > + if (copy_from_user(&flag, argp, sizeof(flag))) > + return -EFAULT; > + if (flag < 0 || flag > 1) > + return -EINVAL; > + break; > + case ECS_IOCTL_APP_SET_DELAY: > + if (copy_from_user(&flag, argp, sizeof(flag))) > + return -EFAULT; No range check? > + break; > + default: > + break; What's the point? > + } > + > + mutex_lock(&akm->flags_lock); > + switch (cmd) { > + case ECS_IOCTL_APP_SET_MFLAG: > + m_flag = flag; > + break; > + case ECS_IOCTL_APP_GET_MFLAG: > + flag = m_flag; > + break; > + case ECS_IOCTL_APP_SET_AFLAG: > + a_flag = flag; > + break; > + case ECS_IOCTL_APP_GET_AFLAG: > + flag = a_flag; > + break; > + case ECS_IOCTL_APP_SET_MVFLAG: > + mv_flag = flag; > + break; > + case ECS_IOCTL_APP_GET_MVFLAG: > + flag = mv_flag; > + break; > + case ECS_IOCTL_APP_SET_DELAY: > + akmd_delay = flag; > + break; > + case ECS_IOCTL_APP_GET_DELAY: > + flag = akmd_delay; > + break; > + default: > + return -ENOTTY; You return with a lock held... > + } > + mutex_unlock(&akm->flags_lock); > + > + switch (cmd) { > + case ECS_IOCTL_APP_GET_MFLAG: > + case ECS_IOCTL_APP_GET_AFLAG: > + case ECS_IOCTL_APP_GET_MVFLAG: > + case ECS_IOCTL_APP_GET_DELAY: > + if (copy_to_user(argp, &flag, sizeof(flag))) > + return -EFAULT; > + break; > + default: > + break; Needless statement. > + } > + > + return 0; > +} > + > +static int akmd_open(struct inode *inode, struct file *file) > +{ > + int err = 0; Useless initialization. > + > + FUNCDBG("called"); > + err = nonseekable_open(inode, file); > + if (err) > + return err; > + > + file->private_data = akmd_data; > + return 0; > +} > + > +static int akmd_release(struct inode *inode, struct file *file) > +{ > + struct akm8975_data *akm = file->private_data; > + > + FUNCDBG("called"); > + akm8975_ecs_close_done(akm); > + return 0; > +} > + > +static int akmd_ioctl(struct inode *inode, struct file *file, unsigned int > cmd, > + unsigned long arg) > +{ > + void __user *argp = (void __user *) arg; > + > + char rwbuf[16]; > + int ret = -1; Useless and dangerous initialization. > + int status; > + short value[12]; > + short delay; > + struct akm8975_data *akm = file->private_data; > + > + FUNCDBG("called"); > + > + switch (cmd) { > + case ECS_IOCTL_READ: > + case ECS_IOCTL_WRITE: > + if (copy_from_user(&rwbuf, argp, sizeof(rwbuf))) > + return -EFAULT; > + break; > + > + case ECS_IOCTL_SET_YPR: > + if (copy_from_user(&value, argp, sizeof(value))) > + return -EFAULT; > + break; > + > + default: > + break; What's the point? > + } > + > + switch (cmd) { > + case ECS_IOCTL_READ: > + if (rwbuf[0] < 1) > + return -EINVAL; Missing upper bound check. > + > + ret = akm8975_i2c_rxdata(akm, &rwbuf[1], rwbuf[0]); > + if (ret < 0) > + return ret; > + break; > + > + case ECS_IOCTL_WRITE: > + if (rwbuf[0] < 2) > + return -EINVAL; > + Missing upper bound check. > + ret = akm8975_i2c_txdata(akm, &rwbuf[1], rwbuf[0]); > + if (ret < 0) > + return ret; > + break; > + case ECS_IOCTL_SET_YPR: > + akm8975_ecs_report_value(akm, value); > + break; > + > + case ECS_IOCTL_GET_OPEN_STATUS: > + wait_event_interruptible(open_wq, > + (atomic_read(&open_flag) != 0)); > + status = atomic_read(&open_flag); > + break; > + case ECS_IOCTL_GET_CLOSE_STATUS: > + wait_event_interruptible(open_wq, > + (atomic_read(&open_flag) == 0)); > + status = atomic_read(&open_flag); > + break; > + > + case ECS_IOCTL_GET_DELAY: > + delay = akmd_delay; > + break; > + > + default: > + FUNCDBG("Unknown cmd\n"); > + return -ENOTTY; > + } > + > + switch (cmd) { > + case ECS_IOCTL_READ: > + if (copy_to_user(argp, &rwbuf, sizeof(rwbuf))) > + return -EFAULT; > + break; > + case ECS_IOCTL_GET_OPEN_STATUS: > + case ECS_IOCTL_GET_CLOSE_STATUS: > + if (copy_to_user(argp, &status, sizeof(status))) > + return -EFAULT; > + break; > + case ECS_IOCTL_GET_DELAY: > + if (copy_to_user(argp, &delay, sizeof(delay))) > + return -EFAULT; > + break; > + default: > + break; Needless statement. > + } > + > + return 0; > +} > + > +/* needed to clear the int. pin */ > +static void akm_work_func(struct work_struct *work) > +{ > + struct akm8975_data *akm = > + container_of(work, struct akm8975_data, work); > + > + FUNCDBG("called"); > + enable_irq(akm->this_client->irq); > +} > + > +static irqreturn_t akm8975_interrupt(int irq, void *dev_id) > +{ > + struct akm8975_data *akm = dev_id; > + FUNCDBG("called"); > + > + disable_irq_nosync(akm->this_client->irq); > + schedule_work(&akm->work); > + return IRQ_HANDLED; > +} > + > +static int akm8975_power_off(struct akm8975_data *akm) > +{ > +#if AK8975DRV_CALL_DBG > + pr_info("%s\n", __func__); > +#endif Why not just FUNCDBG("called") as everywhere else? > + if (akm->pdata->power_off) > + akm->pdata->power_off(); This function could return an error, which you want to transmit. > + > + return 0; > +} > + > +static int akm8975_power_on(struct akm8975_data *akm) > +{ > + int err; > + > +#if AK8975DRV_CALL_DBG > + pr_info("%s\n", __func__); > +#endif > + if (akm->pdata->power_on) { > + err = akm->pdata->power_on(); > + if (err < 0) > + return err; > + } > + return 0; > +} > + > +static int akm8975_init_client(struct i2c_client *client) > +{ > + struct akm8975_data *data; > + int ret; > + > + data = i2c_get_clientdata(client); > + > + ret = request_irq(client->irq, akm8975_interrupt, IRQF_TRIGGER_RISING, > + "akm8975", data); > + > + if (ret < 0) { > + pr_err("akm8975_init_client: request irq failed\n"); Please use dev_err(). > + goto err; > + } > + > + init_waitqueue_head(&open_wq); > + > + mutex_lock(&data->flags_lock); > + m_flag = 1; > + a_flag = 1; > + t_flag = 1; > + mv_flag = 1; > + mutex_unlock(&data->flags_lock); Shouldn't you set up everything _before_ requesting the IRQ? > + > + return 0; > +err: > + return ret; > +} > + > +static const struct file_operations akmd_fops = { > + .owner = THIS_MODULE, > + .open = akmd_open, > + .release = akmd_release, > + .ioctl = akmd_ioctl, > +}; > + > +static const struct file_operations akm_aot_fops = { > + .owner = THIS_MODULE, > + .open = akm_aot_open, > + .release = akm_aot_release, > + .ioctl = akm_aot_ioctl, > +}; > + > +static struct miscdevice akm_aot_device = { > + .minor = MISC_DYNAMIC_MINOR, > + .name = "akm8975_aot", > + .fops = &akm_aot_fops, > +}; > + > +static struct miscdevice akmd_device = { > + .minor = MISC_DYNAMIC_MINOR, > + .name = "akm8975_dev", > + .fops = &akmd_fops, > +}; > + > +int akm8975_probe(struct i2c_client *client, > + const struct i2c_device_id *devid) > +{ > + struct akm8975_data *akm; > + int err; > + FUNCDBG("called"); > + > + if (client->dev.platform_data == NULL) { > + dev_err(&client->dev, "platform data is NULL. exiting.\n"); > + err = -ENODEV; > + goto exit_platform_data_null; > + } > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { This check doesn't match the actual API use. Will need revisiting if you go for i2c_smbus_{write,read}_i2c_block_data anyway. > + dev_err(&client->dev, "platform data is NULL. exiting.\n"); Wrong error message. > + err = -ENODEV; > + goto exit_check_functionality_failed; > + } > + > + akm = kzalloc(sizeof(struct akm8975_data), GFP_KERNEL); > + if (!akm) { > + dev_err(&client->dev, > + "failed to allocate memory for module data\n"); > + err = -ENOMEM; > + goto exit_alloc_data_failed; > + } > + > + akm->pdata = client->dev.platform_data; > + > + mutex_init(&akm->flags_lock); > + INIT_WORK(&akm->work, akm_work_func); > + i2c_set_clientdata(client, akm); > + > + err = akm8975_power_on(akm); > + if (err < 0) > + goto exit_power_on_failed; > + > + akm8975_init_client(client); > + akm->this_client = client; > + akmd_data = akm; > + > + akm->input_dev = input_allocate_device(); > + if (!akm->input_dev) { > + err = -ENOMEM; > + dev_err(&akm->this_client->dev, > + "input device allocate failed\n"); > + goto exit_input_dev_alloc_failed; > + } > + > + set_bit(EV_ABS, akm->input_dev->evbit); > + > + /* yaw */ > + input_set_abs_params(akm->input_dev, ABS_RX, 0, 23040, 0, 0); > + /* pitch */ > + input_set_abs_params(akm->input_dev, ABS_RY, -11520, 11520, 0, 0); > + /* roll */ > + input_set_abs_params(akm->input_dev, ABS_RZ, -5760, 5760, 0, 0); > + /* x-axis acceleration */ > + input_set_abs_params(akm->input_dev, ABS_X, -5760, 5760, 0, 0); > + /* y-axis acceleration */ > + input_set_abs_params(akm->input_dev, ABS_Y, -5760, 5760, 0, 0); > + /* z-axis acceleration */ > + input_set_abs_params(akm->input_dev, ABS_Z, -5760, 5760, 0, 0); > + /* temparature */ > + input_set_abs_params(akm->input_dev, ABS_THROTTLE, -30, 85, 0, 0); > + /* status of magnetic sensor */ > + input_set_abs_params(akm->input_dev, ABS_RUDDER, 0, 3, 0, 0); > + /* status of acceleration sensor */ > + input_set_abs_params(akm->input_dev, ABS_WHEEL, 0, 3, 0, 0); > + /* x-axis of raw magnetic vector */ > + input_set_abs_params(akm->input_dev, ABS_HAT0X, -20480, 20479, 0, 0); > + /* y-axis of raw magnetic vector */ > + input_set_abs_params(akm->input_dev, ABS_HAT0Y, -20480, 20479, 0, 0); > + /* z-axis of raw magnetic vector */ > + input_set_abs_params(akm->input_dev, ABS_BRAKE, -20480, 20479, 0, 0); > + > + akm->input_dev->name = "compass"; > + > + err = input_register_device(akm->input_dev); > + if (err) { > + pr_err("akm8975_probe: Unable to register input device: %s\n", > + akm->input_dev->name); dev_err() > + goto exit_input_register_device_failed; > + } > + > + err = misc_register(&akmd_device); > + if (err) { > + pr_err("akm8975_probe: akmd_device register failed\n"); dev_err() > + goto exit_misc_device_register_failed; > + } > + > + err = misc_register(&akm_aot_device); > + if (err) { > + pr_err("akm8975_probe: akm_aot_device register failed\n"); dev_err() > + goto exit_misc_device_register_failed; > + } > + > + err = device_create_file(&client->dev, &dev_attr_akm_ms1); Missing error handling. > + > + return 0; > + > +exit_misc_device_register_failed: > +exit_input_register_device_failed: > + input_free_device(akm->input_dev); > +exit_input_dev_alloc_failed: > + akm8975_power_off(akm); > +exit_power_on_failed: > + kfree(akm); > +exit_alloc_data_failed: > +exit_check_functionality_failed: > +exit_platform_data_null: It is more common to name the labels based on the next undo action. This avoids having several labels for the same point. > + return err; > +} > + > +static int __devexit akm8975_remove(struct i2c_client *client) > +{ > + struct akm8975_data *akm = i2c_get_clientdata(client); > + FUNCDBG("called"); > + free_irq(client->irq, NULL); > + input_unregister_device(akm->input_dev); > + misc_deregister(&akmd_device); > + misc_deregister(&akm_aot_device); > + akm8975_power_off(akm); > + kfree(akm); > + return 0; > +} > + > +static const struct i2c_device_id akm8975_id[] = { > + { "akm8975", 0 }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(i2c, akm8975_id); > + > +static struct i2c_driver akm8975_driver = { > + .probe = akm8975_probe, > + .remove = akm8975_remove, Missing __devexit_p(). > + .id_table = akm8975_id, > + .driver = { > + .name = "akm8975", > + }, > +}; > + > +static int __init akm8975_init(void) > +{ > + pr_info("AK8975 compass driver: init\n"); Debugging message shouldn't be printed with pr_info(). > + FUNCDBG("AK8975 compass driver: init\n"); > + return i2c_add_driver(&akm8975_driver); > +} > + > +static void __exit akm8975_exit(void) > +{ > + FUNCDBG("AK8975 compass driver: exit\n"); > + i2c_del_driver(&akm8975_driver); > +} > + > +module_init(akm8975_init); > +module_exit(akm8975_exit); > + > +MODULE_AUTHOR("Hou-Kun Chen <[email protected]>"); > +MODULE_DESCRIPTION("AK8975 compass driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/akm8975.h b/include/linux/akm8975.h > new file mode 100644 > index 0000000..7d02120 > --- /dev/null > +++ b/include/linux/akm8975.h This should be include/linux/i2c/akm8975.h, as all other i2c device drivers to. > @@ -0,0 +1,87 @@ > +/* > + * Definitions for akm8975 compass chip. > + */ > +#ifndef AKM8975_H > +#define AKM8975_H Should be __LINUX_I2C_AKM8975_H. > + > +#include <linux/ioctl.h> > + > +/*! \name AK8975 operation mode > + \anchor AK8975_Mode > + Defines an operation mode of the AK8975.*/ > +/*! @{*/ What's this ugly syntax? Please follow the standard documentation syntax. > +#define AK8975_MODE_SNG_MEASURE 0x01 > +#define AK8975_MODE_SELF_TEST 0x08 > +#define AK8975_MODE_FUSE_ACCESS 0x0F > +#define AK8975_MODE_POWER_DOWN 0x00 Incorrect alignment. > +/*! @}*/ > + > +#define RBUFF_SIZE 8 /* Rx buffer size */ > + > +/*! \name AK8975 register address > +\anchor AK8975_REG > +Defines a register address of the AK8975.*/ > +/*! @{*/ > +#define AK8975_REG_WIA 0x00 > +#define AK8975_REG_INFO 0x01 > +#define AK8975_REG_ST1 0x02 > +#define AK8975_REG_HXL 0x03 > +#define AK8975_REG_HXH 0x04 > +#define AK8975_REG_HYL 0x05 > +#define AK8975_REG_HYH 0x06 > +#define AK8975_REG_HZL 0x07 > +#define AK8975_REG_HZH 0x08 > +#define AK8975_REG_ST2 0x09 > +#define AK8975_REG_CNTL 0x0A > +#define AK8975_REG_RSV 0x0B > +#define AK8975_REG_ASTC 0x0C > +#define AK8975_REG_TS1 0x0D > +#define AK8975_REG_TS2 0x0E > +#define AK8975_REG_I2CDIS 0x0F > +/*! @}*/ Do users need to know this? I guess not. Public header files should only contain the API users need. Everything which is only used internally by the driver shouldn't be there. > + > +/*! \name AK8975 fuse-rom address > +\anchor AK8975_FUSE > +Defines a read-only address of the fuse ROM of the AK8975.*/ > +/*! @{*/ > +#define AK8975_FUSE_ASAX 0x10 > +#define AK8975_FUSE_ASAY 0x11 > +#define AK8975_FUSE_ASAZ 0x12 > +/*! @}*/ > + > +#define AKMIO 0xA1 > + > +/* IOCTLs for AKM library */ > +#define ECS_IOCTL_WRITE _IOW(AKMIO, 0x02, char[5]) > +#define ECS_IOCTL_READ _IOWR(AKMIO, 0x03, char[5]) > +#define ECS_IOCTL_GETDATA _IOR(AKMIO, 0x08, char[RBUFF_SIZE]) > +#define ECS_IOCTL_SET_YPR _IOW(AKMIO, 0x0C, short[12]) > +#define ECS_IOCTL_GET_OPEN_STATUS _IOR(AKMIO, 0x0D, int) > +#define ECS_IOCTL_GET_CLOSE_STATUS _IOR(AKMIO, 0x0E, int) > +#define ECS_IOCTL_GET_DELAY _IOR(AKMIO, 0x30, short) > + > +/* IOCTLs for APPs */ > +#define ECS_IOCTL_APP_SET_MFLAG _IOW(AKMIO, 0x11, short) > +#define ECS_IOCTL_APP_GET_MFLAG _IOW(AKMIO, 0x12, short) > +#define ECS_IOCTL_APP_SET_AFLAG _IOW(AKMIO, 0x13, short) > +#define ECS_IOCTL_APP_GET_AFLAG _IOR(AKMIO, 0x14, short) > +#define ECS_IOCTL_APP_SET_DELAY _IOW(AKMIO, 0x18, short) > +#define ECS_IOCTL_APP_GET_DELAY ECS_IOCTL_GET_DELAY > +/* Set raw magnetic vector flag */ > +#define ECS_IOCTL_APP_SET_MVFLAG _IOW(AKMIO, 0x19, short) > +/* Get raw magnetic vector flag */ > +#define ECS_IOCTL_APP_GET_MVFLAG _IOR(AKMIO, 0x1A, short) > +#define ECS_IOCTL_APP_SET_TFLAG _IOR(AKMIO, 0x15, short) > + > + > +struct akm8975_platform_data { > + int intr; > + > + int (*init)(void); > + void (*exit)(void); > + int (*power_on)(void); > + int (*power_off)(void); These "void" parameters seem suspicious. I would expect at least the device to be passed as a parameter. I understand that your driver is currently limited to supporting a single device, but you should avoid designing everything based on this assumption, because hopefully it can be solved someday. > +}; > + > +#endif > + -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
