[alsa-devel] [PATCH v4 3/3] ASoC: fsl: add imx-es8328 machine driver

[Sean Cross]

On 06/18/14 18:31, Mark Brown wrote:
> On Wed, Jun 18, 2014 at 06:22:52PM +0800, Sean Cross wrote:
>> On 06/18/14 18:02, Mark Brown wrote:
>>> This should be handled by the clock bindings not open coded in the
>>> driver - leaving this here most likely won't play nicely when the clock
>>> API can configure the defaults for the tree.  There is supposed to be
>>> support for setting default clock trees going in (or perhaps already in)
>>> the clock bindings.
>> Can you give me more information on it?  Currently, it looks like most
>> boards use a 24 MHz clock, judging from this comment in
>> mach-imx/clk-imx6q.c:
> Look at the clock API, this stuff was introduced in the last merge
> window if it's there yet at all.
I don't see anything mentioning it.  If I remove the parenting calls
then the es8328 will refuse to run as its clock is at 66 MHz rather than
the requested 22.5792 MHz.  But since you say the default clock tree
support is going to be merged, I'd like to try using that code.  Is this
the "assigned-clock-parents" patch you're referring to, or is there a
patchset that will automatically reparent as necessary?

>> This codec requires the more unusual 22.5792 MHz clock.  What is the
>> appropriate method of obtaining this particular frequency?
> clk_set_rate() on the directly connected clock, the problem is fiddling
> about with the parenting rather than setting the rate.
>
>>> No, this is broken.  The CODEC should request its own supplies which
>>> need to correspond to the supplies the physical device has and failing
>>> to get the supplies should be a fatal error unless the device works
>>> without power (in which case why bother enablin them at all?).
>> Not all codecs have power supplies.  Most don't, in fact, it's just this
> The manufactuers of those that don't are being awfully quiet about what
> sounds like a rather impressive feature...
>
>> Additionally, since the regulator is external to the codec (as it
>> physically cuts 3.3V from the power supply), it doesn't make sense to
>> put it in the codec driver.
> I'm not sure you've quite understood what the regulator API is there
> for.
I'm having trouble understanding where the separating line is between
machine drivers and codec drivers.  A random sampling of codec drivers
doesn't yield any drivers grabbing their own power switches.  Is that an
oversight?  Should every codec driver include at least one regulator to
describe its power source?

If you like, I can move the regulator from the machine driver to the
codec driver, and make it non-optional.  But we've done designs in the
past where it just hangs off the 3.3V rail where it's non-switchable,
and the concept of describing a regulator seemed overkill.


Sean

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 880 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140619/52e0d2c6/attachment.sig>