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

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.

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.