Re: [PATCH V3] iio: gyro: Add itg3200

[Lars-Peter Clausen <lars@metafoo.de>]

On 01/31/2013 01:17 PM, Manuel Stahl wrote:
> Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>

Having at least a short commit message wouldn't hurt. E.g. something along
the lines of: "This patch adds support for the InvenSense itg3200. The
itg3200 is a three-axis gyro and ..."

I get one build warning:
	CC      drivers/iio/gyro/itg3200_core.o
	drivers/iio/gyro/itg3200_core.c:302: warning: initialization from
	incompatible pointer type


I've overlooked one issue with the frequency read/write functions the
previous review, sorry for that. And one issue with the newly introduced
avail_scan_masks.

[...]
> diff --git a/drivers/iio/gyro/itg3200_buffer.c b/drivers/iio/gyro/itg3200_buffer.c
> new file mode 100644
> index 0000000..d9efa56
> --- /dev/null
> +++ b/drivers/iio/gyro/itg3200_buffer.c
> @@ -0,0 +1,159 @@
[...]
> +
> +static irqreturn_t itg3200_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct itg3200 *st = iio_priv(indio_dev);
> +	__be16 buf[ITG3200_SCAN_ELEMENTS + sizeof(s64)/sizeof(u16)];
> +
> +	if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) {

You don't need this check either, if no channels are selected we'll never
get here in the first place.

> +		int ret = itg3200_read_all_channels(st->i2c, buf);
> +		if (ret < 0)
> +			goto error_ret;
> +	}
> +
> +	if (indio_dev->scan_timestamp)
> +		memcpy(buf + indio_dev->scan_bytes - sizeof(s64),
> +				&pf->timestamp, sizeof(pf->timestamp));
> +
> +	iio_push_to_buffers(indio_dev, (u8 *)buf);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +error_ret:
> +	return IRQ_HANDLED;
> +}
[...]
> +static int itg3200_read_reg_s16(struct iio_dev *indio_dev, u8 lower_reg_address,
> +		int *val)
> +{
[...]
> +}
> +
> +

Excess newline

> +static ssize_t itg3200_read_raw(struct iio_dev *indio_dev,

This triggers the build warning, the return type should be int.

> +		const struct iio_chan_spec *chan,
> +		int *val, int *val2, long info)
> +{
> +	ssize_t ret = 0;

What I meant with the comment in my previous mail was make ret int.

> +	u8 reg;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		reg = (u8)chan->address;
> +		ret = itg3200_read_reg_s16(indio_dev, reg, val);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		if (chan->type == IIO_TEMP)
> +			*val2 = 1000000000/280;
> +		else
> +			*val2 = 1214142; /* (1 / 14,375) * (PI / 180) */
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		/* Only the temperature channel has an offset */
> +		*val = 23000;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t itg3200_read_frequency(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);

I've overlooked this during the last review. This wont work anymore.
Use struct iio_dev *indio_dev = dev_to_iio_dev(dev); instead

> +	int ret;
> +	u8 val;
> +	int sps;
> +
[...]
> +}
> +
> +static ssize_t itg3200_write_frequency(struct device *dev,
> +		struct device_attribute *attr,
> +		const char *buf,
> +		size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);

Same here

[...]
> +}
> +
> +/*
> + *  Reset device and internal registers to the power-up-default settings

Excess space

> + * Use the gyro clock as reference, as suggested by the datasheet
> + */
> +static int itg3200_reset(struct iio_dev *indio_dev)
[...]
> +
> +static int itg3200_initial_setup(struct iio_dev *indio_dev)
> +{
> +	struct itg3200 *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	u8 val;

I'd add a newline after u8 val; but not before it.

> +	ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_ADDRESS, &val);
> +	if (ret)
> +		goto err_ret;
> +
> +	if (((val >> 1) & 0x3f) != 0x34) {
> +		dev_err(&st->i2c->dev, "invalid reg value 0x%02x", val);
> +		ret = -ENXIO;
> +		goto err_ret;
> +	}
> +
> +	ret = itg3200_reset(indio_dev);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = itg3200_enable_full_scale(indio_dev);
> +err_ret:
> +	return ret;
> +}
[...]
> +static int itg3200_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct itg3200 *st;
> +	struct iio_dev *indio_dev;
> +	const unsigned long avail_scan_masks[] = { 0xffffffff, 0x0 };

This needs to be static. Right now it is on the stack and well gone after
probe has been run.

> +
[...]