| Message ID | 20200219143958.3548-2-jmaneyrol@invensense.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework sensors engines and power management | expand |
On Wed, 19 Feb 2020 15:39:46 +0100 Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote: > i2c auxiliary mux is done by analog switches. You do not need to > set them for every i2c transfer. > Just set i2c bypass bit at init and do noting in i2c de/select. > > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> OK. Given we are keeping the i2c mux stuff mostly for backwards compatibility I'll take this. However, there is still a bit of time if Peter want's to comment before I push this out as a non rebasing tree. Please make sure to cc peter on anything touching i2c mux. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 71 +++++++++-------------- > 1 file changed, 28 insertions(+), 43 deletions(-) > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c > index 1363d3776523..24df880248f2 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c > @@ -20,38 +20,6 @@ static const struct regmap_config inv_mpu_regmap_config = { > > static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id) > { > - struct iio_dev *indio_dev = i2c_mux_priv(muxc); > - struct inv_mpu6050_state *st = iio_priv(indio_dev); > - int ret; > - > - mutex_lock(&st->lock); > - > - ret = inv_mpu6050_set_power_itg(st, true); > - if (ret) > - goto error_unlock; > - > - ret = regmap_write(st->map, st->reg->int_pin_cfg, > - st->irq_mask | INV_MPU6050_BIT_BYPASS_EN); > - > -error_unlock: > - mutex_unlock(&st->lock); > - > - return ret; > -} > - > -static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id) > -{ > - struct iio_dev *indio_dev = i2c_mux_priv(muxc); > - struct inv_mpu6050_state *st = iio_priv(indio_dev); > - > - mutex_lock(&st->lock); > - > - /* It doesn't really matter if any of the calls fail */ > - regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask); > - inv_mpu6050_set_power_itg(st, false); > - > - mutex_unlock(&st->lock); > - > return 0; > } > > @@ -79,19 +47,20 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev) > } > } > > -/* > - * MPU9xxx magnetometer support requires to disable i2c auxiliary bus support. > - * To ensure backward compatibility with existing setups, do not disable > - * i2c auxiliary bus if it used. > - * Check for i2c-gate node in devicetree and set magnetometer disabled. > - * Only MPU6500 is supported by ACPI, no need to check. > - */ > -static int inv_mpu_magn_disable(struct iio_dev *indio_dev) > +static int inv_mpu_i2c_aux_setup(struct iio_dev *indio_dev) > { > struct inv_mpu6050_state *st = iio_priv(indio_dev); > struct device *dev = indio_dev->dev.parent; > struct device_node *mux_node; > + int ret; > > + /* > + * MPU9xxx magnetometer support requires to disable i2c auxiliary bus. > + * To ensure backward compatibility with existing setups, do not disable > + * i2c auxiliary bus if it used. > + * Check for i2c-gate node in devicetree and set magnetometer disabled. > + * Only MPU6500 is supported by ACPI, no need to check. > + */ > switch (st->chip_type) { > case INV_MPU9150: > case INV_MPU9250: > @@ -107,7 +76,24 @@ static int inv_mpu_magn_disable(struct iio_dev *indio_dev) > break; > } > > + /* enable i2c bypass when using i2c auxiliary bus */ > + if (inv_mpu_i2c_aux_bus(dev)) { > + ret = inv_mpu6050_set_power_itg(st, true); > + if (ret) > + return ret; > + ret = regmap_write(st->map, st->reg->int_pin_cfg, > + st->irq_mask | INV_MPU6050_BIT_BYPASS_EN); > + if (ret) > + goto error; > + ret = inv_mpu6050_set_power_itg(st, false); > + if (ret) > + goto error; > + } > + > return 0; > +error: > + inv_mpu6050_set_power_itg(st, false); > + return ret; > } > > /** > @@ -151,7 +137,7 @@ static int inv_mpu_probe(struct i2c_client *client, > } > > result = inv_mpu_core_probe(regmap, client->irq, name, > - inv_mpu_magn_disable, chip_type); > + inv_mpu_i2c_aux_setup, chip_type); > if (result < 0) > return result; > > @@ -160,8 +146,7 @@ static int inv_mpu_probe(struct i2c_client *client, > /* declare i2c auxiliary bus */ > st->muxc = i2c_mux_alloc(client->adapter, &client->dev, > 1, 0, I2C_MUX_LOCKED | I2C_MUX_GATE, > - inv_mpu6050_select_bypass, > - inv_mpu6050_deselect_bypass); > + inv_mpu6050_select_bypass, NULL); > if (!st->muxc) > return -ENOMEM; > st->muxc->priv = dev_get_drvdata(&client->dev);
On 2020-02-21 12:17, Jonathan Cameron wrote: > On Wed, 19 Feb 2020 15:39:46 +0100 > Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote: > >> i2c auxiliary mux is done by analog switches. You do not need to >> set them for every i2c transfer. >> Just set i2c bypass bit at init and do noting in i2c de/select. Nits: i2c -> I2C (in comments, docs, commit messages, etc) noting -> nothing >> >> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> > OK. Given we are keeping the i2c mux stuff mostly for backwards > compatibility I'll take this. However, there is still a bit > of time if Peter want's to comment before I push this out as > a non rebasing tree. Even though I have not tried it, I see no conceptual problem to have an I2C mux (or gate as in this case) that does "nothing". You do get the backwards compatible extra I2C bus for the other side of the gate, keeping the expected I2C adapter tree intact. But do note that a I2C mux that does "nothing" still triggers locking and various extra indirections and operations so it's not a complete NOP. So, Acked-by: Peter Rosin <peda@axentia.se> (one more nit below) > Please make sure to cc peter on anything touching i2c mux. > > Applied to the togreg branch of iio.git and pushed out as testing for > the autobuilders to play with it. > > Thanks, > > Jonathan > >> --- >> drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 71 +++++++++-------------- >> 1 file changed, 28 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> index 1363d3776523..24df880248f2 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> @@ -20,38 +20,6 @@ static const struct regmap_config inv_mpu_regmap_config = { >> >> static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id) >> { >> - struct iio_dev *indio_dev = i2c_mux_priv(muxc); >> - struct inv_mpu6050_state *st = iio_priv(indio_dev); >> - int ret; >> - >> - mutex_lock(&st->lock); >> - >> - ret = inv_mpu6050_set_power_itg(st, true); >> - if (ret) >> - goto error_unlock; >> - >> - ret = regmap_write(st->map, st->reg->int_pin_cfg, >> - st->irq_mask | INV_MPU6050_BIT_BYPASS_EN); >> - >> -error_unlock: >> - mutex_unlock(&st->lock); >> - >> - return ret; >> -} >> - >> -static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id) >> -{ >> - struct iio_dev *indio_dev = i2c_mux_priv(muxc); >> - struct inv_mpu6050_state *st = iio_priv(indio_dev); >> - >> - mutex_lock(&st->lock); >> - >> - /* It doesn't really matter if any of the calls fail */ >> - regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask); >> - inv_mpu6050_set_power_itg(st, false); >> - >> - mutex_unlock(&st->lock); >> - >> return 0; >> } >> >> @@ -79,19 +47,20 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev) >> } >> } >> >> -/* >> - * MPU9xxx magnetometer support requires to disable i2c auxiliary bus support. >> - * To ensure backward compatibility with existing setups, do not disable >> - * i2c auxiliary bus if it used. >> - * Check for i2c-gate node in devicetree and set magnetometer disabled. >> - * Only MPU6500 is supported by ACPI, no need to check. >> - */ >> -static int inv_mpu_magn_disable(struct iio_dev *indio_dev) >> +static int inv_mpu_i2c_aux_setup(struct iio_dev *indio_dev) >> { >> struct inv_mpu6050_state *st = iio_priv(indio_dev); >> struct device *dev = indio_dev->dev.parent; >> struct device_node *mux_node; >> + int ret; >> >> + /* >> + * MPU9xxx magnetometer support requires to disable i2c auxiliary bus. >> + * To ensure backward compatibility with existing setups, do not disable >> + * i2c auxiliary bus if it used. it used -> it is used Cheers, Peter >> + * Check for i2c-gate node in devicetree and set magnetometer disabled. >> + * Only MPU6500 is supported by ACPI, no need to check. >> + */ >> switch (st->chip_type) { >> case INV_MPU9150: >> case INV_MPU9250: >> @@ -107,7 +76,24 @@ static int inv_mpu_magn_disable(struct iio_dev *indio_dev) >> break; >> } >> >> + /* enable i2c bypass when using i2c auxiliary bus */ >> + if (inv_mpu_i2c_aux_bus(dev)) { >> + ret = inv_mpu6050_set_power_itg(st, true); >> + if (ret) >> + return ret; >> + ret = regmap_write(st->map, st->reg->int_pin_cfg, >> + st->irq_mask | INV_MPU6050_BIT_BYPASS_EN); >> + if (ret) >> + goto error; >> + ret = inv_mpu6050_set_power_itg(st, false); >> + if (ret) >> + goto error; >> + } >> + >> return 0; >> +error: >> + inv_mpu6050_set_power_itg(st, false); >> + return ret; >> } >> >> /** >> @@ -151,7 +137,7 @@ static int inv_mpu_probe(struct i2c_client *client, >> } >> >> result = inv_mpu_core_probe(regmap, client->irq, name, >> - inv_mpu_magn_disable, chip_type); >> + inv_mpu_i2c_aux_setup, chip_type); >> if (result < 0) >> return result; >> >> @@ -160,8 +146,7 @@ static int inv_mpu_probe(struct i2c_client *client, >> /* declare i2c auxiliary bus */ >> st->muxc = i2c_mux_alloc(client->adapter, &client->dev, >> 1, 0, I2C_MUX_LOCKED | I2C_MUX_GATE, >> - inv_mpu6050_select_bypass, >> - inv_mpu6050_deselect_bypass); >> + inv_mpu6050_select_bypass, NULL); >> if (!st->muxc) >> return -ENOMEM; >> st->muxc->priv = dev_get_drvdata(&client->dev); >
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c index 1363d3776523..24df880248f2 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c @@ -20,38 +20,6 @@ static const struct regmap_config inv_mpu_regmap_config = { static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id) { - struct iio_dev *indio_dev = i2c_mux_priv(muxc); - struct inv_mpu6050_state *st = iio_priv(indio_dev); - int ret; - - mutex_lock(&st->lock); - - ret = inv_mpu6050_set_power_itg(st, true); - if (ret) - goto error_unlock; - - ret = regmap_write(st->map, st->reg->int_pin_cfg, - st->irq_mask | INV_MPU6050_BIT_BYPASS_EN); - -error_unlock: - mutex_unlock(&st->lock); - - return ret; -} - -static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id) -{ - struct iio_dev *indio_dev = i2c_mux_priv(muxc); - struct inv_mpu6050_state *st = iio_priv(indio_dev); - - mutex_lock(&st->lock); - - /* It doesn't really matter if any of the calls fail */ - regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask); - inv_mpu6050_set_power_itg(st, false); - - mutex_unlock(&st->lock); - return 0; } @@ -79,19 +47,20 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev) } } -/* - * MPU9xxx magnetometer support requires to disable i2c auxiliary bus support. - * To ensure backward compatibility with existing setups, do not disable - * i2c auxiliary bus if it used. - * Check for i2c-gate node in devicetree and set magnetometer disabled. - * Only MPU6500 is supported by ACPI, no need to check. - */ -static int inv_mpu_magn_disable(struct iio_dev *indio_dev) +static int inv_mpu_i2c_aux_setup(struct iio_dev *indio_dev) { struct inv_mpu6050_state *st = iio_priv(indio_dev); struct device *dev = indio_dev->dev.parent; struct device_node *mux_node; + int ret; + /* + * MPU9xxx magnetometer support requires to disable i2c auxiliary bus. + * To ensure backward compatibility with existing setups, do not disable + * i2c auxiliary bus if it used. + * Check for i2c-gate node in devicetree and set magnetometer disabled. + * Only MPU6500 is supported by ACPI, no need to check. + */ switch (st->chip_type) { case INV_MPU9150: case INV_MPU9250: @@ -107,7 +76,24 @@ static int inv_mpu_magn_disable(struct iio_dev *indio_dev) break; } + /* enable i2c bypass when using i2c auxiliary bus */ + if (inv_mpu_i2c_aux_bus(dev)) { + ret = inv_mpu6050_set_power_itg(st, true); + if (ret) + return ret; + ret = regmap_write(st->map, st->reg->int_pin_cfg, + st->irq_mask | INV_MPU6050_BIT_BYPASS_EN); + if (ret) + goto error; + ret = inv_mpu6050_set_power_itg(st, false); + if (ret) + goto error; + } + return 0; +error: + inv_mpu6050_set_power_itg(st, false); + return ret; } /** @@ -151,7 +137,7 @@ static int inv_mpu_probe(struct i2c_client *client, } result = inv_mpu_core_probe(regmap, client->irq, name, - inv_mpu_magn_disable, chip_type); + inv_mpu_i2c_aux_setup, chip_type); if (result < 0) return result; @@ -160,8 +146,7 @@ static int inv_mpu_probe(struct i2c_client *client, /* declare i2c auxiliary bus */ st->muxc = i2c_mux_alloc(client->adapter, &client->dev, 1, 0, I2C_MUX_LOCKED | I2C_MUX_GATE, - inv_mpu6050_select_bypass, - inv_mpu6050_deselect_bypass); + inv_mpu6050_select_bypass, NULL); if (!st->muxc) return -ENOMEM; st->muxc->priv = dev_get_drvdata(&client->dev);
i2c auxiliary mux is done by analog switches. You do not need to set them for every i2c transfer. Just set i2c bypass bit at init and do noting in i2c de/select. Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> --- drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 71 +++++++++-------------- 1 file changed, 28 insertions(+), 43 deletions(-)