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