Re: [PATCH 10/23] iio: accel: kxcjk-1013: Convert to i2c's .probe_new

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

On Tue, Nov 01, 2022 at 10:49:39PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 01, 2022 at 04:54:52PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 01, 2022 at 12:38:43AM +0100, Uwe Kleine-König wrote:
> > > On Mon, Oct 24, 2022 at 12:46:02PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Oct 24, 2022 at 11:14:56AM +0200, Uwe Kleine-König wrote:

I really do appreciate your work on this, but it's pity that my point
is still unclear to you. As a beginning point I assume that the idea
of ->probe_new() is to mimic what SPI core does. That's why I consider
moving the tables is smelling like a half-baked work. Besides that
I tried to explain again on the concerns I have below.

...

> > > > Exactly. And it means let's put my problem to someone's else shoulders.
> > > 
> > > You have a problem that I fail to see. Why is defining the id table
> > > before the probe function bad?
> > > 
> > > Unless I misunderstand you, you seem to assume that in the nearer future
> > > someone will have the urge to put the id table below the probe function
> > > again. What would you think is their motivation?
> > 
> > The problem with moving the table is the sparse locations in the code for
> > semantically relative things, like all ID tables to be near to each other.
> 
> I don't understand that reasoning. Is that important for the compiler or
> the human reader? What is "semantically relative"?

If we have let's say 2+ ID tables, without your patches

...module code...

...TABLE 1...
...TABLE 2...

static struct foo_driver *drv = {
	...references to the tables...
};

With your patches (that move the ID table):


...module header...
...module code...

...TABLE 1...

...more module code...

...TABLE 2...

static struct foo_driver *drv = {
	...references to the tables...
};

which I consider as unneeded sparse for like you said micro-optimization
of the direct access to the table.

There are two ways to solve this:
- move all tables together
- provide an API as done by SPI core

I prefer the latter over the former.

> > With
> > your approach you can easily break that and go for let's put one ID table on
> > top, because some code fails to indirectly access it, and leave another
> > somewhere else. I do not like this.
> > 
> > Besides, your change making unneeded churn of "I like to move it, move it" for
> > no real gain.
> 
> That's not true. It's not that I like to move it. Moving is necessary to
> make use of the local symbol in .probe() without a forward declaration.
> (If you claimed that adding a forward declaration was churn, I'd agree.)

On forward declaration we agree.

But using local symbol directly as a shortcut to access some field of the
instance of some object is against OOP paradigm.

I.o.w. some shortcuts maybe nice, but this kind of approach leads to
layering violation and similar problems to begin with.

...

> > There is another approach in the discussion and Wolfram acknowledged it already
> > (with a new API to retrieve the necessary data).
> 
> Yeah, saw it. And as expected the follow up patch converting
> drivers/iio/pressure/bmp280-i2c.c "suffers" from the double pointer
> dereference. But it looks nice because the effort to determine the table
> via driver is well hidden.

Yes, that is the downside of OOP, I agree.

...

> > > > reduce churn with the using of current i2c_match_id() as you
> > > > showed the long line to get that table.
> > > 
> > > Do you still remember the original patch? That one doesn't have the long
> > > i2c_match_id() line.
> > > 
> > > (Do you see your statement is an argument for my approach? The long line
> > > is an indication that it's complicated to determine the address of the
> > > table via ->driver. You can hide that by pushing the needed effort into
> > > i2c_match_id() or a macro, but in the end the complexity remains for the
> > > CPU.)
> > 
> > Does it matter?
> 
> What is "it"? The long line? (Then no, it doesn't matter because it
> doesn't appear in neither your nor my approach.) The effort to pull the
> table's address out of ->driver? (We seem to disagree. I think it's easy
> enough not to do it to justify the optimisation.) Or to hide the effort?
> (I think hiding effort and so making it easy to pick a more expensive
> approach is at least questionable.)

IT == The length of dereferencing chain to get the pointer to the data
structure in question.

So, it is OOP model versus micro-optimization on a slow path.
That's how I can class the basis of our disagreement.

> > OTOH that will be aligned with SPI framework and idea behind ->probe_new()
> > as I understood it.

...

> What I consider "churn" though is this discussion.

I agree on that as well. Nut it's partially my issue and I'm sorry for that.

> I will stop my participation here. That's a bit sad because in my eyes all
> patches in this series have a positive value

Your patches (that don't move the tables) are nice job!

> and the discussion about (from
> my POV) incomprehensible minor details destroyed my motivation to work on
> the quest to convert all drivers to .probe_new() :-\

It's pity to hear, but you may also imagine how many times I was in
the similar situation and for some cases I lost my motivation, indeed.
I feel your pain.

-- 
With Best Regards,
Andy Shevchenko