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

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrote:
> Quoting Tomasz Figa (2013-08-21 14:34:55)
> > On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote:
> > > On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote:
> > > > Quoting Mark Rutland (2013-08-19 02:35:43)
> > > > 
> > > > > On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote:
> > > > > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote:
> > > > > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote:
> > > > > > > > > > > Also I would make this option required. Use a dummy
> > > > > > > > > > > clock for
> > > > > > > > > > > mux
> > > > > > > > > > > inputs that are grounded for a specific SoC.
> > > > > > > > > > 
> > > > > > > > > > Some clocks are not from CCM and we haven't defined in
> > > > > > > > > > imx6q-clk.txt,
> > > > > > > > > > so in most cases we can't provide a phandle for 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 one is ensured
> > > > > > > > > > even if
> > > > > > > > > > it's
> > > > > > > > > > missing.
> > > > > > > > > 
> > > > > > > > > <&clks 0> is the dummy clock. This can be used for all input
> > > > > > > > > clocks
> > > > > > > > > not
> > > > > > > > > defined by the SoC.
> > > > > > > > 
> > > > > > > > Where does this assumption come from? Is it documented
> > > > > > > > anywhere?
> > > > > > > 
> > > > > > > This is how all i.MX clock bindings currently are. See
> > > > > > > Documentation/devicetree/bindings/clock/imx*-clock.txt
> > > > > > 
> > > > > > OK, thanks.
> > > > > > 
> > > > > > I guess we need some discussion on dummy clocks vs skipped clocks.
> > > > > > I think we want some consistency on this, don't we?
> > > > > > 
> > > > > > If we really need a dummy clock, then we might also want a generic
> > > > > > way to specify it.
> > > > > 
> > > > > What do we actually mean by a "dummy clock"? We already have
> > > > > bindings
> > > > > for "fixed-clock" and co friends describe relatively simple
> > > > > preconfigured clocks.
> > > > 
> > > > Some platforms have a fake clock which defines noops callbacks and
> > > > basically doesn't do anything. This is analogous to the dummy
> > > > regulator
> > > > implementation. A central one could be registered by the clock core,
> > > > as
> > > > is done by the regulator core.
> > > 
> > > When you say some platforms, you presumably mean the platform code in
> > > Linux? A dummy clock sounds like a completely Linux-specific abstraction
> > > rather than a description of the hardware, and I don't see why we need
> > > that in the DT:
> > > 
> > > * If a clock is wired up and running (as presumably the dummy clock is),
> > > then surely it's a fixed-clock (it's running, we and we have no control
> > > over it, but we presumably know its rate) and can be described as such?
> > > 
> > > * If no clock is wired up, then we should be able to describe that. If a
> > > driver believes that a clock is required when it isn't (for some level
> > > of functionality), then that driver should be fixed up to support the
> > > clock as being optional.
> > > 
> > > Am I missing something?
> > 
> > I second that.
> > 
> > Moreover, I don't think that device tree should deal with dummy anything. 
> > It should be able to describe hardware that is available on given system, 
> > not list what hardware is not available.
> 
> I wasn't clear. The dummy clock IS a completely Linux-specific
> abstraction.
> 
> I'm not advocating a dummy clock in DT. I am advocating consolidation of
> 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.
> 
> 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.


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

> 
> 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?

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

Thanks,
Mark.