Re: [PATCH] input: add driver for Bosch Sensortec's BMA150 accelerometer
flat view
From: Alan Cox <hidden>
Date: 2011-05-31 16:21:43
Also in:
lkml
See the driver I posted some time ago. It's in rather better state than this one and as it's been through most of the review process and also handles the BMA023 and SMB380 so I think would be a better place to start from. I'll report that driver in a moment. Alan Some of the first immediately obvious questions though - in particular the lack of clear locking ...
quoted hunk ↗ jump to hunk
+struct bma150acc { + s16 x, + y, + z; +};
Why does this need to be a struct ?
quoted hunk ↗ jump to hunk
+ if ((mode != BMA150_MODE_NORMAL) && + (mode != BMA150_MODE_SLEEP) && + (mode != BMA150_MODE_WAKE_UP)) + return -EINVAL;
How can any other mode get passed ?
quoted hunk ↗ jump to hunk
+ + data1 = i2c_smbus_read_byte_data(client, BMA150_WAKE_UP_REG); + if (data1 < 0) + return ret; + + data1 = (data1 & ~BMA150_WAKE_UP_MSK) | + ((mode << BMA150_WAKE_UP_POS) & BMA150_WAKE_UP_MSK); + + data2 = i2c_smbus_read_byte_data(client, BMA150_SLEEP_REG); + if (data2 < 0) + return ret; + + data2 = (data2 & ~BMA150_SLEEP_MSK) | + (((mode>>1) << BMA150_SLEEP_POS) & BMA150_SLEEP_MSK); + + ret = i2c_smbus_write_byte_data(client, BMA150_WAKE_UP_REG, data1); + if (ret < 0) + return ret; + + ret = i2c_smbus_write_byte_data(client, BMA150_SLEEP_REG, data2); + if (ret < 0) + return ret;
What locks this SMBUS transaction against others
quoted hunk ↗ jump to hunk
+static int bma150_set_range(struct i2c_client *client, unsigned char range) +{ + int ret; + unsigned char data; + + if (range > BMA150_RANGE_8G) + return -EINVAL;
This should be actual values not a register range
quoted hunk ↗ jump to hunk
+ + data = i2c_smbus_read_byte_data(client, BMA150_RANGE_REG); + if (data < 0) + return ret; + + data = (data & ~BMA150_RANGE_MSK) | + ((range << BMA150_RANGE_POS) & BMA150_RANGE_MSK); + + ret = i2c_smbus_write_byte_data(client, BMA150_RANGE_REG, data);
What locks this ?
quoted hunk ↗ jump to hunk
+ if (ret < 0) + return ret; + + return 0;
Why this pointless if ?
quoted hunk ↗ jump to hunk
+} + +static int bma150_get_range(struct i2c_client *client, unsigned char *range) +{ + int ret; + unsigned char data; + + data = i2c_smbus_read_byte_data(client, BMA150_RANGE_REG); + if (data < 0) + return ret; + + *range = (data & BMA150_RANGE_MSK) >> BMA150_RANGE_POS; + return 0;
See comments above
quoted hunk ↗ jump to hunk
+} + +static int bma150_set_bandwidth(struct i2c_client *client, unsigned char bw)
Similar problem
quoted hunk ↗ jump to hunk
+static int bma150_read_accel_xyz(struct i2c_client *client, + struct bma150acc *acc) +{ + unsigned char data[6]; + int ret = i2c_smbus_read_i2c_block_data(client, + BMA150_ACC_X_LSB_REG, 6, data); + if (ret != 6) + return -EIO; + + acc->x = ((0xC0 & data[0]) >> 6) | (data[1] << 2); + acc->y = ((0xC0 & data[2]) >> 6) | (data[3] << 2); + acc->z = ((0xC0 & data[4]) >> 6) | (data[5] << 2); + + /* sign extension */ + acc->x = (s16) (acc->x << 6) >> 6; + acc->y = (s16) (acc->y << 6) >> 6; + acc->z = (s16) (acc->z << 6) >> 6; + + return 0;
Why the separate function and struct
quoted hunk ↗ jump to hunk
+} + +static void bma150_work_func(struct work_struct *work) +{
Threaded IRQ ?
quoted hunk ↗ jump to hunk
+static ssize_t bma150_mode_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = to_i2c_client(dev); + struct bma150_data *bma150 = i2c_get_clientdata(client); + + return sprintf(buf, "%d\n", bma150->mode);
API definition ?
quoted hunk ↗ jump to hunk
+ schedule_delayed_work(&data->work, + msecs_to_jiffies(atomic_read(&data->delay))); +
Why the atomic read ? And there are a ton more issues unfixed in this driver