[PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Jean Delvare]

Use a function pointer to decide whether to call i2c_add_adapter or
i2c_add_numbered_adapter. This makes the code more compact than the
current strategy of having the common code in a separate function.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-bit.c |   21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

--- linux-2.6.37-rc5.orig/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 10:13:49.000000000 +0100
+++ linux-2.6.37-rc5/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:02:38.000000000 +0100
@@ -600,7 +600,8 @@ static const struct i2c_algorithm i2c_bi
 /*
  * registering functions to load algorithms at runtime
  */
-static int i2c_bit_prepare_bus(struct i2c_adapter *adap)
+static int __i2c_bit_add_bus(struct i2c_adapter *adap,
+			     int (*add_adapter)(struct i2c_adapter *))
 {
 	struct i2c_algo_bit_data *bit_adap = adap->algo_data;
 
@@ -614,30 +615,18 @@ static int i2c_bit_prepare_bus(struct i2
 	adap->algo = &i2c_bit_algo;
 	adap->retries = 3;
 
-	return 0;
+	return add_adapter(adap);
 }
 
 int i2c_bit_add_bus(struct i2c_adapter *adap)
 {
-	int err;
-
-	err = i2c_bit_prepare_bus(adap);
-	if (err)
-		return err;
-
-	return i2c_add_adapter(adap);
+	return __i2c_bit_add_bus(adap, i2c_add_adapter);
 }
 EXPORT_SYMBOL(i2c_bit_add_bus);
 
 int i2c_bit_add_numbered_bus(struct i2c_adapter *adap)
 {
-	int err;
-
-	err = i2c_bit_prepare_bus(adap);
-	if (err)
-		return err;
-
-	return i2c_add_numbered_adapter(adap);
+	return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
 }
 EXPORT_SYMBOL(i2c_bit_add_numbered_bus);
 


-- 
Jean Delvare

Re: [PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Ben Dooks]

On Tue, Dec 07, 2010 at 11:06:31AM +0100, Jean Delvare wrote:
> Use a function pointer to decide whether to call i2c_add_adapter or
> i2c_add_numbered_adapter. This makes the code more compact than the
> current strategy of having the common code in a separate function.

ok, how about changing i2c_add_numbered_adapter to take a -1 to mean
assign bus number automatically? or something similar?
 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |   21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)
> 
> --- linux-2.6.37-rc5.orig/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 10:13:49.000000000 +0100
> +++ linux-2.6.37-rc5/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:02:38.000000000 +0100
> @@ -600,7 +600,8 @@ static const struct i2c_algorithm i2c_bi
>  /*
>   * registering functions to load algorithms at runtime
>   */
> -static int i2c_bit_prepare_bus(struct i2c_adapter *adap)
> +static int __i2c_bit_add_bus(struct i2c_adapter *adap,
> +			     int (*add_adapter)(struct i2c_adapter *))
>  {
>  	struct i2c_algo_bit_data *bit_adap = adap->algo_data;
>  
> @@ -614,30 +615,18 @@ static int i2c_bit_prepare_bus(struct i2
>  	adap->algo = &i2c_bit_algo;
>  	adap->retries = 3;
>  
> -	return 0;
> +	return add_adapter(adap);
>  }
>  
>  int i2c_bit_add_bus(struct i2c_adapter *adap)
>  {
> -	int err;
> -
> -	err = i2c_bit_prepare_bus(adap);
> -	if (err)
> -		return err;
> -
> -	return i2c_add_adapter(adap);
> +	return __i2c_bit_add_bus(adap, i2c_add_adapter);
>  }
>  EXPORT_SYMBOL(i2c_bit_add_bus);
>  
>  int i2c_bit_add_numbered_bus(struct i2c_adapter *adap)
>  {
> -	int err;
> -
> -	err = i2c_bit_prepare_bus(adap);
> -	if (err)
> -		return err;
> -
> -	return i2c_add_numbered_adapter(adap);
> +	return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
>  }
>  EXPORT_SYMBOL(i2c_bit_add_numbered_bus);
>  
> 
> 
> -- 
> Jean Delvare
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Michael Lawnick]

Ben Dooks said the following:
> On Tue, Dec 07, 2010 at 11:06:31AM +0100, Jean Delvare wrote:
>> Use a function pointer to decide whether to call i2c_add_adapter or
>> i2c_add_numbered_adapter. This makes the code more compact than the
>> current strategy of having the common code in a separate function.
> 
> ok, how about changing i2c_add_numbered_adapter to take a -1 to mean
> assign bus number automatically? or something similar?

IMHO better: i2c_add_adapter with optional (-1) bus parameter?

-- 
KR
Michael

Re: [PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Jean Delvare]

Hi Michael, Ben,

On Tue, 07 Dec 2010 12:59:35 +0100, Michael Lawnick wrote:
> Ben Dooks said the following:
> > On Tue, Dec 07, 2010 at 11:06:31AM +0100, Jean Delvare wrote:
> >> Use a function pointer to decide whether to call i2c_add_adapter or
> >> i2c_add_numbered_adapter. This makes the code more compact than the
> >> current strategy of having the common code in a separate function.
> > 
> > ok, how about changing i2c_add_numbered_adapter to take a -1 to mean
> > assign bus number automatically? or something similar?
> 
> IMHO better: i2c_add_adapter with optional (-1) bus parameter?

Which problem are you both trying to solve, please?

-- 
Jean Delvare

Re: [PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Michael Lawnick]

Jean Delvare said the following:
> Hi Michael, Ben,
> 
> On Tue, 07 Dec 2010 12:59:35 +0100, Michael Lawnick wrote:
>> Ben Dooks said the following:
>> > On Tue, Dec 07, 2010 at 11:06:31AM +0100, Jean Delvare wrote:
>> >> Use a function pointer to decide whether to call i2c_add_adapter or
>> >> i2c_add_numbered_adapter. This makes the code more compact than the
>> >> current strategy of having the common code in a separate function.
>> > 
>> > ok, how about changing i2c_add_numbered_adapter to take a -1 to mean
>> > assign bus number automatically? or something similar?
>> 
>> IMHO better: i2c_add_adapter with optional (-1) bus parameter?
> 
> Which problem are you both trying to solve, please?
> 
Function pointers tend to hide information. Seeing the targeted function
in source code makes it more clear, IMHO.

-- 
KR
Michael

Re: [PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Jean Delvare]

On Wed, 08 Dec 2010 10:59:18 +0100, Michael Lawnick wrote:
> Jean Delvare said the following:
> > Hi Michael, Ben,
> > 
> > On Tue, 07 Dec 2010 12:59:35 +0100, Michael Lawnick wrote:
> >> Ben Dooks said the following:
> >> > On Tue, Dec 07, 2010 at 11:06:31AM +0100, Jean Delvare wrote:
> >> >> Use a function pointer to decide whether to call i2c_add_adapter or
> >> >> i2c_add_numbered_adapter. This makes the code more compact than the
> >> >> current strategy of having the common code in a separate function.
> >> > 
> >> > ok, how about changing i2c_add_numbered_adapter to take a -1 to mean
> >> > assign bus number automatically? or something similar?
> >> 
> >> IMHO better: i2c_add_adapter with optional (-1) bus parameter?
> > 
> > Which problem are you both trying to solve, please?
>
> Function pointers tend to hide information. Seeing the targeted function
> in source code makes it more clear, IMHO.

This doesn't sound like a valid argument when the provider of the
function pointer is only 20 lines away from the call site in the very
same file, sorry.

Adding a parameter to i2c_add_adapter would mean changing 105 calling
sites. You have to understand that we aren't going to do that without a
very good reason. Ben's proposal is equally invasive, as every current
call to i2c_add_adapter would have to set the id to -1 before. This
means changing 74 drivers for a marginal benefit.

If someday calls to i2c_add_numbered_adapter() outnumber calls to
i2c_add_adapter() by a factor 3, we can reconsider. But this isn't the
case today. I am not particularly happy with the current situation
myself, but it seemed like the best option when
i2c_add_numbered_adapter() was introduced, and I see no reason to
reconsider at this point in time.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Ben Dooks]

On Wed, Dec 08, 2010 at 02:32:36PM +0100, Jean Delvare wrote:
> On Wed, 08 Dec 2010 10:59:18 +0100, Michael Lawnick wrote:
> > Jean Delvare said the following:
> > > Hi Michael, Ben,
> > > 
> > > On Tue, 07 Dec 2010 12:59:35 +0100, Michael Lawnick wrote:
> > >> Ben Dooks said the following:
> > >> > On Tue, Dec 07, 2010 at 11:06:31AM +0100, Jean Delvare wrote:
> > >> >> Use a function pointer to decide whether to call i2c_add_adapter or
> > >> >> i2c_add_numbered_adapter. This makes the code more compact than the
> > >> >> current strategy of having the common code in a separate function.
> > >> > 
> > >> > ok, how about changing i2c_add_numbered_adapter to take a -1 to mean
> > >> > assign bus number automatically? or something similar?
> > >> 
> > >> IMHO better: i2c_add_adapter with optional (-1) bus parameter?
> > > 
> > > Which problem are you both trying to solve, please?
> >
> > Function pointers tend to hide information. Seeing the targeted function
> > in source code makes it more clear, IMHO.
> 
> This doesn't sound like a valid argument when the provider of the
> function pointer is only 20 lines away from the call site in the very
> same file, sorry.
> 
> Adding a parameter to i2c_add_adapter would mean changing 105 calling
> sites. You have to understand that we aren't going to do that without a
> very good reason. Ben's proposal is equally invasive, as every current
> call to i2c_add_adapter would have to set the id to -1 before. This
> means changing 74 drivers for a marginal benefit.
> 
> If someday calls to i2c_add_numbered_adapter() outnumber calls to
> i2c_add_adapter() by a factor 3, we can reconsider. But this isn't the
> case today. I am not particularly happy with the current situation
> myself, but it seemed like the best option when
> i2c_add_numbered_adapter() was introduced, and I see no reason to
> reconsider at this point in time.

How about adding an in-lined (in header) something this like:

static inline int i2c_add_adapter(struct i2c_adapter *adap)
{
	return i2c_add_numbered_adapter(-1, adap);
}

anyway, not desperately important (and code not tested either)

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH 1/2] i2c-algo-bit: Refactor adapter registration

[Jean Delvare]

On Mon, 13 Dec 2010 23:47:23 +0000, Ben Dooks wrote:
> On Wed, Dec 08, 2010 at 02:32:36PM +0100, Jean Delvare wrote:
> > On Wed, 08 Dec 2010 10:59:18 +0100, Michael Lawnick wrote:
> > > Function pointers tend to hide information. Seeing the targeted function
> > > in source code makes it more clear, IMHO.
> > 
> > This doesn't sound like a valid argument when the provider of the
> > function pointer is only 20 lines away from the call site in the very
> > same file, sorry.
> > 
> > Adding a parameter to i2c_add_adapter would mean changing 105 calling
> > sites. You have to understand that we aren't going to do that without a
> > very good reason. Ben's proposal is equally invasive, as every current
> > call to i2c_add_adapter would have to set the id to -1 before. This
> > means changing 74 drivers for a marginal benefit.
> > 
> > If someday calls to i2c_add_numbered_adapter() outnumber calls to
> > i2c_add_adapter() by a factor 3, we can reconsider. But this isn't the
> > case today. I am not particularly happy with the current situation
> > myself, but it seemed like the best option when
> > i2c_add_numbered_adapter() was introduced, and I see no reason to
> > reconsider at this point in time.
> 
> How about adding an in-lined (in header) something this like:
> 
> static inline int i2c_add_adapter(struct i2c_adapter *adap)
> {
> 	return i2c_add_numbered_adapter(-1, adap);
> }
> 
> anyway, not desperately important (and code not tested either)

i2c_add_numbered_adapter doesn't take the bus number as a parameter, it
reads it from adap->nr. So the inline function would rather be:

static inline int i2c_add_adapter(struct i2c_adapter *adap)
{
	adap->nr = -1;
	return i2c_add_numbered_adapter(adap);
}

But then again, which problem are you trying to solve? What you propose
it different from what we have today, but I fail to see how it would be
_better_ than what we have today.

-- 
Jean Delvare