Skip to contentSkip to conversation

iio/axp288_adc: add missing channel info mask

PATCH In review Jacob Pan 2 participants 7 messages Apr 6, 2015 – Apr 12, 2015

Subsystems (3)
IIO X-powers multifunction PMIC device drivers X-powers AXP288 PMIC drivers
lore
Share & tools
lore

Jacob Pan

author
#0 lore raw

PATCH v2 1/1 iio/axp288_adc: add missing channel info mask

Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return if channel havn't info") added a check for valid info masks.

This patch adds missing channel info masks for all ADC channels. Otherwise, iio_read_channel_raw() would return -EINVAL when called by consumer drivers.

Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iio/adc/axp288_adc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/iio/adc/axp288_adc.c +6 -6
index 08bcfb0..56008a8 100644
@@ -53,39 +53,42 @@ static const struct iio_chan_spec const axp288_adc_channels[] = {
 		.channel = 0,
 		.address = AXP288_TS_ADC_H,
 		.datasheet_name = "TS_PIN",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 1,
 		.address = AXP288_PMIC_ADC_H,
 		.datasheet_name = "PMIC_TEMP",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 2,
 		.address = AXP288_GP_ADC_H,
 		.datasheet_name = "GPADC",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 3,
 		.address = AXP20X_BATT_CHRG_I_H,
 		.datasheet_name = "BATT_CHG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 4,
 		.address = AXP20X_BATT_DISCHRG_I_H,
 		.datasheet_name = "BATT_DISCHRG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_VOLTAGE,
 		.channel = 5,
 		.address = AXP20X_BATT_V_H,
 		.datasheet_name = "BATT_V",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	},
 };
 

@@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
 						chan->address))
 			dev_err(&indio_dev->dev, "TS pin restore\n");
 		break;
-	case IIO_CHAN_INFO_PROCESSED:
-		ret = axp288_adc_read_channel(val, chan->address, info->regmap);
-		break;
 	default:
 		ret = -EINVAL;
 	}
Signature · 1 line
1.9.1
maintainer
#1 lore raw
Quoted 8 lines · Jacob Pan

Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return if channel havn't info") added a check for valid info masks.

This patch adds missing channel info masks for all ADC channels. Otherwise, iio_read_channel_raw() would return -EINVAL when called by consumer drivers.

Signed-off-by Jacob Pan <[email protected]>

This makes me a little nervous. Why did we have the separate raw and processed paths in the read_raw function in the first place?

Also this changes the exposed userspace ABI by dropping those processed attributes that already exist..

Jonathan

Quoted 65 lines
---
 drivers/iio/adc/axp288_adc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/iio/adc/axp288_adc.c +6 -6
index 08bcfb0..56008a8 100644
@@ -53,39 +53,42 @@ static const struct iio_chan_spec const axp288_adc_channels[] = {
 		.channel = 0,
 		.address = AXP288_TS_ADC_H,
 		.datasheet_name = "TS_PIN",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 1,
 		.address = AXP288_PMIC_ADC_H,
 		.datasheet_name = "PMIC_TEMP",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 2,
 		.address = AXP288_GP_ADC_H,
 		.datasheet_name = "GPADC",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 3,
 		.address = AXP20X_BATT_CHRG_I_H,
 		.datasheet_name = "BATT_CHG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 4,
 		.address = AXP20X_BATT_DISCHRG_I_H,
 		.datasheet_name = "BATT_DISCHRG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_VOLTAGE,
 		.channel = 5,
 		.address = AXP20X_BATT_V_H,
 		.datasheet_name = "BATT_V",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	},
 };
 

@@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
 						chan->address))
 			dev_err(&indio_dev->dev, "TS pin restore\n");
 		break;
-	case IIO_CHAN_INFO_PROCESSED:
-		ret = axp288_adc_read_channel(val, chan->address, info->regmap);
-		break;
 	default:
 		ret = -EINVAL;
 	}
author
#2 lore raw
Quoted 4 lines · On Thu, 09 Apr 2015 14:18:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 3 lines · Jacob Pan

Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return if channel havn't info") added a check for valid info masks.

Quoted 9 lines
Quoted 5 lines

This patch adds missing channel info masks for all ADC channels. Otherwise, iio_read_channel_raw() would return -EINVAL when called by consumer drivers.

Signed-off-by Jacob Pan <[email protected]>

This makes me a little nervous. Why did we have the separate raw and processed paths in the read_raw function in the first place?

Good point, I failed to explain that the reason to have separate raw and processed was that the raw channels happened to need some special processing in axp288_adc_set_ts(), since I have added a check for address:

	/* channels other than GPADC do not need to switch TS pin */
	if (address != AXP288_GP_ADC_H)
		return 0;

we no longer need special case the channels based on RAW flag. Thinking back it was kind of an abuse.

Quoted 3 lines

Also this changes the exposed userspace ABI by dropping those processed attributes that already exist..

I don't know the current users outside what we develop internally. What is your suggestion?

Quoted 55 lines

Jonathan

Quoted 54 lines
---
 drivers/iio/adc/axp288_adc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
(quoted diff) +0 -0

b/drivers/iio/adc/axp288_adc.c index 08bcfb0..56008a8 100644

drivers/iio/adc/axp288_adc.c +6 -3
@@ -53,39 +53,42 @@ static const struct iio_chan_spec const
 axp288_adc_channels[] = { .channel = 0,
 		.address = AXP288_TS_ADC_H,
 		.datasheet_name = "TS_PIN",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 1,
 		.address = AXP288_PMIC_ADC_H,
 		.datasheet_name = "PMIC_TEMP",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 2,
 		.address = AXP288_GP_ADC_H,
 		.datasheet_name = "GPADC",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 3,
 		.address = AXP20X_BATT_CHRG_I_H,
 		.datasheet_name = "BATT_CHG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 4,
 		.address = AXP20X_BATT_DISCHRG_I_H,
 		.datasheet_name = "BATT_DISCHRG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_VOLTAGE,
 		.channel = 5,
 		.address = AXP20X_BATT_V_H,
 		.datasheet_name = "BATT_V",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	},
 };
Quoted 13 lines
Quoted 10 lines
drivers/iio/adc/axp288_adc.c +0 -3
@@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev
 *indio_dev, chan->address))
 			dev_err(&indio_dev->dev, "TS pin
 restore\n"); break;
-	case IIO_CHAN_INFO_PROCESSED:
-		ret = axp288_adc_read_channel(val, chan->address,
 info->regmap);
-		break;
 	default:
 		ret = -EINVAL;
Quoted 2 lines

}

[Jacob Pan]

maintainer
#3 lore raw
Quoted 24 lines · Jacob Pan
Quoted 4 lines · On Thu, 09 Apr 2015 14:18:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 3 lines · Jacob Pan

Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return if channel havn't info") added a check for valid info masks.

Quoted 9 lines
Quoted 5 lines

This patch adds missing channel info masks for all ADC channels. Otherwise, iio_read_channel_raw() would return -EINVAL when called by consumer drivers.

Signed-off-by Jacob Pan <[email protected]>

This makes me a little nervous. Why did we have the separate raw and processed paths in the read_raw function in the first place?

Good point, I failed to explain that the reason to have separate raw and processed was that the raw channels happened to need some special processing in axp288_adc_set_ts(), since I have added a check for address:

	/* channels other than GPADC do not need to switch TS pin */
	if (address != AXP288_GP_ADC_H)
		return 0;
Quoted 8 lines

we no longer need special case the channels based on RAW flag. Thinking back it was kind of an abuse.

Quoted 3 lines

Also this changes the exposed userspace ABI by dropping those processed attributes that already exist..

I don't know the current users outside what we develop internally. What is your suggestion?

Lets go for it on the basis of it is 'fixing' your abuse of the ABI ;) Then any users should long ago have complained about it anyway (did I say that?) We'll take the age old approach. If no one notices we broke the ABI then it's fine.

Have applied to the fixes-togreg branch of iio.git with the description updated to indicate it also fixes the ABI abuse ;)

Anyhow, won't go now until after the merge window anyway I'm afraid. Marked for stable as well.

Thanks,

Jonathan

Quoted 76 lines
Quoted 56 lines

Jonathan

Quoted 54 lines
---
 drivers/iio/adc/axp288_adc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
(quoted diff) +0 -0

b/drivers/iio/adc/axp288_adc.c index 08bcfb0..56008a8 100644

drivers/iio/adc/axp288_adc.c +6 -3
@@ -53,39 +53,42 @@ static const struct iio_chan_spec const
 axp288_adc_channels[] = { .channel = 0,
 		.address = AXP288_TS_ADC_H,
 		.datasheet_name = "TS_PIN",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 1,
 		.address = AXP288_PMIC_ADC_H,
 		.datasheet_name = "PMIC_TEMP",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 2,
 		.address = AXP288_GP_ADC_H,
 		.datasheet_name = "GPADC",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 3,
 		.address = AXP20X_BATT_CHRG_I_H,
 		.datasheet_name = "BATT_CHG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 4,
 		.address = AXP20X_BATT_DISCHRG_I_H,
 		.datasheet_name = "BATT_DISCHRG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_VOLTAGE,
 		.channel = 5,
 		.address = AXP20X_BATT_V_H,
 		.datasheet_name = "BATT_V",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	},
 };
Quoted 13 lines
Quoted 10 lines
drivers/iio/adc/axp288_adc.c +0 -3
@@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev
 *indio_dev, chan->address))
 			dev_err(&indio_dev->dev, "TS pin
 restore\n"); break;
-	case IIO_CHAN_INFO_PROCESSED:
-		ret = axp288_adc_read_channel(val, chan->address,
 info->regmap);
-		break;
 	default:
 		ret = -EINVAL;
Quoted 2 lines

}

[Jacob Pan]
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#4 lore raw
Quoted 38 lines · Jonathan Cameron
Quoted 24 lines · Jacob Pan
Quoted 4 lines · On Thu, 09 Apr 2015 14:18:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 3 lines · Jacob Pan

Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return if channel havn't info") added a check for valid info masks.

Quoted 9 lines
Quoted 5 lines

This patch adds missing channel info masks for all ADC channels. Otherwise, iio_read_channel_raw() would return -EINVAL when called by consumer drivers.

Signed-off-by Jacob Pan <[email protected]>

This makes me a little nervous. Why did we have the separate raw and processed paths in the read_raw function in the first place?

Good point, I failed to explain that the reason to have separate raw and processed was that the raw channels happened to need some special processing in axp288_adc_set_ts(), since I have added a check for address:

	/* channels other than GPADC do not need to switch TS pin */
	if (address != AXP288_GP_ADC_H)
		return 0;
Quoted 8 lines

we no longer need special case the channels based on RAW flag. Thinking back it was kind of an abuse.

Quoted 3 lines

Also this changes the exposed userspace ABI by dropping those processed attributes that already exist..

I don't know the current users outside what we develop internally. What is your suggestion?

Lets go for it on the basis of it is 'fixing' your abuse of the ABI ;) Then any users should long ago have complained about it anyway (did I say that?) We'll take the age old approach. If no one notices we broke the ABI then it's fine.

Quoted 6 lines

Have applied to the fixes-togreg branch of iio.git with the description updated to indicate it also fixes the ABI abuse ;)

Anyhow, won't go now until after the merge window anyway I'm afraid. Marked for stable as well.

Forgot to say. If it causes any breakage, we simply put back the incorrect processed version as well. That might then cause breakage for any in kernel consumers however as they will use that in preference to the raw version IIRC.

Quoted 85 lines

Thanks,

Jonathan

Quoted 70 lines
Quoted 56 lines

Jonathan

Quoted 54 lines
---
 drivers/iio/adc/axp288_adc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
(quoted diff) +0 -0

b/drivers/iio/adc/axp288_adc.c index 08bcfb0..56008a8 100644

drivers/iio/adc/axp288_adc.c +6 -3
@@ -53,39 +53,42 @@ static const struct iio_chan_spec const
 axp288_adc_channels[] = { .channel = 0,
 		.address = AXP288_TS_ADC_H,
 		.datasheet_name = "TS_PIN",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 1,
 		.address = AXP288_PMIC_ADC_H,
 		.datasheet_name = "PMIC_TEMP",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 2,
 		.address = AXP288_GP_ADC_H,
 		.datasheet_name = "GPADC",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 3,
 		.address = AXP20X_BATT_CHRG_I_H,
 		.datasheet_name = "BATT_CHG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 4,
 		.address = AXP20X_BATT_DISCHRG_I_H,
 		.datasheet_name = "BATT_DISCHRG_I",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_VOLTAGE,
 		.channel = 5,
 		.address = AXP20X_BATT_V_H,
 		.datasheet_name = "BATT_V",
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	},
 };
Quoted 13 lines
Quoted 10 lines
drivers/iio/adc/axp288_adc.c +0 -3
@@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev
 *indio_dev, chan->address))
 			dev_err(&indio_dev->dev, "TS pin
 restore\n"); break;
-	case IIO_CHAN_INFO_PROCESSED:
-		ret = axp288_adc_read_channel(val, chan->address,
 info->regmap);
-		break;
 	default:
 		ret = -EINVAL;
Quoted 2 lines

}

Quoted 6 lines

[Jacob Pan]
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#5 lore raw
Quoted 42 lines · On Thu, 09 Apr 2015 18:52:12 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 41 lines · Jonathan Cameron
Quoted 24 lines · Jacob Pan
Quoted 4 lines · On Thu, 09 Apr 2015 14:18:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 3 lines · Jacob Pan

Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return if channel havn't info") added a check for valid info masks.

Quoted 9 lines
Quoted 5 lines

This patch adds missing channel info masks for all ADC channels. Otherwise, iio_read_channel_raw() would return -EINVAL when called by consumer drivers.

Signed-off-by Jacob Pan <[email protected]>

This makes me a little nervous. Why did we have the separate raw and processed paths in the read_raw function in the first place?

Good point, I failed to explain that the reason to have separate raw and processed was that the raw channels happened to need some special processing in axp288_adc_set_ts(), since I have added a check for address:

	/* channels other than GPADC do not need to switch TS pin
*/ if (address != AXP288_GP_ADC_H)
		return 0;
Quoted 8 lines

we no longer need special case the channels based on RAW flag. Thinking back it was kind of an abuse.

Quoted 3 lines

Also this changes the exposed userspace ABI by dropping those processed attributes that already exist..

I don't know the current users outside what we develop internally. What is your suggestion?

Lets go for it on the basis of it is 'fixing' your abuse of the ABI ;) Then any users should long ago have complained about it anyway (did I say that?) We'll take the age old approach. If no one notices we broke the ABI then it's fine.

Have applied to the fixes-togreg branch of iio.git with the description updated to indicate it also fixes the ABI abuse ;)

Quoted 9 lines
Quoted 3 lines

Anyhow, won't go now until after the merge window anyway I'm afraid. Marked for stable as well.

Forgot to say. If it causes any breakage, we simply put back the incorrect processed version as well. That might then cause breakage for any in kernel consumers however as they will use that in preference to the raw version IIRC.

Quoted 2 lines

Thanks,

Good to know. Thanks a lot.
Perhaps I missed it in the kernel, I wish there were more comments/documentation on the enum to explain its use case for hw provided data format, and implication for sysfs abi etc.

enum iio_chan_info_enum {
	IIO_CHAN_INFO_RAW = 0,
	IIO_CHAN_INFO_PROCESSED,

...

}

Quoted 87 lines
Quoted 86 lines

Jonathan

Quoted 73 lines
Quoted 59 lines

Jonathan

Quoted 57 lines
---
 drivers/iio/adc/axp288_adc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
(quoted diff) +0 -0

b/drivers/iio/adc/axp288_adc.c index 08bcfb0..56008a8 100644

drivers/iio/adc/axp288_adc.c +6 -3
@@ -53,39 +53,42 @@ static const struct iio_chan_spec const
 axp288_adc_channels[] = { .channel = 0,
 		.address = AXP288_TS_ADC_H,
 		.datasheet_name = "TS_PIN",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 1,
 		.address = AXP288_PMIC_ADC_H,
 		.datasheet_name = "PMIC_TEMP",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 2,
 		.address = AXP288_GP_ADC_H,
 		.datasheet_name = "GPADC",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 3,
 		.address = AXP20X_BATT_CHRG_I_H,
 		.datasheet_name = "BATT_CHG_I",
-		.info_mask_separate =
 BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 4,
 		.address = AXP20X_BATT_DISCHRG_I_H,
 		.datasheet_name = "BATT_DISCHRG_I",
-		.info_mask_separate =
 BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_VOLTAGE,
 		.channel = 5,
 		.address = AXP20X_BATT_V_H,
 		.datasheet_name = "BATT_V",
-		.info_mask_separate =
 BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),

},
};

Quoted 13 lines
Quoted 10 lines
drivers/iio/adc/axp288_adc.c +0 -3
@@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev
 *indio_dev, chan->address))
 			dev_err(&indio_dev->dev, "TS pin
 restore\n"); break;
-	case IIO_CHAN_INFO_PROCESSED:
-		ret = axp288_adc_read_channel(val,
 chan->address, info->regmap);
-		break;
 	default:
 		ret = -EINVAL;
Quoted 2 lines

}

Quoted 6 lines

[Jacob Pan]
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line
Jacob Pan
maintainer
#6 lore raw
Quoted 59 lines · Jacob Pan
Quoted 42 lines · On Thu, 09 Apr 2015 18:52:12 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 41 lines · Jonathan Cameron
Quoted 24 lines · Jacob Pan
Quoted 4 lines · On Thu, 09 Apr 2015 14:18:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 3 lines · Jacob Pan

Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return if channel havn't info") added a check for valid info masks.

Quoted 9 lines
Quoted 5 lines

This patch adds missing channel info masks for all ADC channels. Otherwise, iio_read_channel_raw() would return -EINVAL when called by consumer drivers.

Signed-off-by Jacob Pan <[email protected]>

This makes me a little nervous. Why did we have the separate raw and processed paths in the read_raw function in the first place?

Good point, I failed to explain that the reason to have separate raw and processed was that the raw channels happened to need some special processing in axp288_adc_set_ts(), since I have added a check for address:

	/* channels other than GPADC do not need to switch TS pin
*/ if (address != AXP288_GP_ADC_H)
		return 0;
Quoted 8 lines

we no longer need special case the channels based on RAW flag. Thinking back it was kind of an abuse.

Quoted 3 lines

Also this changes the exposed userspace ABI by dropping those processed attributes that already exist..

I don't know the current users outside what we develop internally. What is your suggestion?

Lets go for it on the basis of it is 'fixing' your abuse of the ABI ;) Then any users should long ago have complained about it anyway (did I say that?) We'll take the age old approach. If no one notices we broke the ABI then it's fine.

Have applied to the fixes-togreg branch of iio.git with the description updated to indicate it also fixes the ABI abuse ;)

Quoted 9 lines
Quoted 3 lines

Anyhow, won't go now until after the merge window anyway I'm afraid. Marked for stable as well.

Forgot to say. If it causes any breakage, we simply put back the incorrect processed version as well. That might then cause breakage for any in kernel consumers however as they will use that in preference to the raw version IIRC.

Quoted 2 lines

Thanks,

Good to know. Thanks a lot.
Perhaps I missed it in the kernel, I wish there were more comments/documentation on the enum to explain its use case for hw provided data format, and implication for sysfs abi etc.

Quoted 4 lines
enum iio_chan_info_enum {
	IIO_CHAN_INFO_RAW = 0,
	IIO_CHAN_INFO_PROCESSED,

...

Hmm. More docs almost always good, sadly no one ever gets round to keeping them up to date.

The sysfs side of things fairly neatly lines up with the ABI docs, but I guess we could have more examples. Example wise we do have the dummy driver to illustrate most of the usage. /Documentation/ABI/testing/sysfs-bus-iio /drivers/staging/iio/simple_dummy*

J

Quoted 94 lines

}

Quoted 87 lines
Quoted 86 lines

Jonathan

Quoted 73 lines
Quoted 59 lines

Jonathan

Quoted 57 lines
---
 drivers/iio/adc/axp288_adc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
(quoted diff) +0 -0

b/drivers/iio/adc/axp288_adc.c index 08bcfb0..56008a8 100644

drivers/iio/adc/axp288_adc.c +6 -3
@@ -53,39 +53,42 @@ static const struct iio_chan_spec const
 axp288_adc_channels[] = { .channel = 0,
 		.address = AXP288_TS_ADC_H,
 		.datasheet_name = "TS_PIN",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 1,
 		.address = AXP288_PMIC_ADC_H,
 		.datasheet_name = "PMIC_TEMP",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_TEMP,
 		.channel = 2,
 		.address = AXP288_GP_ADC_H,
 		.datasheet_name = "GPADC",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 3,
 		.address = AXP20X_BATT_CHRG_I_H,
 		.datasheet_name = "BATT_CHG_I",
-		.info_mask_separate =
 BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_CURRENT,
 		.channel = 4,
 		.address = AXP20X_BATT_DISCHRG_I_H,
 		.datasheet_name = "BATT_DISCHRG_I",
-		.info_mask_separate =
 BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 	}, {
 		.indexed = 1,
 		.type = IIO_VOLTAGE,
 		.channel = 5,
 		.address = AXP20X_BATT_V_H,
 		.datasheet_name = "BATT_V",
-		.info_mask_separate =
 BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),

},
};

Quoted 13 lines
Quoted 10 lines
drivers/iio/adc/axp288_adc.c +0 -3
@@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev
 *indio_dev, chan->address))
 			dev_err(&indio_dev->dev, "TS pin
 restore\n"); break;
-	case IIO_CHAN_INFO_PROCESSED:
-		ret = axp288_adc_read_channel(val,
 chan->address, info->regmap);
-		break;
 	default:
 		ret = -EINVAL;
Quoted 2 lines

}

Quoted 6 lines

[Jacob Pan]
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html