| Message ID | 20220727132637.76d6073f@endymion.delvare (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] regulator: mt6380: Fix unused array warning | expand |
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.
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,
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 <jdelvare@suse.de> > Reported-by: kernel test robot <lkp@intel.com> > Link: https://lore.kernel.org/all/202207240252.ZY5hSCNB-lkp@intel.com/ > Cc: Liam Girdwood <lgirdwood@gmail.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Matthias Brugger <matthias.bgg@gmail.com> > Cc: Chenglin Xu <chenglin.xu@mediatek.com> > --- > 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.
--- 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 */ }, };
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 <jdelvare@suse.de> Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/all/202207240252.ZY5hSCNB-lkp@intel.com/ Cc: Liam Girdwood <lgirdwood@gmail.com> Cc: Mark Brown <broonie@kernel.org> Cc: Matthias Brugger <matthias.bgg@gmail.com> Cc: Chenglin Xu <chenglin.xu@mediatek.com> --- 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(-)