| Message ID | 20240213101220.459641-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: meson: add helpers for clk_disable_unprepare | expand |
On 13/02/2024 11:11, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Casting between incompatible function types causes a warning with clang-16 > because it breaks control flow integrity (KCFI) rules: > > sound/soc/meson/aiu.c:243:12: error: cast from 'void (*)(struct clk *)' to 'void (*)(void *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict] > 243 | (void(*)(void *))clk_disable_unprepare, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/device.h:406:34: note: expanded from macro 'devm_add_action_or_reset' > 406 | __devm_add_action_or_reset(dev, action, data, #action) > | ^~~~~~ > sound/soc/meson/t9015.c:274:4: error: cast from 'void (*)(struct clk *)' to 'void (*)(void *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict] > 274 | (void(*)(void *))clk_disable_unprepare, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/device.h:406:34: note: expanded from macro 'devm_add_action_or_reset' > 406 | __devm_add_action_or_reset(dev, action, data, #action) > | ^~~~~~ > > These two meson drivers cast clk_disable_unprepare() into a different type > in order to have it automatically called from the driver relase. Add > trivial helpers to do the same using correct types. > > Fixes: 33901f5b9b16 ("ASoC: meson: add t9015 internal DAC driver") > Fixes: 6ae9ca9ce986 ("ASoC: meson: aiu: add i2s and spdif support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > sound/soc/meson/aiu.c | 9 ++++++--- > sound/soc/meson/t9015.c | 9 ++++++--- > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c > index 7109b81cc3d0..211f360df284 100644 > --- a/sound/soc/meson/aiu.c > +++ b/sound/soc/meson/aiu.c > @@ -209,6 +209,11 @@ static const char * const aiu_spdif_ids[] = { > [MCLK] = "spdif_mclk_sel" > }; > > +static void aiu_clk_disable(void *clk) > +{ > + clk_disable_unprepare(clk); > +} > + > static int aiu_clk_get(struct device *dev) > { > struct aiu *aiu = dev_get_drvdata(dev); > @@ -239,9 +244,7 @@ static int aiu_clk_get(struct device *dev) > return ret; > } > > - ret = devm_add_action_or_reset(dev, > - (void(*)(void *))clk_disable_unprepare, > - aiu->pclk); > + ret = devm_add_action_or_reset(dev, aiu_clk_disable, aiu->pclk); > if (ret) > dev_err(dev, "failed to add reset action on pclk"); > > diff --git a/sound/soc/meson/t9015.c b/sound/soc/meson/t9015.c > index 9c6b4dac6893..e0f9a603a5e9 100644 > --- a/sound/soc/meson/t9015.c > +++ b/sound/soc/meson/t9015.c > @@ -243,6 +243,11 @@ static const struct regmap_config t9015_regmap_config = { > .max_register = POWER_CFG, > }; > > +static void t9015_clk_disable(void *clk) > +{ > + clk_disable_unprepare(clk); > +} > + > static int t9015_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -270,9 +275,7 @@ static int t9015_probe(struct platform_device *pdev) > return ret; > } > > - ret = devm_add_action_or_reset(dev, > - (void(*)(void *))clk_disable_unprepare, > - priv->pclk); > + ret = devm_add_action_or_reset(dev, t9015_clk_disable, priv->pclk); > if (ret) > return ret; > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
On Tue 13 Feb 2024 at 11:11, Arnd Bergmann <arnd@kernel.org> wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Casting between incompatible function types causes a warning with clang-16 > because it breaks control flow integrity (KCFI) rules: > > sound/soc/meson/aiu.c:243:12: error: cast from 'void (*)(struct clk *)' to > 'void (*)(void *)' converts to incompatible function type > [-Werror,-Wcast-function-type-strict] > 243 | (void(*)(void *))clk_disable_unprepare, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/device.h:406:34: note: expanded from macro 'devm_add_action_or_reset' > 406 | __devm_add_action_or_reset(dev, action, data, #action) > | ^~~~~~ > sound/soc/meson/t9015.c:274:4: error: cast from 'void (*)(struct clk *)' to > 'void (*)(void *)' converts to incompatible function type > [-Werror,-Wcast-function-type-strict] > 274 | (void(*)(void *))clk_disable_unprepare, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/device.h:406:34: note: expanded from macro 'devm_add_action_or_reset' > 406 | __devm_add_action_or_reset(dev, action, data, #action) > | ^~~~~~ > > These two meson drivers cast clk_disable_unprepare() into a different type > in order to have it automatically called from the driver relase. Add > trivial helpers to do the same using correct types. > > Fixes: 33901f5b9b16 ("ASoC: meson: add t9015 internal DAC driver") > Fixes: 6ae9ca9ce986 ("ASoC: meson: aiu: add i2s and spdif support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > sound/soc/meson/aiu.c | 9 ++++++--- > sound/soc/meson/t9015.c | 9 ++++++--- > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c > index 7109b81cc3d0..211f360df284 100644 > --- a/sound/soc/meson/aiu.c > +++ b/sound/soc/meson/aiu.c > @@ -209,6 +209,11 @@ static const char * const aiu_spdif_ids[] = { > [MCLK] = "spdif_mclk_sel" > }; > > +static void aiu_clk_disable(void *clk) > +{ > + clk_disable_unprepare(clk); > +} > + > static int aiu_clk_get(struct device *dev) > { > struct aiu *aiu = dev_get_drvdata(dev); > @@ -239,9 +244,7 @@ static int aiu_clk_get(struct device *dev) > return ret; > } > > - ret = devm_add_action_or_reset(dev, > - (void(*)(void *))clk_disable_unprepare, > - aiu->pclk); > + ret = devm_add_action_or_reset(dev, aiu_clk_disable, aiu->pclk); Hi Arnd, This probably pre-dates the introduction of devm_clk_get_enabled() It would probably be better to use that instead of duplicating helper functions which do the same thing. If for any reason it is not possible, a common helper in clk.h would preferable I think. > if (ret) > dev_err(dev, "failed to add reset action on pclk"); > > diff --git a/sound/soc/meson/t9015.c b/sound/soc/meson/t9015.c > index 9c6b4dac6893..e0f9a603a5e9 100644 > --- a/sound/soc/meson/t9015.c > +++ b/sound/soc/meson/t9015.c > @@ -243,6 +243,11 @@ static const struct regmap_config t9015_regmap_config = { > .max_register = POWER_CFG, > }; > > +static void t9015_clk_disable(void *clk) > +{ > + clk_disable_unprepare(clk); > +} > + > static int t9015_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -270,9 +275,7 @@ static int t9015_probe(struct platform_device *pdev) > return ret; > } > > - ret = devm_add_action_or_reset(dev, > - (void(*)(void *))clk_disable_unprepare, > - priv->pclk); > + ret = devm_add_action_or_reset(dev, t9015_clk_disable, priv->pclk); > if (ret) > return ret;
On Tue, Feb 13, 2024, at 11:36, Jerome Brunet wrote: > On Tue 13 Feb 2024 at 11:11, Arnd Bergmann <arnd@kernel.org> wrote: > This probably pre-dates the introduction of devm_clk_get_enabled() > It would probably be better to use that instead of duplicating helper > functions which do the same thing. Ah, I had not thought of that interface either, so you are probably right that this is the best way to do it. Can you send a replacement patch then and add my Reported-by? I also sent the same patch for drivers/nvmem/meson-efuse.c, which I guess will also need the same treatment. I also checked and saw that all three files already had this code in linux-6.0 when devm_clk_get_enabled() got added. > If for any reason it is not possible, a common helper in clk.h would > preferable I think. I can't think of anything that prevents us from using devm_clk_get_enabled() here. Arnd
On Tue 13 Feb 2024 at 11:52, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Tue, Feb 13, 2024, at 11:36, Jerome Brunet wrote: >> On Tue 13 Feb 2024 at 11:11, Arnd Bergmann <arnd@kernel.org> wrote: > >> This probably pre-dates the introduction of devm_clk_get_enabled() >> It would probably be better to use that instead of duplicating helper >> functions which do the same thing. > > Ah, I had not thought of that interface either, so you are probably > right that this is the best way to do it. > Can you send a replacement patch then and add my Reported-by? Sure. How may I reproduce the problem ? Just tried with 'Debian clang version 16.0.6 (19)', no warning. I suppose I need to add something ? > > I also sent the same patch for drivers/nvmem/meson-efuse.c, which > I guess will also need the same treatment. I also checked and saw > that all three files already had this code in linux-6.0 when > devm_clk_get_enabled() got added. > >> If for any reason it is not possible, a common helper in clk.h would >> preferable I think. > > I can't think of anything that prevents us from using > devm_clk_get_enabled() here. > > Arnd
On Tue, Feb 13, 2024, at 12:09, Jerome Brunet wrote: > On Tue 13 Feb 2024 at 11:52, "Arnd Bergmann" <arnd@arndb.de> wrote: > >> On Tue, Feb 13, 2024, at 11:36, Jerome Brunet wrote: >>> On Tue 13 Feb 2024 at 11:11, Arnd Bergmann <arnd@kernel.org> wrote: >> >>> This probably pre-dates the introduction of devm_clk_get_enabled() >>> It would probably be better to use that instead of duplicating helper >>> functions which do the same thing. >> >> Ah, I had not thought of that interface either, so you are probably >> right that this is the best way to do it. >> Can you send a replacement patch then and add my Reported-by? > > Sure. > > How may I reproduce the problem ? > Just tried with 'Debian clang version 16.0.6 (19)', no warning. > > I suppose I need to add something ? The warning is currently only enabled at W=1 level until we have fixed all the existing diagnostic output. I have sent the other ones today, but for now, please test with 'make W=1'. Arnd
diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c index 7109b81cc3d0..211f360df284 100644 --- a/sound/soc/meson/aiu.c +++ b/sound/soc/meson/aiu.c @@ -209,6 +209,11 @@ static const char * const aiu_spdif_ids[] = { [MCLK] = "spdif_mclk_sel" }; +static void aiu_clk_disable(void *clk) +{ + clk_disable_unprepare(clk); +} + static int aiu_clk_get(struct device *dev) { struct aiu *aiu = dev_get_drvdata(dev); @@ -239,9 +244,7 @@ static int aiu_clk_get(struct device *dev) return ret; } - ret = devm_add_action_or_reset(dev, - (void(*)(void *))clk_disable_unprepare, - aiu->pclk); + ret = devm_add_action_or_reset(dev, aiu_clk_disable, aiu->pclk); if (ret) dev_err(dev, "failed to add reset action on pclk"); diff --git a/sound/soc/meson/t9015.c b/sound/soc/meson/t9015.c index 9c6b4dac6893..e0f9a603a5e9 100644 --- a/sound/soc/meson/t9015.c +++ b/sound/soc/meson/t9015.c @@ -243,6 +243,11 @@ static const struct regmap_config t9015_regmap_config = { .max_register = POWER_CFG, }; +static void t9015_clk_disable(void *clk) +{ + clk_disable_unprepare(clk); +} + static int t9015_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -270,9 +275,7 @@ static int t9015_probe(struct platform_device *pdev) return ret; } - ret = devm_add_action_or_reset(dev, - (void(*)(void *))clk_disable_unprepare, - priv->pclk); + ret = devm_add_action_or_reset(dev, t9015_clk_disable, priv->pclk); if (ret) return ret;