| Message ID | 20230311111457.251475-4-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused | expand |
On Sat, 11 Mar 2023 12:14:57 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making > certain data unused: > > drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=] > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Hi Krysztof Thanks for looking at these warnings. Drop the protection macros instead. The tables are trivial in size and the of_match_ptr() breaks some ways this driver can be used. ACPI_PTR() isn't as bad, but is pretty much pointless given this size of the array. Jonathan > --- > drivers/iio/proximity/sx9500.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c > index 8794e75e5bf9..840db1953998 100644 > --- a/drivers/iio/proximity/sx9500.c > +++ b/drivers/iio/proximity/sx9500.c > @@ -1036,13 +1036,13 @@ static const struct acpi_device_id sx9500_acpi_match[] = { > }; > MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match); > > -static const struct of_device_id sx9500_of_match[] = { > +static const struct of_device_id sx9500_of_match[] __maybe_unused = { > { .compatible = "semtech,sx9500", }, > { } > }; > MODULE_DEVICE_TABLE(of, sx9500_of_match); > > -static const struct i2c_device_id sx9500_id[] = { > +static const struct i2c_device_id sx9500_id[] __maybe_unused = { > {"sx9500", 0}, > { }, > };
On 11/03/2023 13:28, Jonathan Cameron wrote: > On Sat, 11 Mar 2023 12:14:57 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making >> certain data unused: >> >> drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=] >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Hi Krysztof > > Thanks for looking at these warnings. > > Drop the protection macros instead. The tables are trivial in size and > the of_match_ptr() breaks some ways this driver can be used. > ACPI_PTR() isn't as bad, but is pretty much pointless given this size of > the array. > For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF platform, OF table is used. What usage exactly is broken here? What ways? Best regards, Krzysztof
On Sat, 11 Mar 2023 13:30:01 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 11/03/2023 13:28, Jonathan Cameron wrote: > > On Sat, 11 Mar 2023 12:14:57 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making > >> certain data unused: > >> > >> drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=] > >> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > Hi Krysztof > > > > Thanks for looking at these warnings. > > > > Drop the protection macros instead. The tables are trivial in size and > > the of_match_ptr() breaks some ways this driver can be used. > > ACPI_PTR() isn't as bad, but is pretty much pointless given this size of > > the array. > > > > For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF > platform, OF table is used. So you would think, but nope.. That's not how it works (I was surprised when I came across this the first time too) PRP0001 is magic and requires no specific support in an individual driver beyond not using that of_match_ptr() macro! https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754 Docs here https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450 > > What usage exactly is broken here? What ways? > > Best regards, > Krzysztof >
On 11/03/2023 19:44, Jonathan Cameron wrote: > On Sat, 11 Mar 2023 13:30:01 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 11/03/2023 13:28, Jonathan Cameron wrote: >>> On Sat, 11 Mar 2023 12:14:57 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making >>>> certain data unused: >>>> >>>> drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=] >>>> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>> >>> Hi Krysztof >>> >>> Thanks for looking at these warnings. >>> >>> Drop the protection macros instead. The tables are trivial in size and >>> the of_match_ptr() breaks some ways this driver can be used. >>> ACPI_PTR() isn't as bad, but is pretty much pointless given this size of >>> the array. >>> >> >> For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF >> platform, OF table is used. > > So you would think, but nope.. That's not how it works (I was surprised > when I came across this the first time too) > > PRP0001 is magic and requires no specific support in an individual > driver beyond not using that of_match_ptr() macro! I know, we talk about ACPI table. > > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754 > Docs here > https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450 The code is compile when CONFIG_ACPI is defined, right? Then you have ACPI table, so what for ACPI platform is missing? ACPI platform has ACPI table. Best regards, Krzysztof
On Sun, 12 Mar 2023 11:17:05 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 11/03/2023 19:44, Jonathan Cameron wrote: > > On Sat, 11 Mar 2023 13:30:01 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> On 11/03/2023 13:28, Jonathan Cameron wrote: > >>> On Sat, 11 Mar 2023 12:14:57 +0100 > >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>> > >>>> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making > >>>> certain data unused: > >>>> > >>>> drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=] > >>>> > >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>> > >>> Hi Krysztof > >>> > >>> Thanks for looking at these warnings. > >>> > >>> Drop the protection macros instead. The tables are trivial in size and > >>> the of_match_ptr() breaks some ways this driver can be used. > >>> ACPI_PTR() isn't as bad, but is pretty much pointless given this size of > >>> the array. > >>> > >> > >> For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF > >> platform, OF table is used. > > > > So you would think, but nope.. That's not how it works (I was surprised > > when I came across this the first time too) > > > > PRP0001 is magic and requires no specific support in an individual > > driver beyond not using that of_match_ptr() macro! > > I know, we talk about ACPI table. I'm not sure I follow. I thought by ACPI table you meant the acpi_device_id table pointed to by acpi_match_table in struct device_driver. That one is not needed for PRP0001. It is irrelevant if there is one or not. Maybe the confusion is that you think the presence of an acpi_match table means we don't also check PRP0001? As you can see here https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L886 it is checked in all cases. If you meant the DSDT table being provide by the firmware I don't see the relevance to this discussion. > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754 > > Docs here > > https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450 > > The code is compile when CONFIG_ACPI is defined, right? Then you have > ACPI table, so what for ACPI platform is missing? ACPI platform has ACPI > table. I don't follow. A given ACPI platform may provide in DSDT one of two choices to bind to this driver. Either it provides an entry from the acpi_device_id table, or it must use PRP0001 and a compatible entry from the of_device_id table. That only works if of_match_ptr() is not used. As a side note, both the IDs in the ACPI match table are not valid IDs for use in DSDT. We are supporting them only because they have been used on shipping devices. Semtech does have a PNP ID of STH but that's not the one used. Anyhow, to be clear. For IIO drivers, don't use of_match_ptr() or ACPI_PTR(). There are some legacy cases that we haven't cleaned up yet, but I'm not taking patches that add any new ones without a very very strong argument in favour and so far no one has successfully made one. Jonathan > > Best regards, > Krzysztof >
On 12/03/2023 15:14, Jonathan Cameron wrote: > On Sun, 12 Mar 2023 11:17:05 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 11/03/2023 19:44, Jonathan Cameron wrote: >>> On Sat, 11 Mar 2023 13:30:01 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 11/03/2023 13:28, Jonathan Cameron wrote: >>>>> On Sat, 11 Mar 2023 12:14:57 +0100 >>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>>> >>>>>> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making >>>>>> certain data unused: >>>>>> >>>>>> drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=] >>>>>> >>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>> >>>>> Hi Krysztof >>>>> >>>>> Thanks for looking at these warnings. >>>>> >>>>> Drop the protection macros instead. The tables are trivial in size and >>>>> the of_match_ptr() breaks some ways this driver can be used. >>>>> ACPI_PTR() isn't as bad, but is pretty much pointless given this size of >>>>> the array. >>>>> >>>> >>>> For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF >>>> platform, OF table is used. >>> >>> So you would think, but nope.. That's not how it works (I was surprised >>> when I came across this the first time too) >>> >>> PRP0001 is magic and requires no specific support in an individual >>> driver beyond not using that of_match_ptr() macro! >> >> I know, we talk about ACPI table. > > I'm not sure I follow. I thought by ACPI table you meant the acpi_device_id > table pointed to by acpi_match_table in struct device_driver. > > That one is not needed for PRP0001. It is irrelevant if there is one or not. > > Maybe the confusion is that you think the presence of an acpi_match table means > we don't also check PRP0001? As you can see here > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L886 > it is checked in all cases. > > If you meant the DSDT table being provide by the firmware I don't see the relevance > to this discussion. > >> >>> >>> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754 >>> Docs here >>> https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450 >> >> The code is compile when CONFIG_ACPI is defined, right? Then you have >> ACPI table, so what for ACPI platform is missing? ACPI platform has ACPI >> table. > > I don't follow. A given ACPI platform may provide in DSDT one of two choices > to bind to this driver. OK, I understand your point. I assumed we do not care at all about PRP0001 if ACPI is enabled, because then we simply use ACPI table. But indeed they might for example be not in sync... > > Either it provides an entry from the acpi_device_id table, or it must > use PRP0001 and a compatible entry from the of_device_id table. That only works > if of_match_ptr() is not used. > > As a side note, both the IDs in the ACPI match table are not valid IDs for use > in DSDT. We are supporting them only because they have been used on shipping devices. > Semtech does have a PNP ID of STH but that's not the one used. > > Anyhow, to be clear. For IIO drivers, don't use of_match_ptr() > or ACPI_PTR(). There are some legacy cases that we haven't cleaned up > yet, but I'm not taking patches that add any new ones without a very very > strong argument in favour and so far no one has successfully made one. Best regards, Krzysztof
diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c index 8794e75e5bf9..840db1953998 100644 --- a/drivers/iio/proximity/sx9500.c +++ b/drivers/iio/proximity/sx9500.c @@ -1036,13 +1036,13 @@ static const struct acpi_device_id sx9500_acpi_match[] = { }; MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match); -static const struct of_device_id sx9500_of_match[] = { +static const struct of_device_id sx9500_of_match[] __maybe_unused = { { .compatible = "semtech,sx9500", }, { } }; MODULE_DEVICE_TABLE(of, sx9500_of_match); -static const struct i2c_device_id sx9500_id[] = { +static const struct i2c_device_id sx9500_id[] __maybe_unused = { {"sx9500", 0}, { }, };
The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making certain data unused: drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=] Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/iio/proximity/sx9500.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)