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

[Peter Rosin]

On 2019-10-16 06:09, Biwen Li wrote:
> This supports property idle-state
> 
> Signed-off-by: Biwen Li <biwen.li@xxxxxxx>
> ---
> 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 | 64 ++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..8777d429269c 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,22 +229,25 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
>  				I2C_SMBUS_BYTE, &dummy);
>  }
>  
> +static int pca954x_calculate_chan(struct pca954x *data, u32 chan)

Should return u8, and "chan" is not what is calculated. Perhaps name
the function pca954x_regval?

(Yes, last_chan is also clearly a bad name, and I suspect you may have
 based this name on it, but changing that is a separate patch.)

> +{
> +	/* 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_calculate_chan(data, chan);

I think I would have kept the empty line here. Not important...

>  	/* Only select the channel if its different from the last channel */
> -	if (data->last_chan != regval) {
> +	if ((data->last_chan & 0xff) != regval) {

The changes on this line are not needed (last_chan and regval are
both u8) and just clutters up the code.

>  		ret = pca954x_reg_write(muxc->parent, client, regval);
>  		data->last_chan = ret < 0 ? 0 : 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,23 @@ 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)
> +{
> +	/*
> +	 * Write the mux register at addr to verify
> +	 * that the mux is in fact present. This also
> +	 * initializes the mux to a channel
> +	 * or disconnected state.
> +	 */

Again, this comment belongs in pca954x_probe before the call to this function.
It does not apply (at least not the first sentence) when pca954x_init is called
from pca954x_resume.

Hmmm, it could be argued that specifying MUX_IDLE_AS_IS should not trigger a
disconnect on init (since the mux is always idle at init) and that some other
method should be used to determine if the chip is present. The difference is
that with the idle-state property you can explicitly request MUX_IDLE_AS_IS,
while the old code only had some default behavior if i2c-mux-idle-disconnect
was not present.

The easy way out of this is to, in the binding, document the situation and
say that "idle-state = <MUX_IDLE_AS_IS>;" is not supported but that similar
functionality can be obtained by leaving out both the i2c-mux-idle-disconnect
and idle-state properties.

> +	if (data->idle_state >= 0) {
> +		data->last_chan = pca954x_calculate_chan(data, data->idle_state);
> +	} else {
> +		/* Disconnect multiplexer */
> +		data->last_chan = 0;
> +	}
> +	return i2c_smbus_write_byte(client, data->last_chan);
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -411,7 +431,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,22 +481,18 @@ static int pca954x_probe(struct i2c_client *client,
>  		}
>  	}
>  
> -	/* Write the mux register at addr to verify
> -	 * that the mux is in fact present. This also
> -	 * initializes the mux to disconnected state.
> -	 */
> -	if (i2c_smbus_write_byte(client, 0) < 0) {
> +	data->idle_state = MUX_IDLE_AS_IS;
> +	if (np && of_property_read_u32(np, "idle-state", &data->idle_state)) {
> +		if (np && of_property_read_bool(np, "i2c-mux-idle-disconnect"))

You do not need to do the "np &&" part for both ifs, since it's already know
that np is non-NULL when you hit the second if. But, it's a NOP in the
first if, since of_property_read_u32 returns -EINVAL if it is. So, I suggest

	if (of_property_read_u32(np, "idle-state", &data->idle_state)) {
		if (np && of_property_read_bool(np, "i2c-mux-idle-disconnect"))

Cheers,
Peter

> +			data->idle_state = MUX_IDLE_DISCONNECT;
> +	}
> +
> +	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)
> @@ -531,8 +546,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
>  
>