Re: [PATCH v2] clk: palmas: Balance external-control clock prepare

[Myeonghun Pak]

Thanks for the review. I split the changes into two patches and sent v3.

2026년 7월 7일 (화) 오전 5:43, Brian Masney <bmasney@xxxxxxxxxx>님이 작성:
>
> Hi Myeonghun,
>
> On Wed, Jun 24, 2026 at 04:05:42PM +0900, Myeonghun Pak wrote:
> > palmas_clks_init_configure() prepares the clock when an external control
> > pin is configured.  The current driver only drops that prepare reference
> > when external control configuration fails.
> >
> > If provider registration fails after that point, or if the driver is later
> > removed, the prepare reference remains held.
> >
> > Use devm_add_action_or_reset() to manage the prepare reference.  Use
> > devm_of_clk_add_hw_provider() to manage provider registration.  This
> > balances the prepare reference on probe failure and driver removal.
> >
> > Fixes: 942d1d674931 ("clk: Add driver for Palmas clk32kg and clk32kgaudio clocks")
> > Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
>
> Please split these into two separate patches. Otherwise, it looks good
> to me.
>
> Brian
>
>
> > ---
> > Changes in v2:
> > - Use devm_of_clk_add_hw_provider() for provider cleanup.
> > - Manage the external-control clk_prepare() with devm_add_action_or_reset().
> > - Remove the manual provider remove callback.
> >
> >  drivers/clk/clk-palmas.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/clk/clk-palmas.c b/drivers/clk/clk-palmas.c
> > index 39049f62db..fdbf8445eb 100644
> > --- a/drivers/clk/clk-palmas.c
> > +++ b/drivers/clk/clk-palmas.c
> > @@ -194,6 +194,13 @@ static void palmas_clks_get_clk_data(struct platform_device *pdev,
> >       cinfo->ext_control_pin = prop;
> >  }
> >
> > +static void palmas_clks_unprepare_ext_control(void *data)
> > +{
> > +     struct palmas_clock_info *cinfo = data;
> > +
> > +     clk_unprepare(cinfo->hw.clk);
> > +}
> > +
> >  static int palmas_clks_init_configure(struct palmas_clock_info *cinfo)
> >  {
> >       int ret;
> > @@ -214,13 +221,18 @@ static int palmas_clks_init_configure(struct palmas_clock_info *cinfo)
> >                       return ret;
> >               }
> >
> > +             ret = devm_add_action_or_reset(cinfo->dev,
> > +                                            palmas_clks_unprepare_ext_control,
> > +                                            cinfo);
> > +             if (ret)
> > +                     return ret;
> > +
> >               ret = palmas_ext_control_req_config(cinfo->palmas,
> >                                       cinfo->clk_desc->sleep_reqstr_id,
> >                                       cinfo->ext_control_pin, true);
> >               if (ret < 0) {
> >                       dev_err(cinfo->dev, "Ext config for %s failed, %d\n",
> >                               cinfo->clk_desc->clk_name, ret);
> > -                     clk_unprepare(cinfo->hw.clk);
> >                       return ret;
> >               }
> >       }
> > @@ -230,7 +242,6 @@ static int palmas_clks_init_configure(struct palmas_clock_info *cinfo)
> >  static int palmas_clks_probe(struct platform_device *pdev)
> >  {
> >       struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
> > -     struct device_node *node = pdev->dev.of_node;
> >       const struct palmas_clks_of_match_data *match_data;
> >       struct palmas_clock_info *cinfo;
> >       int ret;
> > @@ -264,24 +275,19 @@ static int palmas_clks_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > -     ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get, &cinfo->hw);
> > +     ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_simple_get,
> > +                                       &cinfo->hw);
> >       if (ret < 0)
> >               dev_err(&pdev->dev, "Fail to add clock driver, %d\n", ret);
> >       return ret;
> >  }
> >
> > -static void palmas_clks_remove(struct platform_device *pdev)
> > -{
> > -     of_clk_del_provider(pdev->dev.of_node);
> > -}
> > -
> >  static struct platform_driver palmas_clks_driver = {
> >       .driver = {
> >               .name = "palmas-clk",
> >               .of_match_table = palmas_clks_of_match,
> >       },
> >       .probe = palmas_clks_probe,
> > -     .remove = palmas_clks_remove,
> >  };
> >
> >  module_platform_driver(palmas_clks_driver);
> > --
> > 2.47.1
>