| Message ID | 20230717134807.265302-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | ltc4306 driver enhancements | expand |
Hi Biju, On Mon, Jul 17, 2023 at 02:48:07PM +0100, 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 > instead of &chips[ltc_4305]. > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
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"??? 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)<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);
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@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)<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);
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);
Drop enum ltc_type and split the array chips[] as individual variables, and make lines shorter by referring to e.g. <c_4305_chip 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(-)