Re: [PATCH v2] iio: potentiometer: mcp4018: Use i2c_get_match_data()

From: Jonathan Cameron <jic23@kernel.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Peter Rosin <peda@axentia.se>,
	Lars-Peter Clausen <lars@metafoo.de>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v2] iio: potentiometer: mcp4018: Use i2c_get_match_data()
Date: Sun, 23 Jul 2023 10:12:13 +0100	[thread overview]
Message-ID: <20230723101213.66dbae9e@jic23-huawei> (raw)
In-Reply-To: <OS0PR01MB592290B83D0EEA1928DB4D2A863DA@OS0PR01MB5922.jpnprd01.prod.outlook.com>

On Sun, 23 Jul 2023 06:12:20 +0000
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> Hi Peter Rosin,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v2] iio: potentiometer: mcp4018: Use
> > i2c_get_match_data()
> > 
> > Hi!
> > 
> > 2023-07-21 at 09:16, Biju Das wrote:  
> > > Replace of_device_get_match_data() and i2c_match_id() by i2c_get_match
> > > _data() by making similar I2C and DT-based matching table.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v1->v2:
> > >  * Added similar similar I2C and DT-based matching table.
> > >  * Fixed typo i2c_get_match_data(dev)->i2c_get_match_data(client).
> > >  * Dropped error check as all tables have data pointers.
> > >
> > > Note:
> > >  This patch is only compile tested.
> > > ---
> > >  drivers/iio/potentiometer/mcp4018.c | 34
> > > +++++++++++++++--------------
> > >  1 file changed, 18 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/potentiometer/mcp4018.c
> > > b/drivers/iio/potentiometer/mcp4018.c
> > > index 89daecc90305..b064e86ecce8 100644
> > > --- a/drivers/iio/potentiometer/mcp4018.c
> > > +++ b/drivers/iio/potentiometer/mcp4018.c
> > > @@ -99,20 +99,24 @@ static const struct iio_info mcp4018_info = {
> > >  	.write_raw = mcp4018_write_raw,
> > >  };
> > >
> > > +#define MCP4018_ID_TABLE(name, cfg) {				\
> > > +	name, .driver_data = (kernel_ulong_t)&mcp4018_cfg[cfg],	\
> > > +}  
> > 
> > It is inconsistent to have a named field for .driver_data but not for
> > .name. Also, I dislike the cast and wonder if the trivial simplification
> > in probe() is really worth this churn when that ugly cast is needed?   
> 
> It saving lines of code and better than, &mcp4018_cfg[i2c_match_id(mcp4018_id, client)>driver_data]; right??
> 
> 
> >The
> > reason the two tables differ and do not both have pointers already is
> > precisely my dislike for that cast.  
> 
> > 
> > Anyway, something like this instead? Or _name instead of id?
> > Whatever...  
> 
> OK, will use _name.
I dropped previous version of this and the other patch (as comment applies
there as well) from the togreg branch of iio.git

> 
> > 
> > #define MCP4018_ID_TABLE(id, cfg) {				\
> > 	.name = id,						\
> > 	.driver_data = (kernel_ulong_t)&mcp4018_cfg[cfg],	\
> > }
> >   
> > > +
> > >  static const struct i2c_device_id mcp4018_id[] = {
> > > -	{ "mcp4017-502", MCP4018_502 },
> > > -	{ "mcp4017-103", MCP4018_103 },
> > > -	{ "mcp4017-503", MCP4018_503 },
> > > -	{ "mcp4017-104", MCP4018_104 },
> > > -	{ "mcp4018-502", MCP4018_502 },
> > > -	{ "mcp4018-103", MCP4018_103 },
> > > -	{ "mcp4018-503", MCP4018_503 },
> > > -	{ "mcp4018-104", MCP4018_104 },
> > > -	{ "mcp4019-502", MCP4018_502 },
> > > -	{ "mcp4019-103", MCP4018_103 },
> > > -	{ "mcp4019-503", MCP4018_503 },
> > > -	{ "mcp4019-104", MCP4018_104 },
> > > -	{}
> > > +	MCP4018_ID_TABLE("mcp4017-502", MCP4018_502),
> > > +	MCP4018_ID_TABLE("mcp4017-103", MCP4018_103),
> > > +	MCP4018_ID_TABLE("mcp4017-503", MCP4018_503),
> > > +	MCP4018_ID_TABLE("mcp4017-104", MCP4018_104),
> > > +	MCP4018_ID_TABLE("mcp4018-502", MCP4018_502),
> > > +	MCP4018_ID_TABLE("mcp4018-103", MCP4018_103),
> > > +	MCP4018_ID_TABLE("mcp4018-503", MCP4018_503),
> > > +	MCP4018_ID_TABLE("mcp4018-104", MCP4018_104),
> > > +	MCP4018_ID_TABLE("mcp4019-502", MCP4018_502),
> > > +	MCP4018_ID_TABLE("mcp4019-103", MCP4018_103),
> > > +	MCP4018_ID_TABLE("mcp4019-503", MCP4018_503),
> > > +	MCP4018_ID_TABLE("mcp4019-104", MCP4018_104),
> > > +	{ /* sentinel */ }
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, mcp4018_id);
> > >
> > > @@ -157,9 +161,7 @@ static int mcp4018_probe(struct i2c_client  
> > *client)  
> > >  	i2c_set_clientdata(client, indio_dev);
> > >  	data->client = client;
> > >
> > > -	data->cfg = device_get_match_data(dev);
> > > -	if (!data->cfg)
> > > -		data->cfg = &mcp4018_cfg[i2c_match_id(mcp4018_id, client)-
> > >driver_data];
> > > +	data->cfg = i2c_get_match_data(client);  
> > 
> > NULL-check here? I know the original i2c_match_id call is not checked
> > for non-matches, but that feels like a simpler function than
> > i2c_get_match_data. And less prone to changes.
> > 
> > Same comments of course applies to the mcp4531 patch as well.  
> 
> Some subsystem people doesn't want error check as all tables have data pointers. See [1]
> 
> [1] https://lore.kernel.org/linux-renesas-soc/20230717145623.473cffca@booty/
> 
> I am leaving subsystem maintainer to take final word on this.

I'm fine either way. It seems unlikely any changes to that function will introduce
a path for a NULL return if there isn't one there today, but you never know I guess.
However we don't normally spend too much effort protecting against potential
future new return values.

> 
> The new API is very simple [2]
> [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L124
> 
> Cheers,
> Biju
> 
> > >
> > >  	indio_dev->info = &mcp4018_info;
> > >  	indio_dev->channels = &mcp4018_channel;  