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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote:
> 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:

...

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

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

Anyway, thanks for review! My answers below.

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

How? Technically as I mentioned currently it's the case to use
only the date from fraction. The bigger object would be needed
in case of using data that is not fraction. Either way it would
be easy to add a container_of() than supply unneeded data to
the method.

TL;DR: It makes possible not to mix bananas with wooden boxes.

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

I understand your point. I will provide a mult_fract() macro and
apply it to AFE to show the usability of this. Would it work better?

I haven't checked the other possible improvements based on new type, but
I truly believe that the types themselves are good to have.

> 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?

UAPI is another story. It might be possible to extend, but I would like
to avoid expanding the proposed types to UAPI (we don't have many users
and I believe they more or less unique, so v4l2 is rather exception than
usual).

-- 
With Best Regards,
Andy Shevchenko