Received-SPF: pass (google.com: domain of linux-kernel+bounces-161840-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <9d1ac78951439bbcdc2f294713f55ee34e30f84e.camel@gmail.com>
Subject: Re: [PATCH 0/3] iio: cleanup masklength usage
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>, David Lechner
 <dlechner@baylibre.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>, Michael Hennerich
 <Michael.Hennerich@analog.com>, Shawn Guo <shawnguo@kernel.org>, Sascha
 Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team
 <kernel@pengutronix.de>,  Fabio Estevam <festevam@gmail.com>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, 
 imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org
Date: Mon, 29 Apr 2024 09:17:18 +0200
In-Reply-To: <20240428142343.5067c898@jic23-huawei>
References: 
	<20240425-b4-iio-masklength-cleanup-v1-0-d3d16318274d@baylibre.com>
	 <2630ef8c8363b4fa772a2ff2c95cf115ad3c509d.camel@gmail.com>
	 <CAMknhBGQB4MC8ejEs_uLgb=iKehXkoetgHjZnCvCKQbuua5kfA@mail.gmail.com>
	 <20240428142343.5067c898@jic23-huawei>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 
Precedence: bulk
MIME-Version: 1.0

On Sun, 2024-04-28 at 14:23 +0100, Jonathan Cameron wrote:
> On Fri, 26 Apr 2024 10:26:31 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>=20
> > On Fri, Apr 26, 2024 at 2:13=E2=80=AFAM Nuno S=C3=A1 <noname.nuno@gmail=
com> wrote:
> > >=20
> > > On Thu, 2024-04-25 at 10:03 -0500, David Lechner wrote:=C2=A0=20
> > > > While working on other patches I noticed that a few drivers are set=
ting
> > > > the masklength field of struct iio_dev even though it is marked as
> > > > [INTERN]. It looks like maybe this was not always the case, but we =
can
> > > > safely clean it up now without breaking anything.
> > > >=20
> > > > ---
> > > > David Lechner (3):
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 iio: adc: ad7266: don't set maskleng=
th
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 iio: adc: mxs-lradc-adc: don't set m=
asklength
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 iio: buffer: initialize masklength a=
ccumulator to 0
> > > >=20
> > > > =C2=A0drivers/iio/adc/ad7266.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 | 1 -
> > > > =C2=A0drivers/iio/adc/mxs-lradc-adc.c=C2=A0=C2=A0 | 1 -
> > > > =C2=A0drivers/iio/industrialio-buffer.c | 2 +-
> > > > =C2=A03 files changed, 1 insertion(+), 3 deletions(-)
> > > > ---
> > > > base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
> > > > change-id: 20240425-b4-iio-masklength-cleanup-86b632b19901
> > > > =C2=A0
> > >=20
> > > Hi David,
> > >=20
> > > Nice cleanup. The patches look good to me but there's one thing missi=
ng
> > > :). As you
> > > correctly noted, the field should be internal to the IIO core and dri=
vers
> > > should not
> > > touch it. Hence, you need to make sure not driver is using it so we c=
an
> > > move it into
> > > struct iio_dev_opaque [1]. That's the place all the intern fields sho=
uld,
> > > eventually,
> > > end up.
> > >=20
> > > Now, quite some drivers in the trigger handler will read the maskleng=
th
> > > for looping
> > > with for_each_set_bit(). Hence, the straight thing would be an helper=
 to
> > > get it.
> > > Maybe there's a clever way...
> > >=20
> > > I know this is more work than what you had in mind but I think it sho=
uld
> > > be fairly
> > > simple (hopefully) and since you started it :), maybe we can get the =
whole
> > > thing done
> > > and remove another [INTERN] member from the iio_dev struct.
> > >=20
> > > [1]:
> > > https://elixir.bootlin.com/linux/latest/source/include/linux/iio/iio-=
opaque.h#L42
> > >=20
> > > - Nuno S=C3=A1=C2=A0=20
> >=20
> > Sounds like fun. :-p
> >=20
> > I will look into it.
>=20
> I think this one might be miss marked as [INTERN]. It should be constant =
from
> the driver
> point of view, but given active_scan_masks is meant to be visible to the
> driver,
> it's length should probably be as well.
>=20

Yeah, that did crossed my mind. I guess we should just make it [DRIVER] the=
n
(likely with RO statement).

> Sure every driver should be able to trivially work this out for themselve=
s,
> but
> do we care about stopping them using this?
>=20
> It might be worth some nice iterator wrappers with names like
> iio_for_each_active_channel() though I'd expect those to still be accessi=
ng
> these

That looks like a good idea. It would make it more clear that member is not=
 to
be directly accessed.

- Nuno S=C3=9Fa