Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 25 Jun 2023 12:19:16 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     George Stark <stark.georgy@gmail.com>
Cc:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        George Stark <gnstark@sberdevices.ru>, lars@metafoo.de,
        neil.armstrong@linaro.org, khilman@baylibre.com,
        jbrunet@baylibre.com, andriy.shevchenko@linux.intel.com,
        nuno.sa@analog.com, linux-iio@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-amlogic@lists.infradead.org, kernel@sberdevices.ru
Subject: Re: [PATCH v2 6/6] meson saradc: support reading from channel7 mux
 inputs
Message-ID: <20230625121916.32578914@jic23-huawei>
In-Reply-To: <a0d74f3f-4208-d7e3-7eed-481a06b13308@gmail.com>
References: <20230623022334.791026-1-gnstark@sberdevices.ru>
        <20230623022334.791026-7-gnstark@sberdevices.ru>
        <CAFBinCBv993Xv_wk9fE-U0Tw2mzTB1z22Tj6x8Uy1rRw_dztng@mail.gmail.com>
        <a0d74f3f-4208-d7e3-7eed-481a06b13308@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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

> Hello Martin
>=20
> Thanks for review
>=20
> On 6/23/23 09:16, Martin Blumenstingl wrote:
> > Hi George,
> >
> > On Fri, Jun 23, 2023 at 4:23=E2=80=AFAM George Stark <gnstark@sberdevic=
es.ru> wrote:
> > [...] =20
> >> Meson saradc channel 7 is connected to muxer that can switch channel =
=20
> > I think that this should read: ... is connected to a mux that ...
> >
> > [...] =20
> >>   static const struct iio_chan_spec meson_sar_adc_iio_channels[] =3D {
> >> @@ -245,6 +280,11 @@ static const struct iio_chan_spec meson_sar_adc_i=
io_channels[] =3D {
> >>          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 */ =20
> > 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 =3D <&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? =20
> Actually we'd have to make 2 patches to meson.dtsi, the first change=20
> 8->9, than 9 ->14.
> And if that index exposed externally (ABI like) I'd not change it=20
> without good reason at all.
> So I think to return to double definition of meson_sar_adc_iio_channels=20
> and keep the driver backwards compatible.
>=20
> I've just realized another moment with channels defined after=20
> MESON_SAR_ADC_TEMP_CHAN in channel array.
> In dts by default channels are referenced by channel array index not=20
> even by channel number.
> So channel e.g MUX_0_VSS will have the same number (due to enum patch)=20
> but different index on meson8 and gxbb.
> As alternative we can implement fwnode_xlate method in meson adc driver=20
> and use channel numbers in dts
> (probably not in the current patchset).
>=20
> Best regards,
> George
>=20
> >
> > Best regards,
> > Martin
> >
> > _______________________________________________
> > linux-amlogic mailing list
> > linux-amlogic@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-amlogic =20
>=20
>=20