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. <c_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@xxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > 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)<c_4305_chip },
> > + { "ltc4306", .driver_data = (kernel_ulong_t)<c_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 = <c_4305_chip },
> > + { .compatible = "lltc,ltc4306", .data = <c_4306_chip },
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, ltc4306_of_match);
