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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 15.10.2024 17:00, 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@xxxxxxxxxxxx>
> > > > > 
> > > > > 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 :)
> 
> > 
> > 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.

so a single call for all enable/disable calls. Looks good to me.

What we want to do now ?

So if understand, we don't like too much IIO_CHAN_INFO_FREQUENCY
but at the same time, we don't want to have several new calls in the
backend proposing a design change at this stage, where the patch
was (likely) in a good shape.

What about to simply add a IIO_CHAN_INFO_BUS_CLK or similar ? 

> 
> - Nuno Sá
> 





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux