Thread (37 messages) 37 messages, 6 authors, 2013-08-24

Re: [alsa-devel] [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver

flat view

From: Mike Turquette <hidden>
Date: 2013-08-23 21:41:49
Also in: alsa-devel, linux-devicetree 

Quoting Sascha Hauer (2013-08-23 07:01:28)
On Fri, Aug 23, 2013 at 01:58:15PM +0100, Mark Rutland wrote:
quoted
On Fri, Aug 23, 2013 at 07:34:21AM +0100, Sascha Hauer wrote:
quoted
On Fri, Aug 23, 2013 at 12:49:19AM +0200, Tomasz Figa wrote:
quoted
On Thursday 22 of August 2013 15:43:31 Mike Turquette wrote:
quoted
Quoting Sascha Hauer (2013-08-22 14:00:35)
=
quoted
quoted
quoted
quoted
quoted
On Thu, Aug 22, 2013 at 01:09:31PM +0100, Mark Rutland wrote:
quoted
On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrot=
e:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Quoting Tomasz Figa (2013-08-21 14:34:55)
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrot=
e:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquett=
e =

quoted
quoted
quoted
wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Quoting Mark Rutland (2013-08-19 02:35:43)
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Fi=
ga =

quoted
quoted
quoted
wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
On Saturday 17 of August 2013 16:53:16 Sascha Hau=
er =

quoted
quoted
quoted
wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomas=
z Figa =

quoted
quoted
quoted
wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Also I would make this option required.=
 Use a
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
dummy
clock for
mux
inputs that are grounded for a specific=
 SoC.
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Some clocks are not from CCM and we haven=
't
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
defined in
imx6q-clk.txt,
so in most cases we can't provide a phand=
le for
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
them, eg:
spdif_ext.
I think it's a bit hard to force it to be
'required'. An
'optional'
looks more flexible to me and a default o=
ne is
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
ensured
even if
it's
missing.
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
<&clks 0> is the dummy clock. This can be u=
sed for
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
all input
clocks
not
defined by the SoC.
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Where does this assumption come from? Is it
documented
anywhere?
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
This is how all i.MX clock bindings currently a=
re. See
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Documentation/devicetree/bindings/clock/imx*-cl=
ock.txt
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
OK, thanks.
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
I guess we need some discussion on dummy clocks vs
skipped clocks.
I think we want some consistency on this, don't w=
e?
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
If we really need a dummy clock, then we might al=
so want
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
a generic
way to specify it.
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
What do we actually mean by a "dummy clock"? We alr=
eady
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
have
bindings
for "fixed-clock" and co friends describe relatively
simple
preconfigured clocks.
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Some platforms have a fake clock which defines noops
callbacks and
basically doesn't do anything. This is analogous to t=
he
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
dummy
regulator
implementation. A central one could be registered by =
the
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
clock core,
as
is done by the regulator core.
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
When you say some platforms, you presumably mean the pl=
atform
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
code in
Linux? A dummy clock sounds like a completely Linux-spe=
cific
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
abstraction rather than a description of the hardware, =
and I
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
don't see why we need that in the DT:
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
* If a clock is wired up and running (as presumably the=
 dummy
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
clock is), then surely it's a fixed-clock (it's running=
, we
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
and we have no control over it, but we presumably know =
its
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
rate) and can be described as such?
=
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
* If no clock is wired up, then we should be able to de=
scribe
quoted
quoting not expanded further in this message
that. If a driver believes that a clock is required whe=
n it
quoting not expanded further in this message
isn't (for some level of functionality), then that driv=
er
quoting not expanded further in this message
should be fixed up to support the clock as being option=
al.
quoting not expanded further in this message
=
quoting not expanded further in this message
Am I missing something?
=
quoting not expanded further in this message
I second that.
=
quoting not expanded further in this message
Moreover, I don't think that device tree should deal with=
 dummy
quoting not expanded further in this message
anything. It should be able to describe hardware that is
available on given system, not list what hardware is not
available.
=
quoting not expanded further in this message
I wasn't clear. The dummy clock IS a completely Linux-speci=
fic
quoting not expanded further in this message
abstraction.
=
quoting not expanded further in this message
I'm not advocating a dummy clock in DT. I am advocating
consolidation of the implementation of a clock that does no=
thing
quoting not expanded further in this message
into the clock core. This code could easily live in
drivers/clk/clk.c instead of having everyone open-code it.
=
quoting not expanded further in this message
As far as specifying a dummy clock in DT? I dunno. DT should
describe
real hardware so there isn't much use for a dummy clock.
=
quoting not expanded further in this message
Sorry, I misunderstood. Good to hear we're on the same page :)
=
quoting not expanded further in this message
I'm guessing one of the reasons for such a clock are driver=
s do
quoting not expanded further in this message
not
honor the clk.h api and they freak out when clk_get gives t=
hem a
quoting not expanded further in this message
NULL
pointer?
=
quoting not expanded further in this message
I'm not sure. Sascha, could you shed some light on the matter?
=
quoting not expanded further in this message
The original reason introducing the dummy clocks in the i.MX dt=
bs
quoting not expanded further in this message
was to provide devices a clock which the driver requests but is
not software controllable. We often have the case where the same
devices are on several SoCs, but not on all of them all clocks =
have
quoting not expanded further in this message
a bit to en/disable them.
=
quoting not expanded further in this message
Anyway, to accomplish this we don't need dummy clocks. We can j=
ust
quoting not expanded further in this message
describe the real clocks.
=
quoting not expanded further in this message
You could use a dummy clk for the Linux implementation, but the d=
ownside
quoting not expanded further in this message
is that a dummy clock has a rate of 0 always and a your clocks li=
kely
quoting not expanded further in this message
have non-zero rates.
=
quoting not expanded further in this message
It is probably better for you define a clock which only implement=
s the
quoting not expanded further in this message
.recalc_rate callback. If the rate of this clock changes without =
Linux
quoting not expanded further in this message
having knowledge of it you can use the CLK_GET_RATE_NOCACHE flag.
=
quoting not expanded further in this message
I doubt that rate of a dummy clock could ever change... unless it i=
s a =

quoting not expanded further in this message
rather smart dummy.
=
quoting not expanded further in this message
BTW with the S/PDIF core on which not all mux inputs are connec=
ted
quoting not expanded further in this message
to actual clocks we could also describe the unconnected inputs =
as
quoting not expanded further in this message
ground clocks with rate 0. This way we describe something which
is really there instead of dummy clocks ;)
=
quoting not expanded further in this message
Again you could use a dummy clock for this OR a fixed-rate clock =
with a
quoting not expanded further in this message
rate of zero from the perspective of the Linux implementation.
=
quoting not expanded further in this message
Do you think it worthwhile to have a DT binding for a grounded cl=
ock?
quoting not expanded further in this message
That is not an entirely uncommon case.
=
quoting not expanded further in this message
Well, how would that differ from skipping a clock from clocks list,=
 i.e. =

quoting not expanded further in this message
not specifying it in clock-names and clocks properties?
=
quoting not expanded further in this message
The difference is that you can successfully grab it in your driver.
=
quoting not expanded further in this message
That's a driver-specific issue. The driver knows best which clocks it
can live without (if it's poking only a subset of the hardware, it may
not need some just yet, but could for extended functionality in future
when support is extended), and could assign a dummy to those clocks it
knows it doesn't need that aren't described. That doesn't need to be in
the dt, and shouldn't be, because it's OS and driver specific.
=
quoting not expanded further in this message
=
quoting not expanded further in this message
=
quoting not expanded further in this message
Background to why it might be a good idea to connect a ground c=
lock
quoting not expanded further in this message
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?
=
quoting not expanded further in this message
Sorry, I don't follow this last question. Do you mean how to dist=
inguish
quoting not expanded further in this message
based on the value returned from clk_get?
=
quoting not expanded further in this message
Hmm, in theory, a driver could want to distinguish an error case (e=
.g. =

quoting not expanded further in this message
clock specified, but there is a problem with it) from no clock (e.g=
. clock =

quoting not expanded further in this message
not specified in DT, because it is not available on particular boar=
d).
quoting not expanded further in this message
=
quoting not expanded further in this message
Yes, that's what I meant. To illustrate the problem for this driver:
=
quoting not expanded further in this message
    for (i =3D 0; i < STC_TXCLK_SRC_MAX; i++) {
            sprintf(tmp, "rxtx%d", i);
            clk =3D devm_clk_get(&pdev->dev, tmp);
            if (IS_ERR(clk)) {
=
quoting not expanded further in this message
              [...]
=
quoting not expanded further in this message
                      /*
                       * 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 =3D=3D ERR_PTR(-ENOENT)
                              clk =3D NULL;
=
quoting not expanded further in this message
            }
    }
=
quoting not expanded further in this message
This could be solved by always specifying all input clocks in the
devicetree.
=
quoting not expanded further in this message
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.
=
quoting not expanded further in this message
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)
Seems like the regulator framework is solving this with the new
regulator_get_optional() call. This leaves the
optional-versus-not-optional logic up to the driver.

Regards,
Mike

quoting not expanded further in this message
=
quoting not expanded further in this message
Sascha
=
quoting not expanded further in this message
=
quoting not expanded further in this message
-- =
quoting not expanded further in this message
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help