Messages in this thread Patch in this message | | | Date | Wed, 19 Aug 2020 11:15:39 +0100 | From | Lee Jones <> | Subject | Re: [PATCH] mfd: Add ACPI support to Kontron PLD driver |
| |
On Wed, 12 Aug 2020, Michael Brunner wrote:
> Recent Kontron COMe modules identify the PLD device using the hardware > id KEM0001 in the ACPI table. > This patch adds support for probing the device using the HID and also > retrieving the resources. > > As this is not available for all products, the DMI based detection still > needs to be around for older systems. It is executed if no matching ACPI > HID is found during registering the platform driver or no specific > device id is forced. > If a device is detected using ACPI and no resource information is > available, the default io resource is used. > > Forcing a device id with the force_device_id parameter and therefore > manually generating a platform device takes precedence over ACPI during > probing. > > Signed-off-by: Michael Brunner <michael.brunner@kontron.com> > --- > drivers/mfd/kempld-core.c | 97 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 91 insertions(+), 6 deletions(-) > > diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c > index f48e21d8b97c..408cad1958d9 100644 > --- a/drivers/mfd/kempld-core.c > +++ b/drivers/mfd/kempld-core.c > @@ -13,6 +13,7 @@ > #include <linux/dmi.h> > #include <linux/io.h> > #include <linux/delay.h> > +#include <linux/acpi.h> > > #define MAX_ID_LEN 4 > static char force_device_id[MAX_ID_LEN + 1] = ""; > @@ -132,6 +133,7 @@ static const struct kempld_platform_data kempld_platform_data_generic = { > }; > > static struct platform_device *kempld_pdev; > +static bool kempld_acpi_mode; > > static int kempld_create_platform_device(const struct dmi_system_id *id) > { > @@ -434,13 +436,87 @@ static int kempld_detect_device(struct kempld_device_data *pld) > return ret; > } > > +#ifdef CONFIG_ACPI
Not keen on #ifdefery if at all avoidable.
Can you use if (IS_ENABLED(CONFIG_ACPI)) at the call-site instead?
The compiler should take care of the rest, no?
> +static const struct acpi_device_id kempld_acpi_table[] = { > + { "KEM0001", (kernel_ulong_t)&kempld_platform_data_generic }, > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, kempld_acpi_table);
I'd prefer if this was moved down to just above where it's used i.e. where we usually place the of_device_id tables.
> +static int kempld_get_acpi_data(struct platform_device *pdev) > +{ > + struct list_head resource_list; > + struct resource *resources; > + struct resource_entry *rentry; > + struct device *dev = &pdev->dev; > + struct acpi_device *acpi_dev = ACPI_COMPANION(dev); > + const struct kempld_platform_data *pdata; > + int ret; > + int count; > + > + pdata = acpi_device_get_match_data(dev); > + ret = platform_device_add_data(pdev, pdata, > + sizeof(struct kempld_platform_data)); > + if (ret) > + return ret; > + > + INIT_LIST_HEAD(&resource_list); > + ret = acpi_dev_get_resources(acpi_dev, &resource_list, NULL, NULL); > + if (ret < 0) > + goto out; > + > + count = ret;
if (count == 0) { ret = platform_device_add_resources(pdev, pdata->ioresource, 1); goto out; }
Then drop the next check and pull the indented code back:
> + if (count > 0) { > + resources = devm_kcalloc(&acpi_dev->dev, count, > + sizeof(struct resource), GFP_KERNEL);
sizeof(*resources) is preferred.
> + if (!resources) { > + ret = -ENOMEM; > + goto out; > + } > + > + count = 0; > + list_for_each_entry(rentry, &resource_list, node) { > + memcpy(&resources[count], rentry->res, > + sizeof(*resources)); > + count++; > + } > + > + ret = platform_device_add_resources(pdev, resources, count); > + if (ret) > + goto out; > + } else > + ret = platform_device_add_resources(pdev, pdata->ioresource, 1); > + > +out: > + acpi_dev_free_resource_list(&resource_list); > + > + return ret; > +} > +#else > +static int kempld_get_acpi_data(struct platform_device *pdev) > +{ > + return -ENODEV; > +} > +#endif /* CONFIG_ACPI */ > + > static int kempld_probe(struct platform_device *pdev) > { > - const struct kempld_platform_data *pdata = > - dev_get_platdata(&pdev->dev); > + const struct kempld_platform_data *pdata; > struct device *dev = &pdev->dev; > struct kempld_device_data *pld; > struct resource *ioport; > + int ret; > + > + if (kempld_pdev == NULL) {
Comment please. What does !kempld_pdev actually imply?
> + ret = kempld_get_acpi_data(pdev); > + if (ret < 0) > + return ret;
Is 'ret > 0' valid?
If not, then just 'if (ret)'.
> + kempld_acpi_mode = true; > + } else if (kempld_pdev != pdev) {
> + dev_notice(dev, "platform device exists - not using ACPI\n");
Why dev_notice() and not dev_err()?
Is that what 'kempld_pdev != pdev' means?
Could you explain this to me in more depth please?
> + return -ENODEV; > + } > + pdata = dev_get_platdata(dev); > > pld = devm_kzalloc(dev, sizeof(*pld), GFP_KERNEL); > if (!pld) > @@ -482,6 +558,7 @@ static int kempld_remove(struct platform_device *pdev) > static struct platform_driver kempld_driver = { > .driver = { > .name = "kempld", > + .acpi_match_table = ACPI_PTR(kempld_acpi_table), > }, > .probe = kempld_probe, > .remove = kempld_remove, > @@ -800,6 +877,7 @@ MODULE_DEVICE_TABLE(dmi, kempld_dmi_table); > static int __init kempld_init(void) > { > const struct dmi_system_id *id; > + int ret; > > if (force_device_id[0]) { > for (id = kempld_dmi_table; > @@ -809,12 +887,19 @@ static int __init kempld_init(void) > break; > if (id->matches[0].slot == DMI_NONE) > return -ENODEV; > - } else { > - if (!dmi_check_system(kempld_dmi_table)) > - return -ENODEV; > } > > - return platform_driver_register(&kempld_driver); > + ret = platform_driver_register(&kempld_driver); > + if (ret) > + return ret;
Is it guaranteed that the child device has probed at this point?
> + if (!kempld_pdev && !kempld_acpi_mode)
Again, comment please. What has gone on to get to this point?
> + if (!dmi_check_system(kempld_dmi_table)) { > + platform_driver_unregister(&kempld_driver); > + return -ENODEV; > + } > + > + return 0; > } > > static void __exit kempld_exit(void)
-- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
| |