Patchwork [v1,4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

login
register
mail settings
Submitter Doug Anderson
Date Feb. 13, 2013, 6:02 p.m.
Message ID <1360778532-7480-4-git-send-email-dianders@chromium.org>
Download mbox | patch
Permalink /patch/220224/
State Rejected
Headers show

Comments

Doug Anderson - Feb. 13, 2013, 6:02 p.m.
The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
we don't want to force the bus number of the adapter.  This is
non-ideal because:
* 0 is actually a valid bus number to request
* i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
  -1 to mean the same thing.  That means extra logic in
  i2c_add_mux_adapter().

Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
accordingly.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Notes:
- If there's a good reason that force_nr uses 0 for auto then feel
  free to drop this patch.  I've place it at the end of the series to
  make it easy to just drop it.

 drivers/i2c/i2c-mux.c               | 10 +++-------
 drivers/i2c/muxes/i2c-arbitrator.c  |  2 +-
 drivers/i2c/muxes/i2c-mux-gpio.c    |  2 +-
 drivers/i2c/muxes/i2c-mux-pca9541.c |  2 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c |  2 +-
 drivers/i2c/muxes/i2c-mux-pinctrl.c |  4 ++--
 include/linux/i2c-mux.h             |  2 +-
 7 files changed, 10 insertions(+), 14 deletions(-)
Guenter Roeck - Feb. 13, 2013, 8:34 p.m.
On Wed, Feb 13, 2013 at 10:02:12AM -0800, Doug Anderson wrote:
> The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> we don't want to force the bus number of the adapter.  This is
> non-ideal because:
> * 0 is actually a valid bus number to request
> * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
>   -1 to mean the same thing.  That means extra logic in
>   i2c_add_mux_adapter().
> 
> Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> accordingly.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Notes:
> - If there's a good reason that force_nr uses 0 for auto then feel
>   free to drop this patch.  I've place it at the end of the series to
>   make it easy to just drop it.
> 
>  drivers/i2c/i2c-mux.c               | 10 +++-------
>  drivers/i2c/muxes/i2c-arbitrator.c  |  2 +-
>  drivers/i2c/muxes/i2c-mux-gpio.c    |  2 +-
>  drivers/i2c/muxes/i2c-mux-pca9541.c |  2 +-
>  drivers/i2c/muxes/i2c-mux-pca954x.c |  2 +-
>  drivers/i2c/muxes/i2c-mux-pinctrl.c |  4 ++--
>  include/linux/i2c-mux.h             |  2 +-
>  7 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index d94e0ce..8ad1a56 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -103,7 +103,7 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
>  
>  struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  				struct device *mux_dev,
> -				void *mux_priv, u32 force_nr, u32 chan_id,
> +				void *mux_priv, int force_nr, u32 chan_id,
>  				unsigned int class,
>  				int (*select) (struct i2c_adapter *,
>  					       void *, u32),
> @@ -168,12 +168,8 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  		}
>  	}
>  
> -	if (force_nr) {
> -		priv->adap.nr = force_nr;
> -		ret = i2c_add_numbered_adapter(&priv->adap);
> -	} else {
> -		ret = i2c_add_adapter(&priv->adap);
> -	}
> +	priv->adap.nr = force_nr;
> +	ret = i2c_add_numbered_adapter(&priv->adap);
>  	if (ret < 0) {
>  		dev_err(&parent->dev,
>  			"failed to add mux-adapter (error=%d)\n",
> diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
> index c3bbdf7..89d0d06 100644
> --- a/drivers/i2c/muxes/i2c-arbitrator.c
> +++ b/drivers/i2c/muxes/i2c-arbitrator.c
> @@ -173,7 +173,7 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>  		arb->wait_free_us = 50000;
>  
>  	/* Actually add the mux adapter */
> -	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
> +	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, -1, 0, 0,
>  					 i2c_arbitrator_select,
>  					 i2c_arbitrator_deselect);
>  	if (WARN_ON(!arb->child)) {
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 9f50ef0..301ed0b 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->data.n_values; i++) {
> -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
>  		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
>  
>  		mux->adap[i] = i2c_add_mux_adapter(parent, &pdev->dev, mux, nr,
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index f3b8f9a..a58b3c2 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -350,7 +350,7 @@ static int pca9541_probe(struct i2c_client *client,
>  
>  	/* Create mux adapter */
>  
> -	force = 0;
> +	force = -1;
>  	if (pdata)
>  		force = pdata->modes[0].adap_id;
>  	data->mux_adap = i2c_add_mux_adapter(adap, &client->dev, client,
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8e43872..4663ce6 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -215,7 +215,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  	/* Now create an adapter for each channel */
>  	for (num = 0; num < chips[data->type].nchans; num++) {
> -		force = 0;			  /* dynamic adap number */
> +		force = -1;			  /* dynamic adap number */
>  		class = 0;			  /* no class by default */
>  		if (pdata) {
>  			if (num < pdata->num_modes) {
> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> index a43c0ce..401ff5d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> @@ -217,8 +217,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->pdata->bus_count; i++) {
> -		u32 bus = mux->pdata->base_bus_num ?
> -				(mux->pdata->base_bus_num + i) : 0;
> +		int bus = mux->pdata->base_bus_num ?
> +				(mux->pdata->base_bus_num + i) : -1;

Maybe this should be:
		int bus = mux->pdata->base_bus_num >= 0 ?
				(mux->pdata->base_bus_num + i) : -1;

Otherwise you still can not assign bus number 0 as 1st bus. Also, if base_bus_num
ends up being provided as -1, the result would be pretty much unpredictable.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - Feb. 13, 2013, 9:09 p.m.
On 02/13/2013 11:02 AM, Doug Anderson wrote:
> The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> we don't want to force the bus number of the adapter.  This is
> non-ideal because:
> * 0 is actually a valid bus number to request
> * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
>   -1 to mean the same thing.  That means extra logic in
>   i2c_add_mux_adapter().
> 
> Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> accordingly.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Notes:
> - If there's a good reason that force_nr uses 0 for auto then feel
>   free to drop this patch.  I've place it at the end of the series to
>   make it easy to just drop it.

IIRC (and I only vaguely do...) it's because:

> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 9f50ef0..301ed0b 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->data.n_values; i++) {
> -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;

Here, mux->data.base_nr is platform data (or copied directly from it),
and any field in a platform data struct stored in a global variable not
explicitly initialized would be 0, hence 0 would typically mean "no
explicit bus number desired". Since a mux can't exist without a parent
I2C bus, it's unlikely anyone would want a mux to be I2C bus 0, but
rather the parent to have that number.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Delvare - Feb. 14, 2013, 7:15 a.m.
On Wed, 13 Feb 2013 14:09:08 -0700, Stephen Warren wrote:
> On 02/13/2013 11:02 AM, Doug Anderson wrote:
> > The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> > we don't want to force the bus number of the adapter.  This is
> > non-ideal because:
> > * 0 is actually a valid bus number to request
> > * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
> >   -1 to mean the same thing.  That means extra logic in
> >   i2c_add_mux_adapter().
> > 
> > Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> > accordingly.
> > 
> > Signed-off-by: Doug Anderson <dianders@chromium.org>
> > ---
> > Notes:
> > - If there's a good reason that force_nr uses 0 for auto then feel
> >   free to drop this patch.  I've place it at the end of the series to
> >   make it easy to just drop it.
> 
> IIRC (and I only vaguely do...) it's because:
> 
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 9f50ef0..301ed0b 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	for (i = 0; i < mux->data.n_values; i++) {
> > -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> > +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
> 
> Here, mux->data.base_nr is platform data (or copied directly from it),
> and any field in a platform data struct stored in a global variable not
> explicitly initialized would be 0, hence 0 would typically mean "no
> explicit bus number desired". Since a mux can't exist without a parent
> I2C bus, it's unlikely anyone would want a mux to be I2C bus 0, but
> rather the parent to have that number.

Yes, as I recall this is exactly the reason why the current code is the
way it is.

Patch

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index d94e0ce..8ad1a56 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -103,7 +103,7 @@  static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
 
 struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 				struct device *mux_dev,
-				void *mux_priv, u32 force_nr, u32 chan_id,
+				void *mux_priv, int force_nr, u32 chan_id,
 				unsigned int class,
 				int (*select) (struct i2c_adapter *,
 					       void *, u32),
@@ -168,12 +168,8 @@  struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
-	if (force_nr) {
-		priv->adap.nr = force_nr;
-		ret = i2c_add_numbered_adapter(&priv->adap);
-	} else {
-		ret = i2c_add_adapter(&priv->adap);
-	}
+	priv->adap.nr = force_nr;
+	ret = i2c_add_numbered_adapter(&priv->adap);
 	if (ret < 0) {
 		dev_err(&parent->dev,
 			"failed to add mux-adapter (error=%d)\n",
diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
index c3bbdf7..89d0d06 100644
--- a/drivers/i2c/muxes/i2c-arbitrator.c
+++ b/drivers/i2c/muxes/i2c-arbitrator.c
@@ -173,7 +173,7 @@  static int i2c_arbitrator_probe(struct platform_device *pdev)
 		arb->wait_free_us = 50000;
 
 	/* Actually add the mux adapter */
-	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
+	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, -1, 0, 0,
 					 i2c_arbitrator_select,
 					 i2c_arbitrator_deselect);
 	if (WARN_ON(!arb->child)) {
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 9f50ef0..301ed0b 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -208,7 +208,7 @@  static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < mux->data.n_values; i++) {
-		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
+		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
 		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
 
 		mux->adap[i] = i2c_add_mux_adapter(parent, &pdev->dev, mux, nr,
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index f3b8f9a..a58b3c2 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -350,7 +350,7 @@  static int pca9541_probe(struct i2c_client *client,
 
 	/* Create mux adapter */
 
-	force = 0;
+	force = -1;
 	if (pdata)
 		force = pdata->modes[0].adap_id;
 	data->mux_adap = i2c_add_mux_adapter(adap, &client->dev, client,
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8e43872..4663ce6 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -215,7 +215,7 @@  static int pca954x_probe(struct i2c_client *client,
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
-		force = 0;			  /* dynamic adap number */
+		force = -1;			  /* dynamic adap number */
 		class = 0;			  /* no class by default */
 		if (pdata) {
 			if (num < pdata->num_modes) {
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index a43c0ce..401ff5d 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -217,8 +217,8 @@  static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < mux->pdata->bus_count; i++) {
-		u32 bus = mux->pdata->base_bus_num ?
-				(mux->pdata->base_bus_num + i) : 0;
+		int bus = mux->pdata->base_bus_num ?
+				(mux->pdata->base_bus_num + i) : -1;
 
 		mux->busses[i] = i2c_add_mux_adapter(mux->parent, &pdev->dev,
 						     mux, bus, i, 0,
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index 40cb05a..a7bfb55 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -35,7 +35,7 @@ 
  */
 struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 				struct device *mux_dev,
-				void *mux_priv, u32 force_nr, u32 chan_id,
+				void *mux_priv, int force_nr, u32 chan_id,
 				unsigned int class,
 				int (*select) (struct i2c_adapter *,
 					       void *mux_dev, u32 chan_id),