Re: [PATCH 5/5] clk: Add clk_get_by_name_optional

[Simon Glass <sjg@chromium.org>]

Hi Sean,

On Thu, 2 Dec 2021 at 07:25, Sean Anderson <seanga2@gmail.com> wrote:
>
> Hi Simon,
>
> On 12/2/21 8:43 AM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Wed, 1 Dec 2021 at 11:43, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> This adds a helper function for clk_get_by_name in cases where the clock is
> >> optional. Hopefully this helps point driver writers in the right direction.
> >> Also convert some existing users.
> >>
> >> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >> ---
> >>
> >>   drivers/clk/clk_zynq.c  |  5 +++--
> >>   drivers/rng/meson-rng.c |  4 ++--
> >>   include/clk.h           | 24 ++++++++++++++++++++++++
> >>   3 files changed, 29 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/clk/clk_zynq.c b/drivers/clk/clk_zynq.c
> >> index 18915c3e04..e80500e382 100644
> >> --- a/drivers/clk/clk_zynq.c
> >> +++ b/drivers/clk/clk_zynq.c
> >> @@ -472,8 +472,9 @@ static int zynq_clk_probe(struct udevice *dev)
> >>
> >>          for (i = 0; i < 2; i++) {
> >>                  sprintf(name, "gem%d_emio_clk", i);
> >> -               ret = clk_get_by_name(dev, name, &priv->gem_emio_clk[i]);
> >> -               if (ret < 0 && ret != -ENODATA) {
> >> +               ret = clk_get_by_name_optional(dev, name,
> >> +                                              &priv->gem_emio_clk[i]);
> >> +               if (ret) {
> >>                          dev_err(dev, "failed to get %s clock\n", name);
> >>                          return ret;
> >>                  }
> >> diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
> >> index 5a4f45ad5a..e0a1e8c7e0 100644
> >> --- a/drivers/rng/meson-rng.c
> >> +++ b/drivers/rng/meson-rng.c
> >> @@ -91,8 +91,8 @@ static int meson_rng_of_to_plat(struct udevice *dev)
> >>                  return -ENODEV;
> >>
> >>          /* Get optional "core" clock */
> >> -       err = clk_get_by_name(dev, "core", &pdata->clk);
> >> -       if (err && err != -ENODATA)
> >> +       err = clk_get_by_name_optional(dev, "core", &pdata->clk);
> >> +       if (err)
> >>                  return err;
> >>
> >>          return 0;
> >> diff --git a/include/clk.h b/include/clk.h
> >> index 103ef16bf9..36721188d0 100644
> >> --- a/include/clk.h
> >> +++ b/include/clk.h
> >> @@ -292,6 +292,30 @@ static inline int clk_release_all(struct clk *clk, int count)
> >>   }
> >>   #endif
> >>
> >> +/**
> >> + * clk_get_by_name_optional() - Get/request a optional clock by name.
> >
> > Can I suggest we rename this to ..._opt as it is shorter?
>
> This follows the general trend of suffixing _optional. For example (and
> several of these are borrowed from Linux):
>
>         clk_get_optional
>         reset_control_bulk_get_optional_exclusive
>         gpiod_get_optional
>         platform_get_irq_byname_optional
>
> and particularly, the Linux clock subsystem uses _optional and not _opt
> as a suffix. While some of these names can get fairly long-winded, I
> think we should go for consistency here.

Yes I agree.

I do wish people would consider brevity as these names are pretty
hopeless for readability.

Regards,
Simon