linux-kernel - Re: [PATCH v6 7/8] iio: dac: ad3552r: add high-speed platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <61cf3072af74a8b2951c948ddc2383ba1e55954d.camel@gmail.com>
Date: Wed, 16 Oct 2024 09:50:41 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Angelo Dureghello
 <adureghello@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>,  Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich <Michael.Hennerich@...log.com>,  Jonathan Cameron
 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Olivier Moysan
 <olivier.moysan@...s.st.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v6 7/8] iio: dac: ad3552r: add high-speed platform driver

On Tue, 2024-10-15 at 10:23 -0500, David Lechner wrote:
> On 10/15/24 10:00 AM, Nuno Sá wrote:
> > On Tue, 2024-10-15 at 09:38 -0500, David Lechner wrote:
> > > On 10/15/24 1:37 AM, Nuno Sá wrote:
> > > > On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote:
> > > > > On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> > > > > > From: Angelo Dureghello <adureghello@...libre.com>
> > > > > > 
> > > > > > Add High Speed ad3552r platform driver.
> > > > > > 
> > > > > 
> > > > > ...
> > > > > 
> > > > > > +static int ad3552r_hs_read_raw(struct iio_dev *indio_dev,
> > > > > > +			       struct iio_chan_spec const *chan,
> > > > > > +			       int *val, int *val2, long mask)
> > > > > > +{
> > > > > > +	struct ad3552r_hs_state *st = iio_priv(indio_dev);
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	switch (mask) {
> > > > > > +	case IIO_CHAN_INFO_SAMP_FREQ: {
> > > > > > +		int sclk;
> > > > > > +
> > > > > > +		ret = iio_backend_read_raw(st->back, chan, &sclk, 0,
> > > > > > +					   IIO_CHAN_INFO_FREQUENCY);
> > > > > 
> > > > > FWIW, this still seems like an odd way to get the stream mode SCLK
> > > > > rate from the backend to me. How does the backend know that we want
> > > > > the stream mode clock rate and not some other frequency value? 
> > > > 
> > > > In this case the backend has a dedicated compatible so sky is the limit :).
> > > > But
> > > > yeah,
> > > > I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you
> > > > have
> > > > in
> > > > mind? Using the sampling frequency INFO or a dedicated OP?
> > > > 
> > > 
> > > It think it would be most straightforward to have something
> > > like a iio_backend_get_data_stream_clock_rate() callback since
> > > that is what we are getting.
> > 
> > Hmmm, what about exporting an actual clock? Maybe it's overkill but from a
> > correctness point of view, seems what we should actually do :)
> 
> Does seem overkill to me. I wouldn't do it.
> 

Yes it is. But to me (now that I slept on the matter) a new backend OP is also not
the way to go (or at least not coherent). We already have .bus_reg_read() and
.bus_reg_write() shared through the platform_data 'struct ad3552r_hs_platform_data'
interface. Well, in reality we're asking for the bus clock here so better to add a
.bus_clock() to that struct. And since (it seems) we are going the path of just
caring about the high speed rate, we might as well just make it a variable for
simplicity.

> > 
> > > 
> > > Re: the other recent discussions about getting too many
> > > callbacks. Instead of a dedicated function like this, we
> > > could make a set of generic functions:
> > > 
> > > iio_backend_{g,s}et_property_{s,u}(8, 16, 32, 64}()
> > > 
> > 
> > Hmm interesting approach. I don't dislike it. Kind of a generic getter/setter
> > thingy.
> > We could then still have optional inline helpers that would call the generic
> > functions with the proper enum value.
> > 
> > > that take an enum parameter for the property. This way,
> > > for each new property, we just have to add an enum member
> > > instead of creating a get/set callback pair.
> > > 
> > > Unrelated to this particular case, but taking the idea even
> > > farther, we could also do the same with enable/disable
> > > functions. We talked before about cutting the number of
> > > callbacks in half by using a bool parameter instead of
> > > separate enable/disable callbacks. But we could cut it down
> > > even more by having an enum parameter for the thing we are
> > > enabling/disabling.
> > 
> > If we don't get too strict about types it could even fall into the above u8
> > category.
> > 
> > Instead of lot of new simple ops we just grow an enum.
> 
> Sure. For that matter, maybe try to just stick with 32-bit
> for everything to keep it simple. Probably will eventually
> need 64-bit for some things, but might be able to get away
> with avoiding 8 and 16-bit.
> 

Agreed. Anyways, nothing that I will take care in the near future (I would first like
for things to stabilize a bit). That said, if you want (or anybody else), feel free
to send the patches :)

- Nuno Sá