Date:   Thu, 9 Mar 2023 21:16:25 +0000
From:   Wei Liu <wei.liu@kernel.org>
To:     Saurabh Sengar <ssengar@linux.microsoft.com>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
        decui@microsoft.com, daniel.lezcano@linaro.org, tglx@linutronix.de,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hyperv@vger.kernel.org, mikelley@microsoft.com,
        lenb@kernel.org, rafael@kernel.org, linux-acpi@vger.kernel.org
Subject: Re: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support
Message-ID: <ZApMqWPWgWXIju/g@liuwe-devbox-debian-v2>
References: <1677151745-16521-1-git-send-email-ssengar@linux.microsoft.com>
 <1677151745-16521-6-git-send-email-ssengar@linux.microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1677151745-16521-6-git-send-email-ssengar@linux.microsoft.com>
Precedence: bulk

On Thu, Feb 23, 2023 at 03:29:05AM -0800, Saurabh Sengar wrote:
[...]
> +#ifdef CONFIG_ACPI
>  static int vmbus_acpi_add(struct platform_device *pdev)
>  {
>  	acpi_status result;
> @@ -2494,10 +2497,47 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>  		vmbus_mmio_remove();
>  	return ret_val;
>  }
> +#endif
> +
> +static int vmbus_device_add(struct platform_device *pdev)
> +{
> +	struct resource **cur_res = &hyperv_mmio;
> +	struct of_range range;
> +	struct of_range_parser parser;
> +	struct device_node *np = pdev->dev.of_node;
> +	int ret;
> +
> +	hv_dev = &pdev->dev;
> +
> +	ret = of_range_parser_init(&parser, np);
> +	if (ret)
> +		return ret;
> +
> +	for_each_of_range(&parser, &range) {
> +		struct resource *res;
> +
> +		res = kzalloc(sizeof(*res), GFP_ATOMIC);

Why GFP_ATOMIC here? I don't think this function will be called in
atomic context, right?

> +		if (!res)
> +			return -ENOMEM;
> +
> +		res->name = "hyperv mmio";
> +		res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;

Are you sure IORESOURCE_MEM_64 is correct or required? The ACPI method
does not set this flag.

> +		res->start = range.cpu_addr;
> +		res->end = range.cpu_addr + range.size;
> +
> +		*cur_res = res;
> +		cur_res = &res->sibling;
> +	}
> +
> +	return ret;
> +}
>  
>  static int vmbus_platform_driver_probe(struct platform_device *pdev)
>  {
> +#ifdef CONFIG_ACPI
>  	return vmbus_acpi_add(pdev);
> +#endif

Please use #else here.

> +	return vmbus_device_add(pdev);

Is there going to be a configuration that ACPI and OF are available at
the same time? I don't see they are marked as mutually exclusive in the
proposed KConfig.

Thanks,
Wei.

>  }
>  
>  static int vmbus_platform_driver_remove(struct platform_device *pdev)
> @@ -2643,12 +2683,24 @@ static int vmbus_bus_resume(struct device *dev)
>  #define vmbus_bus_resume NULL
>  #endif /* CONFIG_PM_SLEEP */
>  
> +static const __maybe_unused struct of_device_id vmbus_of_match[] = {
> +	{
> +		.compatible = "microsoft,vmbus",
> +	},
> +	{
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, vmbus_of_match);
> +
> +#ifdef CONFIG_ACPI
>  static const struct acpi_device_id vmbus_acpi_device_ids[] = {
>  	{"VMBUS", 0},
>  	{"VMBus", 0},
>  	{"", 0},
>  };
>  MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids);
> +#endif
>  
>  /*
>   * Note: we must use the "no_irq" ops, otherwise hibernation can not work with
> @@ -2677,6 +2729,7 @@ static struct platform_driver vmbus_platform_driver = {
>  	.driver = {
>  		.name = "vmbus",
>  		.acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
> +		.of_match_table = of_match_ptr(vmbus_of_match),
>  		.pm = &vmbus_bus_pm,
>  		.probe_type = PROBE_FORCE_SYNCHRONOUS,
>  	}
> -- 
> 2.34.1
>