Re: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

Wei Liu
2023-03-09T21:16:41Z

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
>