Re: [PATCH 5/5] clk: Add clk_get_by_name_optional
Hi Sean,
On Thu, 2 Dec 2021 at 07:25, Sean Anderson 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 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
> >> ---
> >>
> >> 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, >gem_emio_clk[i]);
> >> - if (ret < 0 && ret != -ENODATA) {
> >> + ret = clk_get_by_name_optional(dev, name,
> >> + >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", >clk);
> >> - if (err && err != -ENODATA)
> >> + err = clk_get_by_name_optional(dev, "core", >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
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 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
---
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, >gem_emio_clk[i]);
- if (ret < 0 && ret != -ENODATA) {
+ ret = clk_get_by_name_optional(dev, name,
+ >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", >clk);
- if (err && err != -ENODATA)
+ err = clk_get_by_name_optional(dev, "core", >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
Re: [PATCH 5/5] clk: Add clk_get_by_name_optional
Hi Sean,
On Wed, 1 Dec 2021 at 11:43, Sean Anderson 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
> ---
>
> 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, >gem_emio_clk[i]);
> - if (ret < 0 && ret != -ENODATA) {
> + ret = clk_get_by_name_optional(dev, name,
> + >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", >clk);
> - if (err && err != -ENODATA)
> + err = clk_get_by_name_optional(dev, "core", >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?
> + * @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
Re: [PATCH 5/5] clk: Add clk_get_by_name_optional
On 01/12/2021 19:43, Sean Anderson 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
> ---
>
> 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, >gem_emio_clk[i]);
> - if (ret < 0 && ret != -ENODATA) {
> + ret = clk_get_by_name_optional(dev, name,
> +>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", >clk);
> - if (err && err != -ENODATA)
> + err = clk_get_by_name_optional(dev, "core", >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.
> + * @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.
>
For meson-rng:
Reviewed-by: Neil Armstrong
[PATCH 5/5] clk: Add clk_get_by_name_optional
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
---
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, >gem_emio_clk[i]);
- if (ret < 0 && ret != -ENODATA) {
+ ret = clk_get_by_name_optional(dev, name,
+ >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", >clk);
- if (err && err != -ENODATA)
+ err = clk_get_by_name_optional(dev, "core", >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.
+ * @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
