Re: [PATCH] iio:magnetometer:ak8975 move out of staging

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

On 02/10/2013 01:14 PM, Jonathan Cameron wrote:
> On 02/09/2013 04:55 PM, Jonathan Cameron wrote:
>> This driver has been clean and correct for quite some time.
>> It is simple and uses only straight forward standard
>> interfaces.
>>
> I'm starting this branch of the thread to ask about the weird
> manging of i2c in the read function.
> ...
> 
>> +/*
>> + * Helper function to write to the I2C device's registers.
>> + */
>> +static int ak8975_write_data(struct i2c_client *client,
>> +			     u8 reg, u8 val, u8 mask, u8 shift)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct ak8975_data *data = iio_priv(indio_dev);
>> +	u8 regval;
>> +	int ret;
>> +
>> +	regval = (data->reg_cache[reg] & ~mask) | (val << shift);
>> +	ret = i2c_smbus_write_byte_data(client, reg, regval);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "Write to device fails status %x\n", ret);
>> +		return ret;
>> +	}
>> +	data->reg_cache[reg] = regval;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Helper function to read a contiguous set of the I2C device's registers.
>> + */
>> +static int ak8975_read_data(struct i2c_client *client,
>> +			    u8 reg, u8 length, u8 *buffer)
>> +{
>> +	int ret;
>> +	struct i2c_msg msg[2] = {
>> +		{
>> +			.addr = client->addr,
>> +			.flags = I2C_M_NOSTART,
> This is 'unusual'.  The result as I read it is that we get
> something like
> 
> START REG  [ACK] START ADDR [DATA] STOP
> (where [] denotes from device)
> 
> The i2c docs specifically tell you that having this flag in the
> first msg is a bad idea.
> 
>>   Flag I2C_M_NOSTART: 
>>     In a combined transaction, no 'S Addr Wr/Rd [A]' is generated at some
>>     point. For example, setting I2C_M_NOSTART on the second partial message
>>     generates something like:
>>       S Addr Rd [A] [Data] NA Data [A] P
>>     If you set the I2C_M_NOSTART variable for the first partial message,
>>     we do not generate Addr, but we do generate the startbit S. This will
>>     probably confuse all other clients on your bus, so don't try this.
>>
>>     This is often used to gather transmits from multiple data buffers in
>>     system memory into something that appears as a single transfer to the
>>     I2C device but may also be used between direction changes by some
>>     rare devices.
> 
> A far as I can tell this function as it stands doesn't make sense for either
> of the AK8975 read protocols.

Yea, this doesn't make much sense. I suppose this got added by accident and
only was tested with a I2C host driver that does not support I2C_M_NOSTART
and simply ignored the flag and did a normal transfer.

- Lars

> 
>> +			.len = 1,
>> +			.buf = &reg,
>> +		}, {
>> +			.addr = client->addr,
>> +			.flags = I2C_M_RD,
>> +			.len = length,
>> +			.buf = buffer,
>> +		}
>> +	};
>> +
>> +	ret = i2c_transfer(client->adapter, msg, 2);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "Read from device fails\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> [...]