RE: [PATCH v2 2/2] i2c: mux: ltc4306: Drop enum ltc_type and split chips[]

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Peter Rosin <peda@axentia.se>
Cc: Michael Hennerich <michael.hennerich@analog.com>,
	"linux-i2c@vger.kernel.org" <linux-i2c@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 2/2] i2c: mux: ltc4306: Drop enum ltc_type and split chips[]
Date: Sat, 5 Aug 2023 14:25:44 +0000	[thread overview]
Message-ID: <TYCPR01MB59334B6AFAEADD75DA94FC2C860EA@TYCPR01MB5933.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <c557d38f-a899-69a0-8c76-b867f0448a6d@axentia.se>

Hi Peter Rosin,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/2] i2c: mux: ltc4306: Drop enum ltc_type and
> split chips[]
> 
> Hi!
> 
> 2023-07-17 at 15:48, Biju Das wrote:
> > Drop enum ltc_type and split the array chips[] as individual
> > variables, and make lines shorter by referring to e.g. &ltc_4305_chip
> 
> What do you mean "make lines shorter"???

Here the main point is ,

1) A single API to find match data for this device from a variety of tables
   OF/ACPI/I2C and tomorrow I2C sysfs. You can avoid lot of if's here.

2) Dropping unnecessary enum.

Basically, it is saving code size for this driver.

Cheers,
Biju

> 
> By a few percent or something (2 chars). When your previous patch made
> those same lines more than 2.5 times as long! I have to say, going from 24
> to 62 isn't exactly making lines shorter...
> 
> Not that I care deeply about line length well below 80, but if one of the
> selling points is line length you should present a patch series that
> actually make lines shorter instead of longer. However, the need to sell a
> patch on shorter line length when the lines are actually made longer by
> the series is a sign that something is not right.
> 
> So, again, I fail to see the point of patches like this.
> 
> Also again, I think it is bad to have a named field for .driver_data but
> not for the other member of the struct i2c_device_id inits. Not that it
> matters much when the whole exercise is pointless.
> 
> Also again, what you need to present to get me on board for patches like
> this is to somehow make it *one* table with driver data. Then it makes
> sense to force all driver data to have the same value. But as I see no
> road map for unifying the tables for driver data there is simply no point
> to the churn...
> 
> Cheers,
> Peter
> 
> > instead of &chips[ltc_4305].
> >
> > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2:
> >  * New patch
> > ---
> >  drivers/i2c/muxes/i2c-mux-ltc4306.c | 28 +++++++++++-----------------
> >  1 file changed, 11 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-ltc4306.c
> > b/drivers/i2c/muxes/i2c-mux-ltc4306.c
> > index c7dfd5eba413..c4f090e8d6db 100644
> > --- a/drivers/i2c/muxes/i2c-mux-ltc4306.c
> > +++ b/drivers/i2c/muxes/i2c-mux-ltc4306.c
> > @@ -34,11 +34,6 @@
> >  #define LTC_GPIO_ALL_INPUT	0xC0
> >  #define LTC_SWITCH_MASK		0xF0
> >
> > -enum ltc_type {
> > -	ltc_4305,
> > -	ltc_4306,
> > -};
> > -
> >  struct chip_desc {
> >  	u8 nchans;
> >  	u8 num_gpios;
> > @@ -50,14 +45,13 @@ struct ltc4306 {
> >  	const struct chip_desc *chip;
> >  };
> >
> > -static const struct chip_desc chips[] = {
> > -	[ltc_4305] = {
> > -		.nchans = LTC4305_MAX_NCHANS,
> > -	},
> > -	[ltc_4306] = {
> > -		.nchans = LTC4306_MAX_NCHANS,
> > -		.num_gpios = 2,
> > -	},
> > +static const struct chip_desc ltc_4305_chip = {
> > +	.nchans = LTC4305_MAX_NCHANS
> > +};
> > +
> > +static const struct chip_desc ltc_4306_chip = {
> > +	.nchans = LTC4306_MAX_NCHANS,
> > +	.num_gpios = 2
> >  };
> >
> >  static bool ltc4306_is_volatile_reg(struct device *dev, unsigned int
> > reg) @@ -192,15 +186,15 @@ static int ltc4306_deselect_mux(struct
> > i2c_mux_core *muxc, u32 chan)  }
> >
> >  static const struct i2c_device_id ltc4306_id[] = {
> > -	{ "ltc4305", .driver_data = (kernel_ulong_t)&chips[ltc_4305] },
> > -	{ "ltc4306", .driver_data = (kernel_ulong_t)&chips[ltc_4306] },
> > +	{ "ltc4305", .driver_data = (kernel_ulong_t)&ltc_4305_chip },
> > +	{ "ltc4306", .driver_data = (kernel_ulong_t)&ltc_4306_chip },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, ltc4306_id);
> >
> >  static const struct of_device_id ltc4306_of_match[] = {
> > -	{ .compatible = "lltc,ltc4305", .data = &chips[ltc_4305] },
> > -	{ .compatible = "lltc,ltc4306", .data = &chips[ltc_4306] },
> > +	{ .compatible = "lltc,ltc4305", .data = &ltc_4305_chip },
> > +	{ .compatible = "lltc,ltc4306", .data = &ltc_4306_chip },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, ltc4306_of_match);