Hello Andy,

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. To benefit from the local
table instead of fishing the table out of client, the table must be
declared already when it's used.

> >  [ ] 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@pengutronix.de

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?

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

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

Best regards
Uwe

[1] .probe_new was created in 2016!

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |