| Message ID | 010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 6 Dec 2017 17:52:34 +0000 Jeremy Cline <jeremy@jcline.org> wrote: > Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI > device. Check for a companion device and handle a second i2c_client > if it is present. > > Signed-off-by: Jeremy Cline <jeremy@jcline.org> The requirement for this is still horrible, but you have done a nice clean job on implementing it. I'll let this sit for a few more days though before applying it. Probably next weekend if we don't get any feedback before then. Thanks, Jonathan > --- > Changes in v2: > - Rather than exposing the bmc150_accel_data struct, use get and set > functions. > > drivers/iio/accel/bmc150-accel-core.c | 20 ++++++++++++++++++++ > drivers/iio/accel/bmc150-accel-i2c.c | 29 ++++++++++++++++++++++++++++- > drivers/iio/accel/bmc150-accel.h | 2 ++ > 3 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > index 870f92ef61c2..7496c5121a8c 100644 > --- a/drivers/iio/accel/bmc150-accel-core.c > +++ b/drivers/iio/accel/bmc150-accel-core.c > @@ -204,6 +204,7 @@ struct bmc150_accel_data { > int ev_enable_state; > int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */ > const struct bmc150_accel_chip_info *chip_info; > + struct i2c_client *second_device; > }; > > static const struct { > @@ -1659,6 +1660,25 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > } > EXPORT_SYMBOL_GPL(bmc150_accel_core_probe); > > +struct i2c_client *bmc150_get_second_device(struct i2c_client *client) > +{ > + struct bmc150_accel_data *data = i2c_get_clientdata(client); > + > + if (!data) > + return NULL; > + > + return data->second_device; > +} > +EXPORT_SYMBOL_GPL(bmc150_get_second_device); > + > +void bmc150_set_second_device(struct i2c_client *client) > +{ > + struct bmc150_accel_data *data = i2c_get_clientdata(client); > + if (data) > + data->second_device = client; > +} > +EXPORT_SYMBOL_GPL(bmc150_set_second_device); > + > int bmc150_accel_core_remove(struct device *dev) > { > struct iio_dev *indio_dev = dev_get_drvdata(dev); > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > index f85014fbaa12..c7341df086e2 100644 > --- a/drivers/iio/accel/bmc150-accel-i2c.c > +++ b/drivers/iio/accel/bmc150-accel-i2c.c > @@ -31,6 +31,10 @@ > static int bmc150_accel_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > + int ret; > + struct acpi_device *adev; > + struct i2c_board_info board_info; > + struct i2c_client *second_dev; > struct regmap *regmap; > const char *name = NULL; > bool block_supported = > @@ -47,12 +51,35 @@ static int bmc150_accel_probe(struct i2c_client *client, > if (id) > name = id->name; > > - return bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, > + ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, > block_supported); > + if (ret) > + return ret; > + > + /* > + * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI > + * device, try instantiating a second i2c_client for an I2cSerialBusV2 > + * ACPI resource with index 1. > + */ > + adev = ACPI_COMPANION(&client->dev); > + if (adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > + memset(&board_info, 0, sizeof(board_info)); > + strlcpy(board_info.type, "bma250e", I2C_NAME_SIZE); > + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); > + if (second_dev) > + bmc150_set_second_device(second_dev); > + } > + > + return ret; > } > > static int bmc150_accel_remove(struct i2c_client *client) > { > + struct i2c_client *second_dev = bmc150_get_second_device(client); > + > + if (second_dev) > + i2c_unregister_device(second_dev); > + > return bmc150_accel_core_remove(&client->dev); > } > > diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h > index ae6118ae11b1..6e965a3ca322 100644 > --- a/drivers/iio/accel/bmc150-accel.h > +++ b/drivers/iio/accel/bmc150-accel.h > @@ -16,6 +16,8 @@ enum { > int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > const char *name, bool block_supported); > int bmc150_accel_core_remove(struct device *dev); > +struct i2c_client *bmc150_get_second_device(struct i2c_client *second_device); > +void bmc150_set_second_device(struct i2c_client *second_device); > extern const struct dev_pm_ops bmc150_accel_pm_ops; > extern const struct regmap_config bmc150_regmap_conf; > -- 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
On 12/10/2017 12:21 PM, Jonathan Cameron wrote: > On Wed, 6 Dec 2017 17:52:34 +0000 > Jeremy Cline <jeremy@jcline.org> wrote: > >> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI >> device. Check for a companion device and handle a second i2c_client >> if it is present. >> >> Signed-off-by: Jeremy Cline <jeremy@jcline.org> > The requirement for this is still horrible, but you have done a nice > clean job on implementing it. > > I'll let this sit for a few more days though before applying it. > Probably next weekend if we don't get any feedback before then. Hey, I didn't see this land anywhere (I was looking in git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git, maybe that's not the right place?) and I just wanted to make sure this didn't get lost in the holiday shuffle. Regards, Jeremy -- 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
On Tue, 9 Jan 2018 21:24:01 +0000 Jeremy Cline <jeremy@jcline.org> wrote: > On 12/10/2017 12:21 PM, Jonathan Cameron wrote: > > On Wed, 6 Dec 2017 17:52:34 +0000 > > Jeremy Cline <jeremy@jcline.org> wrote: > > > >> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI > >> device. Check for a companion device and handle a second i2c_client > >> if it is present. > >> > >> Signed-off-by: Jeremy Cline <jeremy@jcline.org> > > The requirement for this is still horrible, but you have done a nice > > clean job on implementing it. > > > > I'll let this sit for a few more days though before applying it. > > Probably next weekend if we don't get any feedback before then. > > Hey, > > I didn't see this land anywhere (I was looking in > git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git, maybe that's not > the right place?) and I just wanted to make sure this didn't get lost in > the holiday shuffle. It did indeed get lost - thanks for the reminder. Now applied to the togreg branch of iio.git. However, unfortunately we may be too near to the merge window opening for it to make it. Depends on what Linus says later today when rc8 comes out. Jonathan > > Regards, > Jeremy > -- > 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-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 14 Jan 2018 10:43:30 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 9 Jan 2018 21:24:01 +0000 > Jeremy Cline <jeremy@jcline.org> wrote: > > > On 12/10/2017 12:21 PM, Jonathan Cameron wrote: > > > On Wed, 6 Dec 2017 17:52:34 +0000 > > > Jeremy Cline <jeremy@jcline.org> wrote: > > > > > >> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI > > >> device. Check for a companion device and handle a second i2c_client > > >> if it is present. > > >> > > >> Signed-off-by: Jeremy Cline <jeremy@jcline.org> > > > The requirement for this is still horrible, but you have done a nice > > > clean job on implementing it. > > > > > > I'll let this sit for a few more days though before applying it. > > > Probably next weekend if we don't get any feedback before then. > > > > Hey, > > > > I didn't see this land anywhere (I was looking in > > git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git, maybe that's not > > the right place?) and I just wanted to make sure this didn't get lost in > > the holiday shuffle. > It did indeed get lost - thanks for the reminder. Now applied to the > togreg branch of iio.git. However, unfortunately we may be too near > to the merge window opening for it to make it. Depends on what Linus > says later today when rc8 comes out. I've added some #ifdef CONFIG_ACPI defenses against the case of no ACPI support being compiled in. Alternative would be to add stubs for those functions that don't have them... probably just acpi_device_hid. But that would take much longer. Feel free to propose it and a patch removing the ifdef fun if you like! Jonathan > > Jonathan > > > > > Regards, > > Jeremy > > -- > > 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-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-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 28, 2018 at 11:40 AM, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > On Sun, 14 Jan 2018 10:43:30 +0000 > Jonathan Cameron <jic23@kernel.org> wrote: >> On Tue, 9 Jan 2018 21:24:01 +0000 >> Jeremy Cline <jeremy@jcline.org> wrote: >> > On 12/10/2017 12:21 PM, Jonathan Cameron wrote: >> > > On Wed, 6 Dec 2017 17:52:34 +0000 >> > > Jeremy Cline <jeremy@jcline.org> wrote: >> > >> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI >> > >> device. Check for a companion device and handle a second i2c_client >> > >> if it is present. >> > I didn't see this land anywhere (I was looking in >> > git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git, maybe that's not >> > the right place?) and I just wanted to make sure this didn't get lost in >> > the holiday shuffle. >> It did indeed get lost - thanks for the reminder. Now applied to the >> togreg branch of iio.git. However, unfortunately we may be too near >> to the merge window opening for it to make it. Depends on what Linus >> says later today when rc8 comes out. > > I've added some #ifdef CONFIG_ACPI defenses against the case > of no ACPI support being compiled in. Alternative would be to add > stubs for those functions that don't have them... > > probably just acpi_device_hid. > > But that would take much longer. Feel free to propose it and a patch > removing the ifdef fun if you like! Where can I see the patch?
On 01/28/2018 03:40 AM, Jonathan Cameron wrote: > On Sun, 14 Jan 2018 10:43:30 +0000 > Jonathan Cameron <jic23@kernel.org> wrote: > >> On Tue, 9 Jan 2018 21:24:01 +0000 >> Jeremy Cline <jeremy@jcline.org> wrote: >> >>> On 12/10/2017 12:21 PM, Jonathan Cameron wrote: >>>> On Wed, 6 Dec 2017 17:52:34 +0000 >>>> Jeremy Cline <jeremy@jcline.org> wrote: >>>> >>>>> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI >>>>> device. Check for a companion device and handle a second i2c_client >>>>> if it is present. >>>>> >>>>> Signed-off-by: Jeremy Cline <jeremy@jcline.org> >>>> The requirement for this is still horrible, but you have done a nice >>>> clean job on implementing it. >>>> >>>> I'll let this sit for a few more days though before applying it. >>>> Probably next weekend if we don't get any feedback before then. >>> >>> Hey, >>> >>> I didn't see this land anywhere (I was looking in >>> git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git, maybe that's not >>> the right place?) and I just wanted to make sure this didn't get lost in >>> the holiday shuffle. >> It did indeed get lost - thanks for the reminder. Now applied to the >> togreg branch of iio.git. However, unfortunately we may be too near >> to the merge window opening for it to make it. Depends on what Linus >> says later today when rc8 comes out. > > I've added some #ifdef CONFIG_ACPI defenses against the case > of no ACPI support being compiled in. Alternative would be to add > stubs for those functions that don't have them... > > probably just acpi_device_hid. > > But that would take much longer. Feel free to propose it and a patch > removing the ifdef fun if you like! Great, thanks for handling that. I'll look at taking care of the stubs and whatnot. Regards, Jeremy -- 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
On Mon, 29 Jan 2018 16:07:02 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Jan 28, 2018 at 11:40 AM, Jonathan Cameron > <jic23@jic23.retrosnub.co.uk> wrote: > > On Sun, 14 Jan 2018 10:43:30 +0000 > > Jonathan Cameron <jic23@kernel.org> wrote: > >> On Tue, 9 Jan 2018 21:24:01 +0000 > >> Jeremy Cline <jeremy@jcline.org> wrote: > >> > On 12/10/2017 12:21 PM, Jonathan Cameron wrote: > >> > > On Wed, 6 Dec 2017 17:52:34 +0000 > >> > > Jeremy Cline <jeremy@jcline.org> wrote: > > >> > >> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI > >> > >> device. Check for a companion device and handle a second i2c_client > >> > >> if it is present. > > >> > I didn't see this land anywhere (I was looking in > >> > git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git, maybe that's not > >> > the right place?) and I just wanted to make sure this didn't get lost in > >> > the holiday shuffle. > >> It did indeed get lost - thanks for the reminder. Now applied to the > >> togreg branch of iio.git. However, unfortunately we may be too near > >> to the merge window opening for it to make it. Depends on what Linus > >> says later today when rc8 comes out. > > > > I've added some #ifdef CONFIG_ACPI defenses against the case > > of no ACPI support being compiled in. Alternative would be to add > > stubs for those functions that don't have them... > > > > probably just acpi_device_hid. > > > > But that would take much longer. Feel free to propose it and a patch > > removing the ifdef fun if you like! > > Where can I see the patch? > Doh. I clearly forgot to push out. Should be able to push to iio.git on kernel.org later. Jonathan -- 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
On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Mon, 29 Jan 2018 16:07:02 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> > But that would take much longer. Feel free to propose it and a patch >> > removing the ifdef fun if you like! >> Where can I see the patch? > Doh. I clearly forgot to push out. Should be able to push to > iio.git on kernel.org later. Thanks, I can see it now. This patch almost wrong. Not by functionality it brings, but by style. I'll send soon a series of fixes to the driver (compile tested only) to provide my view on the matters. P.S. In the future (I have some kind of deja vu I have told this already to someone), please, Cc one or more of Rafael, Mika and/or me for ACPI matters.
On Tue, Jan 30, 2018 at 6:40 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: >> On Mon, 29 Jan 2018 16:07:02 +0200 >> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>> > But that would take much longer. Feel free to propose it and a patch >>> > removing the ifdef fun if you like! > >>> Where can I see the patch? > >> Doh. I clearly forgot to push out. Should be able to push to >> iio.git on kernel.org later. > > Thanks, I can see it now. > > This patch almost wrong. Not by functionality it brings, but by style. Oy vey, the second device is *not* accelerometer, it is a magnetometer [1]. [1]: https://www.mouser.com/ds/2/783/BST-BMC150-DS000-04-786477.pdf > I'll send soon a series of fixes to the driver (compile tested only) > to provide my view on the matters. > > P.S. In the future (I have some kind of deja vu I have told this > already to someone), please, Cc one or more of Rafael, Mika and/or me > for ACPI matters.
On Tue, Jan 30, 2018 at 7:25 PM, Steve Presser <steve@pressers.name> wrote: > Andy, > > Where did the assertion the second device is a magnetometer come from? Just > the data sheet? Yep. See chapter 8.2. Isn't enough proof? Or you believe in two accelerometers with off-by-one conflicting address on a cheap laptop with left unused two magnetometers on the same time? And we have a driver for magnetometer separately. So, it looks like we need to move ACPI ID to a new "kinda I2C mfd" IIO driver under drivers/iio/imu/bmc150_i2c.c > Steve > > > On Tue, Jan 30, 2018, 12:05 PM Andy Shevchenko <andy.shevchenko@gmail.com> > wrote: >> >> On Tue, Jan 30, 2018 at 6:40 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >> > On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron >> > <Jonathan.Cameron@huawei.com> wrote: >> >> On Mon, 29 Jan 2018 16:07:02 +0200 >> >> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> > >> >>> > But that would take much longer. Feel free to propose it and a >> >>> > patch >> >>> > removing the ifdef fun if you like! >> > >> >>> Where can I see the patch? >> > >> >> Doh. I clearly forgot to push out. Should be able to push to >> >> iio.git on kernel.org later. >> > >> > Thanks, I can see it now. >> > >> > This patch almost wrong. Not by functionality it brings, but by style. >> >> Oy vey, the second device is *not* accelerometer, it is a magnetometer >> [1]. >> >> [1]: https://www.mouser.com/ds/2/783/BST-BMC150-DS000-04-786477.pdf >> >> > I'll send soon a series of fixes to the driver (compile tested only) >> > to provide my view on the matters. >> > >> > P.S. In the future (I have some kind of deja vu I have told this >> > already to someone), please, Cc one or more of Rafael, Mika and/or me >> > for ACPI matters. >> >> -- >> With Best Regards, >> Andy Shevchenko
On Tue, Jan 30, 2018 at 7:38 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jan 30, 2018 at 7:25 PM, Steve Presser <steve@pressers.name> wrote: >> Andy, >> >> Where did the assertion the second device is a magnetometer come from? Just >> the data sheet? > > Yep. See chapter 8.2. Isn't enough proof? Or you believe in two > accelerometers with off-by-one conflicting address on a cheap laptop > with left unused two magnetometers on the same time? > > And we have a driver for magnetometer separately. > > So, it looks like we need to move ACPI ID to a new "kinda I2C mfd" IIO > driver under drivers/iio/imu/bmc150_i2c.c Even Kconfig for one of the driver states so. Looking more to it, I think the patch should be reverted and new driver is created instead. I'm on it. >> Steve >> >> >> On Tue, Jan 30, 2018, 12:05 PM Andy Shevchenko <andy.shevchenko@gmail.com> >> wrote: >>> >>> On Tue, Jan 30, 2018 at 6:40 PM, Andy Shevchenko >>> <andy.shevchenko@gmail.com> wrote: >>> > On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron >>> > <Jonathan.Cameron@huawei.com> wrote: >>> >> On Mon, 29 Jan 2018 16:07:02 +0200 >>> >> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> > >>> >>> > But that would take much longer. Feel free to propose it and a >>> >>> > patch >>> >>> > removing the ifdef fun if you like! >>> > >>> >>> Where can I see the patch? >>> > >>> >> Doh. I clearly forgot to push out. Should be able to push to >>> >> iio.git on kernel.org later. >>> > >>> > Thanks, I can see it now. >>> > >>> > This patch almost wrong. Not by functionality it brings, but by style. >>> >>> Oy vey, the second device is *not* accelerometer, it is a magnetometer >>> [1]. >>> >>> [1]: https://www.mouser.com/ds/2/783/BST-BMC150-DS000-04-786477.pdf >>> >>> > I'll send soon a series of fixes to the driver (compile tested only) >>> > to provide my view on the matters. >>> > >>> > P.S. In the future (I have some kind of deja vu I have told this >>> > already to someone), please, Cc one or more of Rafael, Mika and/or me >>> > for ACPI matters. >>> >>> -- >>> With Best Regards, >>> Andy Shevchenko > > > > -- > With Best Regards, > Andy Shevchenko
On Tue, 30 Jan 2018 20:08:19 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jan 30, 2018 at 7:38 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, Jan 30, 2018 at 7:25 PM, Steve Presser <steve@pressers.name> wrote: > >> Andy, > >> > >> Where did the assertion the second device is a magnetometer come from? Just > >> the data sheet? > > > > Yep. See chapter 8.2. Isn't enough proof? Or you believe in two > > accelerometers with off-by-one conflicting address on a cheap laptop > > with left unused two magnetometers on the same time? > > > > And we have a driver for magnetometer separately. > > > > So, it looks like we need to move ACPI ID to a new "kinda I2C mfd" IIO > > driver under drivers/iio/imu/bmc150_i2c.c > > Even Kconfig for one of the driver states so. > > Looking more to it, I think the patch should be reverted and new > driver is created instead. Whilst the question is still open I have dropped the patch. Was not yet in a non rebasing tree (just in a build test one) so fine to drop it. > > I'm on it. > > >> Steve > >> > >> > >> On Tue, Jan 30, 2018, 12:05 PM Andy Shevchenko <andy.shevchenko@gmail.com> > >> wrote: > >>> > >>> On Tue, Jan 30, 2018 at 6:40 PM, Andy Shevchenko > >>> <andy.shevchenko@gmail.com> wrote: > >>> > On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron > >>> > <Jonathan.Cameron@huawei.com> wrote: > >>> >> On Mon, 29 Jan 2018 16:07:02 +0200 > >>> >> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>> > > >>> >>> > But that would take much longer. Feel free to propose it and a > >>> >>> > patch > >>> >>> > removing the ifdef fun if you like! > >>> > > >>> >>> Where can I see the patch? > >>> > > >>> >> Doh. I clearly forgot to push out. Should be able to push to > >>> >> iio.git on kernel.org later. > >>> > > >>> > Thanks, I can see it now. > >>> > > >>> > This patch almost wrong. Not by functionality it brings, but by style. > >>> > >>> Oy vey, the second device is *not* accelerometer, it is a magnetometer > >>> [1]. > >>> > >>> [1]: https://www.mouser.com/ds/2/783/BST-BMC150-DS000-04-786477.pdf > >>> > >>> > I'll send soon a series of fixes to the driver (compile tested only) > >>> > to provide my view on the matters. > >>> > > >>> > P.S. In the future (I have some kind of deja vu I have told this > >>> > already to someone), please, Cc one or more of Rafael, Mika and/or me > >>> > for ACPI matters. > >>> > >>> -- > >>> With Best Regards, > >>> Andy Shevchenko > > > > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > -- 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
Andy, I apologize for the long response, but there's several issues to address here. First, I believe the "bmc150" in the subject line is in some way a misnomer. You'd have to ask Jeremy for more details on what he intended it to refer to. However, I believe the device in question is actually the bma250[1], which does not have a magnetometer component. I'm unfortunately away from my notes, but I can check later if you need me to verify the exact chip. Second, we're seeing a difference between what's in the data sheet and what's exposed in the wild via ACPI. I own the laptop that started the process of building this patch and I did the original ACPI-tables investigation. The device in question (BOSC0200) appears in the Lenovo Yoga 11e (and possibly other laptops - this happens to be the one I own). These laptops have a 360-degree hinge between the screen and the keyboard, letting them convert into tablets, if the user desires. The 11e implements this mode-switching by placing an accelerometer in each of the screen and keyboard, then doing math with the resulting vectors to figure out the angle between the two. For whatever reason, Lenovo chose to expose these two (physically separate) accelerometers via a single ACPI device which presents two i2c devices at sequential addresses. As part of my original investigation of the Yoga 11e, I wrote a proof-of-concept of pulling accelerometer data from the two devices exposed under the BOSC0200 ID and using that to calculate the position of the screen relative to the keyboard. So based on my empirical experience, I can tell you the BOSC0200 device ID can expose two accelerometers at sequential addresses in the wild. I don't understand why Lenovo has reused the BOSC0200 ACPI device ID for a device that is fundamentally different from the base device. The ID doesn't belong to them and we're (apparently) now stuck in this situation where this ACPI device ID could represent two different device layouts. Finally - Andy, I apologize if I came across as challenging you in my initial mail. I was trying to strike a balance between brevity/respecting your time and asking a question. Evidently I struck the wrong balance and should have given you more background on why I was doubting what you saw. This is my fault and you have my sincerest apologies for any offense I have caused. Steve [1] https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA250E-DS004-06.pdf On 01/30/2018 12:38 PM, Andy Shevchenko wrote: > On Tue, Jan 30, 2018 at 7:25 PM, Steve Presser <steve@pressers.name> wrote: >> Andy, >> >> Where did the assertion the second device is a magnetometer come from? Just >> the data sheet? > Yep. See chapter 8.2. Isn't enough proof? Or you believe in two > accelerometers with off-by-one conflicting address on a cheap laptop > with left unused two magnetometers on the same time? > > And we have a driver for magnetometer separately. > > So, it looks like we need to move ACPI ID to a new "kinda I2C mfd" IIO > driver under drivers/iio/imu/bmc150_i2c.c > > >> Steve >> >> >> On Tue, Jan 30, 2018, 12:05 PM Andy Shevchenko <andy.shevchenko@gmail.com> >> wrote: >>> On Tue, Jan 30, 2018 at 6:40 PM, Andy Shevchenko >>> <andy.shevchenko@gmail.com> wrote: >>>> On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron >>>> <Jonathan.Cameron@huawei.com> wrote: >>>>> On Mon, 29 Jan 2018 16:07:02 +0200 >>>>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>>>>> But that would take much longer. Feel free to propose it and a >>>>>>> patch >>>>>>> removing the ifdef fun if you like! >>>>>> Where can I see the patch? >>>>> Doh. I clearly forgot to push out. Should be able to push to >>>>> iio.git on kernel.org later. >>>> Thanks, I can see it now. >>>> >>>> This patch almost wrong. Not by functionality it brings, but by style. >>> Oy vey, the second device is *not* accelerometer, it is a magnetometer >>> [1]. >>> >>> [1]: https://www.mouser.com/ds/2/783/BST-BMC150-DS000-04-786477.pdf >>> >>>> I'll send soon a series of fixes to the driver (compile tested only) >>>> to provide my view on the matters. >>>> >>>> P.S. In the future (I have some kind of deja vu I have told this >>>> already to someone), please, Cc one or more of Rafael, Mika and/or me >>>> for ACPI matters. >>> -- >>> With Best Regards, >>> Andy Shevchenko > >
On Tue, 30 Jan 2018 18:33:43 +0000 Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > On Tue, 30 Jan 2018 20:08:19 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jan 30, 2018 at 7:38 PM, Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jan 30, 2018 at 7:25 PM, Steve Presser <steve@pressers.name> wrote: > > >> Andy, > > >> > > >> Where did the assertion the second device is a magnetometer come from? Just > > >> the data sheet? > > > > > > Yep. See chapter 8.2. Isn't enough proof? Or you believe in two > > > accelerometers with off-by-one conflicting address on a cheap laptop > > > with left unused two magnetometers on the same time? > > > > > > And we have a driver for magnetometer separately. > > > > > > So, it looks like we need to move ACPI ID to a new "kinda I2C mfd" IIO > > > driver under drivers/iio/imu/bmc150_i2c.c > > > > Even Kconfig for one of the driver states so. > > > > Looking more to it, I think the patch should be reverted and new > > driver is created instead. > > Whilst the question is still open I have dropped the patch. > Was not yet in a non rebasing tree (just in a build test one) > so fine to drop it. Should have said this isn't the final decision or anything, but whilst there are open questions we should take the time to sure of the answer! > > > > > I'm on it. > > > > >> Steve > > >> > > >> > > >> On Tue, Jan 30, 2018, 12:05 PM Andy Shevchenko <andy.shevchenko@gmail.com> > > >> wrote: > > >>> > > >>> On Tue, Jan 30, 2018 at 6:40 PM, Andy Shevchenko > > >>> <andy.shevchenko@gmail.com> wrote: > > >>> > On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron > > >>> > <Jonathan.Cameron@huawei.com> wrote: > > >>> >> On Mon, 29 Jan 2018 16:07:02 +0200 > > >>> >> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > >>> > > > >>> >>> > But that would take much longer. Feel free to propose it and a > > >>> >>> > patch > > >>> >>> > removing the ifdef fun if you like! > > >>> > > > >>> >>> Where can I see the patch? > > >>> > > > >>> >> Doh. I clearly forgot to push out. Should be able to push to > > >>> >> iio.git on kernel.org later. > > >>> > > > >>> > Thanks, I can see it now. > > >>> > > > >>> > This patch almost wrong. Not by functionality it brings, but by style. > > >>> > > >>> Oy vey, the second device is *not* accelerometer, it is a magnetometer > > >>> [1]. > > >>> > > >>> [1]: https://www.mouser.com/ds/2/783/BST-BMC150-DS000-04-786477.pdf > > >>> > > >>> > I'll send soon a series of fixes to the driver (compile tested only) > > >>> > to provide my view on the matters. > > >>> > > > >>> > P.S. In the future (I have some kind of deja vu I have told this > > >>> > already to someone), please, Cc one or more of Rafael, Mika and/or me > > >>> > for ACPI matters. > > >>> > > >>> -- > > >>> With Best Regards, > > >>> Andy Shevchenko > > > > > > > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko > > > > > > > > -- > 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-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 30, 2018 at 8:46 PM, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: >> Whilst the question is still open I have dropped the patch. >> Was not yet in a non rebasing tree (just in a build test one) >> so fine to drop it. > > Should have said this isn't the final decision or anything, but > whilst there are open questions we should take the time to sure > of the answer! Please, drop it. It's broken anyway. I'm going to explain this in the mail to Steve asap.
On Tue, Jan 30, 2018 at 8:34 PM, Steven Presser <steve@pressers.name> wrote: > Andy, > > I apologize for the long response, but there's several issues to address > here. NP, it it a good explanation why. That's what commit message missed apparently. > First, I believe the "bmc150" in the subject line is in some way a misnomer. > You'd have to ask Jeremy for more details on what he intended it to refer > to. However, I believe the device in question is actually the bma250[1], > which does not have a magnetometer component. I'm unfortunately away from > my notes, but I can check later if you need me to verify the exact chip. Please do, I would really be on the safe side here. > Second, we're seeing a difference between what's in the data sheet and > what's exposed in the wild via ACPI. I own the laptop that started the > process of building this patch and I did the original ACPI-tables > investigation. > > The device in question (BOSC0200) appears in the Lenovo Yoga 11e (and > possibly other laptops - this happens to be the one I own). These laptops > have a 360-degree hinge between the screen and the keyboard, letting them > convert into tablets, if the user desires. The 11e implements this > mode-switching by placing an accelerometer in each of the screen and > keyboard, then doing math with the resulting vectors to figure out the angle > between the two. This makes a lot of sense. > For whatever reason, Lenovo chose to expose these two > (physically separate) accelerometers via a single ACPI device which presents > two i2c devices at sequential addresses. > As part of my original investigation of the Yoga 11e, I wrote a > proof-of-concept of pulling accelerometer data from the two devices exposed > under the BOSC0200 ID and using that to calculate the position of the screen > relative to the keyboard. So based on my empirical experience, I can tell > you the BOSC0200 device ID can expose two accelerometers at sequential > addresses in the wild. > > I don't understand why Lenovo has reused the BOSC0200 ACPI device ID for a > device that is fundamentally different from the base device. The ID doesn't > belong to them and we're (apparently) now stuck in this situation where this > ACPI device ID could represent two different device layouts. Bad, bad Lenovo. (DMI strings might help here) > Finally - Andy, I apologize if I came across as challenging you in my > initial mail. I was trying to strike a balance between brevity/respecting > your time and asking a question. Evidently I struck the wrong balance and > should have given you more background on why I was doubting what you saw. > This is my fault and you have my sincerest apologies for any offense I have > caused. No need, the root cause is lack of description in the commit message. Nevertheless, the approach chosen I don't like. It looks like an ugly hack. What we can do here is: - do not contaminate core part with I2C/SPI/etc - do not create another driver via board_info, we already in *the same* driver, so, the better approach here AFAICS is to add DMI quirk into i2c-core-acpi > Steve > > [1] > https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA250E-DS004-06.pdf
On 01/30/2018 02:05 PM, Andy Shevchenko wrote: > On Tue, Jan 30, 2018 at 8:34 PM, Steven Presser <steve@pressers.name> wrote: >> Andy, >> >> I apologize for the long response, but there's several issues to address >> here. > NP, it it a good explanation why. That's what commit message missed apparently. Probably my fault anyway - I don't recall discussing with Jeremy exactly what chip was inside this little Frankenstein. > >> First, I believe the "bmc150" in the subject line is in some way a misnomer. >> You'd have to ask Jeremy for more details on what he intended it to refer >> to. However, I believe the device in question is actually the bma250[1], >> which does not have a magnetometer component. I'm unfortunately away from >> my notes, but I can check later if you need me to verify the exact chip. > Please do, I would really be on the safe side here. Will do. My digital notes indicate I worked from what was exposed back to what chip matched. If you can give me through Friday evening, I'll crack it and do a visual verification. (Alas, I'm traveling and won't be back to it until then). > >> Second, we're seeing a difference between what's in the data sheet and >> what's exposed in the wild via ACPI. I own the laptop that started the >> process of building this patch and I did the original ACPI-tables >> investigation. >> >> The device in question (BOSC0200) appears in the Lenovo Yoga 11e (and >> possibly other laptops - this happens to be the one I own). These laptops >> have a 360-degree hinge between the screen and the keyboard, letting them >> convert into tablets, if the user desires. The 11e implements this >> mode-switching by placing an accelerometer in each of the screen and >> keyboard, then doing math with the resulting vectors to figure out the angle >> between the two. > This makes a lot of sense. > >> For whatever reason, Lenovo chose to expose these two >> (physically separate) accelerometers via a single ACPI device which presents >> two i2c devices at sequential addresses. > >> As part of my original investigation of the Yoga 11e, I wrote a >> proof-of-concept of pulling accelerometer data from the two devices exposed >> under the BOSC0200 ID and using that to calculate the position of the screen >> relative to the keyboard. So based on my empirical experience, I can tell >> you the BOSC0200 device ID can expose two accelerometers at sequential >> addresses in the wild. >> >> I don't understand why Lenovo has reused the BOSC0200 ACPI device ID for a >> device that is fundamentally different from the base device. The ID doesn't >> belong to them and we're (apparently) now stuck in this situation where this >> ACPI device ID could represent two different device layouts. > Bad, bad Lenovo. (DMI strings might help here) What particular DMI strings would be helpful? All of them? > >> Finally - Andy, I apologize if I came across as challenging you in my >> initial mail. I was trying to strike a balance between brevity/respecting >> your time and asking a question. Evidently I struck the wrong balance and >> should have given you more background on why I was doubting what you saw. >> This is my fault and you have my sincerest apologies for any offense I have >> caused. > No need, the root cause is lack of description in the commit message. > > Nevertheless, the approach chosen I don't like. It looks like an ugly hack. > > What we can do here is: > - do not contaminate core part with I2C/SPI/etc > - do not create another driver via board_info, we already in *the same* driver, > so, the better approach here AFAICS is to add DMI quirk into i2c-core-acpi > > > >> Steve >> >> [1] >> https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA250E-DS004-06.pdf
On Tue, Jan 30, 2018 at 9:27 PM, Steven Presser <steve@pressers.name> wrote: > On 01/30/2018 02:05 PM, Andy Shevchenko wrote: >> On Tue, Jan 30, 2018 at 8:34 PM, Steven Presser <steve@pressers.name> >> wrote: >>> First, I believe the "bmc150" in the subject line is in some way a >>> misnomer. >>> You'd have to ask Jeremy for more details on what he intended it to refer >>> to. However, I believe the device in question is actually the bma250[1], >>> which does not have a magnetometer component. I'm unfortunately away >>> from >>> my notes, but I can check later if you need me to verify the exact chip. >> >> Please do, I would really be on the safe side here. > > Will do. My digital notes indicate I worked from what was exposed back to > what chip matched. If you can give me through Friday evening, I'll crack it > and do a visual verification. (Alas, I'm traveling and won't be back to it > until then). We are in the merge window anyway, so, no hurry. I'm looking right now in the clean solution. Looks promising. >> Bad, bad Lenovo. (DMI strings might help here) > What particular DMI strings would be helpful? All of them? Let's do this way. Create a bug on kernel bugzilla, attach output of % acpidump -o tables.dat # tables.dat file % grep -H 15 /sys/bus/acpi/devices/*/status % dmidecode and share the number here. I will take it.
On Tue, Jan 30, 2018 at 10:12 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jan 30, 2018 at 9:27 PM, Steven Presser <steve@pressers.name> wrote: >> On 01/30/2018 02:05 PM, Andy Shevchenko wrote: >>> On Tue, Jan 30, 2018 at 8:34 PM, Steven Presser <steve@pressers.name> >>> wrote: > Let's do this way. Create a bug on kernel bugzilla, attach output of > > % acpidump -o tables.dat # tables.dat file > % grep -H 15 /sys/bus/acpi/devices/*/status > % dmidecode > > and share the number here. I will take it. Here [1] is the branch with another approach. Can you test it and tell if it fixes the issue? [1]: https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi It's based on linux-next + compiletest branch of IIO subsystem.
On Tue, 30 Jan 2018 23:20:28 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jan 30, 2018 at 10:12 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, Jan 30, 2018 at 9:27 PM, Steven Presser <steve@pressers.name> wrote: > >> On 01/30/2018 02:05 PM, Andy Shevchenko wrote: > >>> On Tue, Jan 30, 2018 at 8:34 PM, Steven Presser <steve@pressers.name> > >>> wrote: > > > Let's do this way. Create a bug on kernel bugzilla, attach output of > > > > % acpidump -o tables.dat # tables.dat file > > % grep -H 15 /sys/bus/acpi/devices/*/status > > % dmidecode > > > > and share the number here. I will take it. > > Here [1] is the branch with another approach. Can you test it and tell > if it fixes the issue? > > [1]: https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi > It's based on linux-next + compiletest branch of IIO subsystem. > Thanks Andy - this is much nicer. I really don't like having these 'hacks' make it into the driver so very keen on doing it this way. Now we get to find out what devices have this broken in other ways ;) Jonathan -- 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
H5, On 01/30/2018 06:38 PM, Andy Shevchenko wrote: > On Tue, Jan 30, 2018 at 7:25 PM, Steve Presser <steve@pressers.name> wrote: >> Andy, >> >> Where did the assertion the second device is a magnetometer come from? Just >> the data sheet? > > Yep. See chapter 8.2. Isn't enough proof? Or you believe in two > accelerometers with off-by-one conflicting address on a cheap laptop > with left unused two magnetometers on the same time? This is not a cheap device, this has been seen on a Lenovo Yoga 11e, the yoga's typically have an accelerometer in both the base and the display and have no use for a magnetometer. Not saying that you're wrong, but my expectations are different. Anyways we need to find someone to test this, I asked Jeremy to write a patch for this because we had Yoga 11e user (Lars Kellogg-Stedman in the CC) asking question and Jeremy did ask that Lars to test. It looks like we will need to reach out to Lars and get some testing done to figure this out one way or the other. Lars if you're reading this can you please reply. If you've trouble building your own kernels for testing, would you be willing to install Fedora so that we can provide test kernels for you? Regards, Hans p.s. For reference here is the relevant DSDT blurb from the Yoga 11e: Device (ACC) { Name (_ADR, Zero) // _ADR: Address Name (_HID, "BOSC0200") // _HID: Hardware ID Name (_CID, "BOSC0200") // _CID: Compatible ID Name (_DDN, "Accelerometer") // _DDN: DOS Device Name Name (_UID, One) // _UID: Unique ID Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings { Name (RBUF, ResourceTemplate () { I2cSerialBusV2 (0x0019, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00, ResourceConsumer, , Exclusive, ) I2cSerialBusV2 (0x0018, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\\_SB.PCI0.I2C3", 0x00, ResourceConsumer, , Exclusive, ) }) Return (RBUF) /* \_SB_.PCI0.I2C3.ACC_._CRS.RBUF */ } > > And we have a driver for magnetometer separately. > > So, it looks like we need to move ACPI ID to a new "kinda I2C mfd" IIO > driver under drivers/iio/imu/bmc150_i2c.c > > >> Steve >> >> >> On Tue, Jan 30, 2018, 12:05 PM Andy Shevchenko <andy.shevchenko@gmail.com> >> wrote: >>> >>> On Tue, Jan 30, 2018 at 6:40 PM, Andy Shevchenko >>> <andy.shevchenko@gmail.com> wrote: >>>> On Tue, Jan 30, 2018 at 6:01 PM, Jonathan Cameron >>>> <Jonathan.Cameron@huawei.com> wrote: >>>>> On Mon, 29 Jan 2018 16:07:02 +0200 >>>>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>> >>>>>>> But that would take much longer. Feel free to propose it and a >>>>>>> patch >>>>>>> removing the ifdef fun if you like! >>>> >>>>>> Where can I see the patch? >>>> >>>>> Doh. I clearly forgot to push out. Should be able to push to >>>>> iio.git on kernel.org later. >>>> >>>> Thanks, I can see it now. >>>> >>>> This patch almost wrong. Not by functionality it brings, but by style. >>> >>> Oy vey, the second device is *not* accelerometer, it is a magnetometer >>> [1]. >>> >>> [1]: https://www.mouser.com/ds/2/783/BST-BMC150-DS000-04-786477.pdf >>> >>>> I'll send soon a series of fixes to the driver (compile tested only) >>>> to provide my view on the matters. >>>> >>>> P.S. In the future (I have some kind of deja vu I have told this >>>> already to someone), please, Cc one or more of Rafael, Mika and/or me >>>> for ACPI matters. >>> >>> -- >>> With Best Regards, >>> Andy Shevchenko > > > -- 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
On Wed, Jan 31, 2018 at 1:43 PM, Hans de Goede <hdegoede@redhat.com> wrote: > On 01/30/2018 06:38 PM, Andy Shevchenko wrote: >> On Tue, Jan 30, 2018 at 7:25 PM, Steve Presser <steve@pressers.name> >> wrote: >> Yep. See chapter 8.2. Isn't enough proof? Or you believe in two >> accelerometers with off-by-one conflicting address on a cheap laptop >> with left unused two magnetometers on the same time? > This is not a cheap device, this has been seen on a Lenovo Yoga 11e, > the yoga's typically have an accelerometer in both the base and the display > and have no use for a magnetometer. Not saying that you're wrong, Yep, Steve explained to me. Thanks! > but my expectations are different. Anyways we need to find someone to > test this, I asked Jeremy to write a patch for this because we had > Yoga 11e user (Lars Kellogg-Stedman in the CC) asking question and > Jeremy did ask that Lars to test. > > It looks like we will need to reach out to Lars and get some testing done > to figure this out one way or the other. > > Lars if you're reading this can you please reply. If you've trouble > building your own kernels for testing, would you be willing to install > Fedora so that we can provide test kernels for you? Have you chance a look at the branch I pushed yesterday? > For reference here is the relevant DSDT blurb from the Yoga 11e: Yes, I have googled something like this yesterday, but it doesn't clarify what kind of devices behind this entry. > Device (ACC) > { > Name (_ADR, Zero) // _ADR: Address > Name (_HID, "BOSC0200") // _HID: Hardware ID > Name (_CID, "BOSC0200") // _CID: Compatible ID > Name (_DDN, "Accelerometer") // _DDN: DOS Device Name > Name (_UID, One) // _UID: Unique ID > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource > Settings > { > Name (RBUF, ResourceTemplate () > { > I2cSerialBusV2 (0x0019, ControllerInitiated, > 0x00061A80, > AddressingMode7Bit, "\\_SB.PCI0.I2C3", > 0x00, ResourceConsumer, , Exclusive, > ) > I2cSerialBusV2 (0x0018, ControllerInitiated, > 0x00061A80, > AddressingMode7Bit, "\\_SB.PCI0.I2C3", > 0x00, ResourceConsumer, , Exclusive, > ) > }) > Return (RBUF) /* \_SB_.PCI0.I2C3.ACC_._CRS.RBUF */ > > }
Hi, On 31-01-18 13:25, Andy Shevchenko wrote: > Have you chance a look at the branch I pushed yesterday? I assume you mean: https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi This looks very nice and a much better solution then what Jeremy Cline posted, which is my fault as I suggested that approach to Jeremy :) I've not done a full review, only a quick look, from a quick look this looks good. Especially from a code-design pov. Regards, Hans > >> For reference here is the relevant DSDT blurb from the Yoga 11e: > > Yes, I have googled something like this yesterday, but it doesn't > clarify what kind of devices behind this entry. > >> Device (ACC) >> { >> Name (_ADR, Zero) // _ADR: Address >> Name (_HID, "BOSC0200") // _HID: Hardware ID >> Name (_CID, "BOSC0200") // _CID: Compatible ID >> Name (_DDN, "Accelerometer") // _DDN: DOS Device Name >> Name (_UID, One) // _UID: Unique ID >> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource >> Settings >> { >> Name (RBUF, ResourceTemplate () >> { >> I2cSerialBusV2 (0x0019, ControllerInitiated, >> 0x00061A80, >> AddressingMode7Bit, "\\_SB.PCI0.I2C3", >> 0x00, ResourceConsumer, , Exclusive, >> ) >> I2cSerialBusV2 (0x0018, ControllerInitiated, >> 0x00061A80, >> AddressingMode7Bit, "\\_SB.PCI0.I2C3", >> 0x00, ResourceConsumer, , Exclusive, >> ) >> }) >> Return (RBUF) /* \_SB_.PCI0.I2C3.ACC_._CRS.RBUF */ >> >> } > -- 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
On Wed, Jan 31, 2018 at 4:58 PM, Hans de Goede <hdegoede@redhat.com> wrote: > On 31-01-18 13:25, Andy Shevchenko wrote: >> Have you chance a look at the branch I pushed yesterday? > I assume you mean: > > https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi Correct. > This looks very nice and a much better solution then what > Jeremy Cline posted, which is my fault as I suggested that > approach to Jeremy :) NP. > I've not done a full review, only a quick look, from a quick > look this looks good. Especially from a code-design pov. Thanks. I will wait for test results and if everything okay I will submit through a normal process with Cc list including you.
On 01/31/2018 03:58 PM, Hans de Goede wrote: > Hi, > > On 31-01-18 13:25, Andy Shevchenko wrote: >> Have you chance a look at the branch I pushed yesterday? > > I assume you mean: > > https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi > > This looks very nice and a much better solution then what > Jeremy Cline posted, which is my fault as I suggested that > approach to Jeremy :) Seconded, although I've already demonstrated my lack of knowledge :). Thanks for catching my error and fixing it, Andy! Regards, Jeremy -- 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
All, I had a chance to sit back down with the machine. I didn't take it all the way apart - there are pieces that I'm afraid of breaking without directions on how to properly disassemble them. However, I did recover an exact chip ID - the chips in use are BMA255s [1]. Rather than take the machine apart (and because the chips are 2mmx2mm), I queried the chip over SMBus. On page 50 of the below document, you can see that register 0x00 is a read-only chip ID. This chipID is unique per Bosch product. So, using SMBus, I asked the chip for it's chip ID (0xFA, in this case) and then searched likely products until I found the matching chipID. Does this suffice to settle which chips are in use? If not, I can finish taking the machine apart, I'd just prefer to avoid the risk of breaking something. As soon as I finish screwing everything back together, I'll grab the other software IDs asked for and build the branch referenced elsewhere. Steve [1] https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMA255-DS004-05_published.pdf On 01/30/2018 03:12 PM, Andy Shevchenko wrote: > On Tue, Jan 30, 2018 at 9:27 PM, Steven Presser <steve@pressers.name> wrote: >> On 01/30/2018 02:05 PM, Andy Shevchenko wrote: >>> On Tue, Jan 30, 2018 at 8:34 PM, Steven Presser <steve@pressers.name> >>> wrote: >>>> First, I believe the "bmc150" in the subject line is in some way a >>>> misnomer. >>>> You'd have to ask Jeremy for more details on what he intended it to refer >>>> to. However, I believe the device in question is actually the bma250[1], >>>> which does not have a magnetometer component. I'm unfortunately away >>>> from >>>> my notes, but I can check later if you need me to verify the exact chip. >>> Please do, I would really be on the safe side here. >> Will do. My digital notes indicate I worked from what was exposed back to >> what chip matched. If you can give me through Friday evening, I'll crack it >> and do a visual verification. (Alas, I'm traveling and won't be back to it >> until then). > We are in the merge window anyway, so, no hurry. > > I'm looking right now in the clean solution. Looks promising. > >>> Bad, bad Lenovo. (DMI strings might help here) >> What particular DMI strings would be helpful? All of them? > Let's do this way. Create a bug on kernel bugzilla, attach output of > > % acpidump -o tables.dat # tables.dat file > % grep -H 15 /sys/bus/acpi/devices/*/status > % dmidecode > > and share the number here. I will take it. >
Andy, The information is in kernel bug 198671. Steve On 01/30/2018 04:20 PM, Andy Shevchenko wrote: > On Tue, Jan 30, 2018 at 10:12 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Tue, Jan 30, 2018 at 9:27 PM, Steven Presser <steve@pressers.name> wrote: >>> On 01/30/2018 02:05 PM, Andy Shevchenko wrote: >>>> On Tue, Jan 30, 2018 at 8:34 PM, Steven Presser <steve@pressers.name> >>>> wrote: >> Let's do this way. Create a bug on kernel bugzilla, attach output of >> >> % acpidump -o tables.dat # tables.dat file >> % grep -H 15 /sys/bus/acpi/devices/*/status >> % dmidecode >> >> and share the number here. I will take it. > Here [1] is the branch with another approach. Can you test it and tell > if it fixes the issue? > > [1]: https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi > It's based on linux-next + compiletest branch of IIO subsystem. >
On Sun, Feb 4, 2018 at 7:58 PM, Steven Presser <steve@pressers.name> wrote: > I had a chance to sit back down with the machine. I didn't take it all the > way apart - there are pieces that I'm afraid of breaking without directions > on how to properly disassemble them. No need I think to go so-o deep. > However, I did recover an exact chip ID - the chips in use are BMA255s [1]. > Rather than take the machine apart (and because the chips are 2mmx2mm), I > queried the chip over SMBus. On page 50 of the below document, you can see > that register 0x00 is a read-only chip ID. This chipID is unique per Bosch > product. So, using SMBus, I asked the chip for it's chip ID (0xFA, in this > case) and then searched likely products until I found the matching chipID. Thanks! > Does this suffice to settle which chips are in use? If not, I can finish > taking the machine apart, I'd just prefer to avoid the risk of breaking > something. I think it's pretty much enough. > As soon as I finish screwing everything back together, I'll grab the other > software IDs asked for and build the branch referenced elsewhere. I'm just waiting for your Tested-by in case my patch series works as expected before I send it out.
On Sun, Feb 4, 2018 at 8:25 PM, Steven Presser <steve@pressers.name> wrote: > The information is in kernel bug 198671. Steve, any news on testing? Shall I submit the series for official review? >> Here [1] is the branch with another approach. Can you test it and tell >> if it fixes the issue? >> >> [1]: https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi >> It's based on linux-next + compiletest branch of IIO subsystem.
On Thu, Feb 15, 2018 at 8:06 PM, Steve Presser <steve@pressers.name> wrote: > Hi Andy, > > No good news. It looks like the vanilla 4.15 series doesn't boot on this > system. Still working on tracking down why/when it broke/if this is due to a > Debian patch. Just in case: you may cherry-pick necessary patches (4 or 5 from the tip of mentioned branch) to whatever kernel you are using. Though I'm not sure how far v4.15 from v4.12 in a sense of the code it touches. > Would it still be an effective test if I backported the changes to 4.12 (or > 4.12-debian)? I know that series boots. OK, continue waiting for your testing. >> >> Here [1] is the branch with another approach. Can you test it and tell >> >> if it fixes the issue? >> >> >> >> [1]: https://bitbucket.org/andy-shev/linux/branch/topic/iio-acpi >> >> It's based on linux-next + compiletest branch of IIO subsystem.
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c index 870f92ef61c2..7496c5121a8c 100644 --- a/drivers/iio/accel/bmc150-accel-core.c +++ b/drivers/iio/accel/bmc150-accel-core.c @@ -204,6 +204,7 @@ struct bmc150_accel_data { int ev_enable_state; int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */ const struct bmc150_accel_chip_info *chip_info; + struct i2c_client *second_device; }; static const struct { @@ -1659,6 +1660,25 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, } EXPORT_SYMBOL_GPL(bmc150_accel_core_probe); +struct i2c_client *bmc150_get_second_device(struct i2c_client *client) +{ + struct bmc150_accel_data *data = i2c_get_clientdata(client); + + if (!data) + return NULL; + + return data->second_device; +} +EXPORT_SYMBOL_GPL(bmc150_get_second_device); + +void bmc150_set_second_device(struct i2c_client *client) +{ + struct bmc150_accel_data *data = i2c_get_clientdata(client); + if (data) + data->second_device = client; +} +EXPORT_SYMBOL_GPL(bmc150_set_second_device); + int bmc150_accel_core_remove(struct device *dev) { struct iio_dev *indio_dev = dev_get_drvdata(dev); diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c index f85014fbaa12..c7341df086e2 100644 --- a/drivers/iio/accel/bmc150-accel-i2c.c +++ b/drivers/iio/accel/bmc150-accel-i2c.c @@ -31,6 +31,10 @@ static int bmc150_accel_probe(struct i2c_client *client, const struct i2c_device_id *id) { + int ret; + struct acpi_device *adev; + struct i2c_board_info board_info; + struct i2c_client *second_dev; struct regmap *regmap; const char *name = NULL; bool block_supported = @@ -47,12 +51,35 @@ static int bmc150_accel_probe(struct i2c_client *client, if (id) name = id->name; - return bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, + ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, block_supported); + if (ret) + return ret; + + /* + * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI + * device, try instantiating a second i2c_client for an I2cSerialBusV2 + * ACPI resource with index 1. + */ + adev = ACPI_COMPANION(&client->dev); + if (adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { + memset(&board_info, 0, sizeof(board_info)); + strlcpy(board_info.type, "bma250e", I2C_NAME_SIZE); + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); + if (second_dev) + bmc150_set_second_device(second_dev); + } + + return ret; } static int bmc150_accel_remove(struct i2c_client *client) { + struct i2c_client *second_dev = bmc150_get_second_device(client); + + if (second_dev) + i2c_unregister_device(second_dev); + return bmc150_accel_core_remove(&client->dev); } diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h index ae6118ae11b1..6e965a3ca322 100644 --- a/drivers/iio/accel/bmc150-accel.h +++ b/drivers/iio/accel/bmc150-accel.h @@ -16,6 +16,8 @@ enum { int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, const char *name, bool block_supported); int bmc150_accel_core_remove(struct device *dev); +struct i2c_client *bmc150_get_second_device(struct i2c_client *second_device); +void bmc150_set_second_device(struct i2c_client *second_device); extern const struct dev_pm_ops bmc150_accel_pm_ops; extern const struct regmap_config bmc150_regmap_conf;
Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI device. Check for a companion device and handle a second i2c_client if it is present. Signed-off-by: Jeremy Cline <jeremy@jcline.org> --- Changes in v2: - Rather than exposing the bmc150_accel_data struct, use get and set functions. drivers/iio/accel/bmc150-accel-core.c | 20 ++++++++++++++++++++ drivers/iio/accel/bmc150-accel-i2c.c | 29 ++++++++++++++++++++++++++++- drivers/iio/accel/bmc150-accel.h | 2 ++ 3 files changed, 50 insertions(+), 1 deletion(-)