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

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.

--Sean

...

• • @dev: The client device.
• • @name: The name of the clock to request, within the client's list of

• •         clocks.
    

• • @clk: A pointer to a clock struct to initialize.
• • Behaves the same as clk_get_by_name(), except when there is no clock
• • provider. In the latter case, return 0.
• • Return: 0 if OK, or a negative error code.
• */

+static inline int clk_get_by_name_optional(struct udevice *dev,

•                                      const char *name, struct clk *clk)
  

+{

•   int ret;
  

•   ret = clk_get_by_name_optional(dev, name, clk);
  

•   if (ret == -ENODATA)
  

•           return 0;
  

•   return ret;
  

+}

• /**
  • clk_get_by_name_nodev_optional - Get/request an optinonal clock by name

  •         without a device.
    

-- 2.33.0

Regards, SImon