| Message ID | 20191021080048.47189-2-biwen.li@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] dt-bindings: i2c: support property idle-state | expand |
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 > >
> 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 > > > >
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
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(-)