| Message ID | 20190920145543.1732316-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mbox: qcom: avoid unused-variable warning | expand |
Quoting Arnd Bergmann (2019-09-20 07:55:29) > Without CONFIG_OF, there is no reference to the apcs_clk_match_table[] > array, causing a harmless warning: > > drivers/mailbox/qcom-apcs-ipc-mailbox.c:57:28: error: unused variable 'apcs_clk_match_table' [-Werror,-Wunused-variable] > const struct of_device_id apcs_clk_match_table[] = { > > Move the variable out of the variable scope and mark it 'static' > to avoid the warning (static const variables get silently dropped > by the compiler), and avoid the on-stack copy at the same time. > > Fixes: 78c86458a440 ("mbox: qcom: add APCS child device for QCS404") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/mailbox/qcom-apcs-ipc-mailbox.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c > index eeebafd546e5..10557a950c2d 100644 > --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c > +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c > @@ -45,6 +45,12 @@ static const struct mbox_chan_ops qcom_apcs_ipc_ops = { > .send_data = qcom_apcs_ipc_send_data, > }; > > +static const struct of_device_id apcs_clk_match_table[] = { > + { .compatible = "qcom,msm8916-apcs-kpss-global", }, > + { .compatible = "qcom,qcs404-apcs-apps-global", }, > + {} > +}; > + > static int qcom_apcs_ipc_probe(struct platform_device *pdev) > { > struct qcom_apcs_ipc *apcs; > @@ -54,11 +60,6 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev) > void __iomem *base; > unsigned long i; > int ret; > - const struct of_device_id apcs_clk_match_table[] = { Does marking it static here work too? It would be nice to limit the scope of this variable to this function instead of making it a global. Also, it might be slightly smaller code size if that works. > - { .compatible = "qcom,msm8916-apcs-kpss-global", }, > - { .compatible = "qcom,qcs404-apcs-apps-global", }, > - {} > - }; > > apcs = devm_kzalloc(&pdev->dev, sizeof(*apcs), GFP_KERNEL); > if (!apcs)
On Fri, Sep 20, 2019 at 6:45 PM Stephen Boyd <sboyd@kernel.org> wrote: > > @@ -54,11 +60,6 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev) > > void __iomem *base; > > unsigned long i; > > int ret; > > - const struct of_device_id apcs_clk_match_table[] = { > > Does marking it static here work too? It would be nice to limit the > scope of this variable to this function instead of making it a global. > Also, it might be slightly smaller code size if that works. No, I just tried and the warning returned. Arnd
Hi Stephen, On Fri, Sep 20, 2019 at 11:06 PM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Arnd Bergmann (2019-09-20 12:27:50) > > On Fri, Sep 20, 2019 at 6:45 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > @@ -54,11 +60,6 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev) > > > > void __iomem *base; > > > > unsigned long i; > > > > int ret; > > > > - const struct of_device_id apcs_clk_match_table[] = { > > > > > > Does marking it static here work too? It would be nice to limit the > > > scope of this variable to this function instead of making it a global. > > > Also, it might be slightly smaller code size if that works. > > > > No, I just tried and the warning returned. It's there for the convenience for the user, so he doesn't have to add it himself explicitly. > ("of/device: Nullify match table in of_match_device() for CONFIG_OF=n"), > but it's been 5 years! Surely we can revert this change now that commit > 219a7bc577e6 ("spi: rspi: Use of_device_get_match_data() helper") is > merged. Right, the particular error case in the RSPI driver can no longer happen. Calling of_device_get_match_data() is the better solution anyway, as that uses the match table stored in dev->driver->of_match_table, so there's no need to pass the table explicitly, and conditionally. Hence... > --- a/drivers/leds/leds-pca9532.c > +++ b/drivers/leds/leds-pca9532.c > @@ -472,7 +472,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np) > int i = 0; > const char *state; > > - match = of_match_device(of_pca9532_leds_match, dev); > + match = of_match_device(of_match_ptr(of_pca9532_leds_match), dev); > if (!match) > return ERR_PTR(-ENODEV); Please convert to of_device_get_match_data() instead of adding of_match_ptr() invocations... > @@ -525,7 +525,7 @@ static int pca9532_probe(struct i2c_client *client, > dev_err(&client->dev, "no platform data\n"); > return -EINVAL; > } > - of_id = of_match_device(of_pca9532_leds_match, > + of_id = of_match_device(of_match_ptr(of_pca9532_leds_match), Likewise. > --- a/sound/soc/jz4740/jz4740-i2s.c > +++ b/sound/soc/jz4740/jz4740-i2s.c > @@ -503,7 +503,7 @@ static int jz4740_i2s_dev_probe(struct platform_device *pdev) > if (!i2s) > return -ENOMEM; > > - match = of_match_device(jz4740_of_matches, &pdev->dev); > + match = of_match_device(of_match_ptr(jz4740_of_matches), &pdev->dev); Given of_device_get_match_data() returns NULL, not an error code, even this one could use of_device_get_match_data(). > if (match) > i2s->version = (enum jz47xx_i2s_version)match->data; Gr{oetje,eeting}s, Geert
Quoting Geert Uytterhoeven (2019-09-26 06:07:13) > Hi Stephen, > > On Fri, Sep 20, 2019 at 11:06 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Arnd Bergmann (2019-09-20 12:27:50) > > > On Fri, Sep 20, 2019 at 6:45 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > @@ -54,11 +60,6 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev) > > > > > void __iomem *base; > > > > > unsigned long i; > > > > > int ret; > > > > > - const struct of_device_id apcs_clk_match_table[] = { > > > > > > > > Does marking it static here work too? It would be nice to limit the > > > > scope of this variable to this function instead of making it a global. > > > > Also, it might be slightly smaller code size if that works. > > > > > > No, I just tried and the warning returned. > > It's there for the convenience for the user, so he doesn't have to add it > himself explicitly. > > > ("of/device: Nullify match table in of_match_device() for CONFIG_OF=n"), > > but it's been 5 years! Surely we can revert this change now that commit > > 219a7bc577e6 ("spi: rspi: Use of_device_get_match_data() helper") is > > merged. > > Right, the particular error case in the RSPI driver can no longer happen. > > Calling of_device_get_match_data() is the better solution anyway, as > that uses the match table stored in dev->driver->of_match_table, so > there's no need to pass the table explicitly, and conditionally. > > Hence... > > > --- a/drivers/leds/leds-pca9532.c > > +++ b/drivers/leds/leds-pca9532.c > > @@ -472,7 +472,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np) > > int i = 0; > > const char *state; > > > > - match = of_match_device(of_pca9532_leds_match, dev); > > + match = of_match_device(of_match_ptr(of_pca9532_leds_match), dev); > > if (!match) > > return ERR_PTR(-ENODEV); > > Please convert to of_device_get_match_data() instead of adding > of_match_ptr() invocations... How is this workable? I left it as of_match_device() because the value returned may be 0 for the enum and that looks the same as NULL. > > > @@ -525,7 +525,7 @@ static int pca9532_probe(struct i2c_client *client, > > dev_err(&client->dev, "no platform data\n"); > > return -EINVAL; > > } > > - of_id = of_match_device(of_pca9532_leds_match, > > + of_id = of_match_device(of_match_ptr(of_pca9532_leds_match), > > Likewise. > > > --- a/sound/soc/jz4740/jz4740-i2s.c > > +++ b/sound/soc/jz4740/jz4740-i2s.c > > @@ -503,7 +503,7 @@ static int jz4740_i2s_dev_probe(struct platform_device *pdev) > > if (!i2s) > > return -ENOMEM; > > > > - match = of_match_device(jz4740_of_matches, &pdev->dev); > > + match = of_match_device(of_match_ptr(jz4740_of_matches), &pdev->dev); > > Given of_device_get_match_data() returns NULL, not an error code, even > this one could use of_device_get_match_data(). >
Hi Stephen, On Fri, Sep 27, 2019 at 8:26 PM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Geert Uytterhoeven (2019-09-26 06:07:13) > > On Fri, Sep 20, 2019 at 11:06 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > Quoting Arnd Bergmann (2019-09-20 12:27:50) > > > > On Fri, Sep 20, 2019 at 6:45 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > @@ -54,11 +60,6 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev) > > > > > > void __iomem *base; > > > > > > unsigned long i; > > > > > > int ret; > > > > > > - const struct of_device_id apcs_clk_match_table[] = { > > > > > > > > > > Does marking it static here work too? It would be nice to limit the > > > > > scope of this variable to this function instead of making it a global. > > > > > Also, it might be slightly smaller code size if that works. > > > > > > > > No, I just tried and the warning returned. > > > > It's there for the convenience for the user, so he doesn't have to add it > > himself explicitly. > > > > > ("of/device: Nullify match table in of_match_device() for CONFIG_OF=n"), > > > but it's been 5 years! Surely we can revert this change now that commit > > > 219a7bc577e6 ("spi: rspi: Use of_device_get_match_data() helper") is > > > merged. > > > > Right, the particular error case in the RSPI driver can no longer happen. > > > > Calling of_device_get_match_data() is the better solution anyway, as > > that uses the match table stored in dev->driver->of_match_table, so > > there's no need to pass the table explicitly, and conditionally. > > > > Hence... > > > > > --- a/drivers/leds/leds-pca9532.c > > > +++ b/drivers/leds/leds-pca9532.c > > > @@ -472,7 +472,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np) > > > int i = 0; > > > const char *state; > > > > > > - match = of_match_device(of_pca9532_leds_match, dev); > > > + match = of_match_device(of_match_ptr(of_pca9532_leds_match), dev); > > > if (!match) > > > return ERR_PTR(-ENODEV); > > > > Please convert to of_device_get_match_data() instead of adding > > of_match_ptr() invocations... > > How is this workable? I left it as of_match_device() because the value > returned may be 0 for the enum and that looks the same as NULL. This function is used for the DT case only, so there will always be a match. Hence you can do devid = (int)(uintptr_t)of_device_get_match_data(dev). Gr{oetje,eeting}s, Geert
Quoting Geert Uytterhoeven (2019-09-27 12:10:13) > On Fri, Sep 27, 2019 at 8:26 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Geert Uytterhoeven (2019-09-26 06:07:13) > > > On Fri, Sep 20, 2019 at 11:06 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Arnd Bergmann (2019-09-20 12:27:50) > > > > > On Fri, Sep 20, 2019 at 6:45 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > --- a/drivers/leds/leds-pca9532.c > > > > +++ b/drivers/leds/leds-pca9532.c > > > > @@ -472,7 +472,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np) > > > > int i = 0; > > > > const char *state; > > > > > > > > - match = of_match_device(of_pca9532_leds_match, dev); > > > > + match = of_match_device(of_match_ptr(of_pca9532_leds_match), dev); > > > > if (!match) > > > > return ERR_PTR(-ENODEV); > > > > > > Please convert to of_device_get_match_data() instead of adding > > > of_match_ptr() invocations... > > > > How is this workable? I left it as of_match_device() because the value > > returned may be 0 for the enum and that looks the same as NULL. > > This function is used for the DT case only, so there will always be a match. > Hence you can do devid = (int)(uintptr_t)of_device_get_match_data(dev). > Ok. Let me send out a pile of patches.
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c index eeebafd546e5..10557a950c2d 100644 --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c @@ -45,6 +45,12 @@ static const struct mbox_chan_ops qcom_apcs_ipc_ops = { .send_data = qcom_apcs_ipc_send_data, }; +static const struct of_device_id apcs_clk_match_table[] = { + { .compatible = "qcom,msm8916-apcs-kpss-global", }, + { .compatible = "qcom,qcs404-apcs-apps-global", }, + {} +}; + static int qcom_apcs_ipc_probe(struct platform_device *pdev) { struct qcom_apcs_ipc *apcs; @@ -54,11 +60,6 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev) void __iomem *base; unsigned long i; int ret; - const struct of_device_id apcs_clk_match_table[] = { - { .compatible = "qcom,msm8916-apcs-kpss-global", }, - { .compatible = "qcom,qcs404-apcs-apps-global", }, - {} - }; apcs = devm_kzalloc(&pdev->dev, sizeof(*apcs), GFP_KERNEL); if (!apcs)
Without CONFIG_OF, there is no reference to the apcs_clk_match_table[] array, causing a harmless warning: drivers/mailbox/qcom-apcs-ipc-mailbox.c:57:28: error: unused variable 'apcs_clk_match_table' [-Werror,-Wunused-variable] const struct of_device_id apcs_clk_match_table[] = { Move the variable out of the variable scope and mark it 'static' to avoid the warning (static const variables get silently dropped by the compiler), and avoid the on-stack copy at the same time. Fixes: 78c86458a440 ("mbox: qcom: add APCS child device for QCS404") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/mailbox/qcom-apcs-ipc-mailbox.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)