[alsa-devel] [PATCH 02/13] ASoC: tlv320aic32x4: Model PLL in CCF

[Mark Brown]

On Mon, Mar 18, 2019 at 08:37:45PM -0700, Annaliese McDermond wrote:

> ---
>  sound/soc/codecs/Makefile            |   2 +-
>  sound/soc/codecs/tlv320aic32x4-clk.c | 323 +++++++++++++++++++++++++++
>  sound/soc/codecs/tlv320aic32x4.c     |  93 ++++----
>  sound/soc/codecs/tlv320aic32x4.h     |   5 +
>  4 files changed, 379 insertions(+), 44 deletions(-)

I'm not seeing any Kconfig updates which add dependencies on the clock
API or conditionally build the clock implementation only if that's been
enabled.

> +++ b/sound/soc/codecs/tlv320aic32x4-clk.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * linux/sound/soc/codecs/tlv320aic32x4-clk.c
> + *
> + * Copyright 2019 Annaliese McDermond
> + *
> + * Author: Annaliese McDermond <nh6z at nh6z.net>
> + */

Please make the entire comment a C++ comment so it looks more
intentional.  It's also better to not have the path in the header since
that's prone to bitrot, just write something in words.

> +static const struct clk_ops aic32x4_pll_ops = {
> +	.enable = clk_aic32x4_pll_enable,
> +	.disable = clk_aic32x4_pll_disable,
> +	.is_enabled = clk_aic32x4_pll_is_enabled,

These are enable and disable operations - shouldn't they be prepare and
unprepare?  The device is controlled over buses that require sleeping
but the prepare and enable operations require atomic context.

> +}
> +EXPORT_SYMBOL(aic32x4_register_clocks);

ASoC is all EXPORT_SYMBOL_GPL()...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20190319/37b1c550/attachment.sig>