Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Peter Rosin <peda@axentia.se>]

On 2022-01-25 14:17, Andy Shevchenko wrote:
> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote:
>> On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
>>> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
>>>> On Mon, 10 Jan 2022 21:31:04 +0200
>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>>>
>>>>> Instead of custom data type re-use generic struct s32_fract.
>>>>> No changes intended.
>>>>>
>>>>> The new member is put to be the first one to avoid additional
>>>>> pointer arithmetic. Besides that one may switch to use fract
>>>>> member to perform container_of(), which will be no-op in this
>>>>> case, to get struct rescale.
>>>>>
>>>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>>
>>>> I'm not totally sold on this series showing there is a strong case for
>>>> these macros so interested to hear what others think.
>>>
>>> So far no news :-)
>>
>> Like I mentioned briefly in the other thread[1], I don't really see the
>> advantage for the AFE driver given that it's almost just like renaming
>> the parameters.
> 
> I tend to disagree, perhaps I wasn't so clear in my points.
> 
> The change reveals that the layering can be improved. In OOP
> the object A which is inherited (or encapsulated as we see here)
> allows to clearly get what kind of data the methods are operating
> with / on. As you may see the two functions and the method
> declaration appears to have interest only in the fraction data
> for rescaling. The cleanup I consider helpful in the terms
> of layering and OOP.

Hi!

[Sorry for the delay, I have been far too busy for far too long]

While this is all true for the current set of front-ends, it is not
all that far-fetched to imagine some kind of future front-end that
requires some other parameter, such that the rescaling fraction is no
longer the only thing of interest. So, I have the feeling that changing
the type of the 2nd argument of the ->props functions to just the
fraction instead of the bigger object is conceptually wrong and
something that will later turn out to have been a bad idea.

Regarding the new xyz_fract types, I have no strong opinion. But as
long as there are no helper functions for the new types, I see little
value in them. To me, they look mostly like something that newcomers
(and others) will not know about or expect, and it will just be
another thing that you have to look out for during review, to keep new
numerators/denominators from appearing and causing extra rounds of
review for something that is mostly a bikeshed issue.

My guess is that many times where fractions are used, they are used
since fp math is not available. And that means that there will be a
lot of special handling and shortcuts done since various things about
accuracy and precision can be assumed. I think it will be hard to do
all that centrally and in a comprehensible way. But if it is done it
will of course also be possible to eliminate bugs since people may
have taken too many shortcuts. But simply changing to xyz_fract and
then not take it further than that will not change much.

Also, there is already a v4l2_fract which is exposed in UAPI (maybe
there are others?). I don't see how we bring that one in line with this
new struct xyz_fract scheme?

Cheers,
Peter