[alsa-devel] [RFC 1/2] i2c: Use stable dev_name for ACPI enumerated I2C slaves

[Rafael J. Wysocki]

On Friday, November 01, 2013 01:08:47 AM Rafael J. Wysocki wrote:
> On Monday, October 28, 2013 03:15:25 PM Jarkko Nikula wrote:
> > Hi Rafael
> > 
> > On 10/25/2013 05:08 PM, Rafael J. Wysocki wrote:
> > > On Friday, October 25, 2013  04:30:23 PM Jarkko Nikula wrote:
> >  >>>
> >  >> Hmm, not only. Referencing to dev field in struct acpi_device by
> >  >> dev_name(&adev->dev) here will fail too.
> >  >
> >  > Well, something is not quite right here.
> >  >
> >  > One of the *reasons* for having ACPI_HANDLE() defined this way is to
> >  > avoid using explicit CONFIG_ACPI checks, so if that doesn't work,
> >  > then all of that becomes a bit pointless.
> >  >
> 
> Can you please send patches inline instead of using inline attachments,
> so that people don't have to paste your patches back to comment them?
> 
> > One possible thing to do is to let structure definitions to be available 
> > for non-ACPI builds. Then compiler won't fail on structure access which 
> > will be anyway optimized away by later compiler stages.
> 
> Yes, we can do that, but as I said that means we're giving up on the "why
> ACPI_HANDLE() doesn't work as expected" front.  For now, I'd like to
> understand what's up before making that change.  Moreover ->

OK, so I *think* what happens is that the compiler checks if the particular
symbols make sense before trying to optimize out the whole block.

So the patch below modulo the return value of the acpi_bus_get_device() stub
would be fine by me.

Thanks!

> > With a quick test below vmlinux section sizes don't change for couple 
> > non-ACPI build tests and allow to get rid off IS_ENABLED(CONFIG_ACPI) 
> > test in this patch. It's very minimal as it only moves the CONFIG_ACPI 
> > test just after the struct acpi_device definition and defines 
> > acpi_bus_get_device for non-ACPI case.
> > 
> > What I don't know how consistent it is as there are still couple 
> > structure definitions under CONFIG_ACPI, doesn't touch other acpi 
> > headers and requires to include acpi_bus.h in driver (or move acpi_bus.h 
> > include in linux/acpi.h currently under CONFIG_ACPI).
> >
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index d901982..7ab7870 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -66,8 +66,6 @@ bool acpi_ata_match(acpi_handle handle);
> > 
> >   bool acpi_bay_match(acpi_handle handle);
> >   bool acpi_dock_match(acpi_handle handle);
> > 
> > -#ifdef CONFIG_ACPI
> > -
> > 
> >   #include <linux/proc_fs.h>
> >   
> >   #define ACPI_BUS_FILE_ROOT    "acpi"
> > 
> > @@ -314,6 +312,8 @@ struct acpi_device {
> > 
> >       void (*remove)(struct acpi_device *);
> >   
> >   };
> > 
> > +#if IS_ENABLED(CONFIG_ACPI)
> > +
> > 
> >   static inline void *acpi_driver_data(struct acpi_device *d)
> >   {
> >   
> >       return d->driver_data;
> > 
> > @@ -531,6 +531,8 @@ static inline bool acpi_device_can_poweroff(struct
> > acpi_device *adev)
> > 
> >   static inline int register_acpi_bus_type(void *bus) { return 0; }
> >   static inline int unregister_acpi_bus_type(void *bus) { return 0; }
> > 
> > +static inline int acpi_bus_get_device(acpi_handle handle,
> > +                      struct acpi_device **device) { return 0; }
> 
> This has to return an error code, because otherwise the caller can assume
> *device to be a valid pointer after it returns which may not be the case.
> 
> > 
> >   #endif                /* CONFIG_ACPI */

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.