[v4,1/2] dt-bindings: i2c: support property idle-state

[v4,1/2] dt-bindings: i2c: support property idle-state

[Biwen Li]

This supports property idle-state

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v4:
	- none

Change in v3:
	- update subject and description
	- add some information for property idle-state

Change in v2:
	- update subject and description
	- add property idle-state

 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index 30ac6a60f041..7abda506b828 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -25,6 +25,8 @@ Required Properties:
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the reset input.
+  - idle-state: if present, overrides i2c-mux-idle-disconnect,
+    Please refer to Documentation/devicetree/bindings/mux/mux-controller.txt
   - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
     children in idle state. This is necessary for example, if there are several
     multiplexers on the bus and the devices behind them use same I2C addresses.
-- 
2.17.1

[v4,2/2] i2c: mux: pca954x: support property idle-state

[Biwen Li]

This supports property idle-state

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v4:
	- rename function
	  pca954x_calculate_chan -> pca954x_regval

Change in v3:
	- update subject and description
	- add a helper function pca954x_calculate_chan()

Change in v2:
	- update subject and description
	- add property idle-state

 drivers/i2c/muxes/i2c-mux-pca954x.c | 59 ++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 923aa3a5a3dc..e566c4cd8ba5 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -86,7 +86,7 @@ struct pca954x {
 
 	u8 last_chan;		/* last register value */
 	/* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
-	s8 idle_state;
+	s32 idle_state;
 
 	struct i2c_client *client;
 
@@ -229,20 +229,23 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
 				I2C_SMBUS_BYTE, &dummy);
 }
 
+static u8 pca954x_regval(struct pca954x *data, u8 chan)
+{
+	/* we make switches look like muxes, not sure how to be smarter */
+	if (data->chip->muxtype == pca954x_ismux)
+		return chan | data->chip->enable;
+	else
+		return 1 << chan;
+}
+
 static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
-	const struct chip_desc *chip = data->chip;
 	u8 regval;
 	int ret = 0;
 
-	/* we make switches look like muxes, not sure how to be smarter */
-	if (chip->muxtype == pca954x_ismux)
-		regval = chan | chip->enable;
-	else
-		regval = 1 << chan;
-
+	regval = pca954x_regval(data, (u8)(chan & 0xff));
 	/* Only select the channel if its different from the last channel */
 	if (data->last_chan != regval) {
 		ret = pca954x_reg_write(muxc->parent, client, regval);
@@ -256,7 +259,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
-	s8 idle_state;
+	s32 idle_state;
 
 	idle_state = READ_ONCE(data->idle_state);
 	if (idle_state >= 0)
@@ -402,6 +405,17 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
 	i2c_mux_del_adapters(muxc);
 }
 
+static int pca954x_init(struct i2c_client *client, struct pca954x *data)
+{
+	if (data->idle_state >= 0) {
+		data->last_chan = pca954x_regval(data, (u8)(data->idle_state & 0xff));
+	} else {
+		/* Disconnect multiplexer */
+		data->last_chan = 0;
+	}
+	return i2c_smbus_write_byte(client, data->last_chan);
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -411,7 +425,6 @@ static int pca954x_probe(struct i2c_client *client,
 	struct i2c_adapter *adap = client->adapter;
 	struct device *dev = &client->dev;
 	struct device_node *np = dev->of_node;
-	bool idle_disconnect_dt;
 	struct gpio_desc *gpio;
 	struct i2c_mux_core *muxc;
 	struct pca954x *data;
@@ -462,23 +475,24 @@ static int pca954x_probe(struct i2c_client *client,
 		}
 	}
 
-	/* Write the mux register at addr to verify
+	data->idle_state = MUX_IDLE_AS_IS;
+	if (of_property_read_u32(np, "idle-state", &data->idle_state)) {
+		if (np && of_property_read_bool(np, "i2c-mux-idle-disconnect"))
+			data->idle_state = MUX_IDLE_DISCONNECT;
+	}
+
+	/*
+	 * Write the mux register at addr to verify
 	 * that the mux is in fact present. This also
-	 * initializes the mux to disconnected state.
+	 * initializes the mux to a channel
+	 * or disconnected state.
 	 */
-	if (i2c_smbus_write_byte(client, 0) < 0) {
+	ret = pca954x_init(client, data);
+	if (ret < 0) {
 		dev_warn(dev, "probe failed\n");
 		return -ENODEV;
 	}
 
-	data->last_chan = 0;		   /* force the first selection */
-	data->idle_state = MUX_IDLE_AS_IS;
-
-	idle_disconnect_dt = np &&
-		of_property_read_bool(np, "i2c-mux-idle-disconnect");
-	if (idle_disconnect_dt)
-		data->idle_state = MUX_IDLE_DISCONNECT;
-
 	ret = pca954x_irq_setup(muxc);
 	if (ret)
 		goto fail_cleanup;
@@ -531,8 +545,7 @@ static int pca954x_resume(struct device *dev)
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
 	struct pca954x *data = i2c_mux_priv(muxc);
 
-	data->last_chan = 0;
-	return i2c_smbus_write_byte(client, 0);
+	return pca954x_init(client, data);
 }
 #endif
 
-- 
2.17.1

Re: [v4,2/2] i2c: mux: pca954x: support property idle-state

[Peter Rosin]

On 2019-10-21 10:00, Biwen Li wrote:
> This supports property idle-state
> 

You should expand this a little bit to explain that idle-state, if present,
overrides i2c-mux-idle-disconnect. You could also mention your use case
where you need to avoid disconnects on probe/resume.

> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> ---
> Change in v4:
> 	- rename function
> 	  pca954x_calculate_chan -> pca954x_regval
> 
> Change in v3:
> 	- update subject and description
> 	- add a helper function pca954x_calculate_chan()
> 
> Change in v2:
> 	- update subject and description
> 	- add property idle-state
> 
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 59 ++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..e566c4cd8ba5 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -86,7 +86,7 @@ struct pca954x {
>  
>  	u8 last_chan;		/* last register value */
>  	/* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
> -	s8 idle_state;
> +	s32 idle_state;
>  
>  	struct i2c_client *client;
>  
> @@ -229,20 +229,23 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
>  				I2C_SMBUS_BYTE, &dummy);
>  }
>  
> +static u8 pca954x_regval(struct pca954x *data, u8 chan)
> +{
> +	/* we make switches look like muxes, not sure how to be smarter */

I know you are just moving the comment around, but please fix the sentence
to start with a capital letter and end with a period. Sorry I didn't
catch this in v3.

> +	if (data->chip->muxtype == pca954x_ismux)
> +		return chan | data->chip->enable;
> +	else
> +		return 1 << chan;
> +}
> +
>  static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
>  {
>  	struct pca954x *data = i2c_mux_priv(muxc);
>  	struct i2c_client *client = data->client;
> -	const struct chip_desc *chip = data->chip;
>  	u8 regval;
>  	int ret = 0;
>  
> -	/* we make switches look like muxes, not sure how to be smarter */
> -	if (chip->muxtype == pca954x_ismux)
> -		regval = chan | chip->enable;
> -	else
> -		regval = 1 << chan;
> -
> +	regval = pca954x_regval(data, (u8)(chan & 0xff));

Both a mask and a cast to do what the compiler should be doing all by itself?
If you need to kill a warning, or something, please do just one or them. But
personally I prefer the short, sweet and uncluttered:

	regval = pca954x_regval(data, chan);

>  	/* Only select the channel if its different from the last channel */
>  	if (data->last_chan != regval) {
>  		ret = pca954x_reg_write(muxc->parent, client, regval);
> @@ -256,7 +259,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>  {
>  	struct pca954x *data = i2c_mux_priv(muxc);
>  	struct i2c_client *client = data->client;
> -	s8 idle_state;
> +	s32 idle_state;
>  
>  	idle_state = READ_ONCE(data->idle_state);
>  	if (idle_state >= 0)
> @@ -402,6 +405,17 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>  	i2c_mux_del_adapters(muxc);
>  }
>  
> +static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> +{
> +	if (data->idle_state >= 0) {
> +		data->last_chan = pca954x_regval(data, (u8)(data->idle_state & 0xff));

Dito.

> +	} else {
> +		/* Disconnect multiplexer */
> +		data->last_chan = 0;
> +	}
> +	return i2c_smbus_write_byte(client, data->last_chan);

Here's another thing I missed in the earlier iterations. If i2c_smbus_write_byte
fails here, I think you should set data->last_chan to zero. For the call from
probe it obviously doesn't matter much, but I think the call during resume is
better off with such extra precaution in place.

Cheers,
Peter

> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -411,7 +425,6 @@ static int pca954x_probe(struct i2c_client *client,
>  	struct i2c_adapter *adap = client->adapter;
>  	struct device *dev = &client->dev;
>  	struct device_node *np = dev->of_node;
> -	bool idle_disconnect_dt;
>  	struct gpio_desc *gpio;
>  	struct i2c_mux_core *muxc;
>  	struct pca954x *data;
> @@ -462,23 +475,24 @@ static int pca954x_probe(struct i2c_client *client,
>  		}
>  	}
>  
> -	/* Write the mux register at addr to verify
> +	data->idle_state = MUX_IDLE_AS_IS;
> +	if (of_property_read_u32(np, "idle-state", &data->idle_state)) {
> +		if (np && of_property_read_bool(np, "i2c-mux-idle-disconnect"))
> +			data->idle_state = MUX_IDLE_DISCONNECT;
> +	}
> +
> +	/*
> +	 * Write the mux register at addr to verify
>  	 * that the mux is in fact present. This also
> -	 * initializes the mux to disconnected state.
> +	 * initializes the mux to a channel
> +	 * or disconnected state.
>  	 */
> -	if (i2c_smbus_write_byte(client, 0) < 0) {
> +	ret = pca954x_init(client, data);
> +	if (ret < 0) {
>  		dev_warn(dev, "probe failed\n");
>  		return -ENODEV;
>  	}
>  
> -	data->last_chan = 0;		   /* force the first selection */
> -	data->idle_state = MUX_IDLE_AS_IS;
> -
> -	idle_disconnect_dt = np &&
> -		of_property_read_bool(np, "i2c-mux-idle-disconnect");
> -	if (idle_disconnect_dt)
> -		data->idle_state = MUX_IDLE_DISCONNECT;
> -
>  	ret = pca954x_irq_setup(muxc);
>  	if (ret)
>  		goto fail_cleanup;
> @@ -531,8 +545,7 @@ static int pca954x_resume(struct device *dev)
>  	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>  	struct pca954x *data = i2c_mux_priv(muxc);
>  
> -	data->last_chan = 0;
> -	return i2c_smbus_write_byte(client, 0);
> +	return pca954x_init(client, data);
>  }
>  #endif
>  
> 

RE: [EXT] Re: [v4,2/2] i2c: mux: pca954x: support property idle-state

[Biwen Li]

> Caution: EXT Email
> 
> On 2019-10-21 10:00, Biwen Li wrote:
> > This supports property idle-state
> >
> 
> You should expand this a little bit to explain that idle-state, if present, overrides
> i2c-mux-idle-disconnect. You could also mention your use case where you need
> to avoid disconnects on probe/resume.
Okay, got it. I will add some information in v5.
> 
> > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > ---
> > Change in v4:
> >       - rename function
> >         pca954x_calculate_chan -> pca954x_regval
> >
> > Change in v3:
> >       - update subject and description
> >       - add a helper function pca954x_calculate_chan()
> >
> > Change in v2:
> >       - update subject and description
> >       - add property idle-state
> >
> >  drivers/i2c/muxes/i2c-mux-pca954x.c | 59
> > ++++++++++++++++++-----------
> >  1 file changed, 36 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > index 923aa3a5a3dc..e566c4cd8ba5 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -86,7 +86,7 @@ struct pca954x {
> >
> >       u8 last_chan;           /* last register value */
> >       /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
> > -     s8 idle_state;
> > +     s32 idle_state;
> >
> >       struct i2c_client *client;
> >
> > @@ -229,20 +229,23 @@ static int pca954x_reg_write(struct i2c_adapter
> *adap,
> >                               I2C_SMBUS_BYTE, &dummy);  }
> >
> > +static u8 pca954x_regval(struct pca954x *data, u8 chan) {
> > +     /* we make switches look like muxes, not sure how to be smarter
> > +*/
> 
> I know you are just moving the comment around, but please fix the sentence to
> start with a capital letter and end with a period. Sorry I didn't catch this in v3.
Okay, got it, I will fix it in v5.
> 
> > +     if (data->chip->muxtype == pca954x_ismux)
> > +             return chan | data->chip->enable;
> > +     else
> > +             return 1 << chan;
> > +}
> > +
> >  static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> > {
> >       struct pca954x *data = i2c_mux_priv(muxc);
> >       struct i2c_client *client = data->client;
> > -     const struct chip_desc *chip = data->chip;
> >       u8 regval;
> >       int ret = 0;
> >
> > -     /* we make switches look like muxes, not sure how to be smarter */
> > -     if (chip->muxtype == pca954x_ismux)
> > -             regval = chan | chip->enable;
> > -     else
> > -             regval = 1 << chan;
> > -
> > +     regval = pca954x_regval(data, (u8)(chan & 0xff));
> 
> Both a mask and a cast to do what the compiler should be doing all by itself?
> If you need to kill a warning, or something, please do just one or them. But
> personally I prefer the short, sweet and uncluttered:
Okay, got it, thanks. I will adjust it in v5.
> 
>         regval = pca954x_regval(data, chan);
> 
> >       /* Only select the channel if its different from the last channel */
> >       if (data->last_chan != regval) {
> >               ret = pca954x_reg_write(muxc->parent, client, regval);
> > @@ -256,7 +259,7 @@ static int pca954x_deselect_mux(struct
> > i2c_mux_core *muxc, u32 chan)  {
> >       struct pca954x *data = i2c_mux_priv(muxc);
> >       struct i2c_client *client = data->client;
> > -     s8 idle_state;
> > +     s32 idle_state;
> >
> >       idle_state = READ_ONCE(data->idle_state);
> >       if (idle_state >= 0)
> > @@ -402,6 +405,17 @@ static void pca954x_cleanup(struct i2c_mux_core
> *muxc)
> >       i2c_mux_del_adapters(muxc);
> >  }
> >
> > +static int pca954x_init(struct i2c_client *client, struct pca954x
> > +*data) {
> > +     if (data->idle_state >= 0) {
> > +             data->last_chan = pca954x_regval(data,
> > +(u8)(data->idle_state & 0xff));
> 
> Dito.

Got it, thanks.
> 
> > +     } else {
> > +             /* Disconnect multiplexer */
> > +             data->last_chan = 0;
> > +     }
> > +     return i2c_smbus_write_byte(client, data->last_chan);
> 
> Here's another thing I missed in the earlier iterations. If i2c_smbus_write_byte
> fails here, I think you should set data->last_chan to zero. For the call from probe
> it obviously doesn't matter much, but I think the call during resume is better off
> with such extra precaution in place.
You are right. I will add extra precaution in v5.
> 
> Cheers,
> Peter
> 
> > +}
> > +
> >  /*
> >   * I2C init/probing/exit functions
> >   */
> > @@ -411,7 +425,6 @@ static int pca954x_probe(struct i2c_client *client,
> >       struct i2c_adapter *adap = client->adapter;
> >       struct device *dev = &client->dev;
> >       struct device_node *np = dev->of_node;
> > -     bool idle_disconnect_dt;
> >       struct gpio_desc *gpio;
> >       struct i2c_mux_core *muxc;
> >       struct pca954x *data;
> > @@ -462,23 +475,24 @@ static int pca954x_probe(struct i2c_client *client,
> >               }
> >       }
> >
> > -     /* Write the mux register at addr to verify
> > +     data->idle_state = MUX_IDLE_AS_IS;
> > +     if (of_property_read_u32(np, "idle-state", &data->idle_state)) {
> > +             if (np && of_property_read_bool(np,
> "i2c-mux-idle-disconnect"))
> > +                     data->idle_state = MUX_IDLE_DISCONNECT;
> > +     }
> > +
> > +     /*
> > +      * Write the mux register at addr to verify
> >        * that the mux is in fact present. This also
> > -      * initializes the mux to disconnected state.
> > +      * initializes the mux to a channel
> > +      * or disconnected state.
> >        */
> > -     if (i2c_smbus_write_byte(client, 0) < 0) {
> > +     ret = pca954x_init(client, data);
> > +     if (ret < 0) {
> >               dev_warn(dev, "probe failed\n");
> >               return -ENODEV;
> >       }
> >
> > -     data->last_chan = 0;               /* force the first selection */
> > -     data->idle_state = MUX_IDLE_AS_IS;
> > -
> > -     idle_disconnect_dt = np &&
> > -             of_property_read_bool(np, "i2c-mux-idle-disconnect");
> > -     if (idle_disconnect_dt)
> > -             data->idle_state = MUX_IDLE_DISCONNECT;
> > -
> >       ret = pca954x_irq_setup(muxc);
> >       if (ret)
> >               goto fail_cleanup;
> > @@ -531,8 +545,7 @@ static int pca954x_resume(struct device *dev)
> >       struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> >       struct pca954x *data = i2c_mux_priv(muxc);
> >
> > -     data->last_chan = 0;
> > -     return i2c_smbus_write_byte(client, 0);
> > +     return pca954x_init(client, data);
> >  }
> >  #endif
> >
> >