| Message ID | 20250211-aux-device-create-helper-v3-6-7edb50524909@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | driver core: auxiliary bus: add device creation helper | expand |
Jerome Brunet wrote: > The auxiliary device creation of this driver is simple enough to > use the available auxiliary device creation helper. > > Use it and remove some boilerplate code. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/imx/clk-imx8mp-audiomix.c | 56 ++++------------------------------- > 1 file changed, 6 insertions(+), 50 deletions(-) > > diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c > index c409fc7e061869988f83c7df3ef7860500426323..988a5fffeb4e0e481ec57038d9d1f1b43432fc98 100644 > --- a/drivers/clk/imx/clk-imx8mp-audiomix.c > +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c > @@ -228,64 +228,20 @@ struct clk_imx8mp_audiomix_priv { > struct clk_hw_onecell_data clk_data; > }; > > -#if IS_ENABLED(CONFIG_RESET_CONTROLLER) I see the Kconfig ... select AUXILIARY_BUS if RESET_CONTROLLER But I don't see how this code is omitted without AUXILIARY_BUS. Is this kconfig check safe to remove? Ira > - > -static void clk_imx8mp_audiomix_reset_unregister_adev(void *_adev) > -{ > - struct auxiliary_device *adev = _adev; > - > - auxiliary_device_delete(adev); > - auxiliary_device_uninit(adev); > -} > - > -static void clk_imx8mp_audiomix_reset_adev_release(struct device *dev) > +static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev) > { > - struct auxiliary_device *adev = to_auxiliary_dev(dev); > - > - kfree(adev); > -} > - > -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, > - struct clk_imx8mp_audiomix_priv *priv) > -{ > - struct auxiliary_device *adev __free(kfree) = NULL; > - int ret; > + struct auxiliary_device *adev; > > if (!of_property_present(dev->of_node, "#reset-cells")) > return 0; > > - adev = kzalloc(sizeof(*adev), GFP_KERNEL); > - if (!adev) > - return -ENOMEM; > - > - adev->name = "reset"; > - adev->dev.parent = dev; > - adev->dev.release = clk_imx8mp_audiomix_reset_adev_release; > - > - ret = auxiliary_device_init(adev); > - if (ret) > - return ret; > - > - ret = auxiliary_device_add(adev); > - if (ret) { > - auxiliary_device_uninit(adev); > - return ret; > - } > - > - return devm_add_action_or_reset(dev, clk_imx8mp_audiomix_reset_unregister_adev, > - no_free_ptr(adev)); > -} > - > -#else /* !CONFIG_RESET_CONTROLLER */ > + adev = devm_auxiliary_device_create(dev, "reset", NULL, 0); > + if (IS_ERR_OR_NULL(adev)) > + return PTR_ERR(adev); > > -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, > - struct clk_imx8mp_audiomix_priv *priv) > -{ > return 0; > } > > -#endif /* !CONFIG_RESET_CONTROLLER */ > - > static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save) > { > struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev); > @@ -408,7 +364,7 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev) > if (ret) > goto err_clk_register; > > - ret = clk_imx8mp_audiomix_reset_controller_register(dev, priv); > + ret = clk_imx8mp_audiomix_reset_controller_register(dev); > if (ret) > goto err_clk_register; > > > -- > 2.45.2 >
On Fri 14 Feb 2025 at 10:15, Ira Weiny <ira.weiny@intel.com> wrote: > Jerome Brunet wrote: >> The auxiliary device creation of this driver is simple enough to >> use the available auxiliary device creation helper. >> >> Use it and remove some boilerplate code. >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/clk/imx/clk-imx8mp-audiomix.c | 56 ++++------------------------------- >> 1 file changed, 6 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c >> index c409fc7e061869988f83c7df3ef7860500426323..988a5fffeb4e0e481ec57038d9d1f1b43432fc98 100644 >> --- a/drivers/clk/imx/clk-imx8mp-audiomix.c >> +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c >> @@ -228,64 +228,20 @@ struct clk_imx8mp_audiomix_priv { >> struct clk_hw_onecell_data clk_data; >> }; >> >> -#if IS_ENABLED(CONFIG_RESET_CONTROLLER) > > I see the Kconfig ... > > select AUXILIARY_BUS if RESET_CONTROLLER > > But I don't see how this code is omitted without AUXILIARY_BUS. Is this > kconfig check safe to remove? Ahhh that's what this directive was for. I thought it was really odd to have an #if on RESET while auxialiary device was supposed to properly decouple the clock and reset part. To keep things as they were I'll add an #if on CONFIG_AUXILIARY_BUS I wonder if this driver should select CONFIG_AUXILIARY_BUS instead ? > > Ira > >> - >> -static void clk_imx8mp_audiomix_reset_unregister_adev(void *_adev) >> -{ >> - struct auxiliary_device *adev = _adev; >> - >> - auxiliary_device_delete(adev); >> - auxiliary_device_uninit(adev); >> -} >> - >> -static void clk_imx8mp_audiomix_reset_adev_release(struct device *dev) >> +static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev) >> { >> - struct auxiliary_device *adev = to_auxiliary_dev(dev); >> - >> - kfree(adev); >> -} >> - >> -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, >> - struct clk_imx8mp_audiomix_priv *priv) >> -{ >> - struct auxiliary_device *adev __free(kfree) = NULL; >> - int ret; >> + struct auxiliary_device *adev; >> >> if (!of_property_present(dev->of_node, "#reset-cells")) >> return 0; >> >> - adev = kzalloc(sizeof(*adev), GFP_KERNEL); >> - if (!adev) >> - return -ENOMEM; >> - >> - adev->name = "reset"; >> - adev->dev.parent = dev; >> - adev->dev.release = clk_imx8mp_audiomix_reset_adev_release; >> - >> - ret = auxiliary_device_init(adev); >> - if (ret) >> - return ret; >> - >> - ret = auxiliary_device_add(adev); >> - if (ret) { >> - auxiliary_device_uninit(adev); >> - return ret; >> - } >> - >> - return devm_add_action_or_reset(dev, clk_imx8mp_audiomix_reset_unregister_adev, >> - no_free_ptr(adev)); >> -} >> - >> -#else /* !CONFIG_RESET_CONTROLLER */ >> + adev = devm_auxiliary_device_create(dev, "reset", NULL, 0); >> + if (IS_ERR_OR_NULL(adev)) >> + return PTR_ERR(adev); >> >> -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, >> - struct clk_imx8mp_audiomix_priv *priv) >> -{ >> return 0; >> } >> >> -#endif /* !CONFIG_RESET_CONTROLLER */ >> - >> static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save) >> { >> struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev); >> @@ -408,7 +364,7 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev) >> if (ret) >> goto err_clk_register; >> >> - ret = clk_imx8mp_audiomix_reset_controller_register(dev, priv); >> + ret = clk_imx8mp_audiomix_reset_controller_register(dev); >> if (ret) >> goto err_clk_register; >> >> >> -- >> 2.45.2 >>
Jerome Brunet wrote: > On Fri 14 Feb 2025 at 10:15, Ira Weiny <ira.weiny@intel.com> wrote: > > > Jerome Brunet wrote: > >> The auxiliary device creation of this driver is simple enough to > >> use the available auxiliary device creation helper. > >> > >> Use it and remove some boilerplate code. > >> > >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > >> --- > >> drivers/clk/imx/clk-imx8mp-audiomix.c | 56 ++++------------------------------- > >> 1 file changed, 6 insertions(+), 50 deletions(-) > >> > >> diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c > >> index c409fc7e061869988f83c7df3ef7860500426323..988a5fffeb4e0e481ec57038d9d1f1b43432fc98 100644 > >> --- a/drivers/clk/imx/clk-imx8mp-audiomix.c > >> +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c > >> @@ -228,64 +228,20 @@ struct clk_imx8mp_audiomix_priv { > >> struct clk_hw_onecell_data clk_data; > >> }; > >> > >> -#if IS_ENABLED(CONFIG_RESET_CONTROLLER) > > > > I see the Kconfig ... > > > > select AUXILIARY_BUS if RESET_CONTROLLER > > > > But I don't see how this code is omitted without AUXILIARY_BUS. Is this > > kconfig check safe to remove? > > Ahhh that's what this directive was for. > > I thought it was really odd to have an #if on RESET while auxialiary > device was supposed to properly decouple the clock and reset part. > > To keep things as they were I'll add an #if on CONFIG_AUXILIARY_BUS I > wonder if this driver should select CONFIG_AUXILIARY_BUS instead ? I wonder if .../imx/clk-imx8mp-audiomix.c should be conditionally compiled based off of AUXILIARY_BUS? That is what I expected to see when I dug into the Kconfig/Makefile but it is not there AFAICS. I think for this patch it is best to leave the kconfig as it was. But it seems this is a place which could be cleaned up by the folks who work on this driver? Ira > > > > > Ira > > > >> - > >> -static void clk_imx8mp_audiomix_reset_unregister_adev(void *_adev) > >> -{ > >> - struct auxiliary_device *adev = _adev; > >> - > >> - auxiliary_device_delete(adev); > >> - auxiliary_device_uninit(adev); > >> -} > >> - > >> -static void clk_imx8mp_audiomix_reset_adev_release(struct device *dev) > >> +static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev) > >> { > >> - struct auxiliary_device *adev = to_auxiliary_dev(dev); > >> - > >> - kfree(adev); > >> -} > >> - > >> -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, > >> - struct clk_imx8mp_audiomix_priv *priv) > >> -{ > >> - struct auxiliary_device *adev __free(kfree) = NULL; > >> - int ret; > >> + struct auxiliary_device *adev; > >> > >> if (!of_property_present(dev->of_node, "#reset-cells")) > >> return 0; > >> > >> - adev = kzalloc(sizeof(*adev), GFP_KERNEL); > >> - if (!adev) > >> - return -ENOMEM; > >> - > >> - adev->name = "reset"; > >> - adev->dev.parent = dev; > >> - adev->dev.release = clk_imx8mp_audiomix_reset_adev_release; > >> - > >> - ret = auxiliary_device_init(adev); > >> - if (ret) > >> - return ret; > >> - > >> - ret = auxiliary_device_add(adev); > >> - if (ret) { > >> - auxiliary_device_uninit(adev); > >> - return ret; > >> - } > >> - > >> - return devm_add_action_or_reset(dev, clk_imx8mp_audiomix_reset_unregister_adev, > >> - no_free_ptr(adev)); > >> -} > >> - > >> -#else /* !CONFIG_RESET_CONTROLLER */ > >> + adev = devm_auxiliary_device_create(dev, "reset", NULL, 0); > >> + if (IS_ERR_OR_NULL(adev)) > >> + return PTR_ERR(adev); > >> > >> -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, > >> - struct clk_imx8mp_audiomix_priv *priv) > >> -{ > >> return 0; > >> } > >> > >> -#endif /* !CONFIG_RESET_CONTROLLER */ > >> - > >> static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save) > >> { > >> struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev); > >> @@ -408,7 +364,7 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev) > >> if (ret) > >> goto err_clk_register; > >> > >> - ret = clk_imx8mp_audiomix_reset_controller_register(dev, priv); > >> + ret = clk_imx8mp_audiomix_reset_controller_register(dev); > >> if (ret) > >> goto err_clk_register; > >> > >> > >> -- > >> 2.45.2 > >> > > -- > Jerome
diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c index c409fc7e061869988f83c7df3ef7860500426323..988a5fffeb4e0e481ec57038d9d1f1b43432fc98 100644 --- a/drivers/clk/imx/clk-imx8mp-audiomix.c +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c @@ -228,64 +228,20 @@ struct clk_imx8mp_audiomix_priv { struct clk_hw_onecell_data clk_data; }; -#if IS_ENABLED(CONFIG_RESET_CONTROLLER) - -static void clk_imx8mp_audiomix_reset_unregister_adev(void *_adev) -{ - struct auxiliary_device *adev = _adev; - - auxiliary_device_delete(adev); - auxiliary_device_uninit(adev); -} - -static void clk_imx8mp_audiomix_reset_adev_release(struct device *dev) +static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev) { - struct auxiliary_device *adev = to_auxiliary_dev(dev); - - kfree(adev); -} - -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, - struct clk_imx8mp_audiomix_priv *priv) -{ - struct auxiliary_device *adev __free(kfree) = NULL; - int ret; + struct auxiliary_device *adev; if (!of_property_present(dev->of_node, "#reset-cells")) return 0; - adev = kzalloc(sizeof(*adev), GFP_KERNEL); - if (!adev) - return -ENOMEM; - - adev->name = "reset"; - adev->dev.parent = dev; - adev->dev.release = clk_imx8mp_audiomix_reset_adev_release; - - ret = auxiliary_device_init(adev); - if (ret) - return ret; - - ret = auxiliary_device_add(adev); - if (ret) { - auxiliary_device_uninit(adev); - return ret; - } - - return devm_add_action_or_reset(dev, clk_imx8mp_audiomix_reset_unregister_adev, - no_free_ptr(adev)); -} - -#else /* !CONFIG_RESET_CONTROLLER */ + adev = devm_auxiliary_device_create(dev, "reset", NULL, 0); + if (IS_ERR_OR_NULL(adev)) + return PTR_ERR(adev); -static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev, - struct clk_imx8mp_audiomix_priv *priv) -{ return 0; } -#endif /* !CONFIG_RESET_CONTROLLER */ - static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save) { struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev); @@ -408,7 +364,7 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev) if (ret) goto err_clk_register; - ret = clk_imx8mp_audiomix_reset_controller_register(dev, priv); + ret = clk_imx8mp_audiomix_reset_controller_register(dev); if (ret) goto err_clk_register;
The auxiliary device creation of this driver is simple enough to use the available auxiliary device creation helper. Use it and remove some boilerplate code. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/imx/clk-imx8mp-audiomix.c | 56 ++++------------------------------- 1 file changed, 6 insertions(+), 50 deletions(-)