Re: [PATCH] Add a driver to support InvenSense mpu3050 gyroscope chip.

[Jonathan Cameron <jic23@cam.ac.uk>]

On 04/13/11 10:51, Alan Cox wrote:
> From: Joseph Lai <joseph_lai@wistron.com>
> 
> This driver is registered as an input device. An IRQ is required in this
> basic driver configuration.
For others, description of changes is at the top of mpu3050.c
Basically, driver has been stripped of sysfs interfaces entirely to
aid merging.

Couple of trivial nitpicks/comments inline.

...
> +struct mpu3050_sensor {
> +	struct i2c_client *client;
> +	struct device *dev;
> +	struct input_dev *idev;
> +	struct mutex lock;
> +};
> +
> +/**
> + *	mpu3050_xyz_read_reg	-	read the axes values
> + *	@buffer: provide register addr and get register
> + *	@length: length of register
> + *
> + *	Reads the register values in one transaction or returns a negative
> + *	error code on failure/
> + */

Could just fix the length in this. You only ever use it with the value 6.
Hardly seems worth it's own function.  I guess this may be because it gets
used for other things in the full version?  If so it doesn't do any harm
here so might as well leave it alone.
> +static int mpu3050_xyz_read_reg(struct i2c_client *client,
> +			       u8 *buffer, int length)
> +{
> +	struct i2c_msg msg[] = {
> +		{
> +			.addr = client->addr,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = buffer,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = I2C_M_RD,
> +			.len = length,
> +			.buf = buffer,
> +		},
> +	};
> +	return i2c_transfer(client->adapter, msg, 2);
> +}
> +
> +/**
> + *	mpu3050_read_xyz	-	get co-ordinates from device
> + *	@client: i2c address of sensor
> + *	@coords: co-ordinates to update
> + *
> + *	Return the converted X Y and Z co-ordinates from the sensor device
> + */
> +static void mpu3050_read_xyz(struct i2c_client *client,
> +				struct axis_data *coords)
> +{
> +	u8 buffer[6];
> +	buffer[0] = MPU3050_XOUT_H;
> +	mpu3050_xyz_read_reg(client, buffer, 6);
> +	coords->x = buffer[0];
> +	coords->x = coords->x << 8 | buffer[1];

Better to use le16_tocpu or similar rather than
hand rolling these.

> +	coords->y = buffer[2];
> +	coords->y = coords->y << 8 | buffer[3];
> +	coords->z = buffer[4];
> +	coords->z = coords->z << 8 | buffer[5];
> +	dev_dbg(&client->dev, "%s: x %d, y %d, z %d\n", __func__,
> +					coords->x, coords->y, coords->z);
> +}
> +
...