| Message ID | 20201120161653.445521-1-krzk@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [01/38] ASoC: ak5558: drop of_match_ptr from of_device_id table | expand |
On 11/20/20 17:16, Krzysztof Kozlowski wrote: > The driver can be compile tested with !CONFIG_OF making certain data > unused: > > sound/soc/samsung/i2s.c:1646:42: warning: ‘i2sv5_dai_type_i2s1’ defined but not used [-Wunused-const-variable=] > sound/soc/samsung/i2s.c:1639:42: warning: ‘i2sv7_dai_type’ defined but not used [-Wunused-const-variable=] > > Signed-off-by: Krzysztof Kozlowski<krzk@kernel.org> Reviewed-by: Sylwester Nawrocki <snawrocki@kernel.org>
On 11/20/20 17:16, Krzysztof Kozlowski wrote: > of_match_device() already handles properly !CONFIG_OF case, so passing > the argument via of_match_ptr() is not needed. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> Reviewed-by: Sylwester Nawrocki <snawrocki@kernel.org>
On Fri, Nov 20, 2020 at 05:16:15PM +0100, Krzysztof Kozlowski wrote: > The driver can match only via the DT table so the table should be always > used and the of_match_ptr does not have any sense (this also allows ACPI > matching via PRP0001, even though it is not relevant here). This fixes > compile warning (!CONFIG_OF on x86_64): It would be better to fix these by annotating the table as potentially unused, if nothing else it means if someone wants to add ACPI support (or it just works on their ACPI system with the plain old I2C ID) then they don't need to revert this change.
On Fri, Nov 20, 2020 at 04:56:34PM +0000, Mark Brown wrote: > On Fri, Nov 20, 2020 at 05:16:15PM +0100, Krzysztof Kozlowski wrote: > > The driver can match only via the DT table so the table should be always > > used and the of_match_ptr does not have any sense (this also allows ACPI > > matching via PRP0001, even though it is not relevant here). This fixes > > compile warning (!CONFIG_OF on x86_64): > > It would be better to fix these by annotating the table as potentially > unused, if nothing else it means if someone wants to add ACPI support > (or it just works on their ACPI system with the plain old I2C ID) then > they don't need to revert this change. The point is after this patch - removal of of_match_ptr() - they will already support the ACPI matching through the PRP0001. Keeping of_match_ptr() and maybe_unused will prevent any ACPI re-usage unless explicit ACPI table is added Best regards, Krzysztof
On Fri, Nov 20, 2020 at 08:04:29PM +0000, Mark Brown wrote: > On Fri, Nov 20, 2020 at 08:42:45PM +0100, Krzysztof Kozlowski wrote: > > On Fri, Nov 20, 2020 at 04:56:34PM +0000, Mark Brown wrote: > > > > It would be better to fix these by annotating the table as potentially > > > unused, if nothing else it means if someone wants to add ACPI support > > > (or it just works on their ACPI system with the plain old I2C ID) then > > > they don't need to revert this change. > > > The point is after this patch - removal of of_match_ptr() - they will > > already support the ACPI matching through the PRP0001. > > > Keeping of_match_ptr() and maybe_unused will prevent any ACPI re-usage > > unless explicit ACPI table is added > > Surely if that's the desired outcome the fix is to change the definition > of of_match_ptr() such that it leaves the reference with CONFIG_ACPI, > perhaps hidden behind a config option for PRP0001? That seems better > than going through the entire tree like this. That could be indeed an easier way to achieve this. +Cc Andy, Rafael, I saw you were doing similar way as I did here [1] for the 698fffc2705c ("rtc: ds1307: Drop of_match_ptr and CONFIG_OF protections") with the same reasoning as mine ("These prevent use of this driver with ACPI via PRP0001."). Do you have thoughts on Mark's proposal above (to change the of_match_ptr())? [1] https://lore.kernel.org/lkml/20201120161653.445521-1-krzk@kernel.org/ Best regards, Krzysztof
On Sun, Nov 22, 2020 at 11:59:20AM +0100, Krzysztof Kozlowski wrote: > On Fri, Nov 20, 2020 at 08:04:29PM +0000, Mark Brown wrote: > > On Fri, Nov 20, 2020 at 08:42:45PM +0100, Krzysztof Kozlowski wrote: > > > On Fri, Nov 20, 2020 at 04:56:34PM +0000, Mark Brown wrote: > > > > > > It would be better to fix these by annotating the table as potentially > > > > unused, if nothing else it means if someone wants to add ACPI support > > > > (or it just works on their ACPI system with the plain old I2C ID) then > > > > they don't need to revert this change. > > > > > The point is after this patch - removal of of_match_ptr() - they will > > > already support the ACPI matching through the PRP0001. > > > > > Keeping of_match_ptr() and maybe_unused will prevent any ACPI re-usage > > > unless explicit ACPI table is added > > > > Surely if that's the desired outcome the fix is to change the definition > > of of_match_ptr() such that it leaves the reference with CONFIG_ACPI, > > perhaps hidden behind a config option for PRP0001? That seems better > > than going through the entire tree like this. > > That could be indeed an easier way to achieve this. ...easier and wrong in my opinion. Not all drivers need that. What the point to touch it in the driver which is OF-only? (For IP which will quite unlikely to be present in ACPI world) Or if the device will get the correct ACPI ID? > +Cc Andy, Rafael, I guess Rafael can correct me or others. > I saw you were doing similar way as I did here [1] for the 698fffc2705c > ("rtc: ds1307: Drop of_match_ptr and CONFIG_OF protections") with the > same reasoning as mine ("These prevent use of this driver with ACPI via > PRP0001."). The above is a device which can be connected to any system, including ACPI-based one. The patch has been cooked to have some means to make it usable on such systems (because previous patch removes wrong ACPI IDs). > Do you have thoughts on Mark's proposal above (to change the > of_match_ptr())? > > [1] https://lore.kernel.org/lkml/20201120161653.445521-1-krzk@kernel.org/
On Mon, Nov 23, 2020 at 12:48:32PM +0200, Andy Shevchenko wrote: > On Sun, Nov 22, 2020 at 11:59:20AM +0100, Krzysztof Kozlowski wrote: > > On Fri, Nov 20, 2020 at 08:04:29PM +0000, Mark Brown wrote: > > > Surely if that's the desired outcome the fix is to change the definition > > > of of_match_ptr() such that it leaves the reference with CONFIG_ACPI, > > > perhaps hidden behind a config option for PRP0001? That seems better > > > than going through the entire tree like this. > > That could be indeed an easier way to achieve this. > ...easier and wrong in my opinion. Not all drivers need that. > What the point to touch it in the driver which is OF-only? > (For IP which will quite unlikely to be present in ACPI world) > Or if the device will get the correct ACPI ID? That feels like something that should be done with Kconfig dependencies like a direct OF dependency (possibly a !PRP0001 dependency?) for the driver or possibly with having a variant of_match_ptr() for things that really don't want to support PRP0001. Just removing all the use of of_match_ptr() is both noisy and confusing in that it looks like it's creating issues to fix, it makes it hard to understand when and why one should use the macro.
On Mon, Nov 23, 2020 at 12:37:31PM +0000, Mark Brown wrote: > On Mon, Nov 23, 2020 at 12:48:32PM +0200, Andy Shevchenko wrote: > > On Sun, Nov 22, 2020 at 11:59:20AM +0100, Krzysztof Kozlowski wrote: > > > On Fri, Nov 20, 2020 at 08:04:29PM +0000, Mark Brown wrote: > > > > > Surely if that's the desired outcome the fix is to change the definition > > > > of of_match_ptr() such that it leaves the reference with CONFIG_ACPI, > > > > perhaps hidden behind a config option for PRP0001? That seems better > > > > than going through the entire tree like this. > > > > That could be indeed an easier way to achieve this. > > > ...easier and wrong in my opinion. Not all drivers need that. > > What the point to touch it in the driver which is OF-only? > > (For IP which will quite unlikely to be present in ACPI world) > > Or if the device will get the correct ACPI ID? > > That feels like something that should be done with Kconfig dependencies > like a direct OF dependency (possibly a !PRP0001 dependency?) for the > driver or possibly with having a variant of_match_ptr() for things that > really don't want to support PRP0001. Just removing all the use of > of_match_ptr() is both noisy and confusing in that it looks like it's > creating issues to fix, it makes it hard to understand when and why one > should use the macro. For the OF-only drivers (without other ID table), there is no point to use the macro. Driver can bind only with DT, so what is the point of of_match_ptr? To skip the OF table when building without OF? Driver won't be usable at all in such case. So maybe for compile testing? There is no need to remove OF table for simple build tests. Best regards, Krzysztof
On Mon, Nov 23, 2020 at 12:37:31PM +0000, Mark Brown wrote: > On Mon, Nov 23, 2020 at 12:48:32PM +0200, Andy Shevchenko wrote: > > On Sun, Nov 22, 2020 at 11:59:20AM +0100, Krzysztof Kozlowski wrote: > > > On Fri, Nov 20, 2020 at 08:04:29PM +0000, Mark Brown wrote: > > > > > Surely if that's the desired outcome the fix is to change the definition > > > > of of_match_ptr() such that it leaves the reference with CONFIG_ACPI, > > > > perhaps hidden behind a config option for PRP0001? That seems better > > > > than going through the entire tree like this. > > > > That could be indeed an easier way to achieve this. > > > ...easier and wrong in my opinion. Not all drivers need that. > > What the point to touch it in the driver which is OF-only? > > (For IP which will quite unlikely to be present in ACPI world) > > Or if the device will get the correct ACPI ID? > > That feels like something that should be done with Kconfig dependencies > like a direct OF dependency (possibly a !PRP0001 dependency?) for the > driver or possibly with having a variant of_match_ptr() for things that > really don't want to support PRP0001. Just removing all the use of > of_match_ptr() is both noisy and confusing in that it looks like it's > creating issues to fix, it makes it hard to understand when and why one > should use the macro. My personal opinion is that in 99% using that macro (as well as ACPI_PTR() one) is kinda mistake. We save dozen of bytes here and there by adding macro, necessary ifdeferry, and dropping user to know that the driver might serve for other device IDs as provided by OF / ACPI. But I'm not the one who makes a decision here and I could see some want to have a possibility to reduce their kernel memory footprint as much as possible.
On Mon, Nov 23, 2020 at 01:41:29PM +0100, Krzysztof Kozlowski wrote: > On Mon, Nov 23, 2020 at 12:37:31PM +0000, Mark Brown wrote: > > On Mon, Nov 23, 2020 at 12:48:32PM +0200, Andy Shevchenko wrote: > > > On Sun, Nov 22, 2020 at 11:59:20AM +0100, Krzysztof Kozlowski wrote: > > > > On Fri, Nov 20, 2020 at 08:04:29PM +0000, Mark Brown wrote: > > > > > > > Surely if that's the desired outcome the fix is to change the definition > > > > > of of_match_ptr() such that it leaves the reference with CONFIG_ACPI, > > > > > perhaps hidden behind a config option for PRP0001? That seems better > > > > > than going through the entire tree like this. > > > > > > That could be indeed an easier way to achieve this. > > > > > ...easier and wrong in my opinion. Not all drivers need that. > > > What the point to touch it in the driver which is OF-only? > > > (For IP which will quite unlikely to be present in ACPI world) > > > Or if the device will get the correct ACPI ID? > > > > That feels like something that should be done with Kconfig dependencies > > like a direct OF dependency (possibly a !PRP0001 dependency?) for the > > driver or possibly with having a variant of_match_ptr() for things that > > really don't want to support PRP0001. Just removing all the use of > > of_match_ptr() is both noisy and confusing in that it looks like it's > > creating issues to fix, it makes it hard to understand when and why one > > should use the macro. > > For the OF-only drivers (without other ID table), there is no point to > use the macro. Driver can bind only with DT, so what is the point of > of_match_ptr? To skip the OF table when building without OF? Driver > won't be usable at all in such case. So maybe for compile testing? > There is no need to remove OF table for simple build tests. I'm on the same page here.
On Mon, Nov 23, 2020 at 03:42:25PM +0200, Andy Shevchenko wrote: > On Mon, Nov 23, 2020 at 01:41:29PM +0100, Krzysztof Kozlowski wrote: > > On Mon, Nov 23, 2020 at 12:37:31PM +0000, Mark Brown wrote: > > > On Mon, Nov 23, 2020 at 12:48:32PM +0200, Andy Shevchenko wrote: > > > > On Sun, Nov 22, 2020 at 11:59:20AM +0100, Krzysztof Kozlowski wrote: > > > > > On Fri, Nov 20, 2020 at 08:04:29PM +0000, Mark Brown wrote: > > > > > > > > > Surely if that's the desired outcome the fix is to change the definition > > > > > > of of_match_ptr() such that it leaves the reference with CONFIG_ACPI, > > > > > > perhaps hidden behind a config option for PRP0001? That seems better > > > > > > than going through the entire tree like this. > > > > > > > > That could be indeed an easier way to achieve this. > > > > > > > ...easier and wrong in my opinion. Not all drivers need that. > > > > What the point to touch it in the driver which is OF-only? > > > > (For IP which will quite unlikely to be present in ACPI world) > > > > Or if the device will get the correct ACPI ID? > > > > > > That feels like something that should be done with Kconfig dependencies > > > like a direct OF dependency (possibly a !PRP0001 dependency?) for the > > > driver or possibly with having a variant of_match_ptr() for things that > > > really don't want to support PRP0001. Just removing all the use of > > > of_match_ptr() is both noisy and confusing in that it looks like it's > > > creating issues to fix, it makes it hard to understand when and why one > > > should use the macro. > > > > For the OF-only drivers (without other ID table), there is no point to > > use the macro. Driver can bind only with DT, so what is the point of > > of_match_ptr? To skip the OF table when building without OF? Driver > > won't be usable at all in such case. So maybe for compile testing? > > There is no need to remove OF table for simple build tests. > > I'm on the same page here. I should have elaborated that under OF only I meant rather !ACPI. So, when it has no ID tables, I agree that macro is not needed. But, for instance, I²C device drivers tend to have table even for ->probe_new() callback to be able to instantiate them via user space.
On Mon, Nov 23, 2020 at 01:41:29PM +0100, Krzysztof Kozlowski wrote: > On Mon, Nov 23, 2020 at 12:37:31PM +0000, Mark Brown wrote: > > That feels like something that should be done with Kconfig dependencies > > like a direct OF dependency (possibly a !PRP0001 dependency?) for the > > driver or possibly with having a variant of_match_ptr() for things that > > really don't want to support PRP0001. Just removing all the use of > > of_match_ptr() is both noisy and confusing in that it looks like it's > > creating issues to fix, it makes it hard to understand when and why one > > should use the macro. > For the OF-only drivers (without other ID table), there is no point to > use the macro. Driver can bind only with DT, so what is the point of > of_match_ptr? To skip the OF table when building without OF? Driver > won't be usable at all in such case. So maybe for compile testing? > There is no need to remove OF table for simple build tests. If nothing else it means you don't have to check if the driver is OF only or not. I can see not bothering to add it but actively going round removing some instances of it doesn't seem great, and it seems like people will constantly be adding new uses on the basis that it's just such an obviously correct thing to do.
On Mon, Nov 23, 2020 at 01:50:06PM +0000, Mark Brown wrote: > On Mon, Nov 23, 2020 at 01:41:29PM +0100, Krzysztof Kozlowski wrote: > > On Mon, Nov 23, 2020 at 12:37:31PM +0000, Mark Brown wrote: > > > > That feels like something that should be done with Kconfig dependencies > > > like a direct OF dependency (possibly a !PRP0001 dependency?) for the > > > driver or possibly with having a variant of_match_ptr() for things that > > > really don't want to support PRP0001. Just removing all the use of > > > of_match_ptr() is both noisy and confusing in that it looks like it's > > > creating issues to fix, it makes it hard to understand when and why one > > > should use the macro. > > > For the OF-only drivers (without other ID table), there is no point to > > use the macro. Driver can bind only with DT, so what is the point of > > of_match_ptr? To skip the OF table when building without OF? Driver > > won't be usable at all in such case. So maybe for compile testing? > > There is no need to remove OF table for simple build tests. > > If nothing else it means you don't have to check if the driver is OF > only or not. I can see not bothering to add it but actively going round > removing some instances of it doesn't seem great, and it seems like > people will constantly be adding new uses on the basis that it's just > such an obviously correct thing to do. If my patch was not changing anything, I would agree that it might be just a churn. But the patch fixes a real warning. The other way of fixing warning is the one you proposed at beginning - adding maybe_unused. Here we go to the second reason: Having these of_match_ptr() for OF-only drivers is not the correct way but rather something which is copied from existing drivers into new ones. This is another reason for removing them - people will stop copying this code all over again. Best regards, Krzysztof
On Mon, Nov 23, 2020 at 03:58:31PM +0100, Krzysztof Kozlowski wrote: > Having these of_match_ptr() for OF-only drivers is not the correct way > but rather something which is copied from existing drivers into new > ones. This is another reason for removing them - people will stop > copying this code all over again. Well, it seems that the issue the PRP0001 people are having is that they'd rather that there were fewer OF only drivers! More generally it is good practice to write things as though there might be non-DT support even if it's not there at the present time, it's easier to not have to think if it's strictly needed and it helps anyone coming along later who does want to use things elsewhere.
On Mon, Nov 23, 2020 at 04:43:30PM +0000, Mark Brown wrote: > On Mon, Nov 23, 2020 at 03:58:31PM +0100, Krzysztof Kozlowski wrote: > > > Having these of_match_ptr() for OF-only drivers is not the correct way > > but rather something which is copied from existing drivers into new > > ones. This is another reason for removing them - people will stop > > copying this code all over again. > > Well, it seems that the issue the PRP0001 people are having is that > they'd rather that there were fewer OF only drivers! More generally > it is good practice to write things as though there might be non-DT > support even if it's not there at the present time, it's easier to not > have to think if it's strictly needed and it helps anyone coming along > later who does want to use things elsewhere. I understand. I will send therefore a v2 adding __maybe_unused to the OF table. Best regards, Krzysztof
diff --git a/sound/soc/codecs/ak5558.c b/sound/soc/codecs/ak5558.c index 2f076d5ee284..427d7d51bf53 100644 --- a/sound/soc/codecs/ak5558.c +++ b/sound/soc/codecs/ak5558.c @@ -423,7 +423,7 @@ static const struct of_device_id ak5558_i2c_dt_ids[] = { static struct i2c_driver ak5558_i2c_driver = { .driver = { .name = "ak5558", - .of_match_table = of_match_ptr(ak5558_i2c_dt_ids), + .of_match_table = ak5558_i2c_dt_ids, .pm = &ak5558_pm, }, .probe_new = ak5558_i2c_probe,
The driver can match only via the DT table so the table should be always used and the of_match_ptr does not have any sense (this also allows ACPI matching via PRP0001, even though it is not relevant here). This fixes compile warning (!CONFIG_OF on x86_64): sound/soc/codecs/ak5558.c:418:34: warning: ‘ak5558_i2c_dt_ids’ defined but not used [-Wunused-const-variable=] Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- sound/soc/codecs/ak5558.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)