iio: accel: Convert to i2c's .probe_new

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/adxl367_i2c.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/adxl367_i2c.c +2 -3

index 3606efa25835..070aad724abd 100644
@@ -41,8 +41,7 @@ static const struct adxl367_ops adxl367_i2c_ops = {
 	.read_fifo = adxl367_i2c_read_fifo,
 };
 
-static int adxl367_i2c_probe(struct i2c_client *client,
-			     const struct i2c_device_id *id)
+static int adxl367_i2c_probe(struct i2c_client *client)
 {
 	struct adxl367_i2c_state *st;
 	struct regmap *regmap;

@@ -78,7 +77,7 @@ static struct i2c_driver adxl367_i2c_driver = {
 		.name = "adxl367_i2c",
 		.of_match_table = adxl367_of_match,
 	},
-	.probe = adxl367_i2c_probe,
+	.probe_new = adxl367_i2c_probe,
 	.id_table = adxl367_i2c_id,
 };
 

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/da311.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/da311.c +2 -3

index ec4e29d260f7..080335fa2ad6 100644
@@ -217,8 +217,7 @@ static void da311_disable(void *client)
 	da311_enable(client, false);
 }
 
-static int da311_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int da311_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -279,7 +278,7 @@ static struct i2c_driver da311_driver = {
 		.name = "da311",
 		.pm = pm_sleep_ptr(&da311_pm_ops),
 	},
-	.probe		= da311_probe,
+	.probe_new	= da311_probe,
 	.id_table	= da311_i2c_id,
 };
 

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/dmard06.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/dmard06.c +2 -3

index 4b69c8530f5e..7390509aaac0 100644
@@ -125,8 +125,7 @@ static const struct iio_info dmard06_info = {
 	.read_raw	= dmard06_read_raw,
 };
 
-static int dmard06_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int dmard06_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -218,7 +217,7 @@ static const struct of_device_id dmard06_of_match[] = {
 MODULE_DEVICE_TABLE(of, dmard06_of_match);
 
 static struct i2c_driver dmard06_driver = {
-	.probe = dmard06_probe,
+	.probe_new = dmard06_probe,
 	.id_table = dmard06_id,
 	.driver = {
 		.name = DMARD06_DRV_NAME,

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/dmard09.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/dmard09.c +2 -3

index cb0246ca72f3..4b7a537f617d 100644
@@ -88,8 +88,7 @@ static const struct iio_info dmard09_info = {
 	.read_raw	= dmard09_read_raw,
 };
 
-static int dmard09_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int dmard09_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -136,7 +135,7 @@ static struct i2c_driver dmard09_driver = {
 	.driver = {
 		.name = DMARD09_DRV_NAME
 	},
-	.probe = dmard09_probe,
+	.probe_new = dmard09_probe,
 	.id_table = dmard09_id,
 };
 

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/dmard10.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/dmard10.c +2 -3

index 8ac62ec0a04a..07e68aed8a13 100644
@@ -175,8 +175,7 @@ static void dmard10_shutdown_cleanup(void *client)
 	dmard10_shutdown(client);
 }
 
-static int dmard10_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int dmard10_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -242,7 +241,7 @@ static struct i2c_driver dmard10_driver = {
 		.name = "dmard10",
 		.pm = pm_sleep_ptr(&dmard10_pm_ops),
 	},
-	.probe		= dmard10_probe,
+	.probe_new	= dmard10_probe,
 	.id_table	= dmard10_i2c_id,
 };
 

.probe_new() doesn't get the i2c_device_id * parameter, so determine that explicitly in .probe(). The device_id array has to move up for that to work.

Quoted 3 lines · Uwe Kleine-König

.probe_new() doesn't get the i2c_device_id * parameter, so determine that explicitly in .probe(). The device_id array has to move up for that to work.

...

Review

Quoted 10 lines

(quoted diff) +10 -0

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

I don't like this part. Can we, please, find a way how to dereference this table via struct i2c_client, please?

Quoted 1 line · Andy Shevchenko

On Sun, Oct 23, 2022 at 03:22:49PM +0200, Uwe Kleine-König wrote:

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+static const struct i2c_device_id kxcjk1013_id[] = {
+	{"kxcjk1013", KXCJK1013},
+	{"kxcj91008", KXCJ91008},
+	{"kxtj21009", KXTJ21009},
+	{"kxtf9",     KXTF9},

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	{"kx023-1025", KX0231025},
+	{"SMO8500",   KXCJ91008},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);

I don't like this part. Can we, please, find a way how to dereference this table via struct i2c_client, please?

It would be possible to do (on top of my patch here as PoC):

drivers/iio/accel/kxcjk-1013.c +1 -1

index e043dd698747..00269b25af99 100644
@@ -1445,7 +1445,7 @@ MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);
 
 static int kxcjk1013_probe(struct i2c_client *client)
 {
-	const struct i2c_device_id *id = i2c_match_id(kxcjk1013_id, client);
+	const struct i2c_device_id *id = i2c_match_id(to_i2c_driver(client->dev.driver)->id_table, client);
 	struct kxcjk1013_data *data;
 	struct iio_dev *indio_dev;
 	struct kxcjk_1013_platform_data *pdata;

(only compile tested), you could even create a function or macro to make this a bit prettier on the source level. For the compiler loading the address from a local symbol instead of from two pointer dereferences is (I guess) a bit more effective and IMHO more natural.

*shrug*, I don't care much, but I don't like to have to rework this series just because you don't like this part. You even didn't give a rationale, I can imagine several different ones:

[ ] it makes the patch bigger
[ ] it results in an unnatural order of symbols in the driver [ ] it's some kind of duplication
[ ] something else

     please elaborate: ________________________________

Quoted 2 lines · Uwe Kleine-König

Quoted 1 line · Andy Shevchenko

On Sun, Oct 23, 2022 at 03:22:49PM +0200, Uwe Kleine-König wrote:

...

Quoted 35 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+static const struct i2c_device_id kxcjk1013_id[] = {
+	{"kxcjk1013", KXCJK1013},
+	{"kxcj91008", KXCJ91008},
+	{"kxtj21009", KXTJ21009},
+	{"kxtf9",     KXTF9},

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	{"kx023-1025", KX0231025},
+	{"SMO8500",   KXCJ91008},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);

I don't like this part. Can we, please, find a way how to dereference this table via struct i2c_client, please?

It would be possible to do (on top of my patch here as PoC):

drivers/iio/accel/kxcjk-1013.c +1 -1

index e043dd698747..00269b25af99 100644
@@ -1445,7 +1445,7 @@ MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);
 
 static int kxcjk1013_probe(struct i2c_client *client)
 {
-	const struct i2c_device_id *id = i2c_match_id(kxcjk1013_id, client);
+	const struct i2c_device_id *id = i2c_match_id(to_i2c_driver(client->dev.driver)->id_table, client);
 	struct kxcjk1013_data *data;
 	struct iio_dev *indio_dev;
 	struct kxcjk_1013_platform_data *pdata;

(only compile tested), you could even create a function or macro to make this a bit prettier on the source level. For the compiler loading the address from a local symbol instead of from two pointer dereferences is (I guess) a bit more effective and IMHO more natural.

Quoted 3 lines

*shrug*, I don't care much, but I don't like to have to rework this series just because you don't like this part. You even didn't give a rationale, I can imagine several different ones:

And I don't want to have ping-ponging the pieces of code (ID tables) because some API has to be fixes or so.

Quoted 5 lines

[ ] it makes the patch bigger
[ ] it results in an unnatural order of symbols in the driver [ ] it's some kind of duplication
[ ] something else

     please elaborate: ________________________________

It adds a burden to the future work with no good justification along with

a churn in _this_ series.

While I like the rest of the series, these things I would rather postpone or rework.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Uwe Kleine-König

Quoted 1 line · Andy Shevchenko

On Sun, Oct 23, 2022 at 03:22:49PM +0200, Uwe Kleine-König wrote:

Quoted 35 lines

Quoted 35 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+static const struct i2c_device_id kxcjk1013_id[] = {
+	{"kxcjk1013", KXCJK1013},
+	{"kxcj91008", KXCJ91008},
+	{"kxtj21009", KXTJ21009},
+	{"kxtf9",     KXTF9},

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	{"kx023-1025", KX0231025},
+	{"SMO8500",   KXCJ91008},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);

I don't like this part. Can we, please, find a way how to dereference this table via struct i2c_client, please?

It would be possible to do (on top of my patch here as PoC):

drivers/iio/accel/kxcjk-1013.c +1 -1

index e043dd698747..00269b25af99 100644
@@ -1445,7 +1445,7 @@ MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);
 
 static int kxcjk1013_probe(struct i2c_client *client)
 {
-	const struct i2c_device_id *id = i2c_match_id(kxcjk1013_id, client);
+	const struct i2c_device_id *id = i2c_match_id(to_i2c_driver(client->dev.driver)->id_table, client);
 	struct kxcjk1013_data *data;
 	struct iio_dev *indio_dev;
 	struct kxcjk_1013_platform_data *pdata;

(only compile tested), you could even create a function or macro to make this a bit prettier on the source level. For the compiler loading the address from a local symbol instead of from two pointer dereferences is (I guess) a bit more effective and IMHO more natural.

Quoted 6 lines

Quoted 3 lines

*shrug*, I don't care much, but I don't like to have to rework this series just because you don't like this part. You even didn't give a rationale, I can imagine several different ones:

And I don't want to have ping-ponging the pieces of code (ID tables) because some API has to be fixes or so.

In this series it's only ping without pong.

To benefit from the local
table instead of fishing the table out of client, the table must be declared already when it's used.

Quoted 7 lines

Quoted 5 lines

[ ] it makes the patch bigger
[ ] it results in an unnatural order of symbols in the driver [ ] it's some kind of duplication
[ ] something else

     please elaborate: ________________________________

It adds a burden to the future work with no good justification along with

This burden exists in the drivers that already today have the table above the probe function? (Ok, there are none in this series, but it happens, see for example

	https://lore.kernel.org/linux-rtc/[email protected]

and a few more in the rtc series.) I don't see a burden here, we're talking about the id table being defined before the probe function, right? How is that a burden? What am I missing?

Quoted 1 line

a churn in _this_ series.

The alternatives are: Split the patch into reorder + convert to .probe_new, or add a declaration for the id table. Among these I like the current approach besto.

Quoted 2 lines

While I like the rest of the series, these things I would rather postpone or rework.

There is no win in postponing, is there?[1] What would be your preferred way to rework?

Quoted 6 lines · Uwe Kleine-König

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Uwe Kleine-König

Quoted 2 lines · Andy Shevchenko

On Sun, Oct 23, 2022 at 03:22:49PM +0200, Uwe Kleine-König wrote:

...

Quoted 53 lines

Quoted 43 lines

Quoted 43 lines

Quoted 6 lines

Quoted 6 lines

+static const struct i2c_device_id kxcjk1013_id[] = { +       {"kxcjk1013", KXCJK1013},
+       {"kxcj91008", KXCJ91008},
+       {"kxtj21009", KXTJ21009},
+       {"kxtf9",     KXTF9},
+       {"kx023-1025", KX0231025},

Quoted 8 lines

Review

Quoted 4 lines

+       {"SMO8500",   KXCJ91008},
+       {}

(quoted diff) +2 -0

+};
+MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);

I don't like this part. Can we, please, find a way how to dereference this
table via struct i2c_client, please?

It would be possible to do (on top of my patch here as PoC):

drivers/iio/accel/kxcjk-1013.c +0 -0

index e043dd698747..00269b25af99 100644
@@ -1445,7 +1445,7 @@ MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);

 static int kxcjk1013_probe(struct i2c_client *client) {
-       const struct i2c_device_id *id = i2c_match_id(kxcjk1013_id, client);
+       const struct i2c_device_id *id = i2c_match_id(to_i2c_driver(client->dev.driver)->id_table, client);
        struct kxcjk1013_data *data;
        struct iio_dev *indio_dev;
        struct kxcjk_1013_platform_data *pdata;

(only compile tested), you could even create a function or macro to make
this a bit prettier on the source level. For the compiler loading the
address from a local symbol instead of from two pointer dereferences is
(I guess) a bit more effective and IMHO more natural.

Quoted 9 lines

Quoted 5 lines

*shrug*, I don't care much, but I don't like to have to rework this
series just because you don't like this part. You even didn't give a
rationale, I can imagine several different ones:

And I don't want to have ping-ponging the pieces of code (ID tables) because
some API has to be fixes or so.

Quoted 1 line

In this series it's only ping without pong.

Exactly. And it means let's put my problem to someone's else shoulders.

Quoted 3 lines

To benefit from the local
table instead of fishing the table out of client, the table must be declared already when it's used.

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.

Quoted 14 lines

Quoted 8 lines

Quoted 5 lines

 [ ] it makes the patch bigger
 [ ] it results in an unnatural order of symbols in the driver [ ] it's some kind of duplication
 [ ] something else
     please elaborate: ________________________________

It adds a burden to the future work with no good justification along with

This burden exists in the drivers that already today have the table above the probe function? (Ok, there are none in this series, but it
happens, see for example

Quoted 7 lines

https://lore.kernel.org/linux-rtc/[email protected]

and a few more in the rtc series.) I don't see a burden here, we're talking about the id table being defined before the probe function, right?
How is that a burden? What am I missing?

Yeah, people haven't had no idea about accessing tables via struct driver,
reviewers of that code neither. Should it be excuse for us to follow that
example?

Quoted 6 lines

Quoted 1 line

a churn in _this_ series.

The alternatives are: Split the patch into reorder + convert to .probe_new, or add a declaration for the id table. Among these I like
the current approach besto.

Alternative is to avoid reordering to begin with, no?

Quoted 7 lines

Quoted 3 lines

While I like the rest of the series, these things I would rather postpone
or rework.

There is no win in postponing, is there?[1] What would be your preferred
way to rework?

My understand of the probe_new is that an attempt to unify i2c with how spi
does. So, why not teach i2c_match_id() to handle this nicely for the caller?

This will allow to leave tables where they are (or move closer to struct
driver), reduce churn with the using of current i2c_match_id() as you showed the long line to get that table. This might need a new API to avoid
changing many drivers at once. But it's business as usual.

Quoted 1 line · Andy Shevchenko

On Mon, Oct 24, 2022 at 11:14:56AM +0200, Uwe Kleine-König wrote:

Quoted 9 lines

Quoted 3 lines

There is no win in postponing, is there?[1] What would be your preferred
way to rework?

My understand of the probe_new is that an attempt to unify i2c with how spi
does. So, why not teach i2c_match_id() to handle this nicely for the caller?

Quoted 6 lines

This will allow to leave tables where they are (or move closer to struct
driver), reduce churn with the using of current i2c_match_id() as you showed the long line to get that table. This might need a new API to avoid
changing many drivers at once. But it's business as usual.

I guess something like spi_get_device_id()

Quoted 2 lines · Nuno Sá

Quoted 1 line · Andy Shevchenko

On Mon, Oct 24, 2022 at 11:14:56AM +0200, Uwe Kleine-König wrote:

...

Quoted 16 lines

Quoted 15 lines

Quoted 3 lines

There is no win in postponing, is there?[1] What would be your preferred
way to rework?

My understand of the probe_new is that an attempt to unify i2c with how spi
does. So, why not teach i2c_match_id() to handle this nicely for the caller?

This will allow to leave tables where they are (or move closer to struct
driver), reduce churn with the using of current i2c_match_id() as you showed the long line to get that table. This might need a new API to avoid
changing many drivers at once. But it's business as usual.

Quoted 1 line

I guess something like spi_get_device_id()

Right, that one I have had in mind when responding.

Quoted 4 lines · Uwe Kleine-König

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Uwe Kleine-König

Quoted 1 line · Andy Shevchenko

On Sun, Oct 23, 2022 at 03:22:49PM +0200, Uwe Kleine-König wrote:

...

Quoted 42 lines

Quoted 41 lines

Quoted 30 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+static const struct i2c_device_id kxcjk1013_id[] = {
+	{"kxcjk1013", KXCJK1013},
+	{"kxcj91008", KXCJ91008},
+	{"kxtj21009", KXTJ21009},
+	{"kxtf9",     KXTF9},

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	{"kx023-1025", KX0231025},
+	{"SMO8500",   KXCJ91008},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);

I don't like this part. Can we, please, find a way how to dereference this table via struct i2c_client, please?

It would be possible to do (on top of my patch here as PoC):

drivers/iio/accel/kxcjk-1013.c +1 -1

index e043dd698747..00269b25af99 100644
@@ -1445,7 +1445,7 @@ MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);
 
 static int kxcjk1013_probe(struct i2c_client *client)
 {
-	const struct i2c_device_id *id = i2c_match_id(kxcjk1013_id, client);
+	const struct i2c_device_id *id = i2c_match_id(to_i2c_driver(client->dev.driver)->id_table, client);
 	struct kxcjk1013_data *data;
 	struct iio_dev *indio_dev;
 	struct kxcjk_1013_platform_data *pdata;

Quoted 8 lines

(only compile tested), you could even create a function or macro to make this a bit prettier on the source level. For the compiler loading the address from a local symbol instead of from two pointer dereferences is (I guess) a bit more effective and IMHO more natural.

*shrug*, I don't care much, but I don't like to have to rework this series just because you don't like this part. You even didn't give a rationale, I can imagine several different ones:

And I don't want to have ping-ponging the pieces of code (ID tables) because some API has to be fixes or so.

Quoted 1 line

In this series it's only ping without pong.

Exactly. And it means let's put my problem to someone's else shoulders.

Quoted 3 lines

To benefit from the local
table instead of fishing the table out of client, the table must be declared already when it's used.

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.

Quoted 14 lines

Quoted 7 lines

Quoted 5 lines

[ ] it makes the patch bigger
[ ] it results in an unnatural order of symbols in the driver [ ] it's some kind of duplication
[ ] something else

     please elaborate: ________________________________

It adds a burden to the future work with no good justification along with

This burden exists in the drivers that already today have the table above the probe function? (Ok, there are none in this series, but it happens, see for example

	https://lore.kernel.org/linux-rtc/[email protected]

Quoted 3 lines

and a few more in the rtc series.) I don't see a burden here, we're talking about the id table being defined before the probe function, right? How is that a burden? What am I missing?

Yeah, people haven't had no idea about accessing tables via struct driver, reviewers of that code neither. Should it be excuse for us to follow that example?

Quoted 5 lines

Quoted 1 line

a churn in _this_ series.

The alternatives are: Split the patch into reorder + convert to .probe_new, or add a declaration for the id table. Among these I like the current approach besto.

Alternative is to avoid reordering to begin with, no?

Quoted 5 lines

Quoted 2 lines

While I like the rest of the series, these things I would rather postpone or rework.

There is no win in postponing, is there?[1] What would be your preferred way to rework?

My understand of the probe_new is that an attempt to unify i2c with how spi does.

So, why not teach i2c_match_id() to handle this nicely for the caller?

This will allow to leave tables where they are (or move closer to struct driver),

reduce churn with the using of current i2c_match_id() as you showed the long line to get that table.

This might need a new API to avoid
changing many drivers at once. But it's business as usual.

Quoted 1 line · Andy Shevchenko

On Mon, Oct 24, 2022 at 11:14:56AM +0200, Uwe Kleine-König wrote:

Quoted 1 line

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?

Quoted 3 lines

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.

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.

Quoted 1 line

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:

drivers/iio/accel/kxcjk-1013.c +12 -2

+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[] = {

drivers/iio/accel/kxcjk-1013.c +0 -0

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

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 2 lines

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?

Quoted 2 lines · Uwe Kleine-König

Quoted 1 line · Andy Shevchenko

On Mon, Oct 24, 2022 at 11:14:56AM +0200, Uwe Kleine-König wrote:

...

Quoted 5 lines

Quoted 1 line

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?

Quoted 3 lines

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.

...

Quoted 7 lines

Quoted 3 lines

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.

...

Quoted 4 lines

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.

...

Quoted 2 lines

Quoted 1 line

Alternative is to avoid reordering to begin with, no?

Quoted 4 lines

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

...

Quoted 22 lines

drivers/iio/accel/kxcjk-1013.c +12 -1

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

drivers/iio/accel/kxcjk-1013.c static const struct acpi_device_id kx_acpi_match[] = { · line 1733

Quoted 11 lines

drivers/iio/accel/kxcjk-1013.c +0 -11

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

Quoted 9 lines

drivers/iio/accel/kxcjk-1013.c +0 -0

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

...

Quoted 6 lines

Quoted 1 line

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?

...

Quoted 6 lines

Quoted 2 lines

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.

Quoted 5 lines

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

...

Quoted 4 lines

Quoted 2 lines

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

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Uwe Kleine-König

Quoted 1 line · Andy Shevchenko

On Mon, Oct 24, 2022 at 11:14:56AM +0200, Uwe Kleine-König wrote:

Quoted 9 lines

Quoted 8 lines

Quoted 1 line

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?

Quoted 2 lines

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

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

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

Quoted 1 line

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

Quoted 2 lines

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 will stop my participation here. That's a bit sad because in my eyes all patches in this series have a positive value

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() :-\

What I consider "churn" though is this discussion. I will stop my participation here. That's a bit sad because in my eyes all patches in this series have a positive value 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() :-\

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/kxsd9-i2c.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/kxsd9-i2c.c +2 -3

index 61346ea8ef19..6b3683ddce36 100644
@@ -10,8 +10,7 @@
 
 #include "kxsd9.h"
 
-static int kxsd9_i2c_probe(struct i2c_client *i2c,
-			   const struct i2c_device_id *id)
+static int kxsd9_i2c_probe(struct i2c_client *i2c)
 {
 	static const struct regmap_config config = {
 		.reg_bits = 8,

@@ -55,7 +54,7 @@ static struct i2c_driver kxsd9_i2c_driver = {
 		.of_match_table = kxsd9_of_match,
 		.pm = pm_ptr(&kxsd9_dev_pm_ops),
 	},
-	.probe		= kxsd9_i2c_probe,
+	.probe_new	= kxsd9_i2c_probe,
 	.remove		= kxsd9_i2c_remove,
 	.id_table	= kxsd9_i2c_id,
 };

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/mc3230.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/mc3230.c +2 -3

index 2462000e0519..efc21871de42 100644
@@ -106,8 +106,7 @@ static const struct iio_info mc3230_info = {
 	.read_raw	= mc3230_read_raw,
 };
 
-static int mc3230_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int mc3230_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -191,7 +190,7 @@ static struct i2c_driver mc3230_driver = {
 		.name = "mc3230",
 		.pm = pm_sleep_ptr(&mc3230_pm_ops),
 	},
-	.probe		= mc3230_probe,
+	.probe_new	= mc3230_probe,
 	.remove		= mc3230_remove,
 	.id_table	= mc3230_i2c_id,
 };

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/mma7660.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/mma7660.c +2 -3

index 85829990bbad..b279ca4dcdc0 100644
@@ -169,8 +169,7 @@ static const struct iio_info mma7660_info = {
 	.attrs			= &mma7660_attribute_group,
 };
 
-static int mma7660_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int mma7660_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -267,7 +266,7 @@ static struct i2c_driver mma7660_driver = {
 		.of_match_table = mma7660_of_match,
 		.acpi_match_table = mma7660_acpi_id,
 	},
-	.probe		= mma7660_probe,
+	.probe_new	= mma7660_probe,
 	.remove		= mma7660_remove,
 	.id_table	= mma7660_i2c_id,
 };

.probe_new() doesn't get the i2c_device_id * parameter, so determine that explicitly in .probe(). The device_id array has to move up for that to work.

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/mxc4005.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/mxc4005.c +2 -3

index df600d2917c0..b146fc82738f 100644
@@ -385,8 +385,7 @@ static int mxc4005_chip_init(struct mxc4005_data *data)
 	return 0;
 }
 
-static int mxc4005_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int mxc4005_probe(struct i2c_client *client)
 {
 	struct mxc4005_data *data;
 	struct iio_dev *indio_dev;

@@ -489,7 +488,7 @@ static struct i2c_driver mxc4005_driver = {
 		.name = MXC4005_DRV_NAME,
 		.acpi_match_table = ACPI_PTR(mxc4005_acpi_match),
 	},
-	.probe		= mxc4005_probe,
+	.probe_new	= mxc4005_probe,
 	.id_table	= mxc4005_id,
 };
 

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/mxc6255.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/mxc6255.c +2 -3

index 9aeeadc420d3..aa2e660545f8 100644
@@ -113,8 +113,7 @@ static const struct regmap_config mxc6255_regmap_config = {
 	.readable_reg = mxc6255_is_readable_reg,
 };
 
-static int mxc6255_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int mxc6255_probe(struct i2c_client *client)
 {
 	struct mxc6255_data *data;
 	struct iio_dev *indio_dev;

@@ -184,7 +183,7 @@ static struct i2c_driver mxc6255_driver = {
 		.name = MXC6255_DRV_NAME,
 		.acpi_match_table = ACPI_PTR(mxc6255_acpi_match),
 	},
-	.probe		= mxc6255_probe,
+	.probe_new	= mxc6255_probe,
 	.id_table	= mxc6255_id,
 };
 

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/stk8312.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/stk8312.c +2 -3

index 7b1d6fb692b3..68f680db7505 100644
@@ -498,8 +498,7 @@ static const struct iio_buffer_setup_ops stk8312_buffer_setup_ops = {
 	.postdisable = stk8312_buffer_postdisable,
 };
 
-static int stk8312_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int stk8312_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -645,7 +644,7 @@ static struct i2c_driver stk8312_driver = {
 		.name = STK8312_DRIVER_NAME,
 		.pm = pm_sleep_ptr(&stk8312_pm_ops),
 	},
-	.probe =            stk8312_probe,
+	.probe_new =        stk8312_probe,
 	.remove =           stk8312_remove,
 	.id_table =         stk8312_i2c_id,
 };

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/accel/stk8ba50.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/stk8ba50.c +2 -3

index 2f5e4ab2a6e7..44f6e0fbdfcc 100644
@@ -379,8 +379,7 @@ static const struct iio_buffer_setup_ops stk8ba50_buffer_setup_ops = {
 	.postdisable = stk8ba50_buffer_postdisable,
 };
 
-static int stk8ba50_probe(struct i2c_client *client,
-			  const struct i2c_device_id *id)
+static int stk8ba50_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;

@@ -544,7 +543,7 @@ static struct i2c_driver stk8ba50_driver = {
 		.pm = pm_sleep_ptr(&stk8ba50_pm_ops),
 		.acpi_match_table = ACPI_PTR(stk8ba50_acpi_id),
 	},
-	.probe =            stk8ba50_probe,
+	.probe_new =        stk8ba50_probe,
 	.remove =           stk8ba50_remove,
 	.id_table =         stk8ba50_i2c_id,
 };

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/magnetometer/st_magn_i2c.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/magnetometer/st_magn_i2c.c +2 -3

index c5d8c303db4e..b4098d3b3813 100644
@@ -54,8 +54,7 @@ static const struct of_device_id st_magn_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, st_magn_of_match);
 
-static int st_magn_i2c_probe(struct i2c_client *client,
-			     const struct i2c_device_id *id)
+static int st_magn_i2c_probe(struct i2c_client *client)
 {
 	const struct st_sensor_settings *settings;
 	struct st_sensor_data *mdata;

@@ -107,7 +106,7 @@ static struct i2c_driver st_magn_driver = {
 		.name = "st-magn-i2c",
 		.of_match_table = st_magn_of_match,
 	},
-	.probe = st_magn_i2c_probe,
+	.probe_new = st_magn_i2c_probe,
 	.id_table = st_magn_id_table,
 };
 module_i2c_driver(st_magn_driver);

The probe function doesn't make use of the i2c_device_id * parameter so it can be trivially converted.

Signed-off-by Uwe Kleine-König <[email protected]>

---
 drivers/iio/light/vl6180.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/light/vl6180.c +2 -3

index d47a4f6f4e87..8b56df26c59e 100644
@@ -493,8 +493,7 @@ static int vl6180_init(struct vl6180_data *data)
 	return vl6180_hold(data, false);
 }
 
-static int vl6180_probe(struct i2c_client *client,
-			  const struct i2c_device_id *id)
+static int vl6180_probe(struct i2c_client *client)
 {
 	struct vl6180_data *data;
 	struct iio_dev *indio_dev;

@@ -539,7 +538,7 @@ static struct i2c_driver vl6180_driver = {
 		.name   = VL6180_DRV_NAME,
 		.of_match_table = vl6180_of_match,
 	},
-	.probe  = vl6180_probe,
+	.probe_new = vl6180_probe,
 	.id_table = vl6180_id,
 };