Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On Fri, Mar 15, 2024 at 06:36:19PM +0100, Alexandre Mergnat wrote: > On 15/03/2024 16:15, Mark Brown wrote: > > On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: > > > > In the register. You only need to reset the gain to -40dB at the start > > > > of the ramp. > > > Sorry but I don't understand your logic, I'm not able to implement it... > > > If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the > > > register the value will be -40dB. > > After we've done the ramp and turned the amplifier off we can just > > restore the desired value? The hardware is not going to care what the > > volume is while it's not enabled. > If you do that, HP will be enabled at the saved gain, and after that you > will do the ramp. To avoid pop, the driver should be rewrite to: So reset the volume to -40dB prior to turning the amplifier on... > Read gain in the reg and save it locally > Set -40dB in the reg > Enable HP > Do ramp ...as you yourself suggest? > > > AFAII from the comment in the code, it's done to avoid the "pop noises". > > Yes, that's the usual reason to ramp gains. Though if you've just > > copied the code without checking that it's needed it's possible that > > this is something that's been fixed in current hardware. > I did the test at 24dB with and without the "pop filter". Isn't big but I > ear the pop at the start of the record without the "pop filter". OK, it probably is still doing something then. > To be clear, the algo/behavior of this code is an implementation based on > the 6k+ lines downstream code for this specific audio codec. But the > shape/style is based on upstreamed drivers like mt6358.c. The Mediatek code has a bunch of issues, I wouldn't read too much into something being present in the code. signature.asc Description: PGP signature
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On 15/03/2024 16:15, Mark Brown wrote: On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: On 15/03/2024 15:30, Mark Brown wrote: Let me know, when you change de gain to do a ramp down (start from user gain to gain=-40db), next time for the ramp up, how/where do you find the user gain ? In the register. You only need to reset the gain to -40dB at the start of the ramp. Sorry but I don't understand your logic, I'm not able to implement it... If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the register the value will be -40dB. After we've done the ramp and turned the amplifier off we can just restore the desired value? The hardware is not going to care what the volume is while it's not enabled. If you do that, HP will be enabled at the saved gain, and after that you will do the ramp. To avoid pop, the driver should be rewrite to: Read gain in the reg and save it locally Set -40dB in the reg Enable HP Do ramp And for the shutdown: Read gain in the reg and save it locally Do ramp Disable HP Set saved gain in the reg To resume, that add 4 more steps to save 2 integers into the driver structure. IMHO, I don't think it make the code more readable or optimized, but I don't have a strong opinion about that, so if you think it's better, I will change it. This implementation is also done in other MTK audio codec drivers. Perhaps they should be updated too? When microphone isn't capturing, the gain read back from the register is 0dB. I've put some logs in my code and do capture to show how it works: Is this a property of the hardware or a property of your driver? At the end of the capture, the gain is set to 0dB by the driver. At the start of the capture, the gain is set to the setup gain. So that's a property of the driver then? Yes AFAII from the comment in the code, it's done to avoid the "pop noises". Yes, that's the usual reason to ramp gains. Though if you've just copied the code without checking that it's needed it's possible that this is something that's been fixed in current hardware. I did the test at 24dB with and without the "pop filter". Isn't big but I ear the pop at the start of the record without the "pop filter". To be clear, the algo/behavior of this code is an implementation based on the 6k+ lines downstream code for this specific audio codec. But the shape/style is based on upstreamed drivers like mt6358.c. -- Regards, Alexandre
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: > On 15/03/2024 15:30, Mark Brown wrote: > > > Let me know, when you change de gain to do a ramp down (start from user > > > gain > > > to gain=-40db), next time for the ramp up, how/where do you find the user > > > gain ? > > In the register. You only need to reset the gain to -40dB at the start > > of the ramp. > Sorry but I don't understand your logic, I'm not able to implement it... > If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the > register the value will be -40dB. After we've done the ramp and turned the amplifier off we can just restore the desired value? The hardware is not going to care what the volume is while it's not enabled. > This implementation is also done in other MTK audio codec drivers. Perhaps they should be updated too? > > > When microphone isn't capturing, the gain read back from the register is > > > 0dB. I've put some logs in my code and do capture to show how it works: > > Is this a property of the hardware or a property of your driver? > At the end of the capture, the gain is set to 0dB by the driver. > At the start of the capture, the gain is set to the setup gain. So that's a property of the driver then? > AFAII from the comment in the code, it's done to avoid the "pop noises". Yes, that's the usual reason to ramp gains. Though if you've just copied the code without checking that it's needed it's possible that this is something that's been fixed in current hardware. signature.asc Description: PGP signature
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On 15/03/2024 15:30, Mark Brown wrote:
On Fri, Mar 15, 2024 at 12:01:12PM +0100, Alexandre Mergnat wrote:
On 13/03/2024 18:23, Mark Brown wrote:
On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote:
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.
You can just read the value back when you need to do a ramp?
You can't. Because you will read -40db when HP isn't playing sound. That is
why the gain is saved into the struct.
Let me know, when you change de gain to do a ramp down (start from user gain
to gain=-40db), next time for the ramp up, how/where do you find the user
gain ?
In the register. You only need to reset the gain to -40dB at the start
of the ramp.
Sorry but I don't understand your logic, I'm not able to implement it...
If I'm at -10dB and doing a ramp to reach -40dB, next time I will read
the register the value will be -40dB.
This implementation is also done in other MTK audio codec drivers.
Also, the microphone's gain change when it's enabled/disabled.
I don't understand what this means?
When microphone isn't capturing, the gain read back from the register is
0dB. I've put some logs in my code and do capture to show how it works:
Is this a property of the hardware or a property of your driver?
At the end of the capture, the gain is set to 0dB by the driver.
At the start of the capture, the gain is set to the setup gain.
AFAII from the comment in the code, it's done to avoid the "pop noises".
+ /* 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 ?
See control-names.rst. Run mixer-test on a card with this driver and
fix all the issues it reports.
Ok the name is the issue for you AFAII.
This control isn't for on/off but swap Left and Right.
From the codec documentation:
"Swaps audio UL L/R channel before UL SRC"
This control is overkill, I will remove it
This is turning the swapping on and off.
--
Regards,
Alexandre
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On Fri, Mar 15, 2024 at 12:01:12PM +0100, Alexandre Mergnat wrote:
> On 13/03/2024 18:23, Mark Brown wrote:
> > On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote:
> > > 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.
> > You can just read the value back when you need to do a ramp?
> You can't. Because you will read -40db when HP isn't playing sound. That is
> why the gain is saved into the struct.
> Let me know, when you change de gain to do a ramp down (start from user gain
> to gain=-40db), next time for the ramp up, how/where do you find the user
> gain ?
In the register. You only need to reset the gain to -40dB at the start
of the ramp.
> > > Also, the microphone's gain change when it's enabled/disabled.
> > I don't understand what this means?
> When microphone isn't capturing, the gain read back from the register is
> 0dB. I've put some logs in my code and do capture to show how it works:
Is this a property of the hardware or a property of your driver?
> > > > > + /* 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 ?
> > See control-names.rst. Run mixer-test on a card with this driver and
> > fix all the issues it reports.
> Ok the name is the issue for you AFAII.
> This control isn't for on/off but swap Left and Right.
> From the codec documentation:
> "Swaps audio UL L/R channel before UL SRC"
> This control is overkill, I will remove it
This is turning the swapping on and off.
signature.asc
Description: PGP signature
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On 13/03/2024 18:23, Mark Brown wrote:
On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote:
On 26/02/2024 17:09, Mark Brown wrote:
+ 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.
You can just read the value back when you need to do a ramp?
You can't. Because you will read -40db when HP isn't playing sound. That
is why the gain is saved into the struct.
Let me know, when you change de gain to do a ramp down (start from user
gain to gain=-40db), next time for the ramp up, how/where do you find
the user gain ?
Also, the microphone's gain change when it's enabled/disabled.
I don't understand what this means?
When microphone isn't capturing, the gain read back from the register is
0dB. I've put some logs in my code and do capture to show how it works:
root@i350-evk:~# arecord -D hw:mt8365evk,2,0 -r 48000 -c2 -f s32_le -d
10 recorded_file.wav
[Mar15 09:31] mt8365-afe-pcm 1122.audio-controller:
mt8365_afe_fe_hw_params AWB period = 6000 rate = 48000 channels = 2
[ +0.000126] mt8365-afe-pcm 1122.audio-controller:
mt8365_dai_int_adda_prepare 'Capture' rate = 48000
[ +0.107688] mt6357-sound mt6357-sound: TOTO set mic to stored value
[ +10.072648] mt6357-sound mt6357-sound: TOTO set mic to 0dB
root@i350-evk:~# arecord -D hw:mt8365evk,2,0 -r 48000 -c2 -f s32_le -d
10 recorded_file.wav
[Mar15 09:32] mt8365-afe-pcm 1122.audio-controller:
mt8365_afe_fe_hw_params AWB period = 6000 rate = 48000 channels = 2
[ +0.000133] mt8365-afe-pcm 1122.audio-controller:
mt8365_dai_int_adda_prepare 'Capture' rate = 48000
[ +0.109418] mt6357-sound mt6357-sound: TOTO set mic to stored value
[ +10.164197] mt6357-sound mt6357-sound: TOTO set mic to 0dB
+ /* 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 ?
See control-names.rst. Run mixer-test on a card with this driver and
fix all the issues it reports.
Ok the name is the issue for you AFAII.
This control isn't for on/off but swap Left and Right.
From the codec documentation:
"Swaps audio UL L/R channel before UL SRC"
This control is overkill, I will remove it
I'm stuck to run mixer-test, please check the following message:
https://lore.kernel.org/all/7ddad394-e880-4ef8-8591-cb803a208...@baylibre.com/
--
Regards,
Alexandre
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On Wed, Mar 13, 2024 at 06:11:50PM +0100, Alexandre Mergnat wrote: > On 26/02/2024 17:09, Mark Brown wrote: > > > index ..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. > If I do that, the checkpatch raise a warning: > WARNING: Improper SPDX comment style for > 'sound/soc/mediatek/mt8365/mt8365-afe-clk.c', please use '//' instead > #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1: > +/* SPDX-License-Identifier: GPL-2.0 That's not a C++ comment so checkpatch is correctly warning? signature.asc Description: PGP signature
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote:
> On 26/02/2024 17:09, Mark Brown wrote:
> > > + 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.
You can just read the value back when you need to do a ramp?
> Also, the microphone's gain change when it's enabled/disabled.
I don't understand what this means?
> > > + /* 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 ?
See control-names.rst. Run mixer-test on a card with this driver and
fix all the issues it reports.
> > > +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,
> };
The whole thing with explicitly specfying the mapping is just completely
redundant, you may as well remove it.
signature.asc
Description: PGP signature
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On 26/02/2024 17:09, Mark Brown wrote: index ..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. If I do that, the checkpatch raise a warning: WARNING: Improper SPDX comment style for 'sound/soc/mediatek/mt8365/mt8365-afe-clk.c', please use '//' instead #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1: +/* SPDX-License-Identifier: GPL-2.0 WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1: +/* SPDX-License-Identifier: GPL-2.0 even if I put: /* SPDX-License-Identifier: GPL-2.0 */ IMO, the checkpatch tool should be fixed/update first. -- Regards, Alexandre
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On 26/02/2024 17:09, Mark Brown wrote:
On Mon, Feb 26, 2024 at 03:01:50PM +0100, amerg...@baylibre.com wrote:
index ..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, );
+ 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_INPUT0
Please namespace your defines, these look very generic.
ok
--
Regards,
Alexandre
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
Il 12/03/24 15:50, Alexandre Mergnat ha scritto:
On 26/02/2024 16:25, AngeloGioacchino Del Regno wrote:
+ 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);
Are you sure that you need to write to MODE2_SET *and* to MODE2?!
This is downstream code and these registers aren't in my documentation.
I've removed the MODE2_SET write and test the audio: it's still working.
So I will keep the spurious write removed for v2. :)
Usually, MediaTek registers are laid out like "REG" being R/legacy-W and
"REG_SET/CLR" for setting and clearing bits in "REG" internally, and that
might account for internal latencies and such.
Can you please try to remove the MODE2 write instead of the MODE2_SET one
and check if that works?
You're already using the SETCLR way when manipulating registers in here,
so I would confidently expect that to work.
Cheers,
Angelo
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On 26/02/2024 16:25, AngeloGioacchino Del Regno wrote:
+ 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);
Are you sure that you need to write to MODE2_SET *and* to MODE2?!
This is downstream code and these registers aren't in my documentation.
I've removed the MODE2_SET write and test the audio: it's still working.
So I will keep the spurious write removed for v2. :)
--
Regards,
Alexandre
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
On Mon, Feb 26, 2024 at 03:01:50PM +0100, amerg...@baylibre.com wrote:
> index ..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, );
> + 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, );
> + 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.
signature.asc
Description: PGP signature
Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
Il 26/02/24 15:01, amerg...@baylibre.com ha scritto:
From: Nicolas Belin
Add the support of MT6357 PMIC audio codec.
Signed-off-by: Nicolas Belin
Signed-off-by: Alexandre Mergnat
---
sound/soc/codecs/Kconfig |7 +
sound/soc/codecs/Makefile |2 +
sound/soc/codecs/mt6357.c | 1805 +
sound/soc/codecs/mt6357.h | 674 +
4 files changed, 2488 insertions(+)
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 59f9742e9ff4..9cf2b9b70472 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -153,6 +153,7 @@ config SND_SOC_ALL_CODECS
imply SND_SOC_MC13783
imply SND_SOC_ML26124
imply SND_SOC_MT6351
+ imply SND_SOC_MT6357
imply SND_SOC_MT6358
imply SND_SOC_MT6359
imply SND_SOC_MT6660
@@ -2361,6 +2362,12 @@ config SND_SOC_ML26124
config SND_SOC_MT6351
tristate "MediaTek MT6351 Codec"
+config SND_SOC_MT6357
+ tristate "MediaTek MT6357 Codec"
+ help
+ Enable support for the platform which uses MT6357 as
+ external codec device.
+
config SND_SOC_MT6358
tristate "MediaTek MT6358 Codec"
help
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index f53baa2b9565..33e27612867e 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -169,6 +169,7 @@ snd-soc-ml26124-objs := ml26124.o
snd-soc-msm8916-analog-objs := msm8916-wcd-analog.o
snd-soc-msm8916-digital-objs := msm8916-wcd-digital.o
snd-soc-mt6351-objs := mt6351.o
+snd-soc-mt6357-objs := mt6357.o
snd-soc-mt6358-objs := mt6358.o
snd-soc-mt6359-objs := mt6359.o
snd-soc-mt6359-accdet-objs := mt6359-accdet.o
@@ -554,6 +555,7 @@ obj-$(CONFIG_SND_SOC_ML26124) += snd-soc-ml26124.o
obj-$(CONFIG_SND_SOC_MSM8916_WCD_ANALOG) +=snd-soc-msm8916-analog.o
obj-$(CONFIG_SND_SOC_MSM8916_WCD_DIGITAL) +=snd-soc-msm8916-digital.o
obj-$(CONFIG_SND_SOC_MT6351) += snd-soc-mt6351.o
+obj-$(CONFIG_SND_SOC_MT6357) += snd-soc-mt6357.o
obj-$(CONFIG_SND_SOC_MT6358) += snd-soc-mt6358.o
obj-$(CONFIG_SND_SOC_MT6359) += snd-soc-mt6359.o
obj-$(CONFIG_SND_SOC_MT6359_ACCDET) += mt6359-accdet.o
diff --git a/sound/soc/codecs/mt6357.c b/sound/soc/codecs/mt6357.c
new file mode 100644
index ..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
+ *
+ * Copyright (c) 2024 Baylibre
+ * Author: Nicolas Belin
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "mt6357.h"
+
+static void set_playback_gpio(struct mt6357_priv *priv, bool enable)
+{
you're anyway always doing CLEAR_ALL, so you can do it outside of the if branch.
regmap_write( ... CLEAR_ALL);
if (enable) {
...
} else {
...
}
+ 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);
Are you sure that you need to write to MODE2_SET *and* to MODE2?!
+ regmap_write(priv->regmap, MT6357_GPIO_MODE2,
GPIO8_MODE_AUD_CLK_MOSI |
+
GPIO9_MODE_AUD_DAT_MOSI0 |
+
GPIO10_MODE_AUD_DAT_MOSI1 |
+
GPIO11_MODE_AUD_SYNC_MOSI);
+ } else {
+ /* set pad_aud_*_mosi to GPIO mode and dir input
+* reason:
+* pad_aud_dat_mosi*, because the pin is used as boot strap
+*/
+ regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR,
GPIO_MODE2_CLEAR_ALL);
+ regmap_write(priv->regmap, MT6357_GPIO_MODE2, GPIO8_MODE_GPIO |
+ GPIO9_MODE_GPIO |
+ GPIO10_MODE_GPIO |
+ GPIO11_MODE_GPIO);
+ regmap_update_bits(priv->regmap, MT6357_GPIO_DIR0,
GPIO8_DIR_MASK |
+
GPIO9_DIR_MASK |
+
GPIO10_DIR_MASK |
+
GPIO11_DIR_MASK,
+
GPIO8_DIR_INPUT |
+
GPIO9_DIR_INPUT |
+
[PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec
From: Nicolas Belin
Add the support of MT6357 PMIC audio codec.
Signed-off-by: Nicolas Belin
Signed-off-by: Alexandre Mergnat
---
sound/soc/codecs/Kconfig |7 +
sound/soc/codecs/Makefile |2 +
sound/soc/codecs/mt6357.c | 1805 +
sound/soc/codecs/mt6357.h | 674 +
4 files changed, 2488 insertions(+)
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 59f9742e9ff4..9cf2b9b70472 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -153,6 +153,7 @@ config SND_SOC_ALL_CODECS
imply SND_SOC_MC13783
imply SND_SOC_ML26124
imply SND_SOC_MT6351
+ imply SND_SOC_MT6357
imply SND_SOC_MT6358
imply SND_SOC_MT6359
imply SND_SOC_MT6660
@@ -2361,6 +2362,12 @@ config SND_SOC_ML26124
config SND_SOC_MT6351
tristate "MediaTek MT6351 Codec"
+config SND_SOC_MT6357
+ tristate "MediaTek MT6357 Codec"
+ help
+ Enable support for the platform which uses MT6357 as
+ external codec device.
+
config SND_SOC_MT6358
tristate "MediaTek MT6358 Codec"
help
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index f53baa2b9565..33e27612867e 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -169,6 +169,7 @@ snd-soc-ml26124-objs := ml26124.o
snd-soc-msm8916-analog-objs := msm8916-wcd-analog.o
snd-soc-msm8916-digital-objs := msm8916-wcd-digital.o
snd-soc-mt6351-objs := mt6351.o
+snd-soc-mt6357-objs := mt6357.o
snd-soc-mt6358-objs := mt6358.o
snd-soc-mt6359-objs := mt6359.o
snd-soc-mt6359-accdet-objs := mt6359-accdet.o
@@ -554,6 +555,7 @@ obj-$(CONFIG_SND_SOC_ML26124) += snd-soc-ml26124.o
obj-$(CONFIG_SND_SOC_MSM8916_WCD_ANALOG) +=snd-soc-msm8916-analog.o
obj-$(CONFIG_SND_SOC_MSM8916_WCD_DIGITAL) +=snd-soc-msm8916-digital.o
obj-$(CONFIG_SND_SOC_MT6351) += snd-soc-mt6351.o
+obj-$(CONFIG_SND_SOC_MT6357) += snd-soc-mt6357.o
obj-$(CONFIG_SND_SOC_MT6358) += snd-soc-mt6358.o
obj-$(CONFIG_SND_SOC_MT6359) += snd-soc-mt6359.o
obj-$(CONFIG_SND_SOC_MT6359_ACCDET) += mt6359-accdet.o
diff --git a/sound/soc/codecs/mt6357.c b/sound/soc/codecs/mt6357.c
new file mode 100644
index ..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
+ *
+ * Copyright (c) 2024 Baylibre
+ * Author: Nicolas Belin
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "mt6357.h"
+
+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);
+ regmap_write(priv->regmap, MT6357_GPIO_MODE2,
GPIO8_MODE_AUD_CLK_MOSI |
+
GPIO9_MODE_AUD_DAT_MOSI0 |
+
GPIO10_MODE_AUD_DAT_MOSI1 |
+
GPIO11_MODE_AUD_SYNC_MOSI);
+ } else {
+ /* set pad_aud_*_mosi to GPIO mode and dir input
+* reason:
+* pad_aud_dat_mosi*, because the pin is used as boot strap
+*/
+ regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR,
GPIO_MODE2_CLEAR_ALL);
+ regmap_write(priv->regmap, MT6357_GPIO_MODE2, GPIO8_MODE_GPIO |
+ GPIO9_MODE_GPIO |
+ GPIO10_MODE_GPIO |
+ GPIO11_MODE_GPIO);
+ regmap_update_bits(priv->regmap, MT6357_GPIO_DIR0,
GPIO8_DIR_MASK |
+
GPIO9_DIR_MASK |
+
GPIO10_DIR_MASK |
+
GPIO11_DIR_MASK,
+
GPIO8_DIR_INPUT |
+
GPIO9_DIR_INPUT |
+
GPIO10_DIR_INPUT |
+
GPIO11_DIR_INPUT);
+ }
+}
+
+static void set_capture_gpio(struct mt6357_priv *priv, bool enable)
+{
+ if (enable) {
+ /* set gpio miso mode
