[PATCH] ASoC: rt5640: ifdef for ACPI module table

[PATCH] ASoC: rt5640: ifdef for ACPI module table

[Olof Johansson]

Commit 02b80773de37 (ASoC: rt5640: Add ACPI probing support.) causes
a warning due to unreferenced variable on non-ACPI configs such as
tegra_defconfig on ARM:

  sound/soc/codecs/rt5640.c:2085:30: warning: 'rt5640_acpi_match' defined but not used [-Wunused-variable]

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 sound/soc/codecs/rt5640.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index 0bfb960..a856706 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -2082,11 +2082,13 @@ static const struct i2c_device_id rt5640_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, rt5640_i2c_id);
 
+#if CONFIG_ACPI
 static struct acpi_device_id rt5640_acpi_match[] = {
 	{ "INT33CA", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, rt5640_acpi_match);
+#endif
 
 static int rt5640_parse_dt(struct rt5640_priv *rt5640, struct device_node *np)
 {
-- 
1.7.10.4

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Jarkko Nikula]

On 09/18/2013 09:10 AM, Olof Johansson wrote:
> Commit 02b80773de37 (ASoC: rt5640: Add ACPI probing support.) causes
> a warning due to unreferenced variable on non-ACPI configs such as
> tegra_defconfig on ARM:
>
>    sound/soc/codecs/rt5640.c:2085:30: warning: 'rt5640_acpi_match' defined but not used [-Wunused-variable]
>
> Signed-off-by: Olof Johansson<olof@lixom.net>
> ---
>   sound/soc/codecs/rt5640.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
> index 0bfb960..a856706 100644
> --- a/sound/soc/codecs/rt5640.c
> +++ b/sound/soc/codecs/rt5640.c
> @@ -2082,11 +2082,13 @@ static const struct i2c_device_id rt5640_i2c_id[] = {
>   };
>   MODULE_DEVICE_TABLE(i2c, rt5640_i2c_id);
>
> +#if CONFIG_ACPI
>   static struct acpi_device_id rt5640_acpi_match[] = {
>   	{ "INT33CA", 0 },
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(acpi, rt5640_acpi_match);
> +#endif
My 1c, maybe #ifdef?

-- 
Jarkko

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 774 bytes --]

On Tue, Sep 17, 2013 at 11:10:24PM -0700, Olof Johansson wrote:

Please send things to the advertised maintainer addresses.

> Commit 02b80773de37 (ASoC: rt5640: Add ACPI probing support.) causes
> a warning due to unreferenced variable on non-ACPI configs such as
> tegra_defconfig on ARM:

>   sound/soc/codecs/rt5640.c:2085:30: warning: 'rt5640_acpi_match' defined but not used [-Wunused-variable]

> +#if CONFIG_ACPI
>  static struct acpi_device_id rt5640_acpi_match[] = {
>  	{ "INT33CA", 0 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, rt5640_acpi_match);
> +#endif

Shouldn't the fix for this be to do the same as we're doing for the OF
tables and have an acpi_match_ptr()?  It seems like we're doing the same
thing so we should be handling it in a similar fashion.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Olof Johansson]

On Wed, Sep 18, 2013 at 12:26 AM, Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
> On 09/18/2013 09:10 AM, Olof Johansson wrote:
>>
>> Commit 02b80773de37 (ASoC: rt5640: Add ACPI probing support.) causes
>> a warning due to unreferenced variable on non-ACPI configs such as
>> tegra_defconfig on ARM:
>>
>>    sound/soc/codecs/rt5640.c:2085:30: warning: 'rt5640_acpi_match' defined
>> but not used [-Wunused-variable]
>>
>> Signed-off-by: Olof Johansson<olof@lixom.net>
>> ---
>>   sound/soc/codecs/rt5640.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
>> index 0bfb960..a856706 100644
>> --- a/sound/soc/codecs/rt5640.c
>> +++ b/sound/soc/codecs/rt5640.c
>> @@ -2082,11 +2082,13 @@ static const struct i2c_device_id rt5640_i2c_id[]
>> = {
>>   };
>>   MODULE_DEVICE_TABLE(i2c, rt5640_i2c_id);
>>
>> +#if CONFIG_ACPI
>>   static struct acpi_device_id rt5640_acpi_match[] = {
>>         { "INT33CA", 0 },
>>         { },
>>   };
>>   MODULE_DEVICE_TABLE(acpi, rt5640_acpi_match);
>> +#endif
>
> My 1c, maybe #ifdef?

Yeah, I should have sat on this until morning, I cooked it up too late
last night. :)


-Olof

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Olof Johansson]

On Wed, Sep 18, 2013 at 2:41 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 17, 2013 at 11:10:24PM -0700, Olof Johansson wrote:
>
> Please send things to the advertised maintainer addresses.

I sent it to the person signing off on the patch that this fixes up,
you should probably use your maintainer address for that.

>> Commit 02b80773de37 (ASoC: rt5640: Add ACPI probing support.) causes
>> a warning due to unreferenced variable on non-ACPI configs such as
>> tegra_defconfig on ARM:
>
>>   sound/soc/codecs/rt5640.c:2085:30: warning: 'rt5640_acpi_match' defined but not used [-Wunused-variable]
>
>> +#if CONFIG_ACPI
>>  static struct acpi_device_id rt5640_acpi_match[] = {
>>       { "INT33CA", 0 },
>>       { },
>>  };
>>  MODULE_DEVICE_TABLE(acpi, rt5640_acpi_match);
>> +#endif
>
> Shouldn't the fix for this be to do the same as we're doing for the OF
> tables and have an acpi_match_ptr()?  It seems like we're doing the same
> thing so we should be handling it in a similar fashion.

There's already ACPI_PTR(), which is exactly why this warning shows
up. of_match_ptr() and ACPI_PTR() will evaluate to NULL if
CONFIG_OF/ACPI aren't set, which is the case here -- which in turn is
why there's a warning that the match table is defined but never used.

So, the solution is either to not define the table, i.e. ifdef, or to
not use the ACPI_PTR() macro. It makes sense to not advertise the
driver as capable of probing on ACPI if ACPI is not enabled in the
kernel config, which in turn means ifdef.

Unless I'm missing something here, of course. :)


-Olof

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1667 bytes --]

On Wed, Sep 18, 2013 at 08:13:14AM -0700, Olof Johansson wrote:
> On Wed, Sep 18, 2013 at 2:41 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Sep 17, 2013 at 11:10:24PM -0700, Olof Johansson wrote:

> > Please send things to the advertised maintainer addresses.

> I sent it to the person signing off on the patch that this fixes up,
> you should probably use your maintainer address for that.

In that case you should've also CCed the maintainer address, you should
always CC maintainers on patches.

> > Shouldn't the fix for this be to do the same as we're doing for the OF
> > tables and have an acpi_match_ptr()?  It seems like we're doing the same
> > thing so we should be handling it in a similar fashion.

> There's already ACPI_PTR(), which is exactly why this warning shows
> up. of_match_ptr() and ACPI_PTR() will evaluate to NULL if
> CONFIG_OF/ACPI aren't set, which is the case here -- which in turn is
> why there's a warning that the match table is defined but never used.

> So, the solution is either to not define the table, i.e. ifdef, or to
> not use the ACPI_PTR() macro. It makes sense to not advertise the
> driver as capable of probing on ACPI if ACPI is not enabled in the
> kernel config, which in turn means ifdef.

> Unless I'm missing something here, of course. :)

We're supposed to be able to do this sort of stuff without the ifdef
noise and have the compiler eliminate unreferenced static symbols
automatically, though I can't see how of_match_ptr() actually does this
now.  I don't immedately seem to be able to persuade Kconfig to turn
off OF to verify though we do have some examples which nobody has
complained about...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Olof Johansson]

On Wed, Sep 18, 2013 at 04:38:42PM +0100, Mark Brown wrote:
> On Wed, Sep 18, 2013 at 08:13:14AM -0700, Olof Johansson wrote:
> > On Wed, Sep 18, 2013 at 2:41 AM, Mark Brown <broonie@kernel.org> wrote:
> > > On Tue, Sep 17, 2013 at 11:10:24PM -0700, Olof Johansson wrote:
> 
> > > Please send things to the advertised maintainer addresses.
> 
> > I sent it to the person signing off on the patch that this fixes up,
> > you should probably use your maintainer address for that.
> 
> In that case you should've also CCed the maintainer address, you should
> always CC maintainers on patches.

Sure, will do. But you should also consider using your maintainer address
for your maintainer work. Linaro will surely be credited for it anyway
in the oh-so-important statistics. :-)

> > > Shouldn't the fix for this be to do the same as we're doing for the OF
> > > tables and have an acpi_match_ptr()?  It seems like we're doing the same
> > > thing so we should be handling it in a similar fashion.
> 
> > There's already ACPI_PTR(), which is exactly why this warning shows
> > up. of_match_ptr() and ACPI_PTR() will evaluate to NULL if
> > CONFIG_OF/ACPI aren't set, which is the case here -- which in turn is
> > why there's a warning that the match table is defined but never used.
> 
> > So, the solution is either to not define the table, i.e. ifdef, or to
> > not use the ACPI_PTR() macro. It makes sense to not advertise the
> > driver as capable of probing on ACPI if ACPI is not enabled in the
> > kernel config, which in turn means ifdef.
> 
> > Unless I'm missing something here, of course. :)
> 
> We're supposed to be able to do this sort of stuff without the ifdef
> noise and have the compiler eliminate unreferenced static symbols
> automatically, though I can't see how of_match_ptr() actually does this
> now.  I don't immedately seem to be able to persuade Kconfig to turn
> off OF to verify though we do have some examples which nobody has
> complained about...

The problem doesn't show up when you build the driver as a module, because by
then the MODULE_DEVICE_TABLE() reference means the table is referenced. But
when you don't build as a module, MODULE_GENERIC_TABLE() reduces down to
nothing, thus causing this.

Only way to avoid is as I see it is to mark the table __maybe_unused,
which in this case doesn't look like much of a better option.

Most other drivers, for example those using pci or of module tables, have
Kconfig dependencies such that there's no need to ifdef. There are plenty of
examples of drivers ifdeffing on CONFIG_OF for those tables though.


-Olof

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2003 bytes --]

On Wed, Sep 18, 2013 at 09:53:34AM -0700, Olof Johansson wrote:
> On Wed, Sep 18, 2013 at 04:38:42PM +0100, Mark Brown wrote:

> > In that case you should've also CCed the maintainer address, you should
> > always CC maintainers on patches.

> Sure, will do. But you should also consider using your maintainer address
> for your maintainer work. Linaro will surely be credited for it anyway
> in the oh-so-important statistics. :-)

People like that stuff for marketing outside of the stats and I like
being able to keep personal addresses for that.  TBH it's rarely a
problem, the CC maintainers bit does the right thing.

> > We're supposed to be able to do this sort of stuff without the ifdef
> > noise and have the compiler eliminate unreferenced static symbols
> > automatically, though I can't see how of_match_ptr() actually does this
> > now.  I don't immedately seem to be able to persuade Kconfig to turn
> > off OF to verify though we do have some examples which nobody has
> > complained about...

> The problem doesn't show up when you build the driver as a module, because by
> then the MODULE_DEVICE_TABLE() reference means the table is referenced. But
> when you don't build as a module, MODULE_GENERIC_TABLE() reduces down to
> nothing, thus causing this.

I'd have expected the folks doing the allyesconfig builds to have
complained then...

> Only way to avoid is as I see it is to mark the table __maybe_unused,
> which in this case doesn't look like much of a better option.

> Most other drivers, for example those using pci or of module tables, have
> Kconfig dependencies such that there's no need to ifdef. There are plenty of
> examples of drivers ifdeffing on CONFIG_OF for those tables though.

Yes, you used to have to ifdef everything but there was a thing about
stopping bothering doing that.  The same issue applies to the PM
functions.  It's certainly annoying to have to faff around with the
ifdefs all the time.

ISTR Arnd was telling me about this stuff, CCing him.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Olof Johansson]

On Wed, Sep 18, 2013 at 06:28:58PM +0100, Mark Brown wrote:
> > The problem doesn't show up when you build the driver as a module, because by
> > then the MODULE_DEVICE_TABLE() reference means the table is referenced. But
> > when you don't build as a module, MODULE_GENERIC_TABLE() reduces down to
> > nothing, thus causing this.
> 
> I'd have expected the folks doing the allyesconfig builds to have
> complained then...

Allyes will probably turn on CONFIG_ACPI and thus reference the variable
in the struct device.

> > Only way to avoid is as I see it is to mark the table __maybe_unused,
> > which in this case doesn't look like much of a better option.
> 
> > Most other drivers, for example those using pci or of module tables, have
> > Kconfig dependencies such that there's no need to ifdef. There are plenty of
> > examples of drivers ifdeffing on CONFIG_OF for those tables though.
> 
> Yes, you used to have to ifdef everything but there was a thing about
> stopping bothering doing that.  The same issue applies to the PM
> functions.  It's certainly annoying to have to faff around with the
> ifdefs all the time.
> 
> ISTR Arnd was telling me about this stuff, CCing him.

The PM case is particularly annoying because the device struct members
aren't there when PM is off, so you need the ifdef in the struct device
definition.

With the new of_match_ptr() and ACPI_PTR() you get rid of the
ifdef in struct device for those, but you still want it around
MODULE_DEVICE_TABLE() and the actual definition of the match table.


-Olof

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 929 bytes --]

On Wed, Sep 18, 2013 at 10:36:20AM -0700, Olof Johansson wrote:
> On Wed, Sep 18, 2013 at 06:28:58PM +0100, Mark Brown wrote:

> > I'd have expected the folks doing the allyesconfig builds to have
> > complained then...

> Allyes will probably turn on CONFIG_ACPI and thus reference the variable
> in the struct device.

Yeah, I was thinking of OF which isn't available on all architectures.
Perhaps it is on every architecture people tend to do this stuff on
though.

> With the new of_match_ptr() and ACPI_PTR() you get rid of the
> ifdef in struct device for those, but you still want it around
> MODULE_DEVICE_TABLE() and the actual definition of the match table.

Hrm, perhaps that was what was supposed to happen - have
MODULE_DEVICE_TABLE() create the appropriate eliminateable static
reference so the compiler can figure things out and we don't need the
ifdefs.  It'd certainly be nice to elimiante more of these ifdefs.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Olof Johansson]

On Wed, Sep 18, 2013 at 11:34 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Sep 18, 2013 at 10:36:20AM -0700, Olof Johansson wrote:
>> On Wed, Sep 18, 2013 at 06:28:58PM +0100, Mark Brown wrote:
>
>> > I'd have expected the folks doing the allyesconfig builds to have
>> > complained then...
>
>> Allyes will probably turn on CONFIG_ACPI and thus reference the variable
>> in the struct device.
>
> Yeah, I was thinking of OF which isn't available on all architectures.
> Perhaps it is on every architecture people tend to do this stuff on
> though.
>
>> With the new of_match_ptr() and ACPI_PTR() you get rid of the
>> ifdef in struct device for those, but you still want it around
>> MODULE_DEVICE_TABLE() and the actual definition of the match table.
>
> Hrm, perhaps that was what was supposed to happen - have
> MODULE_DEVICE_TABLE() create the appropriate eliminateable static
> reference so the compiler can figure things out and we don't need the
> ifdefs.  It'd certainly be nice to elimiante more of these ifdefs.

Easiest way to do that is probably to provide a new macro to declare
the match tables with, that compiles out and/or gives appropriate
attributes depending on what is enabled or not in the config. That
gets rid of the ifdefs but we don't have anything like it today.

I'm honestly not sure I think it's needed, the ifdef for the table
doesn't bother me. But if people feel strongly about it then it's
likely the best way forward.


-Olof

Re: [PATCH] ASoC: rt5640: ifdef for ACPI module table

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1150 bytes --]

On Wed, Sep 18, 2013 at 11:44:18AM -0700, Olof Johansson wrote:

> > Hrm, perhaps that was what was supposed to happen - have
> > MODULE_DEVICE_TABLE() create the appropriate eliminateable static
> > reference so the compiler can figure things out and we don't need the
> > ifdefs.  It'd certainly be nice to elimiante more of these ifdefs.

> Easiest way to do that is probably to provide a new macro to declare
> the match tables with, that compiles out and/or gives appropriate
> attributes depending on what is enabled or not in the config. That
> gets rid of the ifdefs but we don't have anything like it today.

Yup.

> I'm honestly not sure I think it's needed, the ifdef for the table
> doesn't bother me. But if people feel strongly about it then it's
> likely the best way forward.

For me it's not the tables per se, it's the general proliferation of
options to get coverage on making things more error prone - the more
things always get seen by the compiler the better.  I'm not sure I care
enough to actually do the work but it's nice when someone has done it.

Anyway Thierry sent the #ifdef version of the patch so this is sorted
now.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]