On 2018-05-04 15:04, Bastian Stender wrote:
> Commit 6ef91fcca8a8 ("i2c: mux: relax locking of the top i2c adapter
> during mux-locked muxing") introduced a new locking concept "mux-locked",
> so that these muxes lock only the muxes on the parent adapter instead of
> the whole i2c bus.
>
> In case the dt property "mux-locked" is present make use of this
> concept. This makes interaction between i2c devices behind the mux and
> devices directly on the bus possible - at least in simple cases. It
> might not work properly for complex i2c topologies.
>
> If the dt property is not present use the parent-locked concept as
> before.
>
> Signed-off-by: Bastian Stender <bst@xxxxxxxxxxxxxx>
> ---
> Changes since implicit v1 ("i2c: mux: pca954x: use relaxed locking of
> the top i2c adapter during mux-locked muxing"):
>
> - fix unlocked SMBus access
> - make mux-locked optional via dt
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 32 +++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 09bafd3e68fa..dfa9e8d350c7 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -219,6 +219,7 @@ MODULE_DEVICE_TABLE(of, pca954x_of_match);
> static int pca954x_reg_write(struct i2c_adapter *adap,
> struct i2c_client *client, u8 val)
> {
> + struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> int ret = -ENODEV;
>
> if (adap->algo->master_xfer) {
> @@ -230,16 +231,26 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
> msg.len = 1;
> buf[0] = val;
> msg.buf = buf;
> - ret = __i2c_transfer(adap, &msg, 1);
> +
> + if (muxc->mux_locked)
> + ret = i2c_transfer(adap, &msg, 1);
> + else
> + ret = __i2c_transfer(adap, &msg, 1);
>
> if (ret >= 0 && ret != 1)
> ret = -EREMOTEIO;
> } else {
> union i2c_smbus_data data;
> - ret = adap->algo->smbus_xfer(adap, client->addr,
> + if (muxc->mux_locked)
> + ret = i2c_smbus_xfer(adap, client->addr,
> client->flags,
> I2C_SMBUS_WRITE,
> val, I2C_SMBUS_BYTE, &data);
> + else
> + ret = adap->algo->smbus_xfer(adap, client->addr,
> + client->flags,
> + I2C_SMBUS_WRITE, val,
> + I2C_SMBUS_BYTE, &data);
> }
>
> return ret;
Hmm, there is no need to handle the smbus/i2c_transfer differently when
mux-locked. Hence, I would prefer to "just" have three branches instead
of four. Also, the comment at the top of the function is now incorrect.
So, I suggest something like this (only compile-tested, I have no pca954x):
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -214,33 +214,38 @@ static const struct of_device_id pca954x_of_match[] = {
MODULE_DEVICE_TABLE(of, pca954x_of_match);
#endif
-/* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
- for this as they will try to lock adapter a second time */
static int pca954x_reg_write(struct i2c_adapter *adap,
struct i2c_client *client, u8 val)
{
- int ret = -ENODEV;
-
- if (adap->algo->master_xfer) {
- struct i2c_msg msg;
- char buf[1];
-
- msg.addr = client->addr;
- msg.flags = 0;
- msg.len = 1;
- buf[0] = val;
- msg.buf = buf;
- ret = __i2c_transfer(adap, &msg, 1);
-
- if (ret >= 0 && ret != 1)
- ret = -EREMOTEIO;
- } else {
- union i2c_smbus_data data;
- ret = adap->algo->smbus_xfer(adap, client->addr,
- client->flags,
- I2C_SMBUS_WRITE,
- val, I2C_SMBUS_BYTE, &data);
- }
+ struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+ struct i2c_msg msg = {
+ .addr = client->addr,
+ .len = 1,
+ .buf = &val,
+ };
+ int ret;
+
+ if (muxc->mux_locked)
+ return i2c_smbus_write_byte(client, val);
+
+ /*
+ * Since we are not mux-locked, we can't use i2c_transfer() or
+ * i2c_smbus_xfer() as they will try to lock the adapter a second
+ * time.
+ */
+ if (!adap->algo->master_xfer) {
+ union i2c_smbus_data data;
+
+ return adap->algo->smbus_xfer(adap, client->addr,
+ client->flags,
+ I2C_SMBUS_WRITE,
+ val, I2C_SMBUS_BYTE, &data);
+ }
+
+ ret = __i2c_transfer(adap, &msg, 1);
+
+ if (ret >= 0 && ret != 1)
+ return -EREMOTEIO;
return ret;
}
Otherwise, this patch LGTM. It could be squashed with 2/3 though.
Cheers,
Peter
> @@ -371,7 +382,9 @@ static int pca954x_probe(struct i2c_client *client,
> bool idle_disconnect_dt;
> struct gpio_desc *gpio;
> int num, force, class;
> + bool mux_locked;
> struct i2c_mux_core *muxc;
> + u32 mux_alloc_flags;
> struct pca954x *data;
> const struct of_device_id *match;
> int ret;
> @@ -379,9 +392,13 @@ static int pca954x_probe(struct i2c_client *client,
> if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE))
> return -ENODEV;
>
> + mux_locked = of_property_read_bool(of_node, "mux-locked");
> +
> + mux_alloc_flags = mux_locked ? I2C_MUX_LOCKED : 0;
> muxc = i2c_mux_alloc(adap, &client->dev,
> - PCA954X_MAX_NCHANS, sizeof(*data), 0,
> - pca954x_select_chan, pca954x_deselect_mux);
> + PCA954X_MAX_NCHANS, sizeof(*data),
> + mux_alloc_flags, pca954x_select_chan,
> + pca954x_deselect_mux);
> if (!muxc)
> return -ENOMEM;
> data = i2c_mux_priv(muxc);
> @@ -389,6 +406,8 @@ static int pca954x_probe(struct i2c_client *client,
> i2c_set_clientdata(client, muxc);
> data->client = client;
>
> + muxc->mux_locked = mux_locked;
> +
> /* Get the mux out of reset if a reset GPIO is specified. */
> gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(gpio))
> @@ -470,8 +489,9 @@ static int pca954x_probe(struct i2c_client *client,
> }
>
> dev_info(&client->dev,
> - "registered %d multiplexed busses for I2C %s %s\n",
> - num, data->chip->muxtype == pca954x_ismux
> + "registered %d multiplexed busses for %s-locked I2C %s %s\n",
> + num, muxc->mux_locked ? "mux" : "parent",
> + data->chip->muxtype == pca954x_ismux
> ? "mux" : "switch", client->name);
>
> return 0;
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
