From: George Stark <[email protected]>
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/[email protected]/
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
Move enums declaration before variables declaration.
Signed-off-by: George Stark <[email protected]>
---
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
Move meson_sar_adc_set_chan7_mux routine upper.
Signed-off-by: George Stark <[email protected]>
---
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
Add attribute 'label' to all iio channles
Signed-off-by: George Stark <[email protected]>
---
Changelog:
v1->v2: update commit message from [1]
[1] https://lore.kernel.org/lkml/[email protected]/
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
Changelog:
v1->v2: update commit message from [1]
[1] https://lore.kernel.org/lkml/[email protected]/
---
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
Add enum for iio channel array indexes.
Signed-off-by: George Stark <[email protected]>
---
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
Hi George,
On Fri, Jun 23, 2023 at 4:23 AM George Stark <[email protected]> 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
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 <[email protected]> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
On Sat, 24 Jun 2023 18:59:31 +0300
George Stark <[email protected]> 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 <[email protected]> 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
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-amlogic
>
>
Hi George,
On Fri, Jun 23, 2023 at 6:38 PM George Stark <[email protected]> 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