Hello Andy,
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:
> > On Mon, Oct 24, 2022 at 11:39:51AM +0300, Andy Shevchenko wrote:
> > > On Mon, Oct 24, 2022 at 09:05:18AM +0200, Uwe Kleine-König wrote:
> > > > On Sun, Oct 23, 2022 at 10:06:11PM +0300, Andy Shevchenko wrote:
> > > > > On Sun, Oct 23, 2022 at 03:22:49PM +0200, Uwe Kleine-König wrote:
>
> ...
>
> > > > > > +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?
> > > >
> > > > It would be possible to do (on top of my patch here as PoC):
> > > >
> > > > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> > > > index e043dd698747..00269b25af99 100644
> > > > --- a/drivers/iio/accel/kxcjk-1013.c
> > > > +++ b/drivers/iio/accel/kxcjk-1013.c
> > > > @@ -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:
> > >
> > > 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.
>
> 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?
> > 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.
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 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/20221021130706.178687-4-u.kleine-koenig@xxxxxxxxxxxxxx
> >
> > 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?
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.
> > > 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?
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:
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -101,9 +101,17 @@ EXPORT_SYMBOL_GPL(i2c_freq_mode_string);
const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
- if (!(id && client))
+ if (!client)
return NULL;
+ if (!id) {
+ struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
+
+ id = driver->id_table;
+ if (!id)
+ return NULL;
+ }
+
while (id->name[0]) {
if (strcmp(client->name, id->name) == 0)
return id;
the patch under discussion could be reduced to:
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index adc66b3615c0..b1d11c96597d 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1432,9 +1432,9 @@ static void kxcjk1013_disable_regulators(void *d)
regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
}
-static int kxcjk1013_probe(struct i2c_client *client,
- const struct i2c_device_id *id)
+static int kxcjk1013_probe(struct i2c_client *client)
{
+ const struct i2c_device_id *id = i2c_match_id(NULL, client);
struct kxcjk1013_data *data;
struct iio_dev *indio_dev;
struct kxcjk_1013_platform_data *pdata;
@@ -1749,7 +1749,7 @@ static struct i2c_driver kxcjk1013_driver = {
.of_match_table = kxcjk1013_of_match,
.pm = &kxcjk1013_pm_ops,
},
- .probe = kxcjk1013_probe,
+ .probe_new = kxcjk1013_probe,
.remove = kxcjk1013_remove,
.id_table = kxcjk1013_id,
};
I assume you agree up to here.
After that change there is some incentive to do
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index b1d11c96597d..e043dd698747 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1432,9 +1432,20 @@ static void kxcjk1013_disable_regulators(void *d)
regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
}
+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);
-
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.)
> > > 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.
My understanding is a bit different, but this detail doesn't matter.
> 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.
> This will allow to leave tables where they are (or move closer to struct
> driver),
As written above, reordering the driver is (IMHO) cheap enough given the
benefit.
> 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.)
> 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?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
