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

...

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

...

> > I don't see benefit of dereferencing tables by name. The table has to be
> > available via struct driver, otherwise, how the heck we even got into the
> > ->probe() there.
> 
> It is possible, it's just cheaper (in cpu cycles) to calculate the
> address of the table directly (i.e. via PC + $offset) instead of via
> dereferencing two pointers.

It's a slow path.

...

> I fail to follow you again. I talked about drivers/rtc/rtc-isl1208.c as
> it is in v6.1-rc1. There the probe function doesn't access the table at
> all. Neither via the driver link nor by name. That driver just defines
> the id table before the probe function.

So, if it is on top, fine, it would be just an inconsistent style.

...

> > Alternative is to avoid reordering to begin with, no?
> 
> Yeah, that could be done. But I don't see the advantage and you fail to
> explain it in a way for me to understand.
> 
> So if i2c_match_id() was changed as follows:

There is another approach in the discussion and Wolfram acknowledged it already
(with a new API to retrieve the necessary data).

...

> +static const struct i2c_device_id kxcjk1013_id[] = {
> +	{"kxcjk1013", KXCJK1013},
> +	{"kxcj91008", KXCJ91008},
> +	{"kxtj21009", KXTJ21009},
> +	{"kxtf9",     KXTF9},
> +	{"kx023-1025", KX0231025},
> +	{"SMO8500",   KXCJ91008},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);
> +
>  static int kxcjk1013_probe(struct i2c_client *client)
>  {
> -	const struct i2c_device_id *id = i2c_match_id(NULL, client);
> +	const struct i2c_device_id *id = i2c_match_id(kxcjk1013_id, client);
>  	struct kxcjk1013_data *data;
>  	struct iio_dev *indio_dev;
>  	struct kxcjk_1013_platform_data *pdata;
> @@ -1720,18 +1731,6 @@ static const struct acpi_device_id kx_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
>  
> -static const struct i2c_device_id kxcjk1013_id[] = {
> -	{"kxcjk1013", KXCJK1013},
> -	{"kxcj91008", KXCJ91008},
> -	{"kxtj21009", KXTJ21009},
> -	{"kxtf9",     KXTF9},
> -	{"kx023-1025", KX0231025},
> -	{"SMO8500",   KXCJ91008},
> -	{}
> -};
> -
> -MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);

Can we avoid moving it?

>  static const struct of_device_id kxcjk1013_of_match[] = {
>  	{ .compatible = "kionix,kxcjk1013", },
>  	{ .compatible = "kionix,kxcj91008", },
> 
> on top to safe two pointer dereferences. The sum of the two driver
> patches is exactly the effect of my patch just without the i2c core
> change. (And the two pointer dereferences that are saved by the 2nd
> patch are introduced by the first, so it's fine to not split that
> change into two parts.)

...

> > So, why not teach i2c_match_id() to handle this nicely for the caller?
> 
> The metric for "nice" is obviously subjective. For me it's nice to pass
> a local symbol to an API function to make the function's job a tad
> easier and more effective to solve. And that even if I have to reorder
> the caller a bit.

So, it seems a preferred design paradigm: straightforward vs. OOP.
Kernel is written with OOP in mind, why to avoid that?

...

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

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

...

> > This might need a new API to avoid
> > changing many drivers at once. But it's business as usual.
> 
> My approach doesn't need a new API. That's nice, isn't it?

It depends. In this case it's not nice since it requires
"I like to move it, move it".

-- 
With Best Regards,
Andy Shevchenko