| Message ID | 20230311111457.251475-2-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:55 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > The driver can be compile tested with !CONFIG_OF making certain data > unused (of_device_id is not used for device matching): It should be used for device matching I think, so I'd rather see it assigned for that purpose than hiding the issue. > > drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=] > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > drivers/iio/dac/ad5755.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c > index beadfa938d2d..a6dc8a0bfc29 100644 > --- a/drivers/iio/dac/ad5755.c > +++ b/drivers/iio/dac/ad5755.c > @@ -862,7 +862,7 @@ static const struct spi_device_id ad5755_id[] = { > }; > MODULE_DEVICE_TABLE(spi, ad5755_id); > > -static const struct of_device_id ad5755_of_match[] = { > +static const struct of_device_id ad5755_of_match[] __maybe_unused = { > { .compatible = "adi,ad5755" }, > { .compatible = "adi,ad5755-1" }, > { .compatible = "adi,ad5757" },
On 11/03/2023 13:22, Jonathan Cameron wrote: > On Sat, 11 Mar 2023 12:14:55 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> The driver can be compile tested with !CONFIG_OF making certain data >> unused (of_device_id is not used for device matching): > > It should be used for device matching I think, so I'd rather see > it assigned for that purpose than hiding the issue. That would require testing and changes. The device matching is via SPI table which has device data. Probably adding OF matching would require bigger changes to for handling the match data. This was intentional design in this driver, so we are not hiding here anything. > > >> >> drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=] > Best regards, Krzysztof
On Sat, 11 Mar 2023 13:25:33 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 11/03/2023 13:22, Jonathan Cameron wrote: > > On Sat, 11 Mar 2023 12:14:55 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> The driver can be compile tested with !CONFIG_OF making certain data > >> unused (of_device_id is not used for device matching): > > > > It should be used for device matching I think, so I'd rather see > > it assigned for that purpose than hiding the issue. > > That would require testing and changes. The device matching is via SPI > table which has device data. Probably adding OF matching would require > bigger changes to for handling the match data. > > This was intentional design in this driver, so we are not hiding here > anything. I doubt it was intentional. Mostly people do this because the magic fallbacks to find the spi_device_id entry work. If we'd noticed at review time it would not have gone in like this. Note that the spi_match_id() use of_modalias_node() which has stripped the vendor id off the compatible then matches against the spi_device_id table. So it 'should' just work. Now ideally we'd switch to spi_get_device_match_data() but that needs more significant changes. Though simple enough ones that review would be enough. Just need to use pointers to the ad75755_chip_info_tbl entries rather than the enum in both the spi id table and the of one - this avoids the issue with the enum value of 0 counting as a failed match. > > > > > > >> > >> drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=] > > > > > Best regards, > Krzysztof >
On 11/03/2023 19:31, Jonathan Cameron wrote: > On Sat, 11 Mar 2023 13:25:33 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 11/03/2023 13:22, Jonathan Cameron wrote: >>> On Sat, 11 Mar 2023 12:14:55 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> The driver can be compile tested with !CONFIG_OF making certain data >>>> unused (of_device_id is not used for device matching): >>> >>> It should be used for device matching I think, so I'd rather see >>> it assigned for that purpose than hiding the issue. >> >> That would require testing and changes. The device matching is via SPI >> table which has device data. Probably adding OF matching would require >> bigger changes to for handling the match data. >> >> This was intentional design in this driver, so we are not hiding here >> anything. > > I doubt it was intentional. Mostly people do this because the magic > fallbacks to find the spi_device_id entry work. > > If we'd noticed at review time it would not have gone in like this. > Note that the spi_match_id() use of_modalias_node() which has stripped the > vendor id off the compatible then matches against the spi_device_id > table. > > So it 'should' just work. Now ideally we'd switch to > spi_get_device_match_data() but that needs more significant changes. > Though simple enough ones that review would be enough. > > Just need to use pointers to the ad75755_chip_info_tbl entries > rather than the enum in both the spi id table and the of one - this > avoids the issue with the enum value of 0 counting as a failed match. It's not that simple change... maybe you are right that adding match data to OF table would not break anything, but to me it is something substantial and requiring testing, which obviously I cannot do. Therefore I am going to skip this one (thus the error stays). Best regards, Krzysztof
diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c index beadfa938d2d..a6dc8a0bfc29 100644 --- a/drivers/iio/dac/ad5755.c +++ b/drivers/iio/dac/ad5755.c @@ -862,7 +862,7 @@ static const struct spi_device_id ad5755_id[] = { }; MODULE_DEVICE_TABLE(spi, ad5755_id); -static const struct of_device_id ad5755_of_match[] = { +static const struct of_device_id ad5755_of_match[] __maybe_unused = { { .compatible = "adi,ad5755" }, { .compatible = "adi,ad5755-1" }, { .compatible = "adi,ad5757" },
The driver can be compile tested with !CONFIG_OF making certain data unused (of_device_id is not used for device matching): drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=] Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/iio/dac/ad5755.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)