On 02/10/2013 10:20 AM, Lars-Peter Clausen wrote:
> On 02/09/2013 05: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.
>>
>> Signed-off-by: Jonathan Cameron <jic23@xxxxxxxxxx>
>
> Two comments, which don't necessarily need to be addressed before moving it
> out of staging, but should probably should be fixed at some point.
>
>> ---
>> drivers/iio/magnetometer/Kconfig | 11 +
>> drivers/iio/magnetometer/Makefile | 1 +
>> drivers/iio/magnetometer/ak8975.c | 520 ++++++++++++++++++++++++++++++
>> drivers/staging/iio/magnetometer/Kconfig | 11 -
>> drivers/staging/iio/magnetometer/Makefile | 1 -
>> drivers/staging/iio/magnetometer/ak8975.c | 520 ------------------------------
>> 6 files changed, 532 insertions(+), 532 deletions(-)
>>
> [...]
>> +struct ak8975_data {
>> + struct i2c_client *client;
>> + struct attribute_group attrs;
>> + struct mutex lock;
>> + u8 asa[3];
>> + long raw_to_gauss[3];
>> + u8 reg_cache[AK8975_MAX_REGS];
>> + int eoc_gpio;
>> + int eoc_irq;
>
> eoc_irq is never really used.
Good spot, I'll put out a trivial patch to remove that before move
this driver.
>
>> +};
>> +
>> +static const int ak8975_index_to_reg[] = {
>> + AK8975_REG_HXL, AK8975_REG_HYL, AK8975_REG_HZL,
>> +};
>> +
>> +/*
>> + * 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;
>
> The reg cache is written to here, but it is never read from again. It may
> make sense to convert the driver to using regmap instead of open-coding the
> register read/write functions.
It is used just above in this function to avoid a read when doing masked writes.
Using regmap might make sense but is not something I'm going to do without
hardware and in this case the gains will be pretty small.
Laxman, do you recall why we have the I2C_M_NOSTART in the driver? If nothing
else the ability to do that should be checked for as it's in the category of
protocol mangling.
>
>> +
>> + return 0;
>> +}
>> +
> [...]
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
