diff mbox series

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

Message ID 20230623022334.791026-7-gnstark@sberdevices.ru (mailing list archive)
State Superseded
Headers show
Series meson saradc: add iio channels to read channel 7 mux inputs | expand

Commit Message

George Stark June 23, 2023, 2:20 a.m. UTC 
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(-)

Comments

Martin Blumenstingl June 23, 2023, 6:16 a.m. UTC | #1 
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
George Stark June 24, 2023, 3:59 p.m. UTC | #2 
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
Jonathan Cameron June 25, 2023, 11:19 a.m. UTC | #3 
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  
> 
>
Martin Blumenstingl June 25, 2023, 8:37 p.m. UTC | #4 
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
diff mbox series

Patch

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;
 }