Re: [PATCH 10/23] iio: accel: kxcjk-1013: Convert to i2c's .probe_new

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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

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