[PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jeremy Cline]

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(-)

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;
 
-- 
2.14.3

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jonathan Cameron]

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;
>  

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jeremy Cline]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jonathan Cameron]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jonathan Cameron]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jonathan Cameron]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[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



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jonathan Cameron]

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  
> 
> 
> 

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jonathan Cameron]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Steven Presser]

[-- Attachment #1: Type: text/plain, Size: 4495 bytes --]

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
>
>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4370 bytes --]

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Steven Presser]

[-- Attachment #1: Type: text/plain, Size: 3844 bytes --]


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



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4370 bytes --]

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jonathan Cameron]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Steven Presser]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

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.
>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4370 bytes --]

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Steven Presser]

[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]

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.
>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4370 bytes --]

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Hans de Goede]

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
> 
> 
> 

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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 */
>
>                 }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Hans de Goede]

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 */
>>
>>                  }
> 

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jeremy Cline]

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

Re: [PATCH v2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Jeremy Cline]

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