| Message ID | 0100016009e7c30a-407c2980-d8d5-4506-ab47-d0fb2fed481d-000000@email.amazonses.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; -- 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 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?) -- 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
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 -- 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, 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. -- 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 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 -- 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, 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 -- 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, 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?
--
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, 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 -- 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
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;
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(-)