| Message ID | 20201125083618.10989-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member | expand |
On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote: > > From: Jeremy Cline <jeremy@jcline.org> > > Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI > device. Normally we would handle this by letting the special > drivers/platform/x86/i2c-multi-instantiate.c driver handle the BOSC0200 > ACPI id and let it instantiate 2 bmc150_accel type i2c_client-s for us. > > But doing so changes the modalias for the first accelerometer > (which is already supported and used on many devices) from > acpi:BOSC0200 to i2c:bmc150_accel. The modalias is not only used > to load the driver, but is also used by hwdb matches in > /lib/udev/hwdb.d/60-sensor.hwdb which provide a mountmatrix to > userspace by setting the ACCEL_MOUNT_MATRIX udev property. > > Switching the handling of the BOSC0200 over to i2c-multi-instantiate.c > will break the hwdb matches causing the ACCEL_MOUNT_MATRIX udev prop > to no longer be set. So switching over to i2c-multi-instantiate.c is > not an option. I'm wondering if we can meanwhile update hwdb to support i2c-multi-instantiate cases in the future and in a few years switch to it. > Changes by Hans de Goede: > -Add explanation to the commit message why i2c-multi-instantiate.c > cannot be used > -Also set the dev_name, fwnode and irq i2c_board_info struct members > for the 2nd client ... > + 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. The !id check avoids recursion when > + * bmc150_accel_probe() gets called for the second client. > + */ > + if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > + struct i2c_board_info board_info = { > + .type = "bmc150_accel", > + /* The 2nd accel sits in the base of 2-in-1s */ > + .dev_name = "BOSC0200:base", Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion with ACPI device naming schema? (Or was it on purpose?) And this seems to be the only device in the system, second as this is not allowed as far as I understand. Right? But theoretically I can create an ACPI SSDT with quite similar excerpt and sensor and enumerate it via ConfigFS (I understand that is quite unlikely). > + .fwnode = client->dev.fwnode, > + .irq = -ENOENT, > + }; > + struct i2c_client *second_dev; > + > + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); > + if (!IS_ERR(second_dev)) > + bmc150_set_second_device(second_dev); > + } ... > static int bmc150_accel_remove(struct i2c_client *client) > { > + struct i2c_client *second_dev = bmc150_get_second_device(client); > + if (second_dev) Redundant. > + i2c_unregister_device(second_dev); > + > return bmc150_accel_core_remove(&client->dev); > }
Hi, On 11/25/20 11:55 AM, Andy Shevchenko wrote: > On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> From: Jeremy Cline <jeremy@jcline.org> >> >> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI >> device. Normally we would handle this by letting the special >> drivers/platform/x86/i2c-multi-instantiate.c driver handle the BOSC0200 >> ACPI id and let it instantiate 2 bmc150_accel type i2c_client-s for us. >> >> But doing so changes the modalias for the first accelerometer >> (which is already supported and used on many devices) from >> acpi:BOSC0200 to i2c:bmc150_accel. The modalias is not only used >> to load the driver, but is also used by hwdb matches in >> /lib/udev/hwdb.d/60-sensor.hwdb which provide a mountmatrix to >> userspace by setting the ACCEL_MOUNT_MATRIX udev property. >> >> Switching the handling of the BOSC0200 over to i2c-multi-instantiate.c >> will break the hwdb matches causing the ACCEL_MOUNT_MATRIX udev prop >> to no longer be set. So switching over to i2c-multi-instantiate.c is >> not an option. > > I'm wondering if we can meanwhile update hwdb to support > i2c-multi-instantiate cases in the future and in a few years switch to > it. Even if we fix current hwdb entries to match on both, then there is no guarantee newly added entries will also contain the new match. Now with the code to get the matrix from the ACPI tables new entries should happen less often, but I saw at least one model where the ACPI provided matrix appears to be wrong (if the ACPI matrix was always correct then breaking hwdb would not really be an issue). So I don't think this is going to work and all in all it feels like a lot of work for little gain. >> Changes by Hans de Goede: >> -Add explanation to the commit message why i2c-multi-instantiate.c >> cannot be used >> -Also set the dev_name, fwnode and irq i2c_board_info struct members >> for the 2nd client > > ... > >> + 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. The !id check avoids recursion when >> + * bmc150_accel_probe() gets called for the second client. >> + */ > >> + if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > >> + struct i2c_board_info board_info = { >> + .type = "bmc150_accel", >> + /* The 2nd accel sits in the base of 2-in-1s */ > >> + .dev_name = "BOSC0200:base", > > Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion > with ACPI device naming schema? (Or was it on purpose?) So with the ':' the end result is: [root@localhost ~]# cd /sys/bus/i2c/devices/ [root@localhost devices]# ls | cat 6-0050 i2c-0 i2c-1 i2c-2 i2c-3 i2c-4 i2c-5 i2c-6 i2c-BOSC0200:00 i2c-BOSC0200:base i2c-WCOM50BD:00 Which looks nice and consistent, which is why I went with the ':' and since base is not a number, there is no chance on conflicting with ACPI device names (it does look somewhat like an ACPI device name, but it is an ACPI enumerated device, so ...). Anyways if there is a strong preference for changing this to a '.' I would be happy to make this change. > And this seems to be the only device in the system, second as this is > not allowed as far as I understand. Right? I don't understand what you are trying to say here, sorry. > But theoretically I can > create an ACPI SSDT with quite similar excerpt and sensor and > enumerate it via ConfigFS (I understand that is quite unlikely). > >> + .fwnode = client->dev.fwnode, >> + .irq = -ENOENT, >> + }; >> + struct i2c_client *second_dev; >> + >> + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); >> + if (!IS_ERR(second_dev)) >> + bmc150_set_second_device(second_dev); >> + } > > ... > >> static int bmc150_accel_remove(struct i2c_client *client) >> { >> + struct i2c_client *second_dev = bmc150_get_second_device(client); > >> + if (second_dev) > > Redundant. True, I will fix this for v2, once the ':' vs '.' thing is settled. Regards, Hans > >> + i2c_unregister_device(second_dev); >> + >> return bmc150_accel_core_remove(&client->dev); >> } >
On Wed, Nov 25, 2020 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/25/20 11:55 AM, Andy Shevchenko wrote: > > On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote: ... > > I'm wondering if we can meanwhile update hwdb to support > > i2c-multi-instantiate cases in the future and in a few years switch to > > it. > > Even if we fix current hwdb entries to match on both, then there > is no guarantee newly added entries will also contain the new match. > > Now with the code to get the matrix from the ACPI tables new entries > should happen less often, but I saw at least one model where the ACPI > provided matrix appears to be wrong (if the ACPI matrix was always > correct then breaking hwdb would not really be an issue). > > So I don't think this is going to work and all in all it feels like > a lot of work for little gain. Okay! ... > >> + .dev_name = "BOSC0200:base", > > > > Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion > > with ACPI device naming schema? (Or was it on purpose?) > > So with the ':' the end result is: > > [root@localhost ~]# cd /sys/bus/i2c/devices/ > [root@localhost devices]# ls | cat > 6-0050 > i2c-0 > i2c-1 > i2c-2 > i2c-3 > i2c-4 > i2c-5 > i2c-6 > i2c-BOSC0200:00 > i2c-BOSC0200:base > i2c-WCOM50BD:00 > > Which looks nice and consistent, which is why I went with the ':' > and since base is not a number, there is no chance on conflicting with > ACPI device names (it does look somewhat like an ACPI device name, but > it is an ACPI enumerated device, so ...). I see. So this was made on purpose. > Anyways if there is a strong preference for changing this to a '.' > I would be happy to make this change. No strong preferences from my side. > > And this seems to be the only device in the system, second as this is > > not allowed as far as I understand. Right? > > I don't understand what you are trying to say here, sorry. > > > But theoretically I can > > create an ACPI SSDT with quite similar excerpt and sensor and > > enumerate it via ConfigFS (I understand that is quite unlikely). What if you have two devices with the same ID and both have two I2cSerialBusV2() resources? Second one can't be instantiated because 'base' is already here. Making it like i2c-BOSC0200:00.base would be much better in my opinion.
Hi, On 11/25/20 12:20 PM, Andy Shevchenko wrote: > On Wed, Nov 25, 2020 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 11/25/20 11:55 AM, Andy Shevchenko wrote: >>> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>> I'm wondering if we can meanwhile update hwdb to support >>> i2c-multi-instantiate cases in the future and in a few years switch to >>> it. >> >> Even if we fix current hwdb entries to match on both, then there >> is no guarantee newly added entries will also contain the new match. >> >> Now with the code to get the matrix from the ACPI tables new entries >> should happen less often, but I saw at least one model where the ACPI >> provided matrix appears to be wrong (if the ACPI matrix was always >> correct then breaking hwdb would not really be an issue). >> >> So I don't think this is going to work and all in all it feels like >> a lot of work for little gain. > > Okay! > > ... > >>>> + .dev_name = "BOSC0200:base", >>> >>> Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion >>> with ACPI device naming schema? (Or was it on purpose?) >> >> So with the ':' the end result is: >> >> [root@localhost ~]# cd /sys/bus/i2c/devices/ >> [root@localhost devices]# ls | cat >> 6-0050 >> i2c-0 >> i2c-1 >> i2c-2 >> i2c-3 >> i2c-4 >> i2c-5 >> i2c-6 >> i2c-BOSC0200:00 >> i2c-BOSC0200:base >> i2c-WCOM50BD:00 >> >> Which looks nice and consistent, which is why I went with the ':' >> and since base is not a number, there is no chance on conflicting with >> ACPI device names (it does look somewhat like an ACPI device name, but >> it is an ACPI enumerated device, so ...). > > I see. So this was made on purpose. > >> Anyways if there is a strong preference for changing this to a '.' >> I would be happy to make this change. > > No strong preferences from my side. > >>> And this seems to be the only device in the system, second as this is >>> not allowed as far as I understand. Right? >> >> I don't understand what you are trying to say here, sorry. >> >>> But theoretically I can >>> create an ACPI SSDT with quite similar excerpt and sensor and >>> enumerate it via ConfigFS (I understand that is quite unlikely). > > What if you have two devices with the same ID and both have two > I2cSerialBusV2() resources? Second one can't be instantiated because > 'base' is already here. > Making it like i2c-BOSC0200:00.base would be much better in my opinion. Ah I see, that is a somewhat valid point. But I really never expect there to be 2 ACPI devices with a BOSC0200 hw-id, while also specifying more then 1 i2c-client per node. That would just be all kinds of messed-up. Thinking about this I think that getting a WARN_ON (and thus a bug report) about a duplicate kobject-name when this happens would actually be good, because then we need to figure out what the beep is going on on that system. Note that other then triggering a WARN_ON the second i2c_acpi_new_device will simply fail in this very unlikely scenario (I know because I triggered this by accident while working on the patch). Since in a way getting this WARN_ON is actually good (lets us know about completely unexpected circumstances) and that making the name dynamic as you suggest requires a bit of extra code I would actually prefer to keep this as. Please let me know if that is ok with you. Regards, Hans
On Wed, Nov 25, 2020 at 6:09 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/25/20 12:20 PM, Andy Shevchenko wrote: > > On Wed, Nov 25, 2020 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 11/25/20 11:55 AM, Andy Shevchenko wrote: ... > >> i2c-BOSC0200:base > > What if you have two devices with the same ID and both have two > > I2cSerialBusV2() resources? Second one can't be instantiated because > > 'base' is already here. > > Making it like i2c-BOSC0200:00.base would be much better in my opinion. > > Ah I see, that is a somewhat valid point. But I really never expect > there to be 2 ACPI devices with a BOSC0200 hw-id, while also specifying > more then 1 i2c-client per node. That would just be all kinds of messed-up. I also don't expect such, but probability is still greater than zero (somebody may copy'n'paste the ASL excerpt from this device and apply as SSDT in one of DIYs kinda projects). > Thinking about this I think that getting a WARN_ON (and thus a bug report) > about a duplicate kobject-name when this happens would actually be good, > because then we need to figure out what the beep is going on on that > system. Note that other then triggering a WARN_ON the second > i2c_acpi_new_device will simply fail in this very unlikely scenario > (I know because I triggered this by accident while working on the patch). > > Since in a way getting this WARN_ON is actually good (lets us know about > completely unexpected circumstances) and that making the name dynamic > as you suggest requires a bit of extra code I would actually prefer to > keep this as. Please let me know if that is ok with you. Can you put a comment in the code that this name is considered global for now as we do not expect such circumstances. Then I'll be fine.
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c index 088716d55855..2976aefad89b 100644 --- a/drivers/iio/accel/bmc150-accel-core.c +++ b/drivers/iio/accel/bmc150-accel-core.c @@ -203,6 +203,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; struct iio_mount_matrix orientation; }; @@ -1659,6 +1660,26 @@ 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 06021c8685a7..117184159bb6 100644 --- a/drivers/iio/accel/bmc150-accel-i2c.c +++ b/drivers/iio/accel/bmc150-accel-i2c.c @@ -29,6 +29,8 @@ static int bmc150_accel_probe(struct i2c_client *client, i2c_check_functionality(client->adapter, I2C_FUNC_I2C) || i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK); + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + int ret; regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf); if (IS_ERR(regmap)) { @@ -39,12 +41,41 @@ 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, - block_supported); + 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. The !id check avoids recursion when + * bmc150_accel_probe() gets called for the second client. + */ + if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { + struct i2c_board_info board_info = { + .type = "bmc150_accel", + /* The 2nd accel sits in the base of 2-in-1s */ + .dev_name = "BOSC0200:base", + .fwnode = client->dev.fwnode, + .irq = -ENOENT, + }; + struct i2c_client *second_dev; + + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); + if (!IS_ERR(second_dev)) + bmc150_set_second_device(second_dev); + } + + return 0; } 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;