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@xxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-amlogic  
> 
>