[trimming a bit of useless context]
> > > > > > Background to why it might be a good idea to connect a ground clock
> > > > > > to the unconnected input pins is that a driver has a chance to
> > > > > > successfully grab all clocks. Otherwise how does the driver
> > > > > > distinguish
> > > > > > between an unconnected and an erroneous clock?
> > > > >
> > > > > Sorry, I don't follow this last question. Do you mean how to distinguish
> > > > > based on the value returned from clk_get?
> > > >
> > > > Hmm, in theory, a driver could want to distinguish an error case (e.g.
> > > > clock specified, but there is a problem with it) from no clock (e.g. clock
> > > > not specified in DT, because it is not available on particular board).
> > >
> > > Yes, that's what I meant. To illustrate the problem for this driver:
> > >
> > > for (i = 0; i < STC_TXCLK_SRC_MAX; i++) {
> > > sprintf(tmp, "rxtx%d", i);
> > > clk = devm_clk_get(&pdev->dev, tmp);
> > > if (IS_ERR(clk)) {
> >
> > [...]
> >
> > /*
> > * ERR_PTR(-ENOENT) returned when clock not
> > * present in the dt (i.e. not wired up). We can
> > * live without this clock, so assign a dummy
> > * (NULL) to simplify the rest of the code. If
> > * the clock is present but something else went
> > * wrong, we'll get a different ERR_PTR value
> > * and actually fail.
> > */
> > if (clk == ERR_PTR(-ENOENT)
> > clk = NULL;
> >
> > > }
> > > }
> > >
> > > This could be solved by always specifying all input clocks in the
> > > devicetree.
> >
> > As far as I can see, the above is sufficient, and leaves the knowledge
> > of skippable clocks in the driver, where I believe it should be.
>
> You miss my point. Once a clock is specified in the devicetree it is no
> longer optional. The driver currently can't find out whether a clock
> hasn't been specified (and it's ok that it's not there) or a clock has
> been specified, but some error prevents actually grabbing the clock (in
> which case the driver should bail out)
Unless I've misunderstood, that's exactly what I was trying to implement
above. As I understand it, what we want is:
* If a clock is not specified in the DT, but it's a clock that we know
we can live without if not wired up, then continue along with a dummy
clock (NULL). This could be changed to a different dummy, what exactly
is needed is driver-specific.
* If a clock is not specified in the DT, but it's *not* an optional
clock, then we must fail. Whether or not a clock is optional is
knowledge only the driver can have.
* If a clock is specified in the DT, but we can't get it for some
reason, fail.
* If a clock is specified in the DT, and we get it, use it.
I see we might also get PTR_ERR(-ENOENT) indirectly from
__of_parse_phandle_with_args, if the "clocks" property isn't present
(but clock-names is), or we're given the empty phandle. The empty
phandle case is arguable, but it would be possible to add a check for
the clocks property in of_clk_get_by_name early on where we could return
-EINVAL to prevent that being a problem.
Otherwise, it's trivial to add a function to explicitly test for this
case (see below). I don't see that we need to leak what is a Linux issue
into the dt.
Thanks,
Mark.
---->8----
>From f0d46f36426ded4ba3609d79664888852f9925e2 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@xxxxxxx>
Date: Fri, 23 Aug 2013 15:46:33 +0100
Subject: [PATCH] clk: add of_clk_is_specified
There's currently no way to perfectly distinguish between an
of_clk_get_by_name that failed because a clock was not provided in
clock-names, or for some other reason (e.g. the clocks property itself
was missing). This is problematic for drivers for some pieces of
hardware that have optional clocks -- those which must be used if wired,
but may not be wired in all cases.
This patch adds a new function of_clk_is_specified, which may be used to
distinguish these cases.
Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
---
drivers/clk/clkdev.c | 11 +++++++++++
include/linux/clk.h | 5 +++++
2 files changed, 16 insertions(+)
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 442a313..7fdeca9 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -91,6 +91,17 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
return clk;
}
EXPORT_SYMBOL(of_clk_get_by_name);
+
+/**
+ *
+ * of_clk_is_specified - Test if a clock was provided by name in a device node
+ * @np: pointer to the clock consumer node
+ * @name: name of the consumer's clock input. Not NULL.
+ */
+bool of_clk_is_specified(struct device_node *np, const char *name)
+{
+ return of_property_match_string(np, "clock-names", name) >= 0;
+}
#endif
/*
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..bf44d94 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -368,6 +368,7 @@ struct of_phandle_args;
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_from_provider(struct of_phandle_args *clkspec);
+bool of_clk_is_specified(struct device_node *np, const char *name);
#else
static inline struct clk *of_clk_get(struct device_node *np, int index)
{
@@ -378,6 +379,10 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
{
return ERR_PTR(-ENOENT);
}
+static bool of_clk_is_specified(struct device_node *np, const char *name)
+{
+ return false;
+}
#endif
#endif
--
1.8.1.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
