Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 19 Aug 2020 11:15:39 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     Michael Brunner <Michael.Brunner@kontron.com>
Cc:     "linux@roeck-us.net" <linux@roeck-us.net>,
        "sameo@linux.intel.com" <sameo@linux.intel.com>,
        "dvhart@linux.intel.com" <dvhart@linux.intel.com>,
        "mibru@gmx.de" <mibru@gmx.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kevin.strasser@linux.intel.com" <kevin.strasser@linux.intel.com>
Subject: Re: [PATCH] mfd: Add ACPI support to Kontron PLD driver
Message-ID: <20200819101539.GE4354@dell>
References: <1e5ff295eacd5cb9eb2d888e1b0175fea62cf2ae.camel@kontron.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1e5ff295eacd5cb9eb2d888e1b0175fea62cf2ae.camel@kontron.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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