On Thu, Apr 29, 2010 at 8:05 AM, matthias <mensch0815@xxxxxxxxxxxxxx> wrote:
> 2010/4/29 Greg KH <greg@xxxxxxxxx>:
>> On Thu, Apr 29, 2010 at 09:54:08AM +0200, matthias wrote:
>>> 2010/4/28 Greg KH <gregkh@xxxxxxx>:
>>> > On Wed, Apr 28, 2010 at 08:45:35PM +0200, matthias wrote:
>>> >> This drivers allows a communication with the Analog Devices ADIS16255
>>> >> Low Power Gyroscope over SPI.
>>> >
>>> > Why is this going to be a staging driver? Is there a problem with it
>>> > going into the main portion of the kernel now?
>>>
>>> - No one else has tested it, so I think if it stays in staging, others
>>> have the possibility to test it.
>>> - It has checkpatch issues
>>> - Odd stuff like the AD_CHK(_ss) macro
>>> - I'm not sure to which subsystem it should be added (maybe industrial
>>> io, but this is still staging. Any suggestions?)
>>> - Improvements of code on protocol hierarchy needed (e.g. shutdown
>>> device commands in remove function; it is not obvious which sample
>>> rate the device uses)
>>
>> Ok, can you resend it after fixing the email problem, and add a TODO
>> file that lists these things and puts your email as the person to
>> contact about the driver?
>
> Yes, here we go. I added most of the things Jiri mentioned.
I don't know, this driver is small enough to just fix completely in 1
or 2 more submission reviews, don't see the point in going through
staging.
> Signed-off-by: Matthias Brugger <mensch0815@xxxxxxxxx>
> ---
> diff --git a/drivers/staging/adis16255/Kconfig
> b/drivers/staging/adis16255/Kconfig
> new file mode 100644
> index 0000000..3952cf9
> --- /dev/null
> +++ b/drivers/staging/adis16255/Kconfig
> @@ -0,0 +1,9 @@
> +config ADIS16255
> + tristate "Ananlog Devices ADIS16250/16255"
> + depends on SPI && SYSFS
> + ---help---
> + If you say yes here you get support for the Analog Devices
> + ADIS16250/16255 Low Power Gyroscope.
Does this ship on any device BTW? Or will it ? :) It might help to
also put a small description here that all this driver does is expose
the gyroscope's coordinates and orientation via sysfs, and *that's
it*.
> +
> + This driver can also be built as a module. If so, the module
> + will be called adis16255.
> diff --git a/drivers/staging/adis16255/Makefile
> b/drivers/staging/adis16255/Makefile
> new file mode 100644
> index 0000000..796dac4
> --- /dev/null
> +++ b/drivers/staging/adis16255/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ADIS16255) += adis16255.o
> diff --git a/drivers/staging/adis16255/TODO b/drivers/staging/adis16255/TODO
> new file mode 100644
> index 0000000..31e4ac3
> --- /dev/null
> +++ b/drivers/staging/adis16255/TODO
> @@ -0,0 +1,8 @@
> +* sample rate changeable or at least readable from sysfs
> +* reset gyroscope
> +* encapsulate adis_init and adis_turn_off
> +* AD_CHK deletion
> +* chip selftest in adis_init
> +* reduce kernel messages to reasonable amount
> +
> +Contact: Matthias Brugger <mensch0815@xxxxxxxxx>
> diff --git a/drivers/staging/adis16255/adis16255.c
> b/drivers/staging/adis16255/adis16255.c
> new file mode 100644
> index 0000000..0d8d6bd
> --- /dev/null
> +++ b/drivers/staging/adis16255/adis16255.c
> @@ -0,0 +1,396 @@
> +/*
> + * Analog Devices ADIS16250/ADIS16255 Low Power Gyroscope
> + *
> + * Written by: Matthias Brugger <m_brugger@xxxxxx>
You surely have a copyright too?
> + * Copyright (C) 2010 Fraunhofer Institute for Integrated Circuits
> + *
> + * 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.
> + *
> + * 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/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include <linux/interrupt.h>
> +#include <linux/sysfs.h>
> +#include <linux/stat.h>
> +#include <linux/delay.h>
> +
> +#include <linux/gpio.h>
> +
> +#include <linux/spi/spi.h>
> +#include <linux/workqueue.h>
> +
> +#include "adis16255.h"
> +
> +#define ADIS_STATUS 0x3d
> +#define ADIS_SMPL_PRD_MSB 0x37
> +#define ADIS_SMPL_PRD_LSB 0x36
> +#define ADIS_MSC_CTRL_MSB 0x35
> +#define ADIS_MSC_CTRL_LSB 0x34
> +#define ADIS_GPIO_CTRL 0x33
> +#define ADIS_ALM_SMPL1 0x25
> +#define ADIS_ALM_MAG1 0x21
> +#define ADIS_GYRO_SCALE 0x17
> +#define ADIS_GYRO_OUT 0x05
> +#define ADIS_SUPPLY_OUT 0x03
> +#define ADIS_ENDURANCE 0x01
> +
> +/*
> + * data structure for every sensor
> + *
> + * @dev: Driver model representation of the device.
> + * @spi: Pointer to the spi device which will manage i/o to spi bus.
> + * @data: Last read data from device.
> + * @irq_adis: GPIO Number of IRQ signal
> + * @irq: irq line manage by kernel
> + * @negative: indicates if sensor is upside down (negative == 1)
> + * @direction: indicates axis (x, y, z) the sensor is meassuring
> + */
> +struct spi_adis16255_data {
> + struct device dev;
> + struct spi_device *spi;
> + s16 data;
> + int irq_adis;
> + int irq;
> + u8 negative;
> + char direction;
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int spi_adis16255_read_data(struct spi_adis16255_data *spiadis,
> + u8 adr,
> + u8 *rbuf)
> +{
> + struct spi_device *spi = spiadis->spi;
> + struct spi_message msg;
> + struct spi_transfer xfer1, xfer2;
> + u8 *buf, *rx;
> + int ret;
> +
> + buf = kmalloc(4, GFP_KERNEL);
So this uses kmalloc() but kzalloc() below, why not use on both
kzalloc() and save you those few lines below on the buf?
> + if (buf == NULL)
> + return -ENOMEM;
> +
> + rx = kzalloc(4, GFP_KERNEL);
> + if (rx == NULL) {
> + ret = -ENOMEM;
> + goto err_buf;
> + }
> +
> + buf[0] = adr;
> + buf[1] = 0x00;
> + buf[2] = 0x00;
> + buf[3] = 0x00;
> +
> + spi_message_init(&msg);
> + memset(&xfer1, 0, sizeof(xfer1));
> + memset(&xfer2, 0, sizeof(xfer2));
> +
> + xfer1.tx_buf = buf;
> + xfer1.rx_buf = buf + 2;
> + xfer1.len = 2;
> + xfer1.delay_usecs = 9;
> +
> + xfer2.tx_buf = rx + 2;
> + xfer2.rx_buf = rx;
> + xfer2.len = 2;
> +
> + spi_message_add_tail(&xfer1, &msg);
> + spi_message_add_tail(&xfer2, &msg);
> +
> + ret = spi_sync(spi, &msg);
> + if (ret == 0) {
> + rbuf[0] = rx[0];
> + rbuf[1] = rx[1];
> + }
> +
> + kfree(rx);
> +err_buf:
> + kfree(buf);
> +
> + return ret;
> +}
> +
> +static int spi_adis16255_write_data(struct spi_adis16255_data *spiadis,
> + u8 adr1,
> + u8 adr2,
> + u8 *wbuf)
> +{
> + struct spi_device *spi = spiadis->spi;
> + struct spi_message msg;
> + struct spi_transfer xfer1, xfer2;
> + u8 *buf, *rx;
> + int ret;
> +
> + buf = kmalloc(4, GFP_KERNEL);
> + if (buf == NULL)
> + return -ENOMEM;
In this case not using kzalloc() does save us 4 re-assignments, so that's fine.
> +
> + rx = kzalloc(4, GFP_KERNEL);
> + if (rx == NULL) {
> + ret = -ENOMEM;
> + goto err_buf;
> + }
> +
> + spi_message_init(&msg);
> + memset(&xfer1, 0, sizeof(xfer1));
> + memset(&xfer2, 0, sizeof(xfer2));
> +
> + buf[0] = adr1 | 0x80;
> + buf[1] = *wbuf;
> +
> + buf[2] = adr2 | 0x80;
> + buf[3] = *(wbuf + 1);
> +
> + xfer1.tx_buf = buf;
> + xfer1.rx_buf = rx;
> + xfer1.len = 2;
> + xfer1.delay_usecs = 9;
> +
> + xfer2.tx_buf = buf+2;
> + xfer2.rx_buf = rx+2;
> + xfer2.len = 2;
> +
> + spi_message_add_tail(&xfer1, &msg);
> + spi_message_add_tail(&xfer2, &msg);
> +
> + ret = spi_sync(spi, &msg);
> + if (ret != 0)
> + dev_warn(&spi->dev, "wirte data to %#x %#x failed\n",
> + buf[0], buf[2]);
> +
> + kfree(rx);
> +err_buf:
> + kfree(buf);
> + return ret;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static irqreturn_t adis_irq_thread(int irq, void *dev_id)
> +{
> + struct spi_adis16255_data *spiadis = dev_id;
> + int status;
> + u16 value;
u16 is unitialized, set it to 0 even though rbuf will be properly set
only if the return status is 0.
> +
> + status = spi_adis16255_read_data(spiadis, ADIS_GYRO_OUT, (u8 *)&value);
> + if (status == 0) {
Better check for the failure first, and bail out immediately with a
goto, that way the below code, the irq handler can be indented all the
way to the left.
> + /* perform on new data only... */
> + if (value & 0x8000) {
This interrupt means there is new data?
> + /* delete error and new data bit */
> + value = value & 0x3fff;
> + /* set negative value */
> + if (value & 0x2000)
> + value = value | 0xe000;
> +
> + if (likely(spiadis->negative))
> + value = -value;
> +
> + spiadis->data = (s16) value;
There's no locking around this guy, this seems wrong.
> + }
> + } else {
> + dev_warn(&spiadis->spi->dev, "SPI FAILED\n");
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +ssize_t adis16255_show_data(struct device *device,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct spi_adis16255_data *spiadis = dev_get_drvdata(device);
> + return snprintf(buf, PAGE_SIZE, "%d\n", spiadis->data);
> +}
> +DEVICE_ATTR(data, S_IRUGO , adis16255_show_data, NULL);
> +
> +ssize_t adis16255_show_direction(struct device *device,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct spi_adis16255_data *spiadis = dev_get_drvdata(device);
> + return snprintf(buf, PAGE_SIZE, "%c\n", spiadis->direction);
> +}
> +DEVICE_ATTR(direction, S_IRUGO , adis16255_show_direction, NULL);
> +
> +static struct attribute *adis16255_attributes[] = {
> + &dev_attr_data.attr,
> + &dev_attr_direction.attr,
> + NULL
> +};
> +
> +static const struct attribute_group adis16255_attr_group = {
> + .attrs = adis16255_attributes,
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int spi_adis16255_probe(struct spi_device *spi)
> +{
> +
> +#define AD_CHK(_ss)\
> + do {\
> + status = _ss;\
> + if (status != 0)\
> + goto irq_err;\
> + } while (0);
> +
> + struct adis16255_init_data *init_data = spi->dev.platform_data;
> + struct spi_adis16255_data *spiadis;
> + int status = 0;
> + u16 value;
> +
> + spiadis = kzalloc(sizeof(*spiadis), GFP_KERNEL);
> + if (!spiadis)
> + return -ENOMEM;
> +
> + spiadis->spi = spi;
> + spiadis->irq_adis = init_data->irq;
> + spiadis->direction = init_data->direction;
> +
> + if (init_data->negative)
> + spiadis->negative = 1;
> +
> + status = gpio_request(spiadis->irq_adis, "adis16255");
> + if (status != 0)
> + goto err;
> +
> + status = gpio_direction_input(spiadis->irq_adis);
> + if (status != 0)
> + goto gpio_err;
> +
> + spiadis->irq = gpio_to_irq(spiadis->irq_adis);
> +
> + status = request_threaded_irq(spiadis->irq,
> + NULL, adis_irq_thread,
> + IRQF_DISABLED, "adis-driver", spiadis);
> +
> + if (status != 0) {
> + dev_err(&spi->dev, "IRQ request failed\n");
> + goto gpio_err;
> + }
> +
> + dev_dbg(&spi->dev, "GPIO %d IRQ %d\n", spiadis->irq_adis, spiadis->irq);
> +
> + dev_set_drvdata(&spi->dev, spiadis);
> + AD_CHK(sysfs_create_group(&spi->dev.kobj, &adis16255_attr_group));
> +
> + dev_info(&spi->dev, "spi_adis16255 driver added!\n");
> +
> + AD_CHK(spi_adis16255_read_data(spiadis, ADIS_SUPPLY_OUT, (u8 *)&value));
> + dev_info(&spi->dev, "sensor works with %d mV (%.4x)!\n",
> + ((value & 0x0fff)*18315)/10000,
> + (value & 0x0fff));
> +
> + AD_CHK(spi_adis16255_read_data(spiadis, ADIS_GYRO_SCALE, (u8 *)&value));
> + dev_info(&spi->dev, "adis GYRO_SCALE is %.4x\n", value);
> +
> + AD_CHK(spi_adis16255_read_data(spiadis, ADIS_STATUS, (u8 *)&value));
> + dev_info(&spi->dev, "adis STATUS is %.4x\n", value);
> +
> + /* timebase = 1.953 ms, Ns = 0 -> 512 Hz sample rate */
> + value = 0x0001;
> + AD_CHK(spi_adis16255_write_data(spiadis,
> + ADIS_SMPL_PRD_MSB, ADIS_SMPL_PRD_LSB,
> + (u8 *)&value));
> + value = 0x0000;
> + AD_CHK(spi_adis16255_read_data(spiadis, ADIS_SMPL_PRD_MSB,
> + (u8 *)&value));
> + dev_info(&spi->dev, "adis SMP_PRD is %.4x\n", value);
> +
> + /* set interrupt on new data... */
> + value = 0x0006;
> + AD_CHK(spi_adis16255_write_data(spiadis,
> + ADIS_MSC_CTRL_MSB, ADIS_MSC_CTRL_LSB,
> + (u8 *)&value));
> + value = 0x0000;
> + AD_CHK(spi_adis16255_read_data(spiadis, ADIS_MSC_CTRL_MSB,
> + (u8 *)&value));
> + dev_info(&spi->dev, "adis MSC_CONTROL is %.4x\n", value);
> +
> + return status;
> +
> +irq_err:
> + free_irq(spiadis->irq, spiadis);
> +gpio_err:
> + gpio_free(spiadis->irq_adis);
> +err:
> + kfree(spiadis);
> + return status;
> +}
> +
> +static int spi_adis16255_remove(struct spi_device *spi)
> +{
> + u16 value = 0;
> + struct spi_adis16255_data *spiadis = dev_get_drvdata(&spi->dev);
> +
> + /* turn sensor off */
> + spi_adis16255_write_data(spiadis,
> + ADIS_SMPL_PRD_MSB, ADIS_SMPL_PRD_LSB,
> + (u8 *)&value);
> + spi_adis16255_write_data(spiadis,
> + ADIS_MSC_CTRL_MSB, ADIS_MSC_CTRL_LSB,
> + (u8 *)&value);
> +
> + dev_info(&spi->dev, "unregister: GPIO %d IRQ %d\n",
> + spiadis->irq_adis, spiadis->irq);
> +
> + free_irq(spiadis->irq, spiadis);
> + gpio_free(spiadis->irq_adis);
> +
> + sysfs_remove_group(&spiadis->spi->dev.kobj, &adis16255_attr_group);
> +
> + kfree(spiadis);
> +
> + dev_info(&spi->dev, "spi_adis16255 driver removed!\n");
> + return 0;
> +}
> +
> +static struct spi_driver spi_adis16255_drv = {
> + .driver = {
> + .name = "spi_adis16255",
> + .owner = THIS_MODULE,
> + },
> + .probe = spi_adis16255_probe,
> + .remove = __devexit_p(spi_adis16255_remove),
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int __init spi_adis16255_init(void)
> +{
> + return spi_register_driver(&spi_adis16255_drv);
> +}
> +module_init(spi_adis16255_init);
> +
> +static void __exit spi_adis16255_exit(void)
> +{
> + spi_unregister_driver(&spi_adis16255_drv);
> +}
> +module_exit(spi_adis16255_exit);
> +
> +MODULE_AUTHOR("Matthias Brugger");
> +MODULE_DESCRIPTION("SPI device driver for ADIS16255 sensor");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/staging/adis16255/adis16255.h
> b/drivers/staging/adis16255/adis16255.h
> new file mode 100644
> index 0000000..03e0700
> --- /dev/null
> +++ b/drivers/staging/adis16255/adis16255.h
> @@ -0,0 +1,12 @@
> +#ifndef ADIS16255_H
> +#define ADIS16255_H
> +
> +#include <linux/types.h>
> +
> +struct adis16255_init_data {
> + char direction;
> + u8 negative;
> + int irq;
> +};
> +
> +#endif
>
> --
> motzblog.wordpress.com
> _______________________________________________
> devel mailing list
> devel@xxxxxxxxxxxxxxxxxxxxxx
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
