On 04/28/2010 08:45 PM, matthias wrote:
> This drivers allows a communication with the Analog Devices ADIS16255
> Low Power Gyroscope over SPI.
Hi.
For the whole code, it is wrongly indented. See the coding style rules.
> Signed-off-by: Matthias Brugger <mensch0815@xxxxxxxxx>
...
> --- /dev/null
> +++ b/drivers/staging/adis16255/adis16255.c
> +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);
> + if (!buf)
> + return -ENOMEM;
> +
> + rx = kzalloc(4, GFP_KERNEL);
> + if (!rx)
> + return -ENOMEM;
buf leaks here. You can just use buffers on stack for 8 bytes (unless it
DMAs there).
> +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)
> + return -ENOMEM;
> +
> + rx = kzalloc(4, GFP_KERNEL);
> + if (!rx)
> + return -ENOMEM;
Again.
> +static irqreturn_t adis_interrupt(int irq, void *dev_id) {
Wrong style, check you patch with checkpatch.
> + struct spi_adis16255_data *spiadis = (struct spi_adis16255_data *) dev_id;
No need to cast.
> + schedule_work(&spiadis->wq);
So this can be threaded irq with NULL handler, no?
> + return IRQ_HANDLED;
> +}
...
> +static int spi_adis16255_probe(struct spi_device *spi)
> +{
> +
> +#define AD_CHK(_ss) \
Better to use
do {} while (0)
> +{ \
> + status = _ss; \
> + if(status != 0) \
> + goto irq_err; \
This is ugly anyway and hides bugs often.
> +}
> +
> + struct adis16255_init_data *init_data = spi->dev.platform_data;
> + struct spi_adis16255_data *spiadis;
> + int status=0;
> + u16 value;
> +
> + /* Allocate driver data */
> + spiadis = kzalloc(sizeof(*spiadis), GFP_KERNEL);
> + if (!spiadis)
> + return -ENOMEM;
> +
> + spiadis->spi = spi; // save spi_device in driver-data
> + spiadis->irq_adis = init_data->irq;
> + spiadis->direction = init_data->direction;
> +
> + if (init_data->negative) {
> + spiadis->negative = 1;
> + }
> +
> + INIT_WORK(&spiadis->wq, spi_workqueue);
> +
> +
> + status = gpio_request(spiadis->irq_adis, "adis16255");
> + if(status != 0)
> + goto irq_err;
> +
> + status = gpio_direction_input(spiadis->irq_adis);
> + if(status != 0)
> + goto irq_err;
> +
> + spiadis->irq = gpio_to_irq(spiadis->irq_adis);
> +
> + status = request_irq(spiadis->irq, adis_interrupt, IRQF_DISABLED,
> "adis-driver", spiadis);
> + if(status != 0) {
> + dev_err(&spi->dev, "IRQ request failed\n");
> + goto irq_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));
No irq freeing? And below...
> + 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);
> +
> + value = 0x0001;// 0x008a;
> + 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:
> + 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);
> + struct adis16255_init_data *init_data = spi->dev.platform_data;
> +
> + if(spiadis == NULL) {
Can this happen for SPI bus?
> +static int __init spi_adis16255_init(void)
> +{
> + int status;
> +
> + status = spi_register_driver(&spi_adis16255_drv);
> + return status;
Only cosmetics: just return spi_register_driver();
> +}
> +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..26c9f84
> --- /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
> _______________________________________________
> devel mailing list
> devel@xxxxxxxxxxxxxxxxxxxxxx
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
--
js
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
