On Mon, Feb 26, 2024 at 03:01:50PM +0100, amergnat@baylibre.com wrote:

index 000000000000..13e95c227114 --- /dev/null +++ b/sound/soc/codecs/mt6357.c @@ -0,0 +1,1805 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • MT6357 ALSA SoC audio codec driver

Please use a C++ comment for the whole comment to make it clearer that this is intentional.

+static void set_playback_gpio(struct mt6357_priv *priv, bool enable) +{

• if (enable) {

• /* set gpio mosi mode */
  

• regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL);
  

• regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, GPIO8_MODE_SET_AUD_CLK_MOSI |
  

• 						  GPIO9_MODE_SET_AUD_DAT_MOSI0 |
  

• 						  GPIO10_MODE_SET_AUD_DAT_MOSI1 |
  

• 						  GPIO11_MODE_SET_AUD_SYNC_MOSI);
  

This would be a lot more legible if you worked out the values to set and then had a single set of writes, currently the indentation makes it very hard to read. Similarly for other similar functions.

+static int mt6357_put_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) +{

• struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
• struct mt6357_priv *priv = snd_soc_component_get_drvdata(component);
• struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value;
• unsigned int reg;
• int ret;
• ret = snd_soc_put_volsw(kcontrol, ucontrol);
• if (ret < 0)

• return ret;
  

• switch (mc->reg) {
• case MT6357_ZCD_CON2:

• regmap_read(priv->regmap, MT6357_ZCD_CON2, &reg);
  

• priv->ana_gain[ANALOG_VOLUME_HPOUTL] =
  

• 	(reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT;
  

• priv->ana_gain[ANALOG_VOLUME_HPOUTR] =
  

• 	(reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT;
  

• break;
  

It would probably be less code and would definitely be clearer and simpler to just read the values when we need them rather than constatly keeping a cache separate to the register cache.

• /* ul channel swap */
• SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0),

On/off controls should end in Switch.

+static const char * const hslo_mux_map[] = {

• "Open", "DACR", "Playback", "Test mode"

+};

+static int hslo_mux_map_value[] = {

• 0x0, 0x1, 0x2, 0x3,

+};

Why not just use a normal mux here, there's no missing values or reordering? Similarly for other muxes.

+static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) +{

• struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec);
• unsigned int val;
• pr_debug("%s() reg = 0x%x", __func__, reg);
• regmap_read(priv->regmap, reg, &val);
• return val;

+}

Remove these, there are vastly more logging facilities as standard in the regmap core.

+/* Reg bit defines */ +/* MT6357_GPIO_DIR0 */ +#define GPIO8_DIR_MASK BIT(8) +#define GPIO8_DIR_INPUT 0

Please namespace your defines, these look very generic.