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, 14 May 2023 18:19:12 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v4 2/3] iio: potentiometer: Add support for the Renesas
 X9250 potentiometers
Message-ID: <20230514181912.314ef781@jic23-huawei>
In-Reply-To: <20230514163233.0c048256@bootlin.com>
References: <20230509160852.158101-1-herve.codina@bootlin.com>
        <20230509160852.158101-3-herve.codina@bootlin.com>
        <20230513193525.43a4475f@jic23-huawei>
        <20230514163233.0c048256@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, 14 May 2023 16:32:33 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Jonathan,
>=20
> On Sat, 13 May 2023 19:35:25 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>=20
> > On Tue,  9 May 2023 18:08:51 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> >  =20
> > > The Renesas X9250 integrates four digitally controlled potentiometers.
> > > On each potentiometer, the X9250T has a 100 kOhms total resistance and
> > > the X9250U has a 50 kOhms total resistance.
> > >=20
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>   =20
> >=20
> > As I only noticed one trivial thing I made the change whilst applying.
> > diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiome=
ter/x9250.c
> > index 3d4ca18d1f14..7e145d7d14f1 100644
> > --- a/drivers/iio/potentiometer/x9250.c
> > +++ b/drivers/iio/potentiometer/x9250.c
> > @@ -176,10 +176,7 @@ static int x9250_probe(struct spi_device *spi)
> > =20
> >         x9250 =3D iio_priv(indio_dev);
> >         x9250->spi =3D spi;
> > -       x9250->cfg =3D device_get_match_data(&spi->dev);
> > -       if (!x9250->cfg)
> > -               x9250->cfg =3D &x9250_cfg[spi_get_device_id(spi)->drive=
r_data];
> > -
> > +       x9250->cfg =3D spi_get_device_match_data(spi);
> >         x9250->wp_gpio =3D devm_gpiod_get_optional(&spi->dev, "wp", GPI=
OD_OUT_LOW);
> >         if (IS_ERR(x9250->wp_gpio))
> >                 return dev_err_probe(&spi->dev, PTR_ERR(x9250->wp_gpio),
> >  =20
>=20
> Are you sure about your modification ?
>=20
> I am not sure (maybe I am wrong) that
>   x9250->cfg =3D spi_get_device_match_data(spi);
> is equivalent to
>   x9250->cfg =3D &x9250_cfg[spi_get_device_id(spi)->driver_data];
>=20
> The spi_get_device_id(spi)->driver_data value I used is a simple integer
> (X9250T or X9250U) and not the x9250_cfg item.
> Maybe the x9250_id_table should be modified to replace X9250T by
> &x9250_cfg[X9250T] to have your modification working.

Excellent point.  I'm was  clearly half asleep. The mod should have included
switching them over to be pointers.

>=20
> The data defined in the driver are the following:
> --- 8< ---
> static const struct x9250_cfg x9250_cfg[] =3D {
> 	[X9250T] =3D { .name =3D "x9250t", .kohms =3D  100, },
> 	[X9250U] =3D { .name =3D "x9250u", .kohms =3D  50, },
> };
>=20
> ...
>=20
> static const struct of_device_id x9250_of_match[] =3D {
> 	{ .compatible =3D "renesas,x9250t", &x9250_cfg[X9250T]},
> 	{ .compatible =3D "renesas,x9250u", &x9250_cfg[X9250U]},
> 	{ }
> };
> MODULE_DEVICE_TABLE(of, x9250_of_match);
>=20
> static const struct spi_device_id x9250_id_table[] =3D {
> 	{ "x9250t", X9250T },
> 	{ "x9250u", X9250U },
So these should be (kernel_ulong_t)&x9250_cfg[X9250T] etc for the data.
I've tweaked it so that is now the case. Oops and thanks for sanity checkin=
g.
Sometimes we see what we expect to see rather than what is there.

Tweak on top of original tweak is:
diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/=
x9250.c
index 7e145d7d14f1..0cc7f72529be 100644
--- a/drivers/iio/potentiometer/x9250.c
+++ b/drivers/iio/potentiometer/x9250.c
@@ -198,8 +198,8 @@ static const struct of_device_id x9250_of_match[] =3D {
 MODULE_DEVICE_TABLE(of, x9250_of_match);
=20
 static const struct spi_device_id x9250_id_table[] =3D {
-       { "x9250t", X9250T },
-       { "x9250u", X9250U },
+       { "x9250t", (kernel_ulong_t)&x9250_cfg[X9250T] },
+       { "x9250u", (kernel_ulong_t)&x9250_cfg[X9250U] },
        { }
 };


Jonathan

> 	{ }
> };
> MODULE_DEVICE_TABLE(spi, x9250_id_table);
>=20
> static struct spi_driver x9250_spi_driver =3D {
> 	.driver  =3D {
> 		.name =3D "x9250",
> 		.of_match_table =3D x9250_of_match,
> 	},
> 	.id_table =3D x9250_id_table,
> 	.probe  =3D x9250_probe,
> };
> --- 8< ---
>=20
>=20
> Best regards,
> Herv=C3=A9
>=20