On Fri, Feb 3, 2023 at 11:36 AM Saurabh Singh Sengar
<ssengar at linux.microsoft.com> wrote:>
> On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote:
> > On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote:
> > > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > > <ssengar at linux.microsoft.com> wrote:
> > > > >
> > > > > Update the driver to support device tree boot as well
along with ACPI.
> > > > > At present the device tree parsing only provides the
mmio region info
> > > > > and is not the exact copy of ACPI parsing. This is
sufficient to cater
> > > > > all the current device tree usecases for VMBus.
> > > > >
> > > > > Signed-off-by: Saurabh Sengar <ssengar at
linux.microsoft.com>
> > > > > ---
> > > > > drivers/hv/vmbus_drv.c | 75
++++++++++++++++++++++++++++++++++++++++--
> > > > > 1 file changed, 73 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hv/vmbus_drv.c
b/drivers/hv/vmbus_drv.c
> > > > > index 49030e756b9f..1741f1348f9f 100644
> > > > > --- a/drivers/hv/vmbus_drv.c
> > > > > +++ b/drivers/hv/vmbus_drv.c
> > > > > @@ -2152,7 +2152,7 @@ void
vmbus_device_unregister(struct hv_device *device_obj)
> > > > > device_unregister(&device_obj->device);
> > > > > }
> (...)
> > > > > struct pci_dev *pdev;
> > > > > @@ -2442,6 +2443,7 @@ void
vmbus_free_mmio(resource_size_t start, resource_size_t size)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> > > > >
> > > > > +#ifdef CONFIG_ACPI
> > > >
> > > > It's better to put C 'if
(!IS_ENABLED(CONFIG_ACPI)' code in the
> > >
> > > I wanted to have separate function for ACPI and device tree flow,
which
> > > can be easily maintained with #ifdef. Please let me know if its
fine.
> >
> > Yes, you can have separate functions:
> >
> > static int vmbus_acpi_add(struct platform_device *pdev)
> > {
> > if (!IS_ENABLED(CONFIG_ACPI))
> > return -ENODEV;
> >
> > ...
> > }
> >
> > The compiler will throw away the function in the end if CONFIG_ACPI is
> > not enabled.
> >
> > That is easier for us to maintain because it reduces the combinations
to
> > build.
> >
>
> I tried removing #ifdef CONFIG_ACPI and use C's
if(!IS_ENABLED(CONFIG_ACPI)) but looks
> compiler is not optimizing out the rest of function, it still throwing
errors
> for acpi functions. This doesn't look 1:1 replacement to me.
> Please let me know if I have missunderstood any of your suggestion.
>
> drivers/hv/vmbus_drv.c:2175:8: error: implicit declaration of function
?acpi_dev_resource_interrupt? [-Werror=implicit-function-
That's a failure of the ACPI headers not having empty function
declarations. The DT functions do...
Also, this is just a broken assumption:
#ifdef CONFIG_ACPI
#else
// Assume DT
#endif
Both ACPI and DT can be enabled at the same time. They may be mutually
exclusive for a platform, but not the kernel. For distro kernels, both
will be enabled typically if the arch supports both. On arm64, DT is
never disabled because the boot interface is always DT.
Furthermore, this makes compile testing your code difficult. The arm64
defconfig, allmodconfig and allyesconfig all will not build the DT
code. The same for x86. This means all the CI builds that happen can't
build test this.
Rob