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

[David Lechner <dlechner@baylibre.com>]

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@baylibre.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.

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}()

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.