| Message ID | 20180808083006.31919-5-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | i2c-multi-instantiate pseudo driver | expand |
On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote: > On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node > per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1 > i2c-device. > > But in some rare cases the manufacturer has decided to describe multiple > i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources. > > An earlier attempt to fix this in the i2c-core resulted in a lot of extra > code to support this corner-case. > > This commit introduces a new i2c-multi-instantiate driver which fixes this > in a different way. This new driver can be built as a module which will > only loaded on affected systems. > > This driver will instantiate a new i2c-client per I2cSerialBus resource, > using the driver_data from the acpi_device_id it is binding to to tell it > which chip-type (and optional irq-resource) to use when instantiating. > > Note this driver depends on a platform device being instantiated for the > ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in > drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). Thanks for an update! My comments below. > +struct i2c_inst_data { > + const char *type; > + int irq_idx; > +}; > +struct i2c_multi_inst_data { > + int no_clients; Name a bit confusing. What about num_clients? > + struct i2c_client *clients[0]; > +}; > + > +static int i2c_multi_inst_probe(struct platform_device *pdev) > +{ > + struct i2c_multi_inst_data *multi; > + const struct acpi_device_id *match; > + const struct i2c_inst_data *inst_data; > + struct i2c_board_info board_info = {}; > + struct device *dev = &pdev->dev; > + struct acpi_device *adev; > + char name[32]; > + int i, ret; > + > + match = acpi_match_device(dev->driver->acpi_match_table, dev); > + if (!match) { > + dev_err(dev, "Error ACPI match data is missing\n"); > + return -ENODEV; > + } > + inst_data = (const struct i2c_inst_data *)match->driver_data; > + > + adev = ACPI_COMPANION(dev); > + > + /* Count number of clients to instantiate */ > + for (i = 0; inst_data[i].type; i++) {} > + > + multi = devm_kmalloc(dev, > + offsetof(struct i2c_multi_inst_data, clients[i]), > + GFP_KERNEL); > + if (!multi) > + return -ENOMEM; Here I see the following: - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof() would be more understandable - there is no guard against i == 0 To solve both, it might be like struct i2c_multi_inst_data { int num_clients; struct i2c_client *clients; }; ... multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL); if (!multi) return -ENOMEM; multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL); if (ZERO_PTR_OR_NULL(multi->clients)) return -ENOMEM; But I would like to hear your (other's) opinion(s). > + > + multi->no_clients = i; > + > + for (i = 0; i < multi->no_clients; i++) { > + memset(&board_info, 0, sizeof(board_info)); > + strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE); > + snprintf(name, sizeof(name), "%s-%s", match->id, > + inst_data[i].type); > + board_info.dev_name = name; > + board_info.irq = 0; > + if (inst_data[i].irq_idx != -1) { >= 0 sounds more robust > + ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); > + if (ret < 0) { > + dev_err(dev, "Error requesting irq at index %d: %d\n", > + inst_data[i].irq_idx, ret); irq -> IRQ in the message. > + goto error; > + } > + board_info.irq = ret; > + } > + multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info); > + if (!multi->clients[i]) { > + dev_err(dev, "Error creating i2c-client, idx %d\n", i); > + ret = -ENODEV; > + goto error; > + } > + } > + > + platform_set_drvdata(pdev, multi); > + return 0; > + > +error: > + while (--i >= 0) It can be simple while (i--) > + i2c_unregister_device(multi->clients[i]); > + > + return ret; > +} > + > +static int i2c_multi_inst_remove(struct platform_device *pdev) > +{ > + struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < multi->no_clients; i++) > + i2c_unregister_device(multi->clients[i]); > + > + return 0; > +} > + > +static const struct i2c_inst_data bsg1160_data[] = { > + { "bmc150_accel", 0 }, > + { "bmc150_magn", -1 }, > + { "bmg160", -1 }, > + {} > +}; > + > +/* > + * Note new device-ids must also be added to i2c_multi_instantiate_ids in > + * drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). > + */ > +static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { > + { "BSG1160", (unsigned long)bsg1160_data }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids); > + > +static struct platform_driver i2c_multi_inst_driver = { > + .driver = { > + .name = "I2C multi instantiate pseudo device driver", > + .acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids), We don't need ACPI_PTR for the driver which depends on ACPI. In the general case we have an inconsistency with variable definition (might be unused). > + }, > + .probe = i2c_multi_inst_probe, > + .remove = i2c_multi_inst_remove, > +}; > +module_platform_driver(i2c_multi_inst_driver); > + > +MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver"); > +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); > +MODULE_LICENSE("GPL"); > -- > 2.18.0 >
On 2018-08-08 11:08, Andy Shevchenko wrote: > On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node >> per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1 >> i2c-device. >> >> But in some rare cases the manufacturer has decided to describe multiple >> i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources. >> >> An earlier attempt to fix this in the i2c-core resulted in a lot of extra >> code to support this corner-case. >> >> This commit introduces a new i2c-multi-instantiate driver which fixes this >> in a different way. This new driver can be built as a module which will >> only loaded on affected systems. >> >> This driver will instantiate a new i2c-client per I2cSerialBus resource, >> using the driver_data from the acpi_device_id it is binding to to tell it >> which chip-type (and optional irq-resource) to use when instantiating. >> >> Note this driver depends on a platform device being instantiated for the >> ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in >> drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). > > Thanks for an update! My comments below. > >> +struct i2c_inst_data { >> + const char *type; >> + int irq_idx; >> +}; > >> +struct i2c_multi_inst_data { > >> + int no_clients; > > Name a bit confusing. What about num_clients? > >> + struct i2c_client *clients[0]; >> +}; >> + >> +static int i2c_multi_inst_probe(struct platform_device *pdev) >> +{ >> + struct i2c_multi_inst_data *multi; >> + const struct acpi_device_id *match; >> + const struct i2c_inst_data *inst_data; >> + struct i2c_board_info board_info = {}; >> + struct device *dev = &pdev->dev; >> + struct acpi_device *adev; >> + char name[32]; >> + int i, ret; >> + >> + match = acpi_match_device(dev->driver->acpi_match_table, dev); >> + if (!match) { >> + dev_err(dev, "Error ACPI match data is missing\n"); >> + return -ENODEV; >> + } >> + inst_data = (const struct i2c_inst_data *)match->driver_data; >> + >> + adev = ACPI_COMPANION(dev); >> + > >> + /* Count number of clients to instantiate */ >> + for (i = 0; inst_data[i].type; i++) {} >> + >> + multi = devm_kmalloc(dev, >> + offsetof(struct i2c_multi_inst_data, clients[i]), >> + GFP_KERNEL); >> + if (!multi) >> + return -ENOMEM; > > Here I see the following: > - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof() > would be more understandable > - there is no guard against i == 0 I don't see why a guard is needed? *Your* code below needs it, but that issue is not a concern for the original code. It might however be a good idea to fail the probe if there are no clients to instantiate, but that's a different issue... > > To solve both, it might be like > > struct i2c_multi_inst_data { > int num_clients; > struct i2c_client *clients; > }; > > ... > multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL); > if (!multi) > return -ENOMEM; > > multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL); > if (ZERO_PTR_OR_NULL(multi->clients)) > return -ENOMEM; > > But I would like to hear your (other's) opinion(s). I think using two allocations is a waste in this case. > >> + >> + multi->no_clients = i; >> + >> + for (i = 0; i < multi->no_clients; i++) { >> + memset(&board_info, 0, sizeof(board_info)); >> + strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE); >> + snprintf(name, sizeof(name), "%s-%s", match->id, >> + inst_data[i].type); >> + board_info.dev_name = name; >> + board_info.irq = 0; > >> + if (inst_data[i].irq_idx != -1) { > >> = 0 sounds more robust But not as flexible/future-proof. Why should 0 be the only valid IRQ index? Cheers, Peter >> + ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); >> + if (ret < 0) { > >> + dev_err(dev, "Error requesting irq at index %d: %d\n", >> + inst_data[i].irq_idx, ret); > > irq -> IRQ in the message. > >> + goto error; >> + } >> + board_info.irq = ret; >> + } >> + multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info); >> + if (!multi->clients[i]) { >> + dev_err(dev, "Error creating i2c-client, idx %d\n", i); >> + ret = -ENODEV; >> + goto error; >> + } >> + } >> + >> + platform_set_drvdata(pdev, multi); >> + return 0; >> + >> +error: > >> + while (--i >= 0) > > It can be simple > > while (i--) > >> + i2c_unregister_device(multi->clients[i]); >> + >> + return ret; >> +} >> + >> +static int i2c_multi_inst_remove(struct platform_device *pdev) >> +{ >> + struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev); >> + int i; >> + >> + for (i = 0; i < multi->no_clients; i++) >> + i2c_unregister_device(multi->clients[i]); >> + >> + return 0; >> +} >> + >> +static const struct i2c_inst_data bsg1160_data[] = { >> + { "bmc150_accel", 0 }, >> + { "bmc150_magn", -1 }, >> + { "bmg160", -1 }, >> + {} >> +}; >> + >> +/* >> + * Note new device-ids must also be added to i2c_multi_instantiate_ids in >> + * drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). >> + */ >> +static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { >> + { "BSG1160", (unsigned long)bsg1160_data }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids); >> + >> +static struct platform_driver i2c_multi_inst_driver = { >> + .driver = { >> + .name = "I2C multi instantiate pseudo device driver", > >> + .acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids), > > We don't need ACPI_PTR for the driver which depends on ACPI. > In the general case we have an inconsistency with variable definition > (might be unused). > >> + }, >> + .probe = i2c_multi_inst_probe, >> + .remove = i2c_multi_inst_remove, >> +}; >> +module_platform_driver(i2c_multi_inst_driver); >> + >> +MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver"); >> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.18.0 >> > > >
On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@axentia.se> wrote: > On 2018-08-08 11:08, Andy Shevchenko wrote: >> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> + /* Count number of clients to instantiate */ >>> + for (i = 0; inst_data[i].type; i++) {} >>> + >>> + multi = devm_kmalloc(dev, >>> + offsetof(struct i2c_multi_inst_data, clients[i]), >>> + GFP_KERNEL); >>> + if (!multi) >>> + return -ENOMEM; >> >> Here I see the following: >> - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof() >> would be more understandable >> - there is no guard against i == 0 > > I don't see why a guard is needed? Because there is no point to have a module loaded when there is none client to serve. > *Your* code below needs it, but that > issue is not a concern for the original code. I can admit that's not a big deal, just making logic slightly more robust. > It might however be a > good idea to fail the probe if there are no clients to instantiate, but > that's a different issue... That's what I have in mind. >> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL); >> if (!multi) >> return -ENOMEM; >> >> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL); >> if (ZERO_PTR_OR_NULL(multi->clients)) >> return -ENOMEM; >> >> But I would like to hear your (other's) opinion(s). > > I think using two allocations is a waste in this case. On the other hand it makes code more readable. With offsetof() it is a bit hard to get it on the first glance. >>> + if (inst_data[i].irq_idx != -1) { >> >>> = 0 sounds more robust > > But not as flexible/future-proof. Why should 0 be the only valid IRQ index? Ah, because > is used usually is a quoting character in email you missed the point. It was written as >= 0.
On 2018-08-08 12:01, Andy Shevchenko wrote: > On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@axentia.se> wrote: >> On 2018-08-08 11:08, Andy Shevchenko wrote: >>> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote: > >>>> + /* Count number of clients to instantiate */ >>>> + for (i = 0; inst_data[i].type; i++) {} >>>> + >>>> + multi = devm_kmalloc(dev, >>>> + offsetof(struct i2c_multi_inst_data, clients[i]), >>>> + GFP_KERNEL); >>>> + if (!multi) >>>> + return -ENOMEM; >>> >>> Here I see the following: >>> - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof() >>> would be more understandable >>> - there is no guard against i == 0 >> >> I don't see why a guard is needed? > > Because there is no point to have a module loaded when there is none > client to serve. > >> *Your* code below needs it, but that >> issue is not a concern for the original code. > > I can admit that's not a big deal, just making logic slightly more robust. > >> It might however be a >> good idea to fail the probe if there are no clients to instantiate, but >> that's a different issue... > > That's what I have in mind. Ah, but there is no reason what-so-ever for i being zero. The whole point of the driver is for cases where i > 1. Or to put it bluntly, anyone defining a struct i2c_inst_data with zero entries deserves to be punished... >>> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL); >>> if (!multi) >>> return -ENOMEM; >>> >>> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL); >>> if (ZERO_PTR_OR_NULL(multi->clients)) >>> return -ENOMEM; >>> >>> But I would like to hear your (other's) opinion(s). >> >> I think using two allocations is a waste in this case. > > On the other hand it makes code more readable. With offsetof() it is a > bit hard to get it on the first glance. The driver is tiny, I think it's good if there is at least one thing that can be a little bit interesting :-) >>>> + if (inst_data[i].irq_idx != -1) { >>> >>>> = 0 sounds more robust >> >> But not as flexible/future-proof. Why should 0 be the only valid IRQ index? > > Ah, because > is used usually is a quoting character in email you > missed the point. > It was written as >= 0. Ahh, good catch, that explains it. Cheers, Peter
Hi, On 08/08/2018 11:08 AM, Andy Shevchenko wrote: > On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node >> per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1 >> i2c-device. >> >> But in some rare cases the manufacturer has decided to describe multiple >> i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources. >> >> An earlier attempt to fix this in the i2c-core resulted in a lot of extra >> code to support this corner-case. >> >> This commit introduces a new i2c-multi-instantiate driver which fixes this >> in a different way. This new driver can be built as a module which will >> only loaded on affected systems. >> >> This driver will instantiate a new i2c-client per I2cSerialBus resource, >> using the driver_data from the acpi_device_id it is binding to to tell it >> which chip-type (and optional irq-resource) to use when instantiating. >> >> Note this driver depends on a platform device being instantiated for the >> ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in >> drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). > > Thanks for an update! My comments below. > >> +struct i2c_inst_data { >> + const char *type; >> + int irq_idx; >> +}; > >> +struct i2c_multi_inst_data { > >> + int no_clients; > > Name a bit confusing. What about num_clients? no is often used as abbreviation for "number of" so this is a quite normal naming scheme. But if you really want me to I can do a v5 renaming this to num_clients. > >> + struct i2c_client *clients[0]; >> +}; >> + >> +static int i2c_multi_inst_probe(struct platform_device *pdev) >> +{ >> + struct i2c_multi_inst_data *multi; >> + const struct acpi_device_id *match; >> + const struct i2c_inst_data *inst_data; >> + struct i2c_board_info board_info = {}; >> + struct device *dev = &pdev->dev; >> + struct acpi_device *adev; >> + char name[32]; >> + int i, ret; >> + >> + match = acpi_match_device(dev->driver->acpi_match_table, dev); >> + if (!match) { >> + dev_err(dev, "Error ACPI match data is missing\n"); >> + return -ENODEV; >> + } >> + inst_data = (const struct i2c_inst_data *)match->driver_data; >> + >> + adev = ACPI_COMPANION(dev); >> + > >> + /* Count number of clients to instantiate */ >> + for (i = 0; inst_data[i].type; i++) {} >> + >> + multi = devm_kmalloc(dev, >> + offsetof(struct i2c_multi_inst_data, clients[i]), >> + GFP_KERNEL); >> + if (!multi) >> + return -ENOMEM; > > Here I see the following: > - it's kinda unusual use of offsetof() There are actually plenty of places doing this, this is the normal way to get the size of a struct which ends with a variable sized array. I would prefer to keep this as is. , perhaps i*sizeof() + sizeof() > would be more understandable > - there is no guard against i == 0 i depends on the driver_data, defining a driver_data where i == 0 is silly, still if this happens nothing bad will happen, so I see no need for a check for this. > > To solve both, it might be like > > struct i2c_multi_inst_data { > int num_clients; > struct i2c_client *clients; > }; > > ... > multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL); > if (!multi) > return -ENOMEM; > > multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL); > if (ZERO_PTR_OR_NULL(multi->clients)) > return -ENOMEM; > > But I would like to hear your (other's) opinion(s). Nack for this option, this just makes the code more complicated and uses allocs instead of 1 for no good reason IMHO. Regards, Hans > >> + >> + multi->no_clients = i; >> + >> + for (i = 0; i < multi->no_clients; i++) { >> + memset(&board_info, 0, sizeof(board_info)); >> + strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE); >> + snprintf(name, sizeof(name), "%s-%s", match->id, >> + inst_data[i].type); >> + board_info.dev_name = name; >> + board_info.irq = 0; > >> + if (inst_data[i].irq_idx != -1) { > >> = 0 sounds more robust > >> + ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); >> + if (ret < 0) { > >> + dev_err(dev, "Error requesting irq at index %d: %d\n", >> + inst_data[i].irq_idx, ret); > > irq -> IRQ in the message. > >> + goto error; >> + } >> + board_info.irq = ret; >> + } >> + multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info); >> + if (!multi->clients[i]) { >> + dev_err(dev, "Error creating i2c-client, idx %d\n", i); >> + ret = -ENODEV; >> + goto error; >> + } >> + } >> + >> + platform_set_drvdata(pdev, multi); >> + return 0; >> + >> +error: > >> + while (--i >= 0) > > It can be simple > > while (i--) > >> + i2c_unregister_device(multi->clients[i]); >> + >> + return ret; >> +} >> + >> +static int i2c_multi_inst_remove(struct platform_device *pdev) >> +{ >> + struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev); >> + int i; >> + >> + for (i = 0; i < multi->no_clients; i++) >> + i2c_unregister_device(multi->clients[i]); >> + >> + return 0; >> +} >> + >> +static const struct i2c_inst_data bsg1160_data[] = { >> + { "bmc150_accel", 0 }, >> + { "bmc150_magn", -1 }, >> + { "bmg160", -1 }, >> + {} >> +}; >> + >> +/* >> + * Note new device-ids must also be added to i2c_multi_instantiate_ids in >> + * drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). >> + */ >> +static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { >> + { "BSG1160", (unsigned long)bsg1160_data }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids); >> + >> +static struct platform_driver i2c_multi_inst_driver = { >> + .driver = { >> + .name = "I2C multi instantiate pseudo device driver", > >> + .acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids), > > We don't need ACPI_PTR for the driver which depends on ACPI. > In the general case we have an inconsistency with variable definition > (might be unused). > >> + }, >> + .probe = i2c_multi_inst_probe, >> + .remove = i2c_multi_inst_remove, >> +}; >> +module_platform_driver(i2c_multi_inst_driver); >> + >> +MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver"); >> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.18.0 >> > > >
$subject still says "i2c:"
Hi, On 08-08-18 19:00, Wolfram Sang wrote: > > $subject still says "i2c:" Ah, good point I will send a v5 with this fixed, I will also do s/no_clients/num_clients/ for v5 as Andy requested. Regards, Hans
diff --git a/MAINTAINERS b/MAINTAINERS index 9b377508f24f..dbe7836e4f6b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -367,6 +367,12 @@ L: linux-acpi@vger.kernel.org S: Maintained F: drivers/acpi/arm64 +ACPI I2C MULTI INSTANTIATE DRIVER +M: Hans de Goede <hdegoede@redhat.com> +L: platform-driver-x86@vger.kernel.org +S: Maintained +F: drivers/platform/x86/i2c-multi-instantiate.c + ACPI PMIC DRIVERS M: "Rafael J. Wysocki" <rjw@rjwysocki.net> M: Len Brown <lenb@kernel.org> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 85a93453237c..64c82592d4b6 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1219,6 +1219,17 @@ config INTEL_CHTDC_TI_PWRBTN To compile this driver as a module, choose M here: the module will be called intel_chtdc_ti_pwrbtn. +config I2C_MULTI_INSTANTIATE + tristate "I2C multi instantiate pseudo device driver" + depends on I2C && ACPI + help + Some ACPI-based systems list multiple i2c-devices in a single ACPI + firmware-node. This driver will instantiate separate i2c-clients + for each device in the firmware-node. + + To compile this driver as a module, choose M here: the module + will be called i2c-multi-instantiate. + endif # X86_PLATFORM_DEVICES config PMC_ATOM diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 8d9477114fb5..e6d1becf81ce 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -91,3 +91,4 @@ obj-$(CONFIG_PMC_ATOM) += pmc_atom.o obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN) += intel_chtdc_ti_pwrbtn.o +obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c new file mode 100644 index 000000000000..4db4b8cabfc9 --- /dev/null +++ b/drivers/platform/x86/i2c-multi-instantiate.c @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * I2C multi-instantiate driver, pseudo driver to instantiate multiple + * i2c-clients from a single fwnode. + * + * Copyright 2018 Hans de Goede <hdegoede@redhat.com> + */ + +#include <linux/acpi.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> + +struct i2c_inst_data { + const char *type; + int irq_idx; +}; + +struct i2c_multi_inst_data { + int no_clients; + struct i2c_client *clients[0]; +}; + +static int i2c_multi_inst_probe(struct platform_device *pdev) +{ + struct i2c_multi_inst_data *multi; + const struct acpi_device_id *match; + const struct i2c_inst_data *inst_data; + struct i2c_board_info board_info = {}; + struct device *dev = &pdev->dev; + struct acpi_device *adev; + char name[32]; + int i, ret; + + match = acpi_match_device(dev->driver->acpi_match_table, dev); + if (!match) { + dev_err(dev, "Error ACPI match data is missing\n"); + return -ENODEV; + } + inst_data = (const struct i2c_inst_data *)match->driver_data; + + adev = ACPI_COMPANION(dev); + + /* Count number of clients to instantiate */ + for (i = 0; inst_data[i].type; i++) {} + + multi = devm_kmalloc(dev, + offsetof(struct i2c_multi_inst_data, clients[i]), + GFP_KERNEL); + if (!multi) + return -ENOMEM; + + multi->no_clients = i; + + for (i = 0; i < multi->no_clients; i++) { + memset(&board_info, 0, sizeof(board_info)); + strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE); + snprintf(name, sizeof(name), "%s-%s", match->id, + inst_data[i].type); + board_info.dev_name = name; + board_info.irq = 0; + if (inst_data[i].irq_idx != -1) { + ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); + if (ret < 0) { + dev_err(dev, "Error requesting irq at index %d: %d\n", + inst_data[i].irq_idx, ret); + goto error; + } + board_info.irq = ret; + } + multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info); + if (!multi->clients[i]) { + dev_err(dev, "Error creating i2c-client, idx %d\n", i); + ret = -ENODEV; + goto error; + } + } + + platform_set_drvdata(pdev, multi); + return 0; + +error: + while (--i >= 0) + i2c_unregister_device(multi->clients[i]); + + return ret; +} + +static int i2c_multi_inst_remove(struct platform_device *pdev) +{ + struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev); + int i; + + for (i = 0; i < multi->no_clients; i++) + i2c_unregister_device(multi->clients[i]); + + return 0; +} + +static const struct i2c_inst_data bsg1160_data[] = { + { "bmc150_accel", 0 }, + { "bmc150_magn", -1 }, + { "bmg160", -1 }, + {} +}; + +/* + * Note new device-ids must also be added to i2c_multi_instantiate_ids in + * drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). + */ +static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { + { "BSG1160", (unsigned long)bsg1160_data }, + { } +}; +MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids); + +static struct platform_driver i2c_multi_inst_driver = { + .driver = { + .name = "I2C multi instantiate pseudo device driver", + .acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids), + }, + .probe = i2c_multi_inst_probe, + .remove = i2c_multi_inst_remove, +}; +module_platform_driver(i2c_multi_inst_driver); + +MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver"); +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); +MODULE_LICENSE("GPL");
On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1 i2c-device. But in some rare cases the manufacturer has decided to describe multiple i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources. An earlier attempt to fix this in the i2c-core resulted in a lot of extra code to support this corner-case. This commit introduces a new i2c-multi-instantiate driver which fixes this in a different way. This new driver can be built as a module which will only loaded on affected systems. This driver will instantiate a new i2c-client per I2cSerialBus resource, using the driver_data from the acpi_device_id it is binding to to tell it which chip-type (and optional irq-resource) to use when instantiating. Note this driver depends on a platform device being instantiated for the ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Rebase on top of 4.18-rc2 Changes in v3: -Change from an i2c-driver using a hack to allow having multiple i2c clients at the same address to a platform-driver Changes in v4: -Tweak MAINTAINERS entry a bit --- MAINTAINERS | 6 + drivers/platform/x86/Kconfig | 11 ++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/i2c-multi-instantiate.c | 131 +++++++++++++++++++ 4 files changed, 149 insertions(+) create mode 100644 drivers/platform/x86/i2c-multi-instantiate.c