On Mon, 2022-10-24 at 12:46 +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.
>
> > 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.
>
I guess something like spi_get_device_id()
- Nuno Sá
