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

["Nuno Sá" <noname.nuno@gmail.com>]

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

I guess something like spi_get_device_id()

- Nuno Sá