[PATCH v2 0/2] ltc4306 driver enhancements

[PATCH v2 0/2] ltc4306 driver enhancements

[Biju Das]

This patch series is based on the code improvement work done for
rtc-isl1208 driver and is just compile tested.

v1->v2:
 * Added Rb tag from Geert for patch#1
 * Added patch#2 for dropping enum ltc_type and split the array chips[] as
   individual variables, and make lines shorter by referring to
   e.g. &ltc_4305_chip instead of &chips[ltc_4305].

Biju Das (2):
  i2c: mux: ltc4306: Simplify probe()
  i2c: mux: ltc4306: Drop enum ltc_type and split chips[]

 drivers/i2c/muxes/i2c-mux-ltc4306.c | 33 ++++++++++++-----------------
 1 file changed, 13 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] i2c: mux: ltc4306: Simplify probe()

[Biju Das]

The ltc4306_id[].driver_data could store a pointer to the chips,
like for DT-based matching, making I2C and DT-based matching
more similar.

After that, we can simplify the probe() by replacing of_device_get_
match_data() and i2c_match_id() by i2c_get_match_data() as we have
similar I2C and DT-based matching table.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2:
 * Added Rb tag from Geert.
---
 drivers/i2c/muxes/i2c-mux-ltc4306.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-ltc4306.c b/drivers/i2c/muxes/i2c-mux-ltc4306.c
index 5a03031519be..c7dfd5eba413 100644
--- a/drivers/i2c/muxes/i2c-mux-ltc4306.c
+++ b/drivers/i2c/muxes/i2c-mux-ltc4306.c
@@ -192,8 +192,8 @@ static int ltc4306_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 }
 
 static const struct i2c_device_id ltc4306_id[] = {
-	{ "ltc4305", ltc_4305 },
-	{ "ltc4306", ltc_4306 },
+	{ "ltc4305", .driver_data = (kernel_ulong_t)&chips[ltc_4305] },
+	{ "ltc4306", .driver_data = (kernel_ulong_t)&chips[ltc_4306] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ltc4306_id);
@@ -216,10 +216,9 @@ static int ltc4306_probe(struct i2c_client *client)
 	unsigned int val = 0;
 	int num, ret;
 
-	chip = of_device_get_match_data(&client->dev);
-
+	chip = i2c_get_match_data(client);
 	if (!chip)
-		chip = &chips[i2c_match_id(ltc4306_id, client)->driver_data];
+		return -ENODEV;
 
 	idle_disc = device_property_read_bool(&client->dev,
 					      "i2c-mux-idle-disconnect");
-- 
2.25.1

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

[Biju Das]

Drop enum ltc_type and split the array chips[] as individual
variables, and make lines shorter by referring to e.g. &ltc_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(-)

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);
-- 
2.25.1

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

[Andi Shyti]

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. &ltc_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

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

[Peter Rosin]

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"???

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);

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

[Biju Das]

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);