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
