Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
From: Alexandre Mergnat
Date: Tue Mar 12 2024 - 14:03:45 EST
On 26/02/2024 17:09, Mark Brown wrote:
On Mon, Feb 26, 2024 at 03:01:50PM +0100, amergnat@xxxxxxxxxxxx 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.
ok
+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.
ok
+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, ®);
+ 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.
Actually you must save the values because the gain selected by the user
will be override to do a ramp => volume_ramp(.....):
- When you switch on the HP, you start from gain=-40db to final_gain
(selected by user).
- When you switch off the HP, you start from final_gain (selected by
user) to gain=-40db.
Also, the microphone's gain change when it's enabled/disabled.
So, it can implemented differently but currently it's aligned with the
other MTK codecs and I don't see any resource wasted here.
+ /* 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.
Sorry, I don't understand your comment. Can you reword it please ?
+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.
I've dug into some other codecs and it's done like that, but I've
probably misunderstood something.
The only bad thing I see is enum is missing currently:
enum {
PGA_MUX_OPEN = 0,
PGA_MUX_DACR,
PGA_MUX_PB,
PGA_MUX_TM,
PGA_MUX_MASK = 0x3,
};
static const char * const hslo_mux_map[] = {
"Open", "DACR", "Playback", "Test mode"
};
static int hslo_mux_map_value[] = {
PGA_MUX_OPEN, PGA_MUX_DACR, PGA_MUX_PB, PGA_MUX_TM,
};
+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.
ok
+/* 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.
ok
--
Regards,
Alexandre