[PATCH] iio: accel: mma8452: Expose temperature channel

[PATCH] iio: accel: mma8452: Expose temperature channel

[Dylan Howey]

FXLS8471Q devices support temperature readout as 8-bit signed value,
0.960C/LSB, -40C to 125C.

Enabling temperature readout reduces the selected ODR by a factor of
2, e.g. with ODR set to 800Hz and temperature enabled the sample rate
will be 400Hz.

Signed-off-by: Dylan Howey <Dylan.Howey@tennantco.com>
---
 drivers/iio/accel/mma8452.c | 119 ++++++++++++++++++++++++++++++------
 1 file changed, 101 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index eb6e3dc789b2..016dcc14e96e 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -83,8 +83,13 @@
 #define MMA8452_OFF_X				0x2f
 #define MMA8452_OFF_Y				0x30
 #define MMA8452_OFF_Z				0x31
+#define FXLS8471_CLEAR_DRDY			0x33
+#define FXLS8471_TEMP_OUT			0x51
+#define FXLS8471_TEMP_EN_MASK			GENMASK(1, 0)
+#define FXLS8471_CTRL_REG6			0x5b
+#define FXLS8471_CTRL_REG7			0x5c
 
-#define MMA8452_MAX_REG				0x31
+#define MMA8452_MAX_REG				0x5c
 
 #define  MMA8452_INT_DRDY			BIT(0)
 #define  MMA8452_INT_FF_MT			BIT(2)
@@ -103,6 +108,7 @@ struct mma8452_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	u8 ctrl_reg1;
+	u8 ctrl_reg6;
 	u8 data_cfg;
 	const struct mma_chip_info *chip_info;
 };
@@ -113,6 +119,7 @@ struct mma8452_data {
  * @channels:			struct iio_chan_spec matching the device's
  *				capabilities
  * @num_channels:		number of channels
+ * @avaiable_scan_masks:		bitmask of available scan elements
  * @mma_scales:			scale factors for converting register values
  *				to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
  *				per mode: m/s^2 and micro m/s^2
@@ -139,6 +146,7 @@ struct mma_chip_info {
 	u8 chip_id;
 	const struct iio_chan_spec *channels;
 	int num_channels;
+	unsigned long available_scan_masks[2];
 	const int mma_scales[3][2];
 	u8 ev_cfg;
 	u8 ev_cfg_ele;
@@ -156,6 +164,7 @@ enum {
 	idx_x,
 	idx_y,
 	idx_z,
+	idx_temp,
 	idx_ts,
 };
 
@@ -204,7 +213,7 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
 	return 0;
 }
 
-static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
+static int mma8452_read(struct mma8452_data *data, u8 buf[7])
 {
 	int ret = mma8452_drdy(data);
 
@@ -216,7 +225,19 @@ static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
 		return ret;
 
 	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
-					    3 * sizeof(__be16), (u8 *)buf);
+					    3 * sizeof(__be16), buf);
+
+	if (FXLS8471_TEMP_EN_MASK & data->ctrl_reg6) {
+		buf[6] = i2c_smbus_read_byte_data(data->client,
+							FXLS8471_TEMP_OUT);
+
+		/*
+		 * A read of the dummy register 33h is required to reset drdy
+		 * for the next interrupt cycle after reading the temperature.
+		 * Value read from register is don't care.
+		 */
+		i2c_smbus_read_byte_data(data->client, FXLS8471_CLEAR_DRDY);
+	}
 
 	ret = mma8452_set_runtime_pm_state(data->client, false);
 
@@ -454,7 +475,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 			    int *val, int *val2, long mask)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
-	__be16 buffer[3];
+	u8 buffer[7];
 	int i, ret;
 
 	switch (mask) {
@@ -470,17 +491,37 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		*val = sign_extend32(be16_to_cpu(
-			buffer[chan->scan_index]) >> chan->scan_type.shift,
+		switch (chan->type) {
+		case IIO_ACCEL:
+			*val = sign_extend32(be16_to_cpu(
+			*(__be16 *)&buffer[2 * chan->scan_index])
+			>> chan->scan_type.shift,
 			chan->scan_type.realbits - 1);
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			if (!(FXLS8471_TEMP_EN_MASK & data->ctrl_reg6))
+				return -EINVAL;
+
+			*val = sign_extend32(buffer[6], 7);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		i = data->data_cfg & MMA8452_DATA_CFG_FS_MASK;
-		*val = data->chip_info->mma_scales[i][0];
-		*val2 = data->chip_info->mma_scales[i][1];
-
-		return IIO_VAL_INT_PLUS_MICRO;
+		switch (chan->type) {
+		case IIO_ACCEL:
+			i = data->data_cfg & MMA8452_DATA_CFG_FS_MASK;
+			*val = data->chip_info->mma_scales[i][0];
+			*val2 = data->chip_info->mma_scales[i][1];
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_TEMP:
+			*val = 960;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		i = mma8452_get_odr_index(data);
 		*val = mma8452_samp_freq[i][0];
@@ -517,6 +558,12 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 
 		*val = mma8452_os_ratio[ret][i];
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = 50; /* 0 LSB @ 25 degree C */
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_ENABLE:
+		*val = !!(data->ctrl_reg6 & FXLS8471_TEMP_EN_MASK);
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
@@ -731,6 +778,16 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 			}
 		}
 		break;
+
+	case IIO_CHAN_INFO_ENABLE:
+		if (val)
+			data->ctrl_reg6 |= FXLS8471_TEMP_EN_MASK;
+		else
+			data->ctrl_reg6 &= ~FXLS8471_TEMP_EN_MASK;
+		/* Note: enabling temperature reduces ODR by a factor of 2. */
+		return mma8452_change_config(data, FXLS8471_CTRL_REG6,
+				data->ctrl_reg6);
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -1003,10 +1060,10 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mma8452_data *data = iio_priv(indio_dev);
-	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
+	u8 buffer[16]; /* 3 16-bit channels + temp + padding + ts */
 	int ret;
 
-	ret = mma8452_read(data, (__be16 *)buffer);
+	ret = mma8452_read(data, buffer);
 	if (ret < 0)
 		goto done;
 
@@ -1159,6 +1216,19 @@ static struct attribute_group mma8452_event_attribute_group = {
 	.num_event_specs = ARRAY_SIZE(mma8452_motion_event), \
 }
 
+#define FXLS8471_TEMP_CHANNEL { \
+	.type = IIO_TEMP, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) | \
+		BIT(IIO_CHAN_INFO_ENABLE), \
+	.scan_index = idx_temp, \
+	.scan_type = { \
+		.sign = 's', \
+		.realbits = 8, \
+		.storagebits = 8, \
+	}, \
+}
+
 static const struct iio_chan_spec mma8451_channels[] = {
 	MMA8452_CHANNEL(X, idx_x, 14),
 	MMA8452_CHANNEL(Y, idx_y, 14),
@@ -1199,6 +1269,15 @@ static const struct iio_chan_spec mma8653_channels[] = {
 	MMA8652_FREEFALL_CHANNEL(IIO_MOD_X_AND_Y_AND_Z),
 };
 
+static const struct iio_chan_spec fxls8471_channels[] = {
+	MMA8452_CHANNEL(X, idx_x, 14),
+	MMA8452_CHANNEL(Y, idx_y, 14),
+	MMA8452_CHANNEL(Z, idx_z, 14),
+	FXLS8471_TEMP_CHANNEL,
+	IIO_CHAN_SOFT_TIMESTAMP(idx_ts),
+	MMA8452_FREEFALL_CHANNEL(IIO_MOD_X_AND_Y_AND_Z),
+};
+
 enum {
 	mma8451,
 	mma8452,
@@ -1213,6 +1292,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
 		.chip_id = MMA8451_DEVICE_ID,
 		.channels = mma8451_channels,
 		.num_channels = ARRAY_SIZE(mma8451_channels),
+		.available_scan_masks = {0x7, 0},
 		/*
 		 * Hardware has fullscale of -2G, -4G, -8G corresponding to
 		 * raw value -8192 for 14 bit, -2048 for 12 bit or -512 for 10
@@ -1237,6 +1317,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
 		.chip_id = MMA8452_DEVICE_ID,
 		.channels = mma8452_channels,
 		.num_channels = ARRAY_SIZE(mma8452_channels),
+		.available_scan_masks = {0x7, 0},
 		.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
 		.ev_cfg = MMA8452_TRANSIENT_CFG,
 		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
@@ -1253,6 +1334,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
 		.chip_id = MMA8453_DEVICE_ID,
 		.channels = mma8453_channels,
 		.num_channels = ARRAY_SIZE(mma8453_channels),
+		.available_scan_masks = {0x7, 0},
 		.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
 		.ev_cfg = MMA8452_TRANSIENT_CFG,
 		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
@@ -1269,6 +1351,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
 		.chip_id = MMA8652_DEVICE_ID,
 		.channels = mma8652_channels,
 		.num_channels = ARRAY_SIZE(mma8652_channels),
+		.available_scan_masks = {0x7, 0},
 		.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
 		.ev_cfg = MMA8452_FF_MT_CFG,
 		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
@@ -1285,6 +1368,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
 		.chip_id = MMA8653_DEVICE_ID,
 		.channels = mma8653_channels,
 		.num_channels = ARRAY_SIZE(mma8653_channels),
+		.available_scan_masks = {0x7, 0},
 		.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
 		.ev_cfg = MMA8452_FF_MT_CFG,
 		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
@@ -1299,8 +1383,9 @@ static const struct mma_chip_info mma_chip_info_table[] = {
 	},
 	[fxls8471] = {
 		.chip_id = FXLS8471_DEVICE_ID,
-		.channels = mma8451_channels,
-		.num_channels = ARRAY_SIZE(mma8451_channels),
+		.channels = fxls8471_channels,
+		.num_channels = ARRAY_SIZE(fxls8471_channels),
+		.available_scan_masks = {0xf, 0},
 		.mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
 		.ev_cfg = MMA8452_TRANSIENT_CFG,
 		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
@@ -1340,8 +1425,6 @@ static const struct iio_info mma8452_info = {
 	.driver_module = THIS_MODULE,
 };
 
-static const unsigned long mma8452_scan_masks[] = {0x7, 0};
-
 static int mma8452_data_rdy_trigger_set_state(struct iio_trigger *trig,
 					      bool state)
 {
@@ -1487,7 +1570,7 @@ static int mma8452_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = data->chip_info->channels;
 	indio_dev->num_channels = data->chip_info->num_channels;
-	indio_dev->available_scan_masks = mma8452_scan_masks;
+	indio_dev->available_scan_masks = data->chip_info->available_scan_masks;
 
 	ret = mma8452_reset(client);
 	if (ret < 0)
-- 
2.17.1

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Jonathan Cameron]

On Wed,  5 Feb 2020 14:32:40 -0600
Dylan Howey <Dylan.Howey@tennantco.com> wrote:

> FXLS8471Q devices support temperature readout as 8-bit signed value,
> 0.960C/LSB, -40C to 125C.
> 
> Enabling temperature readout reduces the selected ODR by a factor of
> 2, e.g. with ODR set to 800Hz and temperature enabled the sample rate
> will be 400Hz.
That needs to be reflected in the interfaces, so if enabled sampling_frequency
needs to halve.

> 
> Signed-off-by: Dylan Howey <Dylan.Howey@tennantco.com>

Using enable as you have here creates a non standard userspace interface
that is effectively unusable by generic code.  Various suggestions inline
but in short, I'd expect:

1) No enable.  We work that out implicitly.
   A) sysfs read results in temp being enabled for the reading and disabled
      afterwards
   B) buffered read allows for explicit control of whether the temp channel
      is on or not.  If it's enabled then the sampling frequency will half
2) Sampling frequency attribute needs to output the new value if the temp
   channel is enabled.  Don't worry too much on it being missleading for sysfs
   reads, people care about that interface for buffered reads.

Anyhow, good to see this support and bad luck that its a bit fiddly.  I'm
guessing that's why no one has taken it on before!

Jonathan

> ---
>  drivers/iio/accel/mma8452.c | 119 ++++++++++++++++++++++++++++++------
>  1 file changed, 101 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index eb6e3dc789b2..016dcc14e96e 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -83,8 +83,13 @@
>  #define MMA8452_OFF_X				0x2f
>  #define MMA8452_OFF_Y				0x30
>  #define MMA8452_OFF_Z				0x31
> +#define FXLS8471_CLEAR_DRDY			0x33
> +#define FXLS8471_TEMP_OUT			0x51
> +#define FXLS8471_TEMP_EN_MASK			GENMASK(1, 0)
> +#define FXLS8471_CTRL_REG6			0x5b
> +#define FXLS8471_CTRL_REG7			0x5c
>  
> -#define MMA8452_MAX_REG				0x31
> +#define MMA8452_MAX_REG				0x5c
>  
>  #define  MMA8452_INT_DRDY			BIT(0)
>  #define  MMA8452_INT_FF_MT			BIT(2)
> @@ -103,6 +108,7 @@ struct mma8452_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
>  	u8 ctrl_reg1;
> +	u8 ctrl_reg6;
>  	u8 data_cfg;
>  	const struct mma_chip_info *chip_info;
>  };
> @@ -113,6 +119,7 @@ struct mma8452_data {
>   * @channels:			struct iio_chan_spec matching the device's
>   *				capabilities
>   * @num_channels:		number of channels
> + * @avaiable_scan_masks:		bitmask of available scan elements
>   * @mma_scales:			scale factors for converting register values
>   *				to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
>   *				per mode: m/s^2 and micro m/s^2
> @@ -139,6 +146,7 @@ struct mma_chip_info {
>  	u8 chip_id;
>  	const struct iio_chan_spec *channels;
>  	int num_channels;
> +	unsigned long available_scan_masks[2];
>  	const int mma_scales[3][2];
>  	u8 ev_cfg;
>  	u8 ev_cfg_ele;
> @@ -156,6 +164,7 @@ enum {
>  	idx_x,
>  	idx_y,
>  	idx_z,
> +	idx_temp,
>  	idx_ts,
>  };
>  
> @@ -204,7 +213,7 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
>  	return 0;
>  }
>  
> -static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
> +static int mma8452_read(struct mma8452_data *data, u8 buf[7])
>  {
>  	int ret = mma8452_drdy(data);
>  
> @@ -216,7 +225,19 @@ static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
>  		return ret;
>  
>  	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
> -					    3 * sizeof(__be16), (u8 *)buf);
> +					    3 * sizeof(__be16), buf);
> +
> +	if (FXLS8471_TEMP_EN_MASK & data->ctrl_reg6) {
> +		buf[6] = i2c_smbus_read_byte_data(data->client,
> +							FXLS8471_TEMP_OUT);

I'd use an extra parameter passed into mma8452_read. Avoids need to end up
with loosing the endian markings.

> +
> +		/*
> +		 * A read of the dummy register 33h is required to reset drdy
> +		 * for the next interrupt cycle after reading the temperature.
> +		 * Value read from register is don't care.
> +		 */
> +		i2c_smbus_read_byte_data(data->client, FXLS8471_CLEAR_DRDY);
> +	}
>  
>  	ret = mma8452_set_runtime_pm_state(data->client, false);
>  
> @@ -454,7 +475,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	__be16 buffer[3];
> +	u8 buffer[7];
>  	int i, ret;
>  
>  	switch (mask) {
> @@ -470,17 +491,37 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		*val = sign_extend32(be16_to_cpu(
> -			buffer[chan->scan_index]) >> chan->scan_type.shift,
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*val = sign_extend32(be16_to_cpu(
> +			*(__be16 *)&buffer[2 * chan->scan_index])
> +			>> chan->scan_type.shift,
>  			chan->scan_type.realbits - 1);
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			if (!(FXLS8471_TEMP_EN_MASK & data->ctrl_reg6))
> +				return -EINVAL;

If it's there and a read occurs, userspace expects to get an answer.
So if this happens you should turn it on and wait appropriate time to
be able to get a reading.

> +
> +			*val = sign_extend32(buffer[6], 7);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
>  
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		i = data->data_cfg & MMA8452_DATA_CFG_FS_MASK;
> -		*val = data->chip_info->mma_scales[i][0];
> -		*val2 = data->chip_info->mma_scales[i][1];
> -
> -		return IIO_VAL_INT_PLUS_MICRO;
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			i = data->data_cfg & MMA8452_DATA_CFG_FS_MASK;
> +			*val = data->chip_info->mma_scales[i][0];
> +			*val2 = data->chip_info->mma_scales[i][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_TEMP:
> +			*val = 960;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		i = mma8452_get_odr_index(data);
>  		*val = mma8452_samp_freq[i][0];
> @@ -517,6 +558,12 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>  
>  		*val = mma8452_os_ratio[ret][i];
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 50; /* 0 LSB @ 25 degree C */
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_ENABLE:
> +		*val = !!(data->ctrl_reg6 & FXLS8471_TEMP_EN_MASK);
> +		return IIO_VAL_INT;
>  	}
>  
>  	return -EINVAL;
> @@ -731,6 +778,16 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>  			}
>  		}
>  		break;
> +
> +	case IIO_CHAN_INFO_ENABLE:
> +		if (val)
> +			data->ctrl_reg6 |= FXLS8471_TEMP_EN_MASK;
> +		else
> +			data->ctrl_reg6 &= ~FXLS8471_TEMP_EN_MASK;
> +		/* Note: enabling temperature reduces ODR by a factor of 2. */

Ouch. That explains the why enable control question. However, it still creates
an unusual userspace interface which basically means no generic code will ever
turn it on.  

Can we instead deal with it as:
1) Sysfs read.   Who cares if the temp read is slow, turn it on only
   when a read occurs.

2) Buffer. Support explicit enable / disable by providing the available_scan_mask
entries and only pay the penalty if it's enabled.  Given there is a sampling
frequency control you need to deal with that changing as well when the
channel is enabled.

> +		return mma8452_change_config(data, FXLS8471_CTRL_REG6,
> +				data->ctrl_reg6);
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -1003,10 +1060,10 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
> +	u8 buffer[16]; /* 3 16-bit channels + temp + padding + ts */
>  	int ret;
>  
> -	ret = mma8452_read(data, (__be16 *)buffer);
> +	ret = mma8452_read(data, buffer);
>  	if (ret < 0)
>  		goto done;
>  
> @@ -1159,6 +1216,19 @@ static struct attribute_group mma8452_event_attribute_group = {
>  	.num_event_specs = ARRAY_SIZE(mma8452_motion_event), \
>  }
>  
> +#define FXLS8471_TEMP_CHANNEL { \
> +	.type = IIO_TEMP, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) | \
> +		BIT(IIO_CHAN_INFO_ENABLE), \

Why does this have an enable control?  That would normally be implicit
in us attempting to read from the device, either via sysfs or by enabling
buffered output of the channel.

> +	.scan_index = idx_temp, \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 8, \
> +		.storagebits = 8, \
> +	}, \
> +}
> +
>  static const struct iio_chan_spec mma8451_channels[] = {
>  	MMA8452_CHANNEL(X, idx_x, 14),
>  	MMA8452_CHANNEL(Y, idx_y, 14),
> @@ -1199,6 +1269,15 @@ static const struct iio_chan_spec mma8653_channels[] = {
>  	MMA8652_FREEFALL_CHANNEL(IIO_MOD_X_AND_Y_AND_Z),
>  };
>  
> +static const struct iio_chan_spec fxls8471_channels[] = {
> +	MMA8452_CHANNEL(X, idx_x, 14),
> +	MMA8452_CHANNEL(Y, idx_y, 14),
> +	MMA8452_CHANNEL(Z, idx_z, 14),
> +	FXLS8471_TEMP_CHANNEL,
> +	IIO_CHAN_SOFT_TIMESTAMP(idx_ts),
> +	MMA8452_FREEFALL_CHANNEL(IIO_MOD_X_AND_Y_AND_Z),
> +};
> +
>  enum {
>  	mma8451,
>  	mma8452,
> @@ -1213,6 +1292,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>  		.chip_id = MMA8451_DEVICE_ID,
>  		.channels = mma8451_channels,
>  		.num_channels = ARRAY_SIZE(mma8451_channels),
> +		.available_scan_masks = {0x7, 0},
>  		/*
>  		 * Hardware has fullscale of -2G, -4G, -8G corresponding to
>  		 * raw value -8192 for 14 bit, -2048 for 12 bit or -512 for 10
> @@ -1237,6 +1317,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>  		.chip_id = MMA8452_DEVICE_ID,
>  		.channels = mma8452_channels,
>  		.num_channels = ARRAY_SIZE(mma8452_channels),
> +		.available_scan_masks = {0x7, 0},
>  		.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
>  		.ev_cfg = MMA8452_TRANSIENT_CFG,
>  		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
> @@ -1253,6 +1334,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>  		.chip_id = MMA8453_DEVICE_ID,
>  		.channels = mma8453_channels,
>  		.num_channels = ARRAY_SIZE(mma8453_channels),
> +		.available_scan_masks = {0x7, 0},
>  		.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
>  		.ev_cfg = MMA8452_TRANSIENT_CFG,
>  		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
> @@ -1269,6 +1351,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>  		.chip_id = MMA8652_DEVICE_ID,
>  		.channels = mma8652_channels,
>  		.num_channels = ARRAY_SIZE(mma8652_channels),
> +		.available_scan_masks = {0x7, 0},
>  		.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
>  		.ev_cfg = MMA8452_FF_MT_CFG,
>  		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
> @@ -1285,6 +1368,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>  		.chip_id = MMA8653_DEVICE_ID,
>  		.channels = mma8653_channels,
>  		.num_channels = ARRAY_SIZE(mma8653_channels),
> +		.available_scan_masks = {0x7, 0},
>  		.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
>  		.ev_cfg = MMA8452_FF_MT_CFG,
>  		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
> @@ -1299,8 +1383,9 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>  	},
>  	[fxls8471] = {
>  		.chip_id = FXLS8471_DEVICE_ID,
> -		.channels = mma8451_channels,
> -		.num_channels = ARRAY_SIZE(mma8451_channels),
> +		.channels = fxls8471_channels,
> +		.num_channels = ARRAY_SIZE(fxls8471_channels),
> +		.available_scan_masks = {0xf, 0},
>  		.mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
>  		.ev_cfg = MMA8452_TRANSIENT_CFG,
>  		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
> @@ -1340,8 +1425,6 @@ static const struct iio_info mma8452_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> -static const unsigned long mma8452_scan_masks[] = {0x7, 0};
> -
>  static int mma8452_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  					      bool state)
>  {
> @@ -1487,7 +1570,7 @@ static int mma8452_probe(struct i2c_client *client,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = data->chip_info->channels;
>  	indio_dev->num_channels = data->chip_info->num_channels;
> -	indio_dev->available_scan_masks = mma8452_scan_masks;
> +	indio_dev->available_scan_masks = data->chip_info->available_scan_masks;
>  
>  	ret = mma8452_reset(client);
>  	if (ret < 0)

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Dylan Howey]

Thank you for the quick feedback. I'll send another patch soon.

-- 
Dylan

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Dylan Howey]

I tried disabling the temperature channel after every raw read, but I'm
running into some issues. When the channel is disabled the data ready
bits in ctrl_reg1 won't go high anymore, and I'm doing the dummy read of
33h as the datasheet says. Data ready interrupts also stop working. So
something is not right with this particular hardware. If you leave the
channel enabled everything works good.

What are my options? I could implicitly enable the channel the first
time it is used and leave it on, but it seems bad to leave the channel
enabled. In that case there would be no other way to disable the channel
other than to reload the module.

Maybe an argument passed to the module to enable temperature output or a
compile time flag to do this?

-- 
Dylan

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Jonathan Cameron]

On Thu, 13 Feb 2020 16:04:08 -0600
Dylan Howey <Dylan.Howey@tennantco.com> wrote:

> I tried disabling the temperature channel after every raw read, but I'm
> running into some issues. When the channel is disabled the data ready
> bits in ctrl_reg1 won't go high anymore, and I'm doing the dummy read of
> 33h as the datasheet says. Data ready interrupts also stop working. So
> something is not right with this particular hardware. If you leave the
> channel enabled everything works good.
> 
> What are my options? I could implicitly enable the channel the first
> time it is used and leave it on, but it seems bad to leave the channel
> enabled. In that case there would be no other way to disable the channel
> other than to reload the module.
> 
> Maybe an argument passed to the module to enable temperature output or a
> compile time flag to do this?

So ideally we'd get someone at Freescale to confirm there is a hardware
rather than a documentation issue (i.e. maybe we need some other magic).

Assuming we can't confirm that, then compile time is not a good idea.
Could do a module parameter.   The reason we might not want to always
have it on is that it limits the maximum sampling frequency. 
The question is whether anyone actually cares about 800Hz?

If they do we probably need to avoid breaking them so would need a module
parameter to be 'enable temp' rather than the more natural option
of 'disable temp' to be set only by people who need the highest
frequency.

Jonathan
> 

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Dylan Howey]

On 02/14/2020 13:24, Jonathan Cameron wrote:
> So ideally we'd get someone at Freescale to confirm there is a hardware
> rather than a documentation issue (i.e. maybe we need some other magic).
I'll try that route first. I have an NXP contact.

> If they do we probably need to avoid breaking them so would need a module
> parameter to be 'enable temp' rather than the more natural option
> of 'disable temp' to be set only by people who need the highest
> frequency.
That's what I had in mind.

> The question is whether anyone actually cares about 800Hz?
I have a use case for this. I'm doing some data logging with this hardware
and a higher sample rate is always better.

By the way, this hardware has a fifo that the driver isn't using. I might
add support for that. Currently I'm trying to log and getting hammered with
an 800Hz interrupt and that is causing some issues.

-- 
Dylan

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Jonathan Cameron]

On Mon, 17 Feb 2020 08:19:08 -0600
Dylan Howey <Dylan.Howey@tennantco.com> wrote:

> On 02/14/2020 13:24, Jonathan Cameron wrote:
> > So ideally we'd get someone at Freescale to confirm there is a hardware
> > rather than a documentation issue (i.e. maybe we need some other magic).  
> I'll try that route first. I have an NXP contact.
> 
> > If they do we probably need to avoid breaking them so would need a module
> > parameter to be 'enable temp' rather than the more natural option
> > of 'disable temp' to be set only by people who need the highest
> > frequency.  
> That's what I had in mind.
> 
> > The question is whether anyone actually cares about 800Hz?  
> I have a use case for this. I'm doing some data logging with this hardware
> and a higher sample rate is always better.
> 
> By the way, this hardware has a fifo that the driver isn't using. I might
> add support for that. Currently I'm trying to log and getting hammered with
> an 800Hz interrupt and that is causing some issues.

Yeah.  Those sorts of rates tend to get tricky without using hardware
fifos.  Great if you can add support.

Thanks,

Jonathan

> 

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Dylan Howey]

On 02/14/2020 13:24, Jonathan Cameron wrote:
> So ideally we'd get someone at Freescale to confirm there is a hardware
> rather than a documentation issue (i.e. maybe we need some other magic).
  
NXP says there's a documentation issue. CTRL_REG6 can only be written
when the device is in standby, similar to the other CTRL_REG* registers.

So I'll need to use mma8452_change_config to set this register. I will
try this out and send a patch.

One other detail - the temperature cannot be buffered by the hwfifo.
If I add hwfifo support, I think the right thing to do would be to
disable hwfifo implicitly if the temperature channel is enabled and
fall back to one interrupt per sample.

-- 
Dylan

Re: [PATCH] iio: accel: mma8452: Expose temperature channel

[Jonathan Cameron]

On Mon, 24 Feb 2020 15:00:20 -0600
Dylan Howey <Dylan.Howey@tennantco.com> wrote:

> On 02/14/2020 13:24, Jonathan Cameron wrote:
> > So ideally we'd get someone at Freescale to confirm there is a hardware
> > rather than a documentation issue (i.e. maybe we need some other magic).  
>   
> NXP says there's a documentation issue. CTRL_REG6 can only be written
> when the device is in standby, similar to the other CTRL_REG* registers.
> 
> So I'll need to use mma8452_change_config to set this register. I will
> try this out and send a patch.
> 
> One other detail - the temperature cannot be buffered by the hwfifo.
> If I add hwfifo support, I think the right thing to do would be to
> disable hwfifo implicitly if the temperature channel is enabled and
> fall back to one interrupt per sample.
> 

It's actually fairly uncommon or people to want to sample the temperature
anywhere near as often as acceleration values, so you might be better
off only supporting sysfs polled reads to the temperature channel.

That way there won't be an unusual result of enabling the temperature
channel.

Anyone who wants the temperature (slow changing) can just grab a few
readings alongside using the buffer for the much faster changing
inertial readings.

Jonathan