| Message ID | 20220825144012.24a33bb0@endymion.delvare (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | iio:accel:dmard06: Optimize when CONFIG_OF isn't set | expand |
On Thu, Aug 25, 2022 at 3:54 PM Jean Delvare <jdelvare@suse.de> wrote: > > When CONFIG_OF isn't set, we can optimize out dmard06_of_match as it > will never be used. NACK. There is a magic PRP0001 for ACPI case.
Hi Andy, On Thu, 25 Aug 2022 23:12:43 +0300, Andy Shevchenko wrote: > On Thu, Aug 25, 2022 at 3:54 PM Jean Delvare <jdelvare@suse.de> wrote: > > > > When CONFIG_OF isn't set, we can optimize out dmard06_of_match as it > > will never be used. > > NACK. There is a magic PRP0001 for ACPI case. OK, I wasn't aware of this. As of_match_device() is a stub when CONFIG_OF isn't set, I thought the table could never be used. But now I see that the acpi subsystem accesses the table directly, so you are correct, the optimization I suggested is bogus. Now I'm curious, is there a well-defined subset of device names that can be found in an ACPI table? If not, does that mean that any driver with an OF entry could match, therefore of_match_ptr() should be removed from the kernel entirely? Another possibility would be to stub out of_match_ptr() only if neither CONFIG_OF nor CONFIG_ACPI is set. But I'm not sure that would be worth, as I expected either to be set on almost all kernels in practice (except on s390x, but you wouldn't build any of these drivers there anyway).
On Fri, Aug 26, 2022 at 1:46 PM Jean Delvare <jdelvare@suse.de> wrote: > On Thu, 25 Aug 2022 23:12:43 +0300, Andy Shevchenko wrote: > > On Thu, Aug 25, 2022 at 3:54 PM Jean Delvare <jdelvare@suse.de> wrote: ... > Now I'm curious, is there a well-defined subset of device names that > can be found in an ACPI table? If not, does that mean that any driver > with an OF entry could match, Yes, anything can be matched by ACPI with any of the compatible strings. > therefore of_match_ptr() should be > removed from the kernel entirely? In most cases yes, like for discrete components that can be connected to any SoC on ACPI/DT/whatever platform. But for some cases it still makes sense: platform is known to never be non-OF, component is known to be used only on such platforms, etc.
On Fri, 26 Aug 2022 18:18:20 +0300, Andy Shevchenko wrote: > On Fri, Aug 26, 2022 at 1:46 PM Jean Delvare <jdelvare@suse.de> wrote: > > therefore of_match_ptr() should be > > removed from the kernel entirely? > > (...) But for some cases it still > makes sense: platform is known to never be non-OF, component is known > to be used only on such platforms, etc. Well, I can't see the value of of_match_ptr() in such case either. In fact I've submitted a couple patches to remove such occurrences lately: https://patchwork.kernel.org/project/linux-mediatek/patch/20220730144833.0a0d9825@endymion.delvare/ https://patchwork.kernel.org/project/linux-pm/patch/20220804135938.7f69f5d9@endymion.delvare/
On Fri, Aug 26, 2022 at 7:06 PM Jean Delvare <jdelvare@suse.de> wrote: > On Fri, 26 Aug 2022 18:18:20 +0300, Andy Shevchenko wrote: > > On Fri, Aug 26, 2022 at 1:46 PM Jean Delvare <jdelvare@suse.de> wrote: > > > therefore of_match_ptr() should be > > > removed from the kernel entirely? > > > > (...) But for some cases it still > > makes sense: platform is known to never be non-OF, component is known > > to be used only on such platforms, etc. > > Well, I can't see the value of of_match_ptr() in such case either. In > fact I've submitted a couple patches to remove such occurrences lately: > > https://patchwork.kernel.org/project/linux-mediatek/patch/20220730144833.0a0d9825@endymion.delvare/ > https://patchwork.kernel.org/project/linux-pm/patch/20220804135938.7f69f5d9@endymion.delvare/ They are different to what we are discussing here, but yes, in common denominator the of_match_ptr() is useless in almost 100% cases.
On Fri, 26 Aug 2022 19:10:33 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Aug 26, 2022 at 7:06 PM Jean Delvare <jdelvare@suse.de> wrote: > > On Fri, 26 Aug 2022 18:18:20 +0300, Andy Shevchenko wrote: > > > On Fri, Aug 26, 2022 at 1:46 PM Jean Delvare <jdelvare@suse.de> wrote: > > > > therefore of_match_ptr() should be > > > > removed from the kernel entirely? > > > > > > (...) But for some cases it still > > > makes sense: platform is known to never be non-OF, component is known > > > to be used only on such platforms, etc. > > > > Well, I can't see the value of of_match_ptr() in such case either. In > > fact I've submitted a couple patches to remove such occurrences lately: > > > > https://patchwork.kernel.org/project/linux-mediatek/patch/20220730144833.0a0d9825@endymion.delvare/ > > https://patchwork.kernel.org/project/linux-pm/patch/20220804135938.7f69f5d9@endymion.delvare/ > > They are different to what we are discussing here, but yes, in common > denominator the of_match_ptr() is useless in almost 100% cases. > Agreed. Ever since PRP0001 came in, it's made little to no sense to have an of_match_ptr() but it's perhaps also not worth the noise of generally removing 1588 cases of it! As a side note, of_match_ptr() did make sense, then moving to trickery along the lines of what was recently done for pm_ptr() to get rid of the need for __maybe_unused would be good. #define pm_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr)) I 'think' the same trick could be used to make the use of the array visible to the compiler but still allow it to remove the array. Not actually tried it though... Jonathan
--- linux-5.19.orig/drivers/iio/accel/dmard06.c 2022-08-25 14:19:11.742351430 +0200 +++ linux-5.19/drivers/iio/accel/dmard06.c 2022-08-25 14:20:13.505276596 +0200 @@ -209,7 +209,7 @@ static const struct i2c_device_id dmard0 }; MODULE_DEVICE_TABLE(i2c, dmard06_id); -static const struct of_device_id dmard06_of_match[] = { +static const struct of_device_id __maybe_unused dmard06_of_match[] = { { .compatible = "domintech,dmard05" }, { .compatible = "domintech,dmard06" }, { .compatible = "domintech,dmard07" }, @@ -222,7 +222,7 @@ static struct i2c_driver dmard06_driver .id_table = dmard06_id, .driver = { .name = DMARD06_DRV_NAME, - .of_match_table = dmard06_of_match, + .of_match_table = of_match_ptr(dmard06_of_match), .pm = pm_sleep_ptr(&dmard06_pm_ops), }, };
When CONFIG_OF isn't set, we can optimize out dmard06_of_match as it will never be used. Signed-off-by: Jean Delvare <jdelvare@suse.de> Cc: Jonathan Cameron <jic23@kernel.org> Cc: Lars-Peter Clausen <lars@metafoo.de> --- drivers/iio/accel/dmard06.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)