Re: [PATCH 1/2] iio: imu: inv_mpu6050: Add i2c mux for by pass

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux