Message-ID: <YfE45cImAQpOeziT@smile.fi.intel.com>
Date:   Wed, 26 Jan 2022 14:04:53 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Peter Rosin <peda@...ntia.se>
Cc:     Liam Beguin <liambeguin@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Andreas Kemnade <andreas@...nade.info>,
        linux-arm-msm@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Lars-Peter Clausen <lars@...afoo.de>
Subject: Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct
 s32_fract

On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
> On 2022-01-25 19:17, Andy Shevchenko wrote:
> > 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@...ux.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.
> 
> It's easy to both remove and to add back "the bigger object". I just
> don't see the point of the churn. Technically you can probably rearrange
> stuff in probe and remove the 2nd argument to ->props() altogether and
> chase pointers from the dev object instead. I don't see the point of
> that either. It doesn't really make things simpler, it doesn't really
> make things easier to read. To me, it's just pointless churn.

Since you still haven't got a point the conclusions are wrong.
The point is (I dunno how more clear to make it) is to have proper
layering from the (current) design perspective.

If we go to the road full of "if it will come XYZ then this sucks".
The future is uncertain and neither of us may prove the current
change good or bad in terms of the future (unknown and uncertain)
changes.

Preventing this patch to land is to tell "Oh, my design is bad,
but I will keep it, because in the future everything may change".
So, why don't you make this future to be now?

> > TL;DR: It makes possible not to mix bananas with wooden boxes.
> 
> Which is all good until you need to ship an apple in the box with the
> bananas. (e.g. if you for some reason need the bananas to get ripe real
> quick, apples produce ethylene)

Really. arguments about the future changes are weak. If you have
patches in mind, send them, We will see in practice what you meant.

> >> 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?
> 
> In my experience, burying stuff in macros will make it harder to follow
> what is really happening. Something that has proven hard as-is in this
> driver. While reviewing the changes from Liam, I have repeatedly looked
> up the various division variants to find out what they are actually
> doing. Here, it's not the individual steps that are difficult. I feel
> that if we start adding macros for fractions they will soon multiply and
> I'm not really looking forward to also have a set of similar fraction
> macros to juggle.

The problem here is that every driver would like to do this differently
and since it's related to the calculation we will have all possible error
prone implementations which do miscalculations (yes, one may not notice
traditional off-by-one until it becomes a huge issue by using additional
conversion formulas or so).

> But sure, feel free to suggest something. But please hold until the
> current work from Liam is merged.
> That series is clearly more
> important, and I'm not really interested in neither adding more work for
> him nor a cleanup of the current code without those pending changes.

I'm very well fine with that. As I mentioned from the beginning, I may rebase
this on top of the Liam's work.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists