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

Quoting Sascha Hauer (2013-08-22 14:00:35)

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 wrote:

quoted

Quoting Tomasz Figa (2013-08-21 14:34:55)

quoted

On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote:

quoted

On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote:

quoted

Quoting Mark Rutland (2013-08-19 02:35:43)
=

quoted

quoted

quoted

quoted

quoted

quoted

On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote:

quoted

On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote:

quoted

On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wro=

te:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Also I would make this option required. Use a dum=

my

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

clock for
mux
inputs that are grounded for a specific SoC.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Some clocks are not from CCM and we haven't defined=

 in

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

imx6q-clk.txt,
so in most cases we can't provide a phandle for the=

m, eg:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

spdif_ext.
I think it's a bit hard to force it to be 'required=

'. An

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

'optional'
looks more flexible to me and a default one is ensu=

red

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

even if
it's
missing.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

<&clks 0> is the dummy clock. This can be used for al=

l input

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

clocks
not
defined by the SoC.

=

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

This is how all i.MX clock bindings currently are. See
Documentation/devicetree/bindings/clock/imx*-clock.txt

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

OK, thanks.
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I guess we need some discussion on dummy clocks vs skipped =

clocks.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I think we want some consistency on this, don't we?
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

If we really need a dummy clock, then we might also want a =

generic

quoted

quoted

quoted

quoted

quoted

quoted

quoted

way to specify it.

=

quoted

quoted

quoted

quoted

quoted

quoted

What do we actually mean by a "dummy clock"? We already have
bindings
for "fixed-clock" and co friends describe relatively simple
preconfigured clocks.

=

quoted

quoted

quoted

quoted

quoted

Some platforms have a fake clock which defines noops callbacks =

and

quoted

quoted

quoted

quoted

quoted

basically doesn't do anything. This is analogous to the dummy
regulator
implementation. A central one could be registered by the clock =

core,

quoted

quoted

quoted

quoted

quoted

as
is done by the regulator core.

=

quoted

quoted

quoted

quoted

When you say some platforms, you presumably mean the platform cod=

e in

quoted

quoted

quoted

quoted

Linux? A dummy clock sounds like a completely Linux-specific abst=

raction

quoted

quoted

quoted

quoted

rather than a description of the hardware, and I don't see why we=

 need

quoted

quoted

quoted

quoted

that in the DT:
=

quoted

quoted

quoted

quoted

* If a clock is wired up and running (as presumably the dummy clo=

ck is),

quoted

quoted

quoted

quoted

then surely it's a fixed-clock (it's running, we and we have no c=

ontrol

quoted

quoted

quoted

quoted

over it, but we presumably know its rate) and can be described as=

 such?

quoted

quoted

quoted

quoted

=

quoted

quoted

quoted

quoted

* If no clock is wired up, then we should be able to describe tha=

t. If a

quoted

quoted

quoted

quoted

driver believes that a clock is required when it isn't (for some =

level

quoted

quoted

quoted

quoted

of functionality), then that driver should be fixed up to support=

 the

quoted

quoted

quoted

quoted

clock as being optional.
=

quoted

quoted

quoted

quoted

Am I missing something?

=

quoted

quoted

quoted

I second that.
=

quoted

quoted

quoted

Moreover, I don't think that device tree should deal with dummy any=

thing. =

quoted

quoted

quoted

It should be able to describe hardware that is available on given s=

ystem, =

quoted

quoted

quoted

not list what hardware is not available.

=

quoted

quoted

I wasn't clear. The dummy clock IS a completely Linux-specific
abstraction.
=

quoted

quoted

I'm not advocating a dummy clock in DT. I am advocating consolidation=

 of

quoted

quoted

the implementation of a clock that does nothing into the clock core.
This code could easily live in drivers/clk/clk.c instead of having
everyone open-code it.
=

quoted

quoted

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.

=

quoted

=

quoted

Sorry, I misunderstood. Good to hear we're on the same page :)
=

quoted

quoted

=

quoted

quoted

I'm guessing one of the reasons for such a clock are drivers do not
honor the clk.h api and they freak out when clk_get gives them a NULL
pointer?

=

quoted

I'm not sure. Sascha, could you shed some light on the matter?

=

The original reason introducing the dummy clocks in the i.MX dtbs
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
a bit to en/disable them.
=

Anyway, to accomplish this we don't need dummy clocks. We can just
describe the real clocks.

You could use a dummy clk for the Linux implementation, but the downside
is that a dummy clock has a rate of 0 always and a your clocks likely
have non-zero rates.

It is probably better for you define a clock which only implements the
.recalc_rate callback. If the rate of this clock changes without Linux
having knowledge of it you can use the CLK_GET_RATE_NOCACHE flag.

=

BTW with the S/PDIF core on which not all mux inputs are connected
to actual clocks we could also describe the unconnected inputs as
ground clocks with rate 0. This way we describe something which
is really there instead of dummy clocks ;)

Again you could use a dummy clock for this OR a fixed-rate clock with a
rate of zero from the perspective of the Linux implementation.

Do you think it worthwhile to have a DT binding for a grounded clock?
That is not an entirely uncommon case.

=

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?

Regards,
Mike

=

Sascha
=

-- =

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 |