linux-kernel - Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Jan 2022 12:03:49 +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-26 14:01, Andy Shevchenko wrote:
> On Wed, Jan 26, 2022 at 01:35:09PM +0100, Peter Rosin wrote:
>> On 2022-01-26 13:04, Andy Shevchenko wrote:
>>> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
>>>> 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.
>>
>> I think got the gist of it. I simply do not agree with your conclusion
>> about what the "proper layering" should be.
> 
> And I see no real argument against it. With the patch applied I see
> a better structure of the code and exactly necessary data to be passed
> to the method. Which makes me think that current implementation is
> either a leftover or was something like "let's take a bigger object
> _just in case_", which I can't take as a proper layering.

The bigger object, or the one and only object as the current code is
written, is given to ->props() by design.

BTW, you don't seem to understand the ->props() functions. There is no
data "passed to" the ->props() functions. These functions instead fill
in properties. Currently this boils down to the scaling fraction, but I
can imagine other properties.

On 2022-01-25 19:17, Andy Shevchenko wrote:
>                              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.

You argued that it is easy to "break out" to the bigger object in case
it's needed. Which in my book is a sign of poor layering.
It's way easier to *not* change things, it's perfectly fine as-is.

The argument against making the change you propose is that it does not
make this small driver any easier to understand. It would still just
change things for the sake of changing them, and I do not see the point
of erasing the existing future-proofing when it has no cost.

To sum up, I'm ok with introducing fract_s32 in this driver, but I
don't agree with the signature change of ->props().

Cheers,
Peter