Behaves the same as (devm_)clk_get except where there is no clock
producer. In this case, instead of returning -ENOENT, the function
returns NULL. This makes error checking simpler and allows
clk_prepare_enable, etc to be called on the returned reference
without additional checks.
Signed-off-by: Phil Edworthy <[email protected]>
---
drivers/clk/clk-devres.c | 11 +++++++++++
drivers/clk/clkdev.c | 11 +++++++++++
include/linux/clk.h | 27 +++++++++++++++++++++++++++
3 files changed, 49 insertions(+)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index d854e26..63295d9 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -34,6 +34,17 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
}
EXPORT_SYMBOL(devm_clk_get);
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+ struct clk *c = devm_clk_get(dev, id);
+
+ if (PTR_ERR(c) == -ENOENT)
+ return NULL;
+
+ return c;
+}
+EXPORT_SYMBOL(devm_clk_get_optional);
+
struct clk_bulk_devres {
struct clk_bulk_data *clks;
int num_clks;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 7513411..7f2cd1e 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -209,6 +209,17 @@ struct clk *clk_get(struct device *dev, const char *con_id)
}
EXPORT_SYMBOL(clk_get);
+struct clk *clk_get_optional(struct device *dev, const char *id)
+{
+ struct clk *c = clk_get(dev, id);
+
+ if (PTR_ERR(c) == -ENOENT)
+ return NULL;
+
+ return c;
+}
+EXPORT_SYMBOL(clk_get_optional);
+
void clk_put(struct clk *clk)
{
__clk_put(clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 0dbd088..907202b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -258,6 +258,14 @@ static inline void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
struct clk *clk_get(struct device *dev, const char *id);
/**
+ * clk_get_optional - lookup and obtain a reference to optional clock producer.
+ *
+ * Behaves the same as clk_get except where there is no clock producer. In this
+ * case, instead of returning -ENOENT, the function returns NULL.
+ */
+struct clk *clk_get_optional(struct device *dev, const char *id);
+
+/**
* clk_bulk_get - lookup and obtain a number of references to clock producer.
* @dev: device for clock "consumer"
* @num_clks: the number of clk_bulk_data
@@ -316,6 +324,14 @@ int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
struct clk *devm_clk_get(struct device *dev, const char *id);
/**
+ * devm_clk_get_optional - lookup and obtain a managed reference to an optional
+ * clock producer.
+ * Behaves the same as devm_clk_get except where there is no clock producer. In
+ * this case, instead of returning -ENOENT, the function returns NULL.
+ */
+struct clk *devm_clk_get_optional(struct device *dev, const char *id);
+
+/**
* devm_get_clk_from_child - lookup and obtain a managed reference to a
* clock producer from child node.
* @dev: device for clock "consumer"
@@ -603,6 +619,11 @@ static inline struct clk *clk_get(struct device *dev, const char *id)
return NULL;
}
+static inline struct clk *clk_get_optional(struct device *dev, const char *id)
+{
+ return NULL;
+}
+
static inline int __must_check clk_bulk_get(struct device *dev, int num_clks,
struct clk_bulk_data *clks)
{
@@ -614,6 +635,12 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
return NULL;
}
+static inline struct clk *devm_clk_get_optional(struct device *dev,
+ const char *id)
+{
+ return NULL;
+}
+
static inline int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
struct clk_bulk_data *clks)
{
--
2.7.4
On Wed, Jul 18, 2018 at 01:57:38PM +0100, Phil Edworthy wrote:
> Behaves the same as (devm_)clk_get except where there is no clock
> producer. In this case, instead of returning -ENOENT, the function
> returns NULL. This makes error checking simpler and allows
> clk_prepare_enable, etc to be called on the returned reference
> without additional checks.
How does this work with non-DT systems, where looking a clock up which
isn't yet registered with clkdev returns -ENOENT ?
(clkdev doesn't know when all clocks are registered with it.)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
Hi Russell,
On Wed, Jul 18, 2018 at 3:02 PM Russell King - ARM Linux
<[email protected]> wrote:
> On Wed, Jul 18, 2018 at 01:57:38PM +0100, Phil Edworthy wrote:
> > Behaves the same as (devm_)clk_get except where there is no clock
> > producer. In this case, instead of returning -ENOENT, the function
> > returns NULL. This makes error checking simpler and allows
> > clk_prepare_enable, etc to be called on the returned reference
> > without additional checks.
>
> How does this work with non-DT systems, where looking a clock up which
> isn't yet registered with clkdev returns -ENOENT ?
>
> (clkdev doesn't know when all clocks are registered with it.)
Good question.
I guess all drivers trying to handle optional clocks this way are already broken
on non-DT systems where clocks may be registered late...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Russell,
On 18 July 2018 14:19, Geert Uytterhoeven wrote:
> On Wed, Jul 18, 2018 at 3:02 PM Russell King - ARM Linux wrote:
> > On Wed, Jul 18, 2018 at 01:57:38PM +0100, Phil Edworthy wrote:
> > > Behaves the same as (devm_)clk_get except where there is no clock
> > > producer. In this case, instead of returning -ENOENT, the function
> > > returns NULL. This makes error checking simpler and allows
> > > clk_prepare_enable, etc to be called on the returned reference
> > > without additional checks.
> >
> > How does this work with non-DT systems, where looking a clock up which
> > isn't yet registered with clkdev returns -ENOENT ?
> >
> > (clkdev doesn't know when all clocks are registered with it.)
>
> Good question.
>
> I guess all drivers trying to handle optional clocks this way are already broken
> on non-DT systems where clocks may be registered late...
So how do non-DT systems that look a clock up which isn't yet
registered with clkdev, determine that an optional clock is there
or not?
Thanks
Phil
Hi Phil,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on clk/clk-next]
[also build test WARNING on v4.18-rc5 next-20180718]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Phil-Edworthy/clk-Add-functions-to-get-optional-clocks/20180719-024451
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
reproduce: make htmldocs
All warnings (new ones prefixed by >>):
WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
mm/mempool.c:228: warning: Function parameter or member 'pool' not described in 'mempool_init'
Error: Cannot open file arch/x86/kernel/cpu/mtrr/main.c
Error: Cannot open file arch/x86/kernel/cpu/mtrr/main.c
WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -export arch/x86/kernel/cpu/mtrr/main.c' failed with return code 2
>> include/linux/clk.h:300: warning: Function parameter or member 'dev' not described in 'clk_get_optional'
>> include/linux/clk.h:300: warning: Function parameter or member 'id' not described in 'clk_get_optional'
>> include/linux/clk.h:366: warning: Function parameter or member 'dev' not described in 'devm_clk_get_optional'
>> include/linux/clk.h:366: warning: Function parameter or member 'id' not described in 'devm_clk_get_optional'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev'
include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev'
include/net/mac80211.h:2282: warning: Function parameter or member 'radiotap_timestamp.units_pos' not described in 'ieee80211_hw'
include/net/mac80211.h:2282: warning: Function parameter or member 'radiotap_timestamp.accuracy' not described in 'ieee80211_hw'
include/net/mac80211.h:955: warning: Function parameter or member 'control.rates' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.rts_cts_rate_idx' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.use_rts' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.use_cts_prot' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.short_preamble' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.skip_table' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.jiffies' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.vif' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.hw_key' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.flags' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'control.enqueue_time' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'ack' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'ack.cookie' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.rates' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.ack_signal' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info'
include/net/mac80211.h:955: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info'
kernel/sched/fair.c:3760: warning: Function parameter or member 'flags' not described in 'attach_entity_load_avg'
include/linux/device.h:93: warning: bad line: this bus.
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf'
include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
include/linux/gpio/driver.h:142: warning: Function parameter or member 'request_key' not described in 'gpio_irq_chip'
include/linux/iio/hw-consumer.h:1: warning: no structured comments found
include/linux/device.h:94: warning: bad line: this bus.
include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry'
include/linux/regulator/driver.h:227: warning: Function parameter or member 'resume_early' not described in 'regulator_ops'
drivers/regulator/core.c:4465: warning: Excess function parameter 'state' description in 'regulator_suspend_late'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb'
drivers/usb/dwc3/gadget.c:510: warning: Excess function parameter 'dwc' description in 'dwc3_gadget_start_config'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
drivers/gpu/drm/i915/i915_vma.h:48: warning: cannot understand function prototype: 'struct i915_vma '
drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
include/drm/tinydrm/tinydrm.h:34: warning: Function parameter or member 'fb_dirty' not described in 'tinydrm_device'
drivers/gpu/drm/tinydrm/mipi-dbi.c:272: warning: Function parameter or member 'crtc_state' not described in 'mipi_dbi_enable_flush'
drivers/gpu/drm/tinydrm/mipi-dbi.c:272: warning: Function parameter or member 'plane_state' not described in 'mipi_dbi_enable_flush'
vim +300 include/linux/clk.h
> 300
301 /**
302 * clk_bulk_get - lookup and obtain a number of references to clock producer.
303 * @dev: device for clock "consumer"
304 * @num_clks: the number of clk_bulk_data
305 * @clks: the clk_bulk_data table of consumer
306 *
307 * This helper function allows drivers to get several clk consumers in one
308 * operation. If any of the clk cannot be acquired then any clks
309 * that were obtained will be freed before returning to the caller.
310 *
311 * Returns 0 if all clocks specified in clk_bulk_data table are obtained
312 * successfully, or valid IS_ERR() condition containing errno.
313 * The implementation uses @dev and @clk_bulk_data.id to determine the
314 * clock consumer, and thereby the clock producer.
315 * The clock returned is stored in each @clk_bulk_data.clk field.
316 *
317 * Drivers must assume that the clock source is not enabled.
318 *
319 * clk_bulk_get should not be called from within interrupt context.
320 */
321 int __must_check clk_bulk_get(struct device *dev, int num_clks,
322 struct clk_bulk_data *clks);
323
324 /**
325 * devm_clk_bulk_get - managed get multiple clk consumers
326 * @dev: device for clock "consumer"
327 * @num_clks: the number of clk_bulk_data
328 * @clks: the clk_bulk_data table of consumer
329 *
330 * Return 0 on success, an errno on failure.
331 *
332 * This helper function allows drivers to get several clk
333 * consumers in one operation with management, the clks will
334 * automatically be freed when the device is unbound.
335 */
336 int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
337 struct clk_bulk_data *clks);
338
339 /**
340 * devm_clk_get - lookup and obtain a managed reference to a clock producer.
341 * @dev: device for clock "consumer"
342 * @id: clock consumer ID
343 *
344 * Returns a struct clk corresponding to the clock producer, or
345 * valid IS_ERR() condition containing errno. The implementation
346 * uses @dev and @id to determine the clock consumer, and thereby
347 * the clock producer. (IOW, @id may be identical strings, but
348 * clk_get may return different clock producers depending on @dev.)
349 *
350 * Drivers must assume that the clock source is not enabled.
351 *
352 * devm_clk_get should not be called from within interrupt context.
353 *
354 * The clock will automatically be freed when the device is unbound
355 * from the bus.
356 */
357 struct clk *devm_clk_get(struct device *dev, const char *id);
358
359 /**
360 * devm_clk_get_optional - lookup and obtain a managed reference to an optional
361 * clock producer.
362 * Behaves the same as devm_clk_get except where there is no clock producer. In
363 * this case, instead of returning -ENOENT, the function returns NULL.
364 */
365 struct clk *devm_clk_get_optional(struct device *dev, const char *id);
> 366
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Quoting Phil Edworthy (2018-07-18 06:56:26)
> Hi Russell,
>
> On 18 July 2018 14:19, Geert Uytterhoeven wrote:
> > On Wed, Jul 18, 2018 at 3:02 PM Russell King - ARM Linux wrote:
> > > On Wed, Jul 18, 2018 at 01:57:38PM +0100, Phil Edworthy wrote:
> > > > Behaves the same as (devm_)clk_get except where there is no clock
> > > > producer. In this case, instead of returning -ENOENT, the function
> > > > returns NULL. This makes error checking simpler and allows
> > > > clk_prepare_enable, etc to be called on the returned reference
> > > > without additional checks.
> > >
> > > How does this work with non-DT systems, where looking a clock up which
> > > isn't yet registered with clkdev returns -ENOENT ?
> > >
> > > (clkdev doesn't know when all clocks are registered with it.)
> >
> > Good question.
> >
> > I guess all drivers trying to handle optional clocks this way are already broken
> > on non-DT systems where clocks may be registered late...
>
> So how do non-DT systems that look a clock up which isn't yet
> registered with clkdev, determine that an optional clock is there
> or not?
>
Short answer is they don't. I'd still prefer we have this API though.
Can you rework this patch to be a little more invasive into the
clk_get() path, perhaps by reworking __of_clk_get_by_name() a little to
take an 'optional' argument, so that it only returns NULL when the clk
is looked up from DT? The fallback path in clkdev where we have a DT
based system looking up a clk through clkdev lookups doesn't seem to be
a real scenario that we should worry about here. I think sometimes
people use clkdev lookups when they're migrating to DT systems and
things aren't wired up properly in DT, but that isn't the norm.
Hi Stephen,
On 25 July 2018 23:37, Stephen Boyd wrote:
> Quoting Phil Edworthy (2018-07-18 06:56:26)
> > On 18 July 2018 14:19, Geert Uytterhoeven wrote:
> > > On Wed, Jul 18, 2018 at 3:02 PM Russell King - ARM Linux wrote:
> > > > On Wed, Jul 18, 2018 at 01:57:38PM +0100, Phil Edworthy wrote:
> > > > > Behaves the same as (devm_)clk_get except where there is no
> > > > > clock producer. In this case, instead of returning -ENOENT, the
> > > > > function returns NULL. This makes error checking simpler and
> > > > > allows clk_prepare_enable, etc to be called on the returned
> > > > > reference without additional checks.
> > > >
> > > > How does this work with non-DT systems, where looking a clock up
> > > > which isn't yet registered with clkdev returns -ENOENT ?
> > > >
> > > > (clkdev doesn't know when all clocks are registered with it.)
> > >
> > > Good question.
> > >
> > > I guess all drivers trying to handle optional clocks this way are
> > > already broken on non-DT systems where clocks may be registered late...
> >
> > So how do non-DT systems that look a clock up which isn't yet
> > registered with clkdev, determine that an optional clock is there or
> > not?
> >
>
> Short answer is they don't. I'd still prefer we have this API though.
>
> Can you rework this patch to be a little more invasive into the
> clk_get() path, perhaps by reworking __of_clk_get_by_name() a little to
> take an 'optional' argument, so that it only returns NULL when the clk is
> looked up from DT? The fallback path in clkdev where we have a DT based
> system looking up a clk through clkdev lookups doesn't seem to be a real
> scenario that we should worry about here. I think sometimes people use
> clkdev lookups when they're migrating to DT systems and things aren't wired
> up properly in DT, but that isn't the norm.
Do you mean something like this:
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 7f2cd1e..42a7d4e 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -57,7 +57,8 @@ EXPORT_SYMBOL(of_clk_get);
static struct clk *__of_clk_get_by_name(struct device_node *np,
const char *dev_id,
- const char *name)
+ const char *name,
+ bool optional)
{
struct clk *clk = ERR_PTR(-ENOENT);
@@ -79,6 +80,8 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
if (PTR_ERR(clk) != -EPROBE_DEFER)
pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
np, name ? name : "", index);
+ if (optional && PTR_ERR(clk) == -ENOENT)
+ clk = NULL;
return clk;
}
@@ -109,15 +112,38 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
if (!np)
return ERR_PTR(-ENOENT);
- return __of_clk_get_by_name(np, np->full_name, name);
+ return __of_clk_get_by_name(np, np->full_name, name, false);
}
EXPORT_SYMBOL(of_clk_get_by_name);
+/**
+ * of_clk_get_by_name_optional() - Parse and lookup an optional clock referenced
+ * by a device node
+ * @np: pointer to clock consumer node
+ * @name: name of consumer's clock input, or NULL for the first clock reference
+ *
+ * This function parses the clocks and clock-names properties,
+ * and uses them to look up the struct clk from the registered list of clock
+ * providers.
+ * It behaves the same as of_clk_get_by_name(), except when no clock is found.
+ * In this case, instead of returning -ENOENT, it returns NULL.
+ */
+struct clk *of_clk_get_by_name_optional(struct device_node *np,
+ const char *name)
+{
+ if (!np)
+ return ERR_PTR(-ENOENT);
+
+ return __of_clk_get_by_name(np, np->full_name, name, true);
+}
+EXPORT_SYMBOL(of_clk_get_by_name_optional);
+
#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
static struct clk *__of_clk_get_by_name(struct device_node *np,
const char *dev_id,
- const char *name)
+ const char *name,
+ bool optional)
{
return ERR_PTR(-ENOENT);
}
@@ -200,7 +226,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
struct clk *clk;
if (dev) {
- clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
+ clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id, false);
if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
return clk;
}
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 907202b..830209a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -771,6 +771,7 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
struct clk *of_clk_get(struct device_node *np, int index);
struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
+struct clk *of_clk_get_by_name_optional(struct device_node *np, const char *name);
struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
#else
static inline struct clk *of_clk_get(struct device_node *np, int index)
---
A lot of drivers use devm_clk_get() so I think a devm_clk_get_optional()
version would be useful. That would probably need an additional
clk_get_optional() function.
Thanks
Phil
Quoting Phil Edworthy (2018-07-27 08:38:12)
> On 25 July 2018 23:37, Stephen Boyd wrote:
> > Short answer is they don't. I'd still prefer we have this API though.
> >
> > Can you rework this patch to be a little more invasive into the
> > clk_get() path, perhaps by reworking __of_clk_get_by_name() a little to
> > take an 'optional' argument, so that it only returns NULL when the clk is
> > looked up from DT? The fallback path in clkdev where we have a DT based
> > system looking up a clk through clkdev lookups doesn't seem to be a real
> > scenario that we should worry about here. I think sometimes people use
> > clkdev lookups when they're migrating to DT systems and things aren't wired
> > up properly in DT, but that isn't the norm.
> Do you mean something like this:
>
[...]
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 907202b..830209a 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -771,6 +771,7 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
> #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
> struct clk *of_clk_get(struct device_node *np, int index);
> struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
> +struct clk *of_clk_get_by_name_optional(struct device_node *np, const char *name);
> struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
> #else
> static inline struct clk *of_clk_get(struct device_node *np, int index)
>
> ---
> A lot of drivers use devm_clk_get() so I think a devm_clk_get_optional()
> version would be useful. That would probably need an additional
> clk_get_optional() function.
>
Yes, this would be the first patch, and then the second patch would add
clk_get_optional() and devm_clk_get_optional(). We shouldn't encourage
consumers to move from device based to DT node based clk_get() for the
optional clk API.