Re: [PATCH 2/7] ASoC: codec: lpass-rx-macro: add support for lpass rx macro

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]

On Thu, Jan 28, 2021 at 02:19:24PM +0000, Srinivas Kandagatla wrote:

>  snd-soc-lpass-wsa-macro-objs := lpass-wsa-macro.o
>  snd-soc-lpass-va-macro-objs := lpass-va-macro.o
> +snd-soc-lpass-rx-macro-objs := lpass-rx-macro.o

Please keep things sorted.

> @@ -0,0 +1,2020 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +static const char *const rx_macro_ear_mode_text[] = {"OFF", "ON"};
> +static const struct soc_enum rx_macro_ear_mode_enum =
> +	SOC_ENUM_SINGLE_EXT(2, rx_macro_ear_mode_text);

On/off controls should be standard Switch controls.

> +		if (rx->rx_mclk_users == 0) {
> +			regcache_mark_dirty(regmap);
> +			regcache_sync(regmap);

I'd expect this to be joined up with whatever caused the register state
to become invalid, this looks like it's inviting bugs.  This also seems
to have only one caller...

> +	SOC_ENUM_EXT("RX_HPH HD2 Mode", rx_macro_hph_hd2_mode_enum,
> +		rx_macro_get_hph_hd2_mode, rx_macro_put_hph_hd2_mode),
> +
> +	SOC_ENUM_EXT("RX_HPH_PWR_MODE", rx_macro_hph_pwr_mode_enum,
> +		rx_macro_get_hph_pwr_mode, rx_macro_put_hph_pwr_mode),

The naming seems a bit random here.

> +static int rx_swrm_clock(struct rx_macro *rx, bool enable)
> +{

> +static int swclk_gate_enable(struct clk_hw *hw)
> +{
> +	return rx_swrm_clock(to_rx_macro(hw), true);
> +}
> +
> +static void swclk_gate_disable(struct clk_hw *hw)
> +{
> +	rx_swrm_clock(to_rx_macro(hw), false);
> +}

This all seems very redundant and like it'll get in the way of grepping
for users.  It would be better to just inline the operation into the clk
API functions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]