Message-ID: <34c121fa-2a3b-fb6b-f6d5-fc2be2a5c6b7@axentia.se>
Date:   Wed, 26 Jan 2022 11:26:50 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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 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.

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

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

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.

Cheers,
Peter

Powered by blists - more mailing lists