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

[PATCH 2/2] 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>
---
 drivers/iio/accel/bmc150-accel-i2c.c | 33 ++++++++++++++++++++++++++++++++-
 drivers/iio/accel/bmc150-accel.h     |  1 +
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index f85014fbaa12..c4557e18123c 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 bmc150_accel_data *data;
 	struct regmap *regmap;
 	const char *name = NULL;
 	bool block_supported =
@@ -47,12 +51,39 @@ 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) {
+		data = i2c_get_clientdata(client);
+		memset(&board_info, 0, sizeof(board_info));
+		strlcpy(board_info.type, "bma250e", I2C_NAME_SIZE);
+		data->driver_priv = i2c_acpi_new_device(&client->dev,
+							1, &board_info);
+		/*
+		 * Don't check for bosc0200 == NULL since most BOSC0200 ACPI
+		 * devices describe only one i2c_client
+		 */
+	}
+
+	return ret;
 }
 
 static int bmc150_accel_remove(struct i2c_client *client)
 {
+	struct bmc150_accel_data *data = i2c_get_clientdata(client);
+
+	if (data->driver_priv)
+		i2c_unregister_device(data->driver_priv);
+
 	return bmc150_accel_core_remove(&client->dev);
 }
 
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index c38754452883..7f49a09b136f 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -47,6 +47,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;
+	void *driver_priv;
 };
 
 struct regmap;
-- 
2.14.3

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

[Jonathan Cameron]

On Wed, 29 Nov 2017 22:31:12 +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.

+ Mika and Wolfram - please cc them on anything odd and i2c / ACPI related.
(I like to share the pain)

My usual question, just out of curiosity as we have to cope with this
fun anyway.  Are you actually allowed to do this under the ACPI spec
or not?  I would assume an acpi device is supposed to be just that A
device...  I fall asleep every time I try to read that spec ;)

Ah well. Rant over :)  Oh for the server world where mostly you just
send a WTF to the bios writer and they fix it.

So how to do this cleanly.. hmm.

One minor comment inline.  Don't hide what we are doing with the
private data member in the structure.  There is no reason to not
give it a type.

At least this one is a reasonably small hack ;)

Jonathan
> 
> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
> ---
>  drivers/iio/accel/bmc150-accel-i2c.c | 33 ++++++++++++++++++++++++++++++++-
>  drivers/iio/accel/bmc150-accel.h     |  1 +
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> index f85014fbaa12..c4557e18123c 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 bmc150_accel_data *data;
>  	struct regmap *regmap;
>  	const char *name = NULL;
>  	bool block_supported =
> @@ -47,12 +51,39 @@ 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) {
> +		data = i2c_get_clientdata(client);
> +		memset(&board_info, 0, sizeof(board_info));
> +		strlcpy(board_info.type, "bma250e", I2C_NAME_SIZE);
> +		data->driver_priv = i2c_acpi_new_device(&client->dev,
> +							1, &board_info);
> +		/*
> +		 * Don't check for bosc0200 == NULL since most BOSC0200 ACPI
> +		 * devices describe only one i2c_client
> +		 */
> +	}
> +
> +	return ret;
>  }
>  
>  static int bmc150_accel_remove(struct i2c_client *client)
>  {
> +	struct bmc150_accel_data *data = i2c_get_clientdata(client);
> +
> +	if (data->driver_priv)
> +		i2c_unregister_device(data->driver_priv);
> +
>  	return bmc150_accel_core_remove(&client->dev);
>  }
>  
> diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> index c38754452883..7f49a09b136f 100644
> --- a/drivers/iio/accel/bmc150-accel.h
> +++ b/drivers/iio/accel/bmc150-accel.h
> @@ -47,6 +47,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;
> +	void *driver_priv;

I'd be explicit about what this is rather than just calling it driver_priv.

Also patch 1 was entirely to expose this data element.  Adding simple
bmc150_get_second_device() / bcm150_set_second_device call would avoid that.

>  };
>  
>  struct regmap;

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

[Mika Westerberg]

On Sat, Dec 02, 2017 at 12:19:27PM +0000, Jonathan Cameron wrote:
> On Wed, 29 Nov 2017 22:31:12 +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.
> 
> + Mika and Wolfram - please cc them on anything odd and i2c / ACPI related.
> (I like to share the pain)
> 
> My usual question, just out of curiosity as we have to cope with this
> fun anyway.  Are you actually allowed to do this under the ACPI spec
> or not?  I would assume an acpi device is supposed to be just that A
> device...  I fall asleep every time I try to read that spec ;)

Yes, it is allowed. Typically you have an ACPI device and it can have
multiple I2cSerialBus() connections.

Linux ACPI/I2C core then picks the first one and creates i2c_client from
that but the additional connections need to be created by the driver in
question.

BTW, there is a function i2c_new_secondary_device() that is supposed to
be used for this but it does not have ACPI support yet (maybe it is good
time to add it now, with this patch series?)

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

[Hans de Goede]

Hi,

On 04-12-17 10:58, Mika Westerberg wrote:
> On Sat, Dec 02, 2017 at 12:19:27PM +0000, Jonathan Cameron wrote:
>> On Wed, 29 Nov 2017 22:31:12 +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.
>>
>> + Mika and Wolfram - please cc them on anything odd and i2c / ACPI related.
>> (I like to share the pain)
>>
>> My usual question, just out of curiosity as we have to cope with this
>> fun anyway.  Are you actually allowed to do this under the ACPI spec
>> or not?  I would assume an acpi device is supposed to be just that A
>> device...  I fall asleep every time I try to read that spec ;)
> 
> Yes, it is allowed. Typically you have an ACPI device and it can have
> multiple I2cSerialBus() connections.
> 
> Linux ACPI/I2C core then picks the first one and creates i2c_client from
> that but the additional connections need to be created by the driver in
> question.
> 
> BTW, there is a function i2c_new_secondary_device() that is supposed to
> be used for this but it does not have ACPI support yet (maybe it is good
> time to add it now, with this patch series?)

i2c_new_secondary_device() is for a different purpose, this is for when
a single i2c device listens on multiple addresses and the driver wants
separate i2c_client-s to use to talk to each address.

In this case there are 2 separate devices, not a single device listening
on multiple addresses.  Something like i2c_new_secondary_device() ACPI
support might be useful for i2c devices where a single device / "IC" listens
on multiple addresses and all these addresses are listed in the ACPI resource
table, but not for this specific case.

Regards,

Hans

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

[Mika Westerberg]

On Mon, Dec 04, 2017 at 11:29:31AM +0100, Hans de Goede wrote:
> i2c_new_secondary_device() is for a different purpose, this is for when
> a single i2c device listens on multiple addresses and the driver wants
> separate i2c_client-s to use to talk to each address.
> 
> In this case there are 2 separate devices, not a single device listening
> on multiple addresses.  Something like i2c_new_secondary_device() ACPI
> support might be useful for i2c devices where a single device / "IC" listens
> on multiple addresses and all these addresses are listed in the ACPI resource
> table, but not for this specific case.

Right, thanks Hans for correcting me.

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

[Jonathan Cameron]

On Mon, 4 Dec 2017 11:58:19 +0200
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Sat, Dec 02, 2017 at 12:19:27PM +0000, Jonathan Cameron wrote:
> > On Wed, 29 Nov 2017 22:31:12 +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.  
> > 
> > + Mika and Wolfram - please cc them on anything odd and i2c / ACPI related.
> > (I like to share the pain)
> > 
> > My usual question, just out of curiosity as we have to cope with this
> > fun anyway.  Are you actually allowed to do this under the ACPI spec
> > or not?  I would assume an acpi device is supposed to be just that A
> > device...  I fall asleep every time I try to read that spec ;)  
> 
> Yes, it is allowed. Typically you have an ACPI device and it can have
> multiple I2cSerialBus() connections.
> 
> Linux ACPI/I2C core then picks the first one and creates i2c_client from
> that but the additional connections need to be created by the driver in
> question.

Why does it not make sense to just create them all from the ACPI/I2C core?

> 
> BTW, there is a function i2c_new_secondary_device() that is supposed to
> be used for this but it does not have ACPI support yet (maybe it is good
> time to add it now, with this patch series?)
> --
> 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 2/2] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Mika Westerberg]

On Tue, Dec 05, 2017 at 11:27:38AM +0000, Jonathan Cameron wrote:
> Why does it not make sense to just create them all from the ACPI/I2C core?

How do you know in ACPI/I2C core what is the right thing to do? Is it a
single device, like EEPROM with multiple addresses, or is it multiple
completely separate devices like in case of many sensors?

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

[Jonathan Cameron]

On Tue, 5 Dec 2017 13:38:01 +0200
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Tue, Dec 05, 2017 at 11:27:38AM +0000, Jonathan Cameron wrote:
> > Why does it not make sense to just create them all from the ACPI/I2C core?  
> 
> How do you know in ACPI/I2C core what is the right thing to do? Is it a
> single device, like EEPROM with multiple addresses, or is it multiple
> completely separate devices like in case of many sensors?

Fine, though this seems like a flaw in the ACPI description as it
isn't possible to tell the difference.  Why it allows on ACPI description
for multiple devices in one ACPI device is beyond me...

More ACPI specific driver code that may eventually end up in every
driver.  Goody. Perhaps we can define a helper function to at least
make this trivial and minimize the burden. Ultimately if this happens
enough we could probably figure out how to move it into the I2C core
entirely - flag in the i2c_driver structure for example.

Jonathan

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

[Jeremy Cline]

On Sat, Dec 02, 2017 at 12:19:27PM +0000, Jonathan Cameron wrote:
> > diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> > index c38754452883..7f49a09b136f 100644
> > --- a/drivers/iio/accel/bmc150-accel.h
> > +++ b/drivers/iio/accel/bmc150-accel.h
> > @@ -47,6 +47,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;
> > +	void *driver_priv;
> 
> I'd be explicit about what this is rather than just calling it driver_priv.

Ah, okay. I was worried about putting i2c-specific stuff in there.

> Also patch 1 was entirely to expose this data element.  Adding simple
> bmc150_get_second_device() / bcm150_set_second_device call would avoid that.

That hadn't occurred to me. I'll take a look at doing it that way.

Thanks for the feedback!

Regards,
Jeremy

[PATCH 1/2] iio: accel: bmc150: Move struct definitions into the header

[Jeremy Cline]

The I2C driver needs to access the bmc150_accel_data struct defined in
bcm150-accel-core.c, so move it and the structs it requires into the
header file.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
---
 drivers/iio/accel/bmc150-accel-core.c | 43 --------------------------------
 drivers/iio/accel/bmc150-accel.h      | 47 +++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 870f92ef61c2..340914677322 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -163,49 +163,6 @@ struct bmc150_accel_chip_info {
 	const struct bmc150_scale_info scale_table[4];
 };
 
-struct bmc150_accel_interrupt {
-	const struct bmc150_accel_interrupt_info *info;
-	atomic_t users;
-};
-
-struct bmc150_accel_trigger {
-	struct bmc150_accel_data *data;
-	struct iio_trigger *indio_trig;
-	int (*setup)(struct bmc150_accel_trigger *t, bool state);
-	int intr;
-	bool enabled;
-};
-
-enum bmc150_accel_interrupt_id {
-	BMC150_ACCEL_INT_DATA_READY,
-	BMC150_ACCEL_INT_ANY_MOTION,
-	BMC150_ACCEL_INT_WATERMARK,
-	BMC150_ACCEL_INTERRUPTS,
-};
-
-enum bmc150_accel_trigger_id {
-	BMC150_ACCEL_TRIGGER_DATA_READY,
-	BMC150_ACCEL_TRIGGER_ANY_MOTION,
-	BMC150_ACCEL_TRIGGERS,
-};
-
-struct bmc150_accel_data {
-	struct regmap *regmap;
-	int irq;
-	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
-	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
-	struct mutex mutex;
-	u8 fifo_mode, watermark;
-	s16 buffer[8];
-	u8 bw_bits;
-	u32 slope_dur;
-	u32 slope_thres;
-	u32 range;
-	int ev_enable_state;
-	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
-	const struct bmc150_accel_chip_info *chip_info;
-};
-
 static const struct {
 	int val;
 	int val2;
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index ae6118ae11b1..c38754452883 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -2,6 +2,53 @@
 #ifndef _BMC150_ACCEL_H_
 #define _BMC150_ACCEL_H_
 
+struct bmc150_accel_chip_info;
+struct bmc150_accel_data;
+struct bmc150_accel_interrupt_info;
+
+struct bmc150_accel_interrupt {
+	const struct bmc150_accel_interrupt_info *info;
+	atomic_t users;
+};
+
+struct bmc150_accel_trigger {
+	struct bmc150_accel_data *data;
+	struct iio_trigger *indio_trig;
+	int (*setup)(struct bmc150_accel_trigger *t, bool state);
+	int intr;
+	bool enabled;
+};
+
+enum bmc150_accel_interrupt_id {
+	BMC150_ACCEL_INT_DATA_READY,
+	BMC150_ACCEL_INT_ANY_MOTION,
+	BMC150_ACCEL_INT_WATERMARK,
+	BMC150_ACCEL_INTERRUPTS,
+};
+
+enum bmc150_accel_trigger_id {
+	BMC150_ACCEL_TRIGGER_DATA_READY,
+	BMC150_ACCEL_TRIGGER_ANY_MOTION,
+	BMC150_ACCEL_TRIGGERS,
+};
+
+struct bmc150_accel_data {
+	struct regmap *regmap;
+	int irq;
+	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
+	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
+	struct mutex mutex;
+	u8 fifo_mode, watermark;
+	s16 buffer[8];
+	u8 bw_bits;
+	u32 slope_dur;
+	u32 slope_thres;
+	u32 range;
+	int ev_enable_state;
+	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
+	const struct bmc150_accel_chip_info *chip_info;
+};
+
 struct regmap;
 
 enum {
-- 
2.14.3