Re: [PATCH v6 08/24] iio: imu: inv_mpu6050: convert to use an explicit i2c mux core

[Jonathan Cameron]

On 03/04/16 12:51, Peter Rosin wrote:
> On 2016-04-03 12:51, Jonathan Cameron wrote:
>> On 03/04/16 09:52, Peter Rosin wrote:
>>> From: Peter Rosin <peda@xxxxxxxxxx>
>>>
>>> Allocate an explicit i2c mux core to handle parent and child adapters
>>> etc. Update the select/deselect ops to be in terms of the i2c mux core
>>> instead of the child adapter.
>>>
>>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>> I'm mostly fine with this (though one unrelated change seems to have snuck
>> in).  However, I'm not set up to test it - hence other than fixing the change
>> you can have my ack, but ideal would be a tested by from someone with
>> relevant hardware...  However, it looks to be a fairly mechanical change so
>> if no one is currently setup to test it, then don't let it hold up the
>> series too long!
>>
>> Acked-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> 
> Thanks for your acks!
> 
>> Jonathan
>>> ---
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c |  2 +-
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  1 -
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  | 32 +++++++++++++-----------------
>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  3 ++-
>>>  4 files changed, 17 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
>>> index 2771106fd650..f62b8bd9ad7e 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
>>> @@ -183,7 +183,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client *client)
>>>  			} else
>>>  				return 0; /* no secondary addr, which is OK */
>>>  		}
>>> -		st->mux_client = i2c_new_device(st->mux_adapter, &info);
>>> +		st->mux_client = i2c_new_device(st->muxc->adapter[0], &info);
>>>  		if (!st->mux_client)
>>>  			return -ENODEV;
>>>  	}
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> index d192953e9a38..0c2bded2b5b7 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> @@ -23,7 +23,6 @@
>>>  #include <linux/kfifo.h>
>>>  #include <linux/spinlock.h>
>>>  #include <linux/iio/iio.h>
>>> -#include <linux/i2c-mux.h>
>>>  #include <linux/acpi.h>
>>>  #include "inv_mpu_iio.h"
>>>  
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>> index f581256d9d4c..0d429d788106 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>> @@ -15,7 +15,6 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/err.h>
>>>  #include <linux/i2c.h>
>>> -#include <linux/i2c-mux.h>
>>>  #include <linux/iio/iio.h>
>>>  #include <linux/module.h>
>>>  #include "inv_mpu_iio.h"
>>> @@ -52,10 +51,9 @@ static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
>>>  	return 0;
>>>  }
>>>  
>>> -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void *mux_priv,
>>> -				     u32 chan_id)
>>> +static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>>>  {
>>> -	struct i2c_client *client = mux_priv;
>>> +	struct i2c_client *client = i2c_mux_priv(muxc);
>>>  	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
> 
> Here, the existing code uses drv_get_drvdata to get from i2c_client to iio_dev...
> 
>>>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>>  	int ret = 0;
>>> @@ -84,10 +82,9 @@ write_error:
>>>  	return ret;
>>>  }
>>>  
>>> -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
>>> -				       void *mux_priv, u32 chan_id)
>>> +static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>>>  {
>>> -	struct i2c_client *client = mux_priv;
>>> +	struct i2c_client *client = i2c_mux_priv(muxc);
>>>  	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
> 
> ...and here too...
> 
>>>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>>  
>>> @@ -136,16 +133,15 @@ static int inv_mpu_probe(struct i2c_client *client,
>>>  		return result;
>>>  
>>>  	st = iio_priv(dev_get_drvdata(&client->dev));
>>> -	st->mux_adapter = i2c_add_mux_adapter(client->adapter,
>>> -					      &client->dev,
>>> -					      client,
>>> -					      0, 0, 0,
>>> -					      inv_mpu6050_select_bypass,
>>> -					      inv_mpu6050_deselect_bypass);
>>> -	if (!st->mux_adapter) {
>>> -		result = -ENODEV;
>>> +	st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0,
>>> +				       0, 0, 0,
>>> +				       inv_mpu6050_select_bypass,
>>> +				       inv_mpu6050_deselect_bypass);
>>> +	if (IS_ERR(st->muxc)) {
>>> +		result = PTR_ERR(st->muxc);
>>>  		goto out_unreg_device;
>>>  	}
>>> +	st->muxc->priv = client;
>>>  
>>>  	result = inv_mpu_acpi_create_mux_client(client);
>>>  	if (result)
>>> @@ -154,7 +150,7 @@ static int inv_mpu_probe(struct i2c_client *client,
>>>  	return 0;
>>>  
>>>  out_del_mux:
>>> -	i2c_del_mux_adapter(st->mux_adapter);
>>> +	i2c_mux_del_adapters(st->muxc);
>>>  out_unreg_device:
>>>  	inv_mpu_core_remove(&client->dev);
>>>  	return result;
>>> @@ -162,11 +158,11 @@ out_unreg_device:
>>>  
>>>  static int inv_mpu_remove(struct i2c_client *client)
>>>  {
>>> -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
>> Why this change?  Seems unrelated.
> 
> ...which is why I made this change. Maybe a bad call, but the inconsistency
> disturbed me and I was changing the function anyway. I could split it out
> to its own commit I suppose, or should I just not bother at all?
Funny thing is I'd say the i2c_get_clientdata option is the better of the two!

I don't really care though either way.

J
> 
> Cheers,
> Peter
> 
>>>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>>  
>>>  	inv_mpu_acpi_delete_mux_client(client);
>>> -	i2c_del_mux_adapter(st->mux_adapter);
>>> +	i2c_mux_del_adapters(st->muxc);
>>>  
>>>  	return inv_mpu_core_remove(&client->dev);
>>>  }
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> index e302a49703bf..bb3cef6d7059 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> @@ -11,6 +11,7 @@
>>>  * GNU General Public License for more details.
>>>  */
>>>  #include <linux/i2c.h>
>>> +#include <linux/i2c-mux.h>
>>>  #include <linux/kfifo.h>
>>>  #include <linux/spinlock.h>
>>>  #include <linux/iio/iio.h>
>>> @@ -127,7 +128,7 @@ struct inv_mpu6050_state {
>>>  	const struct inv_mpu6050_hw *hw;
>>>  	enum   inv_devices chip_type;
>>>  	spinlock_t time_stamp_lock;
>>> -	struct i2c_adapter *mux_adapter;
>>> +	struct i2c_mux_core *muxc;
>>>  	struct i2c_client *mux_client;
>>>  	unsigned int powerup_count;
>>>  	struct inv_mpu6050_platform_data plat_data;
>>>
>>
> --
> 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
>