Re: [staging] driver for adis16255 gyroscope

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

2010/4/29 Jiri Slaby <jirislaby@xxxxxxxxx>:
> 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).

I'm not sure if I understand you right. A buffer can not be put on the
stack if it is not multiple of 8. Why? (sorry, maybe off topic here)
Would it fix the problem if I put the tx and rx buffer for SPI in
struct spiadis?

>
>> +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?

Why? I request the interrupt in the probe function, passing spiadis struct.
Should I check for spiadis, for defensive programming sake?

>
>> +   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?

No.

>
>> +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
>



-- 
motzblog.wordpress.com
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux