| Message ID | 1459673574-11440-9-git-send-email-peda@lysator.liu.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/04/16 09:52, Peter Rosin wrote: > From: Peter Rosin <peda@axentia.se> > > 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@axentia.se> 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@kernel.org> 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); > 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); > 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. > 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-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-04-03 12:51, Jonathan Cameron wrote: > On 03/04/16 09:52, Peter Rosin wrote: >> From: Peter Rosin <peda@axentia.se> >> >> 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@axentia.se> > 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@kernel.org> 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? 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-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@axentia.se> >>> >>> 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@axentia.se> >> 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@kernel.org> > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/03/2016 11:52 AM, Peter Rosin wrote: > From: Peter Rosin <peda@axentia.se> > > 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. > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > @@ -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; I just tested this patch on mpu9150 (combo mpu6050+ak8975) and I got a crash on probe which looks sort of like this: [ 5.565374] RIP: 0010:[<ffffffff81481aed>] [<ffffffff81481aed>] mutex_lock+0xd/0x30 [ 5.565374] Call Trace: [ 5.565374] [<ffffffff813be34c>] inv_mpu6050_select_bypass+0x1c/0xa0 ... [ 5.565374] [<ffffffff81392141>] i2c_transfer+0x51/0x90 ... [ 5.565374] [<ffffffff81392cb5>] i2c_smbus_read_i2c_block_data+0x45/0xd0 [ 5.565374] [<ffffffff813bec5a>] ak8975_probe+0x14a/0x5c0 ... [ 5.565374] [<ffffffff813933d8>] i2c_new_device+0x178/0x1e0 [ 5.565374] [<ffffffff8139362d>] of_i2c_register_device+0xdd/0x1a0 [ 5.565374] [<ffffffff8139372b>] of_i2c_register_devices+0x3b/0x60 [ 5.565374] [<ffffffff81393e88>] i2c_register_adapter+0x178/0x410 [ 5.565374] [<ffffffff81394203>] i2c_add_adapter+0x73/0x90 [ 5.565374] [<ffffffff81395f3d>] i2c_mux_add_adapter+0x2ed/0x400 [ 5.565374] [<ffffffff81396091>] i2c_mux_one_adapter+0x41/0x70 [ 5.565374] [<ffffffff813be48a>] inv_mpu_probe+0xba/0x1a0 This happens because muxc->priv is not initialized when probing devices behind the mux as described by devicetree. This can be easily fixed by using muxc->dev instead of muxc->priv, or perhaps by calling i2c_mux_alloc, initializing priv and later doing i2c_mux_add_adapter. These fixes are driver-specific and other drivers might experience similar issues. Perhaps i2c_mux_one_adapter should take void *priv just like old i2c_add_mux_adapter? After I fix this locally the deadlock when reading from both devices no longer happens. -- Regards, Leonard -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
<hans.verkuil@cisco.com>,Arnd Bergmann <arnd@arndb.de>,Tommi Rantala <tt.rantala@gmail.com>,linux-i2c@vger.kernel.org,linux-doc@vger.kernel.org,linux-iio@vger.kernel.org,linux-media@vger.kernel.org,devicetree@vger.kernel.org Message-ID: <B09CD200-56D1-4BE0-B567-90CC23507ED5@lysator.liu.se> On April 19, 2016 5:58:11 PM CEST, Crestez Dan Leonard <leonard.crestez@intel.com> wrote: > On 04/03/2016 11:52 AM, Peter Rosin wrote: > > From: Peter Rosin <peda@axentia.se> > > > > 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. > > > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c > > @@ -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; > > I just tested this patch on mpu9150 (combo mpu6050+ak8975) and I got a > crash on probe which looks sort of like this: > > [ 5.565374] RIP: 0010:[<ffffffff81481aed>] [<ffffffff81481aed>] > mutex_lock+0xd/0x30 > [ 5.565374] Call Trace: > [ 5.565374] [<ffffffff813be34c>] > inv_mpu6050_select_bypass+0x1c/0xa0 > ... > [ 5.565374] [<ffffffff81392141>] i2c_transfer+0x51/0x90 > ... > [ 5.565374] [<ffffffff81392cb5>] > i2c_smbus_read_i2c_block_data+0x45/0xd0 > [ 5.565374] [<ffffffff813bec5a>] ak8975_probe+0x14a/0x5c0 > ... > [ 5.565374] [<ffffffff813933d8>] i2c_new_device+0x178/0x1e0 > [ 5.565374] [<ffffffff8139362d>] of_i2c_register_device+0xdd/0x1a0 > [ 5.565374] [<ffffffff8139372b>] of_i2c_register_devices+0x3b/0x60 > [ 5.565374] [<ffffffff81393e88>] i2c_register_adapter+0x178/0x410 > [ 5.565374] [<ffffffff81394203>] i2c_add_adapter+0x73/0x90 > [ 5.565374] [<ffffffff81395f3d>] i2c_mux_add_adapter+0x2ed/0x400 > [ 5.565374] [<ffffffff81396091>] i2c_mux_one_adapter+0x41/0x70 > [ 5.565374] [<ffffffff813be48a>] inv_mpu_probe+0xba/0x1a0 > > This happens because muxc->priv is not initialized when probing > devices > behind the mux as described by devicetree. This can be easily fixed by > using muxc->dev instead of muxc->priv, or perhaps by calling > i2c_mux_alloc, initializing priv and later doing i2c_mux_add_adapter. > > These fixes are driver-specific and other drivers might experience > similar issues. Perhaps i2c_mux_one_adapter should take void *priv > just > like old i2c_add_mux_adapter? > > After I fix this locally the deadlock when reading from both devices > no > longer happens. > > -- > Regards, > Leonard Duh. Obvious now that you point it out. Thanks for catching this! I will just remove the i2c_mux_one_adapter interface for v7 and have the affected drivers do i2c_mux_alloc as a separate step (which was one of your suggested paths forward...) Thanks again, and sorry for the inconvenience, Peter (Written on my phone, sorry for crappy formatting) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); 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); 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); 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;