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@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 :)
Does seem overkill to me. I wouldn't do it.
>
>>
>> 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.
>
> - Nuno Sá
>
