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?
> + 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.
> + }
> + 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;
> +
> 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);
>
--
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
