On 12/17/18 1:14 PM, Pierre-Louis Bossart wrote:
On 12/17/18 1:02 PM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
On 12/17/18 11:39 AM, Mark Brown wrote:
That looks a lot like the CODEC should be exporting a GPIO driver
so the
machine driver doesn't actually need the regmap? The only register
touched is _GPIO_CONTROL_1.
I am not sure what you meant by 'exporting a GPIO driver' (mostly
because I
am not familiar with any GPIO framework) but indeed the local
oscillator
choice is controlled by a single register accessible through regmap
- and
changes to that register should only happen when the device is a
specific
state to prevent click/pops.
The GPIO framework provides a fairly simple view of GPIOs - from a user
point of view it's just getting or setting the value of a line. It
looks like the register you're controlling isn't actually controlling a
chip feature directly but rather is setting a GPIO on the chip which
controls an external clock generator. I could be misparsing things,
though. I did glance at the pcm512x datasheet and didn't see pins or
anything that looked like an oscillator but I could've missed something.
You are correct, there are two variants of the Hifiberry DAC+, the
'PRO' version with two oscillators on board (SoC configured as
bitclock and fsync slave) and the regular without (all clocks provided
by the SoC in that case). It's indeed a GPIO control of an external
component, not an on-chip capability.
The machine driver should use clk_set_rate() and not directly handle
regmap
or codec stuff. If it does, or if the clock framework isn't relevant
here
then we can simplify all this as suggested in
https://patchwork.kernel.org/patch/10444387/. What I was trying to
do with
the github update is to keep the clock framework, tie it closer with
the
codec parts with a state variable that prevents wild changes without
going
back to a 'safe' idle state (similar idea as PulseAudio clock
changes, which
can only happen when the PCM is not opened and used).
Right, I think bringing in the clock framework more is good -
effectively all I'm suggesting is changing the control interface used to
set the clock to add an indirection through gpiolib so you don't need to
pass the raw register map around.
this looks elegant indeed, will look into it. I'll have to learn more
on gpios :-)
I was able to expose the 6 PCM512x GPIOs, toggle them from userspace as
well as from the clock driver, it's indeed simple enough and could be
useful for other usages, but there are still two issues with the concept:
1. After toggling a GPIO to enable the 44.1 or 48kHz oscillators, the
clock driver cannot verify that the codec is locked, for this it'd need
access to the regmap to check the PCM512x_RATE_DET_4 register. This is
really needed to detect if the local oscillators are populated or not
and hence detect the DAC+ PRO vs. the plain vanilla DAC+. An alternative
would be to let the clock driver program GPIOs, and check in the codec
driver is the clock is valid or not after the call to
clk_prepare_enable() and fall back to slave mode in the latter case. I
believe it'd be more elegant to do all these checks in the clock driver
itself, and only register a clock if those local oscillators actually
exist. I also can't figure out how to pass a regmap as platform data,
the size argument can't be just a pointer and sizeof(*regmap) won't work
either.
2. there is a mutual dependency between codec and clock driver. The
codec exposes the gpios needed by the clock driver which in turn
registers a clock needed by the codec driver. is it acceptable to create
the clock platform device from the codec driver to remove any sort of
race conditions or need for deferred probes?
Thanks
-Pierre
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
