With the following configuration options:
CONFIG_OF is not set
CONFIG_REGULATOR_MT6380=y
we get the following build warning:
CC drivers/regulator/mt6380-regulator.o
drivers/regulator/mt6380-regulator.c:322:34: warning: ‘mt6380_of_match’ defined but not used [-Wunused-const-variable=]
Fix this by annotating that array with __maybe_unused, as done in
various regulator drivers.
Signed-off-by: Jean Delvare <[email protected]>
Reported-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Matthias Brugger <[email protected]>
Cc: Chenglin Xu <[email protected]>
---
This assumes that the mt6380 driver can be used without OF support.
However, I can't find any in-tree piece of code instantiating the
"mt6380-regulator" platform device by name. So unless there's an
out-of-tree user, a better fix would be to remove mt6380_platform_ids
and make the driver depend on OF. Chenglin, would that be OK with
you?
drivers/regulator/mt6380-regulator.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-5.18.orig/drivers/regulator/mt6380-regulator.c 2022-07-27 11:55:21.672421481 +0200
+++ linux-5.18/drivers/regulator/mt6380-regulator.c 2022-07-27 12:01:53.151833378 +0200
@@ -319,7 +319,7 @@ static const struct platform_device_id m
};
MODULE_DEVICE_TABLE(platform, mt6380_platform_ids);
-static const struct of_device_id mt6380_of_match[] = {
+static const struct of_device_id __maybe_unused mt6380_of_match[] = {
{ .compatible = "mediatek,mt6380-regulator", },
{ /* sentinel */ },
};
--
Jean Delvare
SUSE L3 Support
On Wed, Jul 27, 2022 at 01:26:37PM +0200, Jean Delvare wrote:
> This assumes that the mt6380 driver can be used without OF support.
> However, I can't find any in-tree piece of code instantiating the
> "mt6380-regulator" platform device by name. So unless there's an
> out-of-tree user, a better fix would be to remove mt6380_platform_ids
> and make the driver depend on OF. Chenglin, would that be OK with
> you?
It's helpful to keep the build coverage high.
On Wed, 27 Jul 2022 13:01:29 +0100, Mark Brown wrote:
> On Wed, Jul 27, 2022 at 01:26:37PM +0200, Jean Delvare wrote:
>
> > This assumes that the mt6380 driver can be used without OF support.
> > However, I can't find any in-tree piece of code instantiating the
> > "mt6380-regulator" platform device by name. So unless there's an
> > out-of-tree user, a better fix would be to remove mt6380_platform_ids
> > and make the driver depend on OF. Chenglin, would that be OK with
> > you?
>
> It's helpful to keep the build coverage high.
OF does not depend on the architecture, anyone can enable it. So I
can't see any problem of coverage with making drivers depend on OF.
--
Jean Delvare
SUSE L3 Support
On Wed, Jul 27, 2022 at 02:08:09PM +0200, Jean Delvare wrote:
> On Wed, 27 Jul 2022 13:01:29 +0100, Mark Brown wrote:
> > It's helpful to keep the build coverage high.
> OF does not depend on the architecture, anyone can enable it. So I
> can't see any problem of coverage with making drivers depend on OF.
It still reduces a barrier to entry. It's also that it's easier to just
prefer the conditional compilation pattern rather than either check to
see which cases is needed or have people copy an example that doesn't
use it when they should.
Hi Mark,
On Wed, 27 Jul 2022 13:24:35 +0100, Mark Brown wrote:
> On Wed, Jul 27, 2022 at 02:08:09PM +0200, Jean Delvare wrote:
> > On Wed, 27 Jul 2022 13:01:29 +0100, Mark Brown wrote:
>
> > > It's helpful to keep the build coverage high.
>
> > OF does not depend on the architecture, anyone can enable it. So I
> > can't see any problem of coverage with making drivers depend on OF.
>
> It still reduces a barrier to entry.
Can't see how that would be a goal. By allowing randomconfig to pick
configuration option combinations which do not make sense, we are not
increasing coverage. That's quite the opposite. We are limited by the
overall power of the build farm, so every test build of such a useless
combination is a waste of resources. We'd rather use that machine time
to test a configuration option combination which real people would be
using, as these are the ones we care about.
> It's also that it's easier to just
> prefer the conditional compilation pattern rather than either check to
> see which cases is needed or have people copy an example that doesn't
> use it when they should.
In my experience, there's always a very easy way to silent a warning,
but in most cases, that easy way is wrong because it hides the warning
instead of fixing its cause.
Very much to the point, the build farm pointed us to a combination of
options which triggers warnings which developers had apparently never
noticed before, which is a hint that maybe this combination is not
something we should support in the first place. We can of course silent
all such warnings with __maybe_unused, but that should not be our first
choice (else we might as well stop building with -Wunused).
Not only that, but in this case it might also be that we have kernel
code that can be removed because it isn't needed. Not much, but that
would still be a gain, methinks.
I also don't think that one goal of the kernel code is to be easy to
copy and paste without understanding what you are doing.
Anyway, this is your subsystem, so the decision is yours. My patch
removes the warning, if you are happy with it, feel free to apply it.
Thanks,
--
Jean Delvare
SUSE L3 Support
On Wed, 27 Jul 2022 13:26:37 +0200, Jean Delvare wrote:
> With the following configuration options:
> CONFIG_OF is not set
> CONFIG_REGULATOR_MT6380=y
> we get the following build warning:
>
> CC drivers/regulator/mt6380-regulator.o
> drivers/regulator/mt6380-regulator.c:322:34: warning: ‘mt6380_of_match’ defined but not used [-Wunused-const-variable=]
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/1] regulator: mt6380: Fix unused array warning
commit: 9cc0590ae351a354c51375a1ee22edc2e4931fd0
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Il 27/07/22 13:26, Jean Delvare ha scritto:
> With the following configuration options:
> CONFIG_OF is not set
> CONFIG_REGULATOR_MT6380=y
> we get the following build warning:
>
> CC drivers/regulator/mt6380-regulator.o
> drivers/regulator/mt6380-regulator.c:322:34: warning: ‘mt6380_of_match’ defined but not used [-Wunused-const-variable=]
>
> Fix this by annotating that array with __maybe_unused, as done in
> various regulator drivers.
I know I'm late to the party, but I would've preferred to see the
of_match_ptr() dropped instead of adding a __maybe_unused.
Cheers,
Angelo
>
> Signed-off-by: Jean Delvare <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Cc: Liam Girdwood <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Cc: Chenglin Xu <[email protected]>
> ---
> This assumes that the mt6380 driver can be used without OF support.
> However, I can't find any in-tree piece of code instantiating the
> "mt6380-regulator" platform device by name. So unless there's an
> out-of-tree user, a better fix would be to remove mt6380_platform_ids
> and make the driver depend on OF. Chenglin, would that be OK with
> you?
>
> drivers/regulator/mt6380-regulator.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-5.18.orig/drivers/regulator/mt6380-regulator.c 2022-07-27 11:55:21.672421481 +0200
> +++ linux-5.18/drivers/regulator/mt6380-regulator.c 2022-07-27 12:01:53.151833378 +0200
> @@ -319,7 +319,7 @@ static const struct platform_device_id m
> };
> MODULE_DEVICE_TABLE(platform, mt6380_platform_ids);
>
> -static const struct of_device_id mt6380_of_match[] = {
> +static const struct of_device_id __maybe_unused mt6380_of_match[] = {
> { .compatible = "mediatek,mt6380-regulator", },
> { /* sentinel */ },
> };
>
On Thu, 28 Jul 2022 13:15:14 +0200, AngeloGioacchino Del Regno wrote:
> Il 27/07/22 13:26, Jean Delvare ha scritto:
> > With the following configuration options:
> > CONFIG_OF is not set
> > CONFIG_REGULATOR_MT6380=y
> > we get the following build warning:
> >
> > CC drivers/regulator/mt6380-regulator.o
> > drivers/regulator/mt6380-regulator.c:322:34: warning: ‘mt6380_of_match’ defined but not used [-Wunused-const-variable=]
> >
> > Fix this by annotating that array with __maybe_unused, as done in
> > various regulator drivers.
>
> I know I'm late to the party, but I would've preferred to see the
> of_match_ptr() dropped instead of adding a __maybe_unused.
Doing that for this driver would basically prevent the compiler from
dropping out the variable in the !CONFIG_OF case. If this is a valid
case (and I really would like Mediatek people to comment on that) then
I can't see how that would be an improvement.
--
Jean Delvare
SUSE L3 Support