Re: [PATCH 5/7] iio: mlx90614: Allow tuning EEPROM configuration

[Jonathan Cameron <jic23@kernel.org>]

On 25/02/15 15:55, Vianney le Clément de Saint-Marcq wrote:
> Add device attributes for getting/setting emissivity, IIR, and FIR
> coefficients, and getting the gain (which should not be modified in
> order to keep factory calibration).
> 
> The attributes provide raw values whose meaning is described in the
> datasheet [1].
> 
> Writing to EEPROM requires an explicit erase by writing zero.  In
> addition, it takes 20ms for the erase/write to complete.  During this
> time no EEPROM register should be accessed.  Therefore, two msleep()s
> are added to the write function and a mutex protects against concurrent
> access.
> 
> Since it is not expected to be updated frequently, the configuration
> register is read before modifying it rather than caching it.
> 
> [1] http://melexis.com/Assets/IR-sensor-thermometer-MLX90614-Datasheet-5152.aspx
> 
> Signed-off-by: Vianney le Clément de Saint-Marcq <vianney.leclement@essensium.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Wolfram, bit of odd i2c usage inline I'd like you to take quick look at.
> ---
>  drivers/iio/temperature/mlx90614.c | 164 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 163 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index 0b36746..ab98fb6 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -12,14 +12,16 @@
>   *
>   * (7-bit I2C slave address 0x5a, 100KHz bus speed only!)
>   *
> - * TODO: sleep mode, configuration EEPROM
> + * TODO: sleep mode
>   */
>  
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
>  
>  #define MLX90614_OP_RAM		0x00
>  #define MLX90614_OP_EEPROM	0x20
> @@ -53,8 +55,47 @@
>  
>  struct mlx90614_data {
>  	struct i2c_client *client;
> +	struct mutex lock; /* for EEPROM access only */
>  };
>  
> +/*
> + * Erase an address and write word.
> + * The mutex must be locked before calling.
> + */
> +static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
> +			       u16 value)
> +{
> +	/*
> +	 * Note: The mlx90614 requires a PEC on writing but does not send us a
> +	 * valid PEC on reading.  Hence, we cannot set I2C_CLIENT_PEC in
> +	 * i2c_client.flags.  As a workaround, we use i2c_smbus_xfer here.
> +	 */
That's particularly hideous and someone should shout at whoever implemented that!
Ah well, such is life.  I want a go ahead on doing this from Wolfram however.
It's the sort of thing that might well get randomly broken in the future.
Alternative is to add another flag that indicates to the i2c core that the PEC
that comes back will be wrong.
 
> +	union i2c_smbus_data data;
> +	s32 ret;
> +
> +	dev_dbg(&client->dev, "Writing 0x%x to address 0x%x", value, command);
> +
> +	data.word = 0x0000; /* erase command */
> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
> +			     client->flags | I2C_CLIENT_PEC,
> +			     I2C_SMBUS_WRITE, command,
> +			     I2C_SMBUS_WORD_DATA, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(MLX90614_TIMING_EEPROM);
> +
> +	data.word = value; /* actual write */
> +	ret = i2c_smbus_xfer(client->adapter, client->addr,
> +			     client->flags | I2C_CLIENT_PEC,
> +			     I2C_SMBUS_WRITE, command,
> +			     I2C_SMBUS_WORD_DATA, &data);
> +
> +	msleep(MLX90614_TIMING_EEPROM);
> +
> +	return ret;
> +}
> +
>  static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *channel, int *val,
>  			    int *val2, long mask)
> @@ -111,6 +152,102 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static ssize_t mlx90614_show_emissivity(struct device *dev,
> +					struct device_attribute *devattr,
> +					char *buf)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	s32 ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_EMISSIVITY);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +static ssize_t mlx90614_store_emissivity(struct device *dev,
> +					 struct device_attribute *devattr,
> +					 const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	u16 val;
> +	s32 ret;
> +
> +	if (kstrtou16(buf, 0, &val))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +	ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY, val);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
Emissivity is well defined. I'd like this to be a standard attribute with
full documentation please rather than a driver specific one.  Happy
to have this added to the infomask elements if it make sense.
> +
> +static ssize_t mlx90614_show_config(struct device *dev,
> +				    struct device_attribute *devattr,
> +				    char *buf)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
> +	u16 mask = indio_attr->address & 0xffff;
> +	u16 shift = indio_attr->address >> 16;
> +	s32 ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = (ret & mask) >> shift;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +static ssize_t mlx90614_store_config(struct device *dev,
> +				     struct device_attribute *devattr,
> +				     const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mlx90614_data *data = iio_priv(indio_dev);
> +	struct iio_dev_attr *indio_attr = to_iio_dev_attr(devattr);
> +	u16 mask = indio_attr->address & 0xffff;
> +	u16 shift = indio_attr->address >> 16;
> +	u16 val;
> +	s32 ret;
> +
> +	if (kstrtou16(buf, 0, &val) || val != (val & (mask >> shift)))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
> +	if (ret < 0)
> +		goto out;
> +
> +	val = (val << shift) | ((u16)ret & ~mask);
> +	ret = mlx90614_write_word(data->client, MLX90614_CONFIG, val);
> +
> +out:
> +	mutex_unlock(&data->lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
>  static const struct iio_chan_spec mlx90614_channels[] = {
>  	{
>  		.type = IIO_TEMP,
> @@ -154,7 +291,31 @@ static const struct iio_chan_spec mlx90614_channels[] = {
>  	},
>  };
>  
> +static IIO_DEVICE_ATTR(emissivity, S_IRUGO | S_IWUSR, mlx90614_show_emissivity,
> +		mlx90614_store_emissivity, -1);
> +static IIO_DEVICE_ATTR(iir, S_IRUGO | S_IWUSR,
> +		mlx90614_show_config, mlx90614_store_config,
> +		(MLX90614_CONFIG_IIR_SHIFT << 16) | MLX90614_CONFIG_IIR_MASK);
> +static IIO_DEVICE_ATTR(fir, S_IRUGO | S_IWUSR,
> +		mlx90614_show_config, mlx90614_store_config,
> +		(MLX90614_CONFIG_FIR_SHIFT << 16) | MLX90614_CONFIG_FIR_MASK);
> +static IIO_DEVICE_ATTR(gain, S_IRUGO, mlx90614_show_config, NULL,
> +		(MLX90614_CONFIG_GAIN_SHIFT << 16) | MLX90614_CONFIG_GAIN_MASK);
> +
> +static struct attribute *mlx90614_attr[] = {
> +	&iio_dev_attr_emissivity.dev_attr.attr,
> +	&iio_dev_attr_iir.dev_attr.attr,
Hmm odd to have two different filter types with separate control.
However we have _filter_low_pass_3db_frequency and whilst it is a pain
obviously to map this to that form that's the way to go from a useful
userspace interface point of view.  If we need to extend the filter
description interface (filter type has been suggested before) then please
propose that.

Here I see they are suggesting one filter is effectively for ignoreing
fast passing objects, and the other for noise prevention.  They both
effect the settling time.

Anyhow it's not entirely obvious what the right answer is, but its
definitely not a driver specific interface such as we have.

Peter, any thoughts?
> +	&iio_dev_attr_fir.dev_attr.attr,
> +	&iio_dev_attr_gain.dev_attr.attr,
Please use the standard IIO infomask elements for scale (or perhaps
calibscale - I haven't checked which is appropriate).
> +	NULL
> +};
> +
> +static struct attribute_group mlx90614_attr_group = {
> +	.attrs = mlx90614_attr,
> +};
> +
>  static const struct iio_info mlx90614_info = {
> +	.attrs = &mlx90614_attr_group,
>  	.read_raw = mlx90614_read_raw,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -189,6 +350,7 @@ static int mlx90614_probe(struct i2c_client *client,
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> +	mutex_init(&data->lock);
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->name = id->name;
>