[v5,2/3] i2c: mux: pca954x: support property idle-state
diff mbox series

Message ID 20191022041152.3663-2-biwen.li@nxp.com
State New
Delegated to: Peter Rosin
Headers show
Series
  • [v5,1/3] dt-bindings: i2c: support property idle-state
Related show

Commit Message

Biwen Li Oct. 22, 2019, 4:11 a.m. UTC
This supports property idle-state,if present,
overrides i2c-mux-idle-disconnect.

My use cases:
	- Use the property idle-state to fix
	  an errata on LS2085ARDB and LS2088ARDB.
	- Errata id: E-00013(board LS2085ARDB and
	  LS2088ARDB revision on Rev.B, Rev.C and Rev.D).
	- About E-00013:
	  - Description: I2C1 and I2C3 buses
	    are missing pull-up.
	  - Impact: When the PCA954x device is tri-stated, the I2C bus
	    will float. This makes the I2C bus and its associated
	    downstream devices inaccessible.
	  - Hardware fix: Populate resistors R189 and R190 for I2C1
	    and resistors R228 and R229 for I2C3.
	  - Software fix: Remove the tri-state option from the PCA954x
	    driver(PCA954x always on enable status, specify a
	    channel zero in dts to fix the errata E-00013).

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v5:
	- add extra precaution for pca954x_init 	

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 | 67 +++++++++++++++++++----------
 1 file changed, 44 insertions(+), 23 deletions(-)

Comments

Biwen Li Nov. 15, 2019, 10:21 a.m. UTC | #1
Hi Peter,

Any comments?
If no comments, could you give me a reviewed-by?

Best Regards,
Biwen Li
> This supports property idle-state,if present, overrides i2c-mux-idle-disconnect.
> 
> My use cases:
> 	- Use the property idle-state to fix
> 	  an errata on LS2085ARDB and LS2088ARDB.
> 	- Errata id: E-00013(board LS2085ARDB and
> 	  LS2088ARDB revision on Rev.B, Rev.C and Rev.D).
> 	- About E-00013:
> 	  - Description: I2C1 and I2C3 buses
> 	    are missing pull-up.
> 	  - Impact: When the PCA954x device is tri-stated, the I2C bus
> 	    will float. This makes the I2C bus and its associated
> 	    downstream devices inaccessible.
> 	  - Hardware fix: Populate resistors R189 and R190 for I2C1
> 	    and resistors R228 and R229 for I2C3.
> 	  - Software fix: Remove the tri-state option from the PCA954x
> 	    driver(PCA954x always on enable status, specify a
> 	    channel zero in dts to fix the errata E-00013).
> 
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> ---
> Change in v5:
> 	- add extra precaution for pca954x_init
> 
> 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 | 67 +++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..218ba1a5ed7e 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, 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,25 @@ 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) {
> +	int ret;
> +	if (data->idle_state >= 0) {
> +		data->last_chan = pca954x_regval(data, data->idle_state);
> +	} else {
> +		/* Disconnect multiplexer */
> +		data->last_chan = 0;
> +	}
> +	ret = i2c_smbus_write_byte(client, data->last_chan);
> +	if (ret < 0) {
> +		data->last_chan = 0;
> +		dev_err(&client->dev, "failed to verify the mux, \
> +			the mux maybe not present in fact\n");
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -411,7 +433,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 +483,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 +553,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
Peter Rosin Nov. 18, 2019, 11:03 p.m. UTC | #2
On 2019-11-15 11:21, Biwen Li wrote:
> Hi Peter,
> 
> Any comments?
> If no comments, could you give me a reviewed-by?

I would have liked a comment from the driver maintainer or a tested-by from
someone with another case than yours. But now that I check I see that there -
contrary to my assumption - is no maintainer listed. So, I think it looks fine,
and I'm sure it compiles cleanly etc if I test that, but I can't test runtime
behavior myself since I don't have the HW. I should have been clearer about
this, and should have double checked the maintainer status instead of relying
on what I thought. I have simply been extremely busy and that slipped. Sorry.

But I'll try to squeeze this into linux-next tomorrow, because if look safe,
and hopefully any problem will become apparent.

Giving a reviewed-by seemed pointless, when I'm the one picking it up :-)

Cheers,
Peter
Peter Rosin Nov. 19, 2019, 10:17 p.m. UTC | #3
Hi!

I had a second read of this patch, and found a new issue.

On 2019-10-22 06:11, Biwen Li wrote:
> This supports property idle-state,if present,
> overrides i2c-mux-idle-disconnect.
> 
> My use cases:
> 	- Use the property idle-state to fix
> 	  an errata on LS2085ARDB and LS2088ARDB.
> 	- Errata id: E-00013(board LS2085ARDB and
> 	  LS2088ARDB revision on Rev.B, Rev.C and Rev.D).
> 	- About E-00013:
> 	  - Description: I2C1 and I2C3 buses
> 	    are missing pull-up.
> 	  - Impact: When the PCA954x device is tri-stated, the I2C bus
> 	    will float. This makes the I2C bus and its associated
> 	    downstream devices inaccessible.
> 	  - Hardware fix: Populate resistors R189 and R190 for I2C1
> 	    and resistors R228 and R229 for I2C3.
> 	  - Software fix: Remove the tri-state option from the PCA954x
> 	    driver(PCA954x always on enable status, specify a
> 	    channel zero in dts to fix the errata E-00013).
> 
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> ---
> Change in v5:
> 	- add extra precaution for pca954x_init 	
> 
> 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 | 67 +++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..218ba1a5ed7e 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, 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,25 @@ 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)
> +{
> +	int ret;
> +	if (data->idle_state >= 0) {
> +		data->last_chan = pca954x_regval(data, data->idle_state);
> +	} else {
> +		/* Disconnect multiplexer */
> +		data->last_chan = 0;
> +	}
> +	ret = i2c_smbus_write_byte(client, data->last_chan);
> +	if (ret < 0) {
> +		data->last_chan = 0;
> +		dev_err(&client->dev, "failed to verify the mux, \
> +			the mux maybe not present in fact\n");

This dev_err will trigger on probe failure, which already prints a
message on failure. If you think you need a message if this fails
in resume, I think that message needs to be printed in resume
instead of here. But the message seems targeted at the probe
failure, which is an argument for simply removing it. It is also
not desirable to split a string with that technique. It's generally
better to "use this technique to do string continuation on a new"
"line." However, splitting strings over multiple lines are frowned
upon since it breaks grepping for the error message. The language
is also a bit strange.

Again, sorry for not being able to be as responsive as I would have
liked, and thanks for your patience!

Cheers,
Peter

> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -411,7 +433,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 +483,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 +553,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
>  
>
Biwen Li Nov. 28, 2019, 10:45 a.m. UTC | #4
> Hi!
> 
> I had a second read of this patch, and found a new issue.
> 
> On 2019-10-22 06:11, Biwen Li wrote:
> > This supports property idle-state,if present, overrides
> > i2c-mux-idle-disconnect.
> >
> > My use cases:
> >       - Use the property idle-state to fix
> >         an errata on LS2085ARDB and LS2088ARDB.
> >       - Errata id: E-00013(board LS2085ARDB and
> >         LS2088ARDB revision on Rev.B, Rev.C and Rev.D).
> >       - About E-00013:
> >         - Description: I2C1 and I2C3 buses
> >           are missing pull-up.
> >         - Impact: When the PCA954x device is tri-stated, the I2C bus
> >           will float. This makes the I2C bus and its associated
> >           downstream devices inaccessible.
> >         - Hardware fix: Populate resistors R189 and R190 for I2C1
> >           and resistors R228 and R229 for I2C3.
> >         - Software fix: Remove the tri-state option from the PCA954x
> >           driver(PCA954x always on enable status, specify a
> >           channel zero in dts to fix the errata E-00013).
> >
> > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > ---
> > Change in v5:
> >       - add extra precaution for pca954x_init
> >
> > 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 | 67
> > +++++++++++++++++++----------
> >  1 file changed, 44 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > index 923aa3a5a3dc..218ba1a5ed7e 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, 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,25 @@ 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) {
> > +     int ret;
> > +     if (data->idle_state >= 0) {
> > +             data->last_chan = pca954x_regval(data, data->idle_state);
> > +     } else {
> > +             /* Disconnect multiplexer */
> > +             data->last_chan = 0;
> > +     }
> > +     ret = i2c_smbus_write_byte(client, data->last_chan);
> > +     if (ret < 0) {
> > +             data->last_chan = 0;
> > +             dev_err(&client->dev, "failed to verify the mux, \
> > +                     the mux maybe not present in fact\n");
> 
> This dev_err will trigger on probe failure, which already prints a message on
> failure. If you think you need a message if this fails in resume, I think that
> message needs to be printed in resume instead of here. But the message seems
> targeted at the probe failure, which is an argument for simply removing it. It is
> also not desirable to split a string with that technique. It's generally better to
> "use this technique to do string continuation on a new"
> "line." However, splitting strings over multiple lines are frowned upon since it
> breaks grepping for the error message. The language is also a bit strange.
> 
> Again, sorry for not being able to be as responsive as I would have liked, and
> thanks for your patience!
Okay, got it, thanks, I will adjust it in v6.
Best Regards,
Biwen Li
> 
> Cheers,
> Peter
> 
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  /*
> >   * I2C init/probing/exit functions
> >   */
> > @@ -411,7 +433,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 +483,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 +553,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
> >
> >
Biwen Li Nov. 28, 2019, 10:46 a.m. UTC | #5
> Caution: EXT Email
> 
> On 2019-11-15 11:21, Biwen Li wrote:
> > Hi Peter,
> >
> > Any comments?
> > If no comments, could you give me a reviewed-by?
> 
> I would have liked a comment from the driver maintainer or a tested-by from
> someone with another case than yours. But now that I check I see that there -
> contrary to my assumption - is no maintainer listed. So, I think it looks fine, and
> I'm sure it compiles cleanly etc if I test that, but I can't test runtime behavior
> myself since I don't have the HW. I should have been clearer about this, and
> should have double checked the maintainer status instead of relying on what I
> thought. I have simply been extremely busy and that slipped. Sorry.
> 
> But I'll try to squeeze this into linux-next tomorrow, because if look safe, and
> hopefully any problem will become apparent.
> 
> Giving a reviewed-by seemed pointless, when I'm the one picking it up :-)
Okay, got it, thanks.

Best Regards,
Biwen Li
> 
> Cheers,
> Peter

Patch
diff mbox series

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 923aa3a5a3dc..218ba1a5ed7e 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, 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,25 @@  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)
+{
+	int ret;
+	if (data->idle_state >= 0) {
+		data->last_chan = pca954x_regval(data, data->idle_state);
+	} else {
+		/* Disconnect multiplexer */
+		data->last_chan = 0;
+	}
+	ret = i2c_smbus_write_byte(client, data->last_chan);
+	if (ret < 0) {
+		data->last_chan = 0;
+		dev_err(&client->dev, "failed to verify the mux, \
+			the mux maybe not present in fact\n");
+	}
+
+	return ret;
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -411,7 +433,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 +483,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 +553,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