On Sun, 2014-10-26 at 21:53 +0100, Hartmut Knaack wrote:
> Srinivas Pandruvada schrieb am 17.10.2014 19:11:
> > This chip has a mode in which this chipset can be i2c master. But
> > the current upstream driver doesn't support such mode as there is
> > some limited support of clients, which can be connected.
> >
> > To attach such clients to main i2c bus chip has to be set up in
> > bypass mode. Creates an i2c mux, which will enable/disable this mode.
> > This was discussed for a while in mailing list, this was the decision.
> >
> > This change also provides an API, which allows clients to be created for
> > this mux adapter.
> >
> Hi,
> I found a few things in line, which could be improved.
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > ---
> > drivers/iio/imu/inv_mpu6050/Kconfig | 1 +
> > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 163 +++++++++++++++++++++++++++--
> > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 9 ++
> > 3 files changed, 167 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_mpu6050/Kconfig b/drivers/iio/imu/inv_mpu6050/Kconfig
> > index 2d0608b..48fbc0b 100644
> > --- a/drivers/iio/imu/inv_mpu6050/Kconfig
> > +++ b/drivers/iio/imu/inv_mpu6050/Kconfig
> > @@ -7,6 +7,7 @@ config INV_MPU6050_IIO
> > depends on I2C && SYSFS
> > select IIO_BUFFER
> > select IIO_TRIGGERED_BUFFER
> > + select I2C_MUX
> > help
> > This driver supports the Invensense MPU6050 devices.
> > This driver can also support MPU6500 in MPU6050 compatibility mode
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index b75519d..9864362 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -23,6 +23,7 @@
> > #include <linux/kfifo.h>
> > #include <linux/spinlock.h>
> > #include <linux/iio/iio.h>
> > +#include <linux/i2c-mux.h>
> > #include "inv_mpu_iio.h"
> >
> > /*
> > @@ -52,6 +53,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
> > .int_enable = INV_MPU6050_REG_INT_ENABLE,
> > .pwr_mgmt_1 = INV_MPU6050_REG_PWR_MGMT_1,
> > .pwr_mgmt_2 = INV_MPU6050_REG_PWR_MGMT_2,
> > + .int_pin_cfg = INV_MPU6050_REG_INT_PIN_CFG,
> > };
> >
> > static const struct inv_mpu6050_chip_config chip_config_6050 = {
> > @@ -72,11 +74,147 @@ static const struct inv_mpu6050_hw hw_info[INV_NUM_PARTS] = {
> > },
> > };
> >
> > +static struct i2c_adapter *inv_mux_adapter;
> > +
> > int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 d)
> > {
> > return i2c_smbus_write_i2c_block_data(st->client, reg, 1, &d);
> > }
> >
> > +/*
> > + * The i2c read/write needs to happen in unlocked mode. As the parent
> > + * adapter is common. If we use locked versions, it will fail as
> > + * the mux adapter will lock the parent i2c adapter, while calling
> > + * select/deselect functions.
> > + */
> > +static int inv_mpu6050_write_reg_unlocked(struct inv_mpu6050_state *st,
> > + int reg, u8 d)
> reg should be u8.
> > +{
> > + int ret;
> > + u8 buf[2];
> > + struct i2c_msg msg[1] = {
> > + {
> > + .addr = st->client->addr,
> > + .flags = 0,
> > + .len = sizeof(buf),
> > + .buf = buf,
> > + }
> > + };
> > +
> > + buf[0] = reg;
> > + buf[1] = d;
> > + ret = __i2c_transfer(st->client->adapter, msg, 1);
> > + if (ret != 1)
> > + return ret;
> > +
> > + return 0;
> return (ret != 1) ? ret : 0;
> > +}
> > +
> > +static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void *mux_priv,
> > + u32 chan_id)
> > +{
> > + struct iio_dev *indio_dev = mux_priv;
> > + struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > + int ret = 0;
> > +
> > + /* Use the same mutex which was used everywhere to protect power-op */
> Did you mean power-up?
I meant to say power - operation, which includes power-up and
power-down.
> > + mutex_lock(&indio_dev->mlock);
> > + if (!st->powerup_count) {
> > + ret = inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
> > + 0);
> > + if (!ret)
> > + msleep(INV_MPU6050_REG_UP_TIME);
> A bit of an uncommon way. It would be more straight-forward (although longer) to do:
> if (ret)
> goto label_of_mutex_unlock;
> msleep(...);
> > + }
> > + if (!ret) {
> > + st->powerup_count++;
> > + ret = inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
> > + st->client->irq |
> > + INV_MPU6050_BIT_BYPASS_EN);
> Indentation of these parameters should be improved.
The usual alignment doesn't work as the last INV_.. goes beyond 80
chars.
> > + }
> > + mutex_unlock(&indio_dev->mlock);
> > +
> > + return ret;
> > +}
> > +
> > +static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
> > + void *mux_priv, u32 chan_id)
> > +{
> > + struct iio_dev *indio_dev = mux_priv;
> > + struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > +
> > + mutex_lock(&indio_dev->mlock);
> > + /* It doesn't really mattter, if any of the calls fails */
> > + inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
> > + st->client->irq);
> > + st->powerup_count--;
> > + if (!st->powerup_count)
> > + inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
> > + INV_MPU6050_BIT_SLEEP);
> > + mutex_unlock(&indio_dev->mlock);
> > +
> > + return 0;
> > +}
> > +
> > +static int inv_mpu6050_create_mux(struct iio_dev *indio_dev)
> > +{
> > + struct inv_mpu6050_state *mpu_st = iio_priv(indio_dev);
> Better be consistent with instance names for inv_mpu6050_state.
> > + struct i2c_client *client = mpu_st->client;
> > +
> > + mpu_st->mux_adapter = i2c_add_mux_adapter(client->adapter,
> > + &client->dev,
> > + indio_dev,
> > + 0, 0, 0,
> > + inv_mpu6050_select_bypass,
> > + inv_mpu6050_deselect_bypass);
> > + if (mpu_st->mux_adapter == NULL) {
> > + dev_err(&mpu_st->client->dev, "Mux create Failed\n");
> > + return -ENODEV;
> > + }
> > + inv_mux_adapter = mpu_st->mux_adapter;
> > +
> > + return 0;
> > +}
> > +
> > +static void inv_mpu6050_destroy_mux(struct iio_dev *indio_dev)
> > +{
> > + struct inv_mpu6050_state *mpu_st = iio_priv(indio_dev);
> > +
> > + if (mpu_st->mux_adapter)
> > + i2c_del_mux_adapter(mpu_st->mux_adapter);
> > + inv_mux_adapter = NULL;
> > +}
> > +
> > +struct i2c_client *inv_mpu6050_add_mux_client(char *name, int addr,
> addr could be unsigned short.
> > + int irq, void *plat_data)
> > +{
> > + struct i2c_client *client;
> > + struct i2c_board_info info;
> > +
> > + if (!inv_mux_adapter)
> > + return NULL;
> > +
> > + memset(&info, 0, sizeof(struct i2c_board_info));
> > + strlcpy(info.type, name, sizeof(info.type));
> > + info.addr = addr;
> > + info.irq = irq;
> > + info.platform_data = plat_data;
> > + client = i2c_new_device(inv_mux_adapter, &info);
> > + if (!client) {
> > + dev_err(&inv_mux_adapter->dev,
> > + "failed to create mux clint %d\n", addr);
> Typo: client
> > + return NULL;
> > + }
> > +
> > + return client;
> > +}
> > +EXPORT_SYMBOL_GPL(inv_mpu6050_add_mux_client);
> > +
> > +void inv_mpu6050_del_mux_client(struct i2c_client *client)
> > +{
> > + i2c_unregister_device(client);
> > +}
> > +EXPORT_SYMBOL_GPL(inv_mpu6050_del_mux_client);
> > +
> > int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
> > {
> > u8 d, mgmt_1;
> > @@ -133,13 +271,22 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
> >
> > int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
> > {
> > - int result;
> > + int result = 0;
> > +
> > + if (power_on) {
> > + /* Already under indio-dev->mlock mutex */
> > + if (!st->powerup_count)
> > + result = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1,
> > + 0);
> > + if (!result)
> > + st->powerup_count++;
> > + } else {
> > + st->powerup_count--;
> > + if (!st->powerup_count)
> > + result = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1,
> > + INV_MPU6050_BIT_SLEEP);
> > + }
> >
> > - if (power_on)
> > - result = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1, 0);
> > - else
> > - result = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1,
> > - INV_MPU6050_BIT_SLEEP);
> > if (result)
> > return result;
> >
> > @@ -673,6 +820,7 @@ static int inv_mpu_probe(struct i2c_client *client,
> >
> > st = iio_priv(indio_dev);
> > st->client = client;
> > + st->powerup_count = 0;
> > pdata = dev_get_platdata(&client->dev);
> > if (pdata)
> > st->plat_data = *pdata;
> > @@ -720,6 +868,8 @@ static int inv_mpu_probe(struct i2c_client *client,
> > goto out_remove_trigger;
> > }
> >
> > + inv_mpu6050_create_mux(indio_dev);
> Shouldn't that be tested like this?
> result = inv_mpu6050_create_mux(indio_dev);
> if (result)
> goto out_device_unregister;
> > +
Makes sense. I avoided bailing out because of mux creation for folks who
didn't connect anything to INVXX i2c bus.
> > return 0;
> >
> out_device_unregister:
> iio_device_unregister(indio_dev);
> > out_remove_trigger:
> > @@ -734,6 +884,7 @@ static int inv_mpu_remove(struct i2c_client *client)
> > struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > struct inv_mpu6050_state *st = iio_priv(indio_dev);
> >
> > + inv_mpu6050_destroy_mux(indio_dev);
> > iio_device_unregister(indio_dev);
> > inv_mpu6050_remove_trigger(st);
> > iio_triggered_buffer_cleanup(indio_dev);
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > index e779931..90697c6 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > @@ -54,6 +54,7 @@ struct inv_mpu6050_reg_map {
> > u8 int_enable;
> > u8 pwr_mgmt_1;
> > u8 pwr_mgmt_2;
> > + u8 int_pin_cfg;
> > };
> >
> > /*device enum */
> > @@ -119,6 +120,8 @@ struct inv_mpu6050_state {
> > enum inv_devices chip_type;
> > spinlock_t time_stamp_lock;
> > struct i2c_client *client;
> > + struct i2c_adapter *mux_adapter;
> > + int powerup_count;
> unsigned int?
> > struct inv_mpu6050_platform_data plat_data;
> > DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
> > };
> > @@ -179,6 +182,9 @@ struct inv_mpu6050_state {
> > /* 6 + 6 round up and plus 8 */
> > #define INV_MPU6050_OUTPUT_DATA_SIZE 24
> >
> > +#define INV_MPU6050_REG_INT_PIN_CFG 0x37
> > +#define INV_MPU6050_BIT_BYPASS_EN 0x2
> > +
> > /* init parameters */
> > #define INV_MPU6050_INIT_FIFO_RATE 50
> > #define INV_MPU6050_TIME_STAMP_TOR 5
> > @@ -245,3 +251,6 @@ int inv_reset_fifo(struct iio_dev *indio_dev);
> > int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask);
> > int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
> > int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
> > +struct i2c_client *inv_mpu6050_add_mux_client(char *name, int adddr,
> u8 addr? (also typo addr)
> > + int irq, void *plat_data);
> > +void inv_mpu6050_del_mux_client(struct i2c_client *client);
> >
>
Thanks,
Srinivas
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
