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.
> 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 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?
> > 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?
> > 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.
--
With Best Regards,
Andy Shevchenko
