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
