[PATCH v2 0/6] meson saradc: add iio channels to read channel 7 mux inputs

[PATCH v2 0/6] meson saradc: add iio channels to read channel 7 mux inputs

[George Stark]

From: George Stark <GNStark@sberdevices.ru>

Hello Andy

Thanks for review.

Changelog:
v1->v2:
split refactoring patch [1] into 4 smaller patches, fix comment style

[1] https://lore.kernel.org/lkml/20230621062715.455652-2-gnstark@sberdevices.ru/

George Stark (6):
  meson saradc: move enums declaration before variables declaration
  meson saradc: move meson_sar_adc_set_chan7_mux routine upper
  meson saradc: unite iio channel array definitions
  meson saradc: add enum for iio channel array indexes
  meson saradc: add channel labels
  meson saradc: support reading from channel7 mux inputs

 drivers/iio/adc/meson_saradc.c | 164 +++++++++++++++++++++++----------
 1 file changed, 114 insertions(+), 50 deletions(-)

-- 
2.38.4

[PATCH v2 1/6] meson saradc: move enums declaration before variables declaration

[George Stark]

Move enums declaration before variables declaration.

Signed-off-by: George Stark <GNStark@sberdevices.ru>
---
 drivers/iio/adc/meson_saradc.c | 44 +++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 18937a262af6..af38d95bd504 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -202,6 +202,28 @@
 	.datasheet_name = "TEMP_SENSOR",				\
 }
 
+enum meson_sar_adc_avg_mode {
+	NO_AVERAGING = 0x0,
+	MEAN_AVERAGING = 0x1,
+	MEDIAN_AVERAGING = 0x2,
+};
+
+enum meson_sar_adc_num_samples {
+	ONE_SAMPLE = 0x0,
+	TWO_SAMPLES = 0x1,
+	FOUR_SAMPLES = 0x2,
+	EIGHT_SAMPLES = 0x3,
+};
+
+enum meson_sar_adc_chan7_mux_sel {
+	CHAN7_MUX_VSS = 0x0,
+	CHAN7_MUX_VDD_DIV4 = 0x1,
+	CHAN7_MUX_VDD_DIV2 = 0x2,
+	CHAN7_MUX_VDD_MUL3_DIV4 = 0x3,
+	CHAN7_MUX_VDD = 0x4,
+	CHAN7_MUX_CH7_INPUT = 0x7,
+};
+
 static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
 	MESON_SAR_ADC_CHAN(0),
 	MESON_SAR_ADC_CHAN(1),
@@ -227,28 +249,6 @@ static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(9),
 };
 
-enum meson_sar_adc_avg_mode {
-	NO_AVERAGING = 0x0,
-	MEAN_AVERAGING = 0x1,
-	MEDIAN_AVERAGING = 0x2,
-};
-
-enum meson_sar_adc_num_samples {
-	ONE_SAMPLE = 0x0,
-	TWO_SAMPLES = 0x1,
-	FOUR_SAMPLES = 0x2,
-	EIGHT_SAMPLES = 0x3,
-};
-
-enum meson_sar_adc_chan7_mux_sel {
-	CHAN7_MUX_VSS = 0x0,
-	CHAN7_MUX_VDD_DIV4 = 0x1,
-	CHAN7_MUX_VDD_DIV2 = 0x2,
-	CHAN7_MUX_VDD_MUL3_DIV4 = 0x3,
-	CHAN7_MUX_VDD = 0x4,
-	CHAN7_MUX_CH7_INPUT = 0x7,
-};
-
 struct meson_sar_adc_param {
 	bool					has_bl30_integration;
 	unsigned long				clock_rate;
-- 
2.38.4

[PATCH v2 2/6] meson saradc: move meson_sar_adc_set_chan7_mux routine upper

[George Stark]

Move meson_sar_adc_set_chan7_mux routine upper.

Signed-off-by: George Stark <GNStark@sberdevices.ru>
---
 drivers/iio/adc/meson_saradc.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index af38d95bd504..6e69b40b3309 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -338,6 +338,19 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev *indio_dev)
 					       1, 10000);
 }
 
+static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
+					enum meson_sar_adc_chan7_mux_sel sel)
+{
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
+	u32 regval;
+
+	regval = FIELD_PREP(MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, sel);
+	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
+			   MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, regval);
+
+	usleep_range(10, 20);
+}
+
 static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
 					 const struct iio_chan_spec *chan,
 					 int *val)
@@ -434,19 +447,6 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
 	}
 }
 
-static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
-					enum meson_sar_adc_chan7_mux_sel sel)
-{
-	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
-	u32 regval;
-
-	regval = FIELD_PREP(MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, sel);
-	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
-			   MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, regval);
-
-	usleep_range(10, 20);
-}
-
 static void meson_sar_adc_start_sample_engine(struct iio_dev *indio_dev)
 {
 	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
-- 
2.38.4

[PATCH v2 3/6] meson saradc: unite iio channel array definitions

[George Stark]

Instead of having several similar channel arrays which are different
only by one item unite the arrays and put the unique item in the end.

Signed-off-by: George Stark <GNStark@sberdevices.ru>
---
 drivers/iio/adc/meson_saradc.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 6e69b40b3309..ffd4de950a63 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -189,9 +189,8 @@
 	.datasheet_name = "SAR_ADC_CH"#_chan,				\
 }
 
-#define MESON_SAR_ADC_TEMP_CHAN(_chan) {				\
+#define MESON_SAR_ADC_TEMP_CHAN() {					\
 	.type = IIO_TEMP,						\
-	.channel = _chan,						\
 	.address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,		\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
 				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
@@ -234,19 +233,7 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
 	MESON_SAR_ADC_CHAN(6),
 	MESON_SAR_ADC_CHAN(7),
 	IIO_CHAN_SOFT_TIMESTAMP(8),
-};
-
-static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
-	MESON_SAR_ADC_CHAN(0),
-	MESON_SAR_ADC_CHAN(1),
-	MESON_SAR_ADC_CHAN(2),
-	MESON_SAR_ADC_CHAN(3),
-	MESON_SAR_ADC_CHAN(4),
-	MESON_SAR_ADC_CHAN(5),
-	MESON_SAR_ADC_CHAN(6),
-	MESON_SAR_ADC_CHAN(7),
-	MESON_SAR_ADC_TEMP_CHAN(8),
-	IIO_CHAN_SOFT_TIMESTAMP(9),
+	MESON_SAR_ADC_TEMP_CHAN(), /* must be the last item */
 };
 
 struct meson_sar_adc_param {
@@ -1242,15 +1229,11 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	if (priv->temperature_sensor_calibrated) {
-		indio_dev->channels = meson_sar_adc_and_temp_iio_channels;
-		indio_dev->num_channels =
-			ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels);
-	} else {
-		indio_dev->channels = meson_sar_adc_iio_channels;
-		indio_dev->num_channels =
-			ARRAY_SIZE(meson_sar_adc_iio_channels);
-	}
+	indio_dev->channels = meson_sar_adc_iio_channels;
+	indio_dev->num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels);
+	/* last item is temp channel */
+	if (!priv->temperature_sensor_calibrated)
+		indio_dev->num_channels--;
 
 	ret = meson_sar_adc_init(indio_dev);
 	if (ret)
-- 
2.38.4

[PATCH v2 4/6] meson saradc: add enum for iio channel array indexes

[George Stark]

Add enum for iio channel array indexes.

Signed-off-by: George Stark <GNStark@sberdevices.ru>
---
 drivers/iio/adc/meson_saradc.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index ffd4de950a63..7ef006650982 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -223,16 +223,28 @@ enum meson_sar_adc_chan7_mux_sel {
 	CHAN7_MUX_CH7_INPUT = 0x7,
 };
 
+enum meson_sar_adc_channel_index {
+	INDEX_CHAN_0,
+	INDEX_CHAN_1,
+	INDEX_CHAN_2,
+	INDEX_CHAN_3,
+	INDEX_CHAN_4,
+	INDEX_CHAN_5,
+	INDEX_CHAN_6,
+	INDEX_CHAN_7,
+	INDEX_CHAN_SOFT_TIMESTAMP,
+};
+
 static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
-	MESON_SAR_ADC_CHAN(0),
-	MESON_SAR_ADC_CHAN(1),
-	MESON_SAR_ADC_CHAN(2),
-	MESON_SAR_ADC_CHAN(3),
-	MESON_SAR_ADC_CHAN(4),
-	MESON_SAR_ADC_CHAN(5),
-	MESON_SAR_ADC_CHAN(6),
-	MESON_SAR_ADC_CHAN(7),
-	IIO_CHAN_SOFT_TIMESTAMP(8),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_0),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_1),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_2),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_3),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_4),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_5),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_6),
+	MESON_SAR_ADC_CHAN(INDEX_CHAN_7),
+	IIO_CHAN_SOFT_TIMESTAMP(INDEX_CHAN_SOFT_TIMESTAMP),
 	MESON_SAR_ADC_TEMP_CHAN(), /* must be the last item */
 };
 
@@ -1003,7 +1015,7 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
 	meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_VDD_DIV4);
 	usleep_range(10, 20);
 	ret = meson_sar_adc_get_sample(indio_dev,
-				       &indio_dev->channels[7],
+				       &indio_dev->channels[INDEX_CHAN_7],
 				       MEAN_AVERAGING, EIGHT_SAMPLES, &value0);
 	if (ret < 0)
 		goto out;
@@ -1011,7 +1023,7 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
 	meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_VDD_MUL3_DIV4);
 	usleep_range(10, 20);
 	ret = meson_sar_adc_get_sample(indio_dev,
-				       &indio_dev->channels[7],
+				       &indio_dev->channels[INDEX_CHAN_7],
 				       MEAN_AVERAGING, EIGHT_SAMPLES, &value1);
 	if (ret < 0)
 		goto out;
-- 
2.38.4

[PATCH v2 5/6] meson saradc: add channel labels

[George Stark]

Add attribute 'label' to all iio channles

Signed-off-by: George Stark <GNStark@sberdevices.ru>
---

Changelog:
v1->v2: update commit message from [1]

[1] https://lore.kernel.org/lkml/20230621062715.455652-3-gnstark@sberdevices.ru/

---
 drivers/iio/adc/meson_saradc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 7ef006650982..e7eb154b151f 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -1044,8 +1044,20 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
 	return ret;
 }
 
+static int read_label(struct iio_dev *indio_dev,
+		      struct iio_chan_spec const *chan,
+		      char *label)
+{
+	if (chan->type == IIO_TEMP)
+		return sprintf(label, "%s\n", "temp-sensor");
+	if (chan->type == IIO_VOLTAGE)
+		return sprintf(label, "channel-%d\n", chan->channel);
+	return 0;
+}
+
 static const struct iio_info meson_sar_adc_iio_info = {
 	.read_raw = meson_sar_adc_iio_info_read_raw,
+	.read_label = read_label,
 };
 
 static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
-- 
2.38.4

[PATCH v2 6/6] meson saradc: support reading from channel7 mux inputs

[George Stark]

Add iio channel for every channel 7 muxer input.
Meson saradc channel 7 is connected to muxer that can switch channel
input to well-known sources like Vdd, GND and several Vdd dividers.

Signed-off-by: George Stark <GNStark@sberdevices.ru>
---

Changelog:
v1->v2: update commit message from [1]

[1] https://lore.kernel.org/lkml/20230621062715.455652-4-gnstark@sberdevices.ru/

---
 drivers/iio/adc/meson_saradc.c | 65 +++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index e7eb154b151f..ecaebb569e8e 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -163,6 +163,7 @@
 #define MESON_SAR_ADC_MAX_FIFO_SIZE				32
 #define MESON_SAR_ADC_TIMEOUT					100 /* ms */
 #define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL			6
+#define MESON_SAR_ADC_VOLTAGE_AND_MUX_CHANNEL			7
 #define MESON_SAR_ADC_TEMP_OFFSET				27
 
 /* temperature sensor calibration information in eFuse */
@@ -201,6 +202,19 @@
 	.datasheet_name = "TEMP_SENSOR",				\
 }
 
+#define MESON_SAR_ADC_MUX(_chan, _sel) {				\
+	.type = IIO_VOLTAGE,						\
+	.channel = _chan,						\
+	.indexed = 1,							\
+	.address = MESON_SAR_ADC_VOLTAGE_AND_MUX_CHANNEL,		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
+				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
+	.datasheet_name = "SAR_ADC_MUX_"#_sel,				\
+}
+
 enum meson_sar_adc_avg_mode {
 	NO_AVERAGING = 0x0,
 	MEAN_AVERAGING = 0x1,
@@ -233,6 +247,27 @@ enum meson_sar_adc_channel_index {
 	INDEX_CHAN_6,
 	INDEX_CHAN_7,
 	INDEX_CHAN_SOFT_TIMESTAMP,
+	INDEX_MUX_0_VSS,
+	INDEX_MUX_1_VDD_DIV4,
+	INDEX_MUX_2_VDD_DIV2,
+	INDEX_MUX_3_VDD_MUL3_DIV4,
+	INDEX_MUX_4_VDD,
+};
+
+static enum meson_sar_adc_chan7_mux_sel chan7_mux_values[] = {
+	CHAN7_MUX_VSS,
+	CHAN7_MUX_VDD_DIV4,
+	CHAN7_MUX_VDD_DIV2,
+	CHAN7_MUX_VDD_MUL3_DIV4,
+	CHAN7_MUX_VDD,
+};
+
+static const char * const chan7_mux_names[] = {
+	"gnd",
+	"0.25vdd",
+	"0.5vdd",
+	"0.75vdd",
+	"vdd",
 };
 
 static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
@@ -245,6 +280,11 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
 	MESON_SAR_ADC_CHAN(INDEX_CHAN_6),
 	MESON_SAR_ADC_CHAN(INDEX_CHAN_7),
 	IIO_CHAN_SOFT_TIMESTAMP(INDEX_CHAN_SOFT_TIMESTAMP),
+	MESON_SAR_ADC_MUX(INDEX_MUX_0_VSS, 0),
+	MESON_SAR_ADC_MUX(INDEX_MUX_1_VDD_DIV4, 1),
+	MESON_SAR_ADC_MUX(INDEX_MUX_2_VDD_DIV2, 2),
+	MESON_SAR_ADC_MUX(INDEX_MUX_3_VDD_MUL3_DIV4, 3),
+	MESON_SAR_ADC_MUX(INDEX_MUX_4_VDD, 4),
 	MESON_SAR_ADC_TEMP_CHAN(), /* must be the last item */
 };
 
@@ -284,6 +324,7 @@ struct meson_sar_adc_priv {
 	bool					temperature_sensor_calibrated;
 	u8					temperature_sensor_coefficient;
 	u16					temperature_sensor_adc_val;
+	enum meson_sar_adc_chan7_mux_sel	chan7_mux_sel;
 };
 
 static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
@@ -348,6 +389,8 @@ static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
 			   MESON_SAR_ADC_REG3_CTRL_CHAN7_MUX_SEL_MASK, regval);
 
 	usleep_range(10, 20);
+
+	priv->chan7_mux_sel = sel;
 }
 
 static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
@@ -443,6 +486,15 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
 		regmap_update_bits(priv->regmap,
 				   MESON_SAR_ADC_DELTA_10,
 				   MESON_SAR_ADC_DELTA_10_TEMP_SEL, regval);
+	} else if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_MUX_CHANNEL) {
+		enum meson_sar_adc_chan7_mux_sel sel;
+
+		if (chan->channel == INDEX_CHAN_7)
+			sel = CHAN7_MUX_CH7_INPUT;
+		else
+			sel = chan7_mux_values[chan->channel - INDEX_MUX_0_VSS];
+		if (sel != priv->chan7_mux_sel)
+			meson_sar_adc_set_chan7_mux(indio_dev, sel);
 	}
 }
 
@@ -1015,7 +1067,7 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
 	meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_VDD_DIV4);
 	usleep_range(10, 20);
 	ret = meson_sar_adc_get_sample(indio_dev,
-				       &indio_dev->channels[INDEX_CHAN_7],
+				       &indio_dev->channels[INDEX_MUX_1_VDD_DIV4],
 				       MEAN_AVERAGING, EIGHT_SAMPLES, &value0);
 	if (ret < 0)
 		goto out;
@@ -1023,7 +1075,7 @@ static int meson_sar_adc_calib(struct iio_dev *indio_dev)
 	meson_sar_adc_set_chan7_mux(indio_dev, CHAN7_MUX_VDD_MUL3_DIV4);
 	usleep_range(10, 20);
 	ret = meson_sar_adc_get_sample(indio_dev,
-				       &indio_dev->channels[INDEX_CHAN_7],
+				       &indio_dev->channels[INDEX_MUX_3_VDD_MUL3_DIV4],
 				       MEAN_AVERAGING, EIGHT_SAMPLES, &value1);
 	if (ret < 0)
 		goto out;
@@ -1050,8 +1102,13 @@ static int read_label(struct iio_dev *indio_dev,
 {
 	if (chan->type == IIO_TEMP)
 		return sprintf(label, "%s\n", "temp-sensor");
-	if (chan->type == IIO_VOLTAGE)
-		return sprintf(label, "channel-%d\n", chan->channel);
+	if (chan->type == IIO_VOLTAGE) {
+		if (chan->channel <= INDEX_CHAN_7)
+			return sprintf(label, "channel-%d\n", chan->channel);
+		if (chan->channel >= INDEX_MUX_0_VSS)
+			return sprintf(label, "%s\n",
+				chan7_mux_names[chan->channel - INDEX_MUX_0_VSS]);
+	}
 	return 0;
 }
 
-- 
2.38.4

Re: [PATCH v2 6/6] meson saradc: support reading from channel7 mux inputs

[Martin Blumenstingl]

Hi George,

On Fri, Jun 23, 2023 at 4:23 AM George Stark <gnstark@sberdevices.ru> wrote:
[...]
> Meson saradc channel 7 is connected to muxer that can switch channel
I think that this should read: ... is connected to a mux that ...

[...]
>  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> @@ -245,6 +280,11 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>         MESON_SAR_ADC_CHAN(INDEX_CHAN_6),
>         MESON_SAR_ADC_CHAN(INDEX_CHAN_7),
>         IIO_CHAN_SOFT_TIMESTAMP(INDEX_CHAN_SOFT_TIMESTAMP),
> +       MESON_SAR_ADC_MUX(INDEX_MUX_0_VSS, 0),
> +       MESON_SAR_ADC_MUX(INDEX_MUX_1_VDD_DIV4, 1),
> +       MESON_SAR_ADC_MUX(INDEX_MUX_2_VDD_DIV2, 2),
> +       MESON_SAR_ADC_MUX(INDEX_MUX_3_VDD_MUL3_DIV4, 3),
> +       MESON_SAR_ADC_MUX(INDEX_MUX_4_VDD, 4),
>         MESON_SAR_ADC_TEMP_CHAN(), /* must be the last item */
I haven't had the chance to run these patches yet but: I think they
are breaking the temperature sensor readings on Meson8/8b/8m2 boards.
See arch/arm/boot/dts/meson.dtsi where the temperature channel is
being referenced:
  io-channels = <&saradc 8>

With this series (this patch and I think also patch 3/6 "meson saradc:
unite iio channel array definitions") the numbering of the temperature
sensor channel changes.

To make things worse: in theory we can use meson_saradc to read the
SoC temperature sensor on GXBB, GXL and GXM boards (possibly on AXG as
well but I can't recall from the top of my head) instead of going
through SCPI.
I have experimented with this in the past but never got it to work.
Doing so in the future could lead to another channel index change,
depending on how we decide to go forward now.

There's two that I can think of:
- update meson.dtsi to use the new channel numbering (I don't expect
many 32-bit SoC users out there using new kernel + old .dtbs, but it's
impossible to say for sure)
- or keep the driver backwards compatible (that involves re-adding the
channel tables)

What do you think?


Best regards,
Martin

Re: [PATCH v2 6/6] meson saradc: support reading from channel7 mux inputs

[George Stark]

Hello Martin

Thanks for review

On 6/23/23 09:16, Martin Blumenstingl wrote:
> Hi George,
>
> On Fri, Jun 23, 2023 at 4:23 AM George Stark <gnstark@sberdevices.ru> wrote:
> [...]
>> Meson saradc channel 7 is connected to muxer that can switch channel
> I think that this should read: ... is connected to a mux that ...
>
> [...]
>>   static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>> @@ -245,6 +280,11 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>>          MESON_SAR_ADC_CHAN(INDEX_CHAN_6),
>>          MESON_SAR_ADC_CHAN(INDEX_CHAN_7),
>>          IIO_CHAN_SOFT_TIMESTAMP(INDEX_CHAN_SOFT_TIMESTAMP),
>> +       MESON_SAR_ADC_MUX(INDEX_MUX_0_VSS, 0),
>> +       MESON_SAR_ADC_MUX(INDEX_MUX_1_VDD_DIV4, 1),
>> +       MESON_SAR_ADC_MUX(INDEX_MUX_2_VDD_DIV2, 2),
>> +       MESON_SAR_ADC_MUX(INDEX_MUX_3_VDD_MUL3_DIV4, 3),
>> +       MESON_SAR_ADC_MUX(INDEX_MUX_4_VDD, 4),
>>          MESON_SAR_ADC_TEMP_CHAN(), /* must be the last item */
> I haven't had the chance to run these patches yet but: I think they
> are breaking the temperature sensor readings on Meson8/8b/8m2 boards.
> See arch/arm/boot/dts/meson.dtsi where the temperature channel is
> being referenced:
>    io-channels = <&saradc 8>
>
> With this series (this patch and I think also patch 3/6 "meson saradc:
> unite iio channel array definitions") the numbering of the temperature
> sensor channel changes.
>
> To make things worse: in theory we can use meson_saradc to read the
> SoC temperature sensor on GXBB, GXL and GXM boards (possibly on AXG as
> well but I can't recall from the top of my head) instead of going
> through SCPI.
> I have experimented with this in the past but never got it to work.
> Doing so in the future could lead to another channel index change,
> depending on how we decide to go forward now.
>
> There's two that I can think of:
> - update meson.dtsi to use the new channel numbering (I don't expect
> many 32-bit SoC users out there using new kernel + old .dtbs, but it's
> impossible to say for sure)
> - or keep the driver backwards compatible (that involves re-adding the
> channel tables)
>
> What do you think?
Actually we'd have to make 2 patches to meson.dtsi, the first change 
8->9, than 9 ->14.
And if that index exposed externally (ABI like) I'd not change it 
without good reason at all.
So I think to return to double definition of meson_sar_adc_iio_channels 
and keep the driver backwards compatible.

I've just realized another moment with channels defined after 
MESON_SAR_ADC_TEMP_CHAN in channel array.
In dts by default channels are referenced by channel array index not 
even by channel number.
So channel e.g MUX_0_VSS will have the same number (due to enum patch) 
but different index on meson8 and gxbb.
As alternative we can implement fwnode_xlate method in meson adc driver 
and use channel numbers in dts
(probably not in the current patchset).

Best regards,
George

>
> Best regards,
> Martin
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 6/6] meson saradc: support reading from channel7 mux inputs

[Jonathan Cameron]

On Sat, 24 Jun 2023 18:59:31 +0300
George Stark <stark.georgy@gmail.com> wrote:

> Hello Martin
> 
> Thanks for review
> 
> On 6/23/23 09:16, Martin Blumenstingl wrote:
> > Hi George,
> >
> > On Fri, Jun 23, 2023 at 4:23 AM George Stark <gnstark@sberdevices.ru> wrote:
> > [...]  
> >> Meson saradc channel 7 is connected to muxer that can switch channel  
> > I think that this should read: ... is connected to a mux that ...
> >
> > [...]  
> >>   static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >> @@ -245,6 +280,11 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >>          MESON_SAR_ADC_CHAN(INDEX_CHAN_6),
> >>          MESON_SAR_ADC_CHAN(INDEX_CHAN_7),
> >>          IIO_CHAN_SOFT_TIMESTAMP(INDEX_CHAN_SOFT_TIMESTAMP),
> >> +       MESON_SAR_ADC_MUX(INDEX_MUX_0_VSS, 0),
> >> +       MESON_SAR_ADC_MUX(INDEX_MUX_1_VDD_DIV4, 1),
> >> +       MESON_SAR_ADC_MUX(INDEX_MUX_2_VDD_DIV2, 2),
> >> +       MESON_SAR_ADC_MUX(INDEX_MUX_3_VDD_MUL3_DIV4, 3),
> >> +       MESON_SAR_ADC_MUX(INDEX_MUX_4_VDD, 4),
> >>          MESON_SAR_ADC_TEMP_CHAN(), /* must be the last item */  
> > I haven't had the chance to run these patches yet but: I think they
> > are breaking the temperature sensor readings on Meson8/8b/8m2 boards.
> > See arch/arm/boot/dts/meson.dtsi where the temperature channel is
> > being referenced:
> >    io-channels = <&saradc 8>
> >
> > With this series (this patch and I think also patch 3/6 "meson saradc:
> > unite iio channel array definitions") the numbering of the temperature
> > sensor channel changes.
> >
> > To make things worse: in theory we can use meson_saradc to read the
> > SoC temperature sensor on GXBB, GXL and GXM boards (possibly on AXG as
> > well but I can't recall from the top of my head) instead of going
> > through SCPI.
> > I have experimented with this in the past but never got it to work.
> > Doing so in the future could lead to another channel index change,
> > depending on how we decide to go forward now.
> >
> > There's two that I can think of:
> > - update meson.dtsi to use the new channel numbering (I don't expect
> > many 32-bit SoC users out there using new kernel + old .dtbs, but it's
> > impossible to say for sure)
> > - or keep the driver backwards compatible (that involves re-adding the
> > channel tables)

Agreed. Put the table back.  It might make the code slightly more complex
but we need to avoid shifting channel numbers around where possible.

Jonathan


> >
> > What do you think?  
> Actually we'd have to make 2 patches to meson.dtsi, the first change 
> 8->9, than 9 ->14.
> And if that index exposed externally (ABI like) I'd not change it 
> without good reason at all.
> So I think to return to double definition of meson_sar_adc_iio_channels 
> and keep the driver backwards compatible.
> 
> I've just realized another moment with channels defined after 
> MESON_SAR_ADC_TEMP_CHAN in channel array.
> In dts by default channels are referenced by channel array index not 
> even by channel number.
> So channel e.g MUX_0_VSS will have the same number (due to enum patch) 
> but different index on meson8 and gxbb.
> As alternative we can implement fwnode_xlate method in meson adc driver 
> and use channel numbers in dts
> (probably not in the current patchset).
> 
> Best regards,
> George
> 
> >
> > Best regards,
> > Martin
> >
> > _______________________________________________
> > linux-amlogic mailing list
> > linux-amlogic@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-amlogic  
> 
> 

Re: [PATCH v2 6/6] meson saradc: support reading from channel7 mux inputs

[Martin Blumenstingl]

Hi George,

On Fri, Jun 23, 2023 at 6:38 PM George Stark <gnstark@sberdevices.ru> wrote:
[...]
> So I think to return to double definition of meson_sar_adc_iio_channels and keep the driver backwards compatible.
Ack!

> I've just realized another moment with channels defined after MESON_SAR_ADC_TEMP_CHAN in channel array.
Good catch

> In dts by default channels are referenced by channel array index not even by channel number.
> So channel e.g MUX_0_VSS will have the same number (due to enum patch) but different index on meson8 and gxbb.
> As alternative we can implement fwnode_xlate method in meson adc driver and use channel numbers in dts (probably not in the current patchset)
That is actually an interesting third approach. But as you said: let's
start simple and add the tables back for now.


Best regards,
Martin