Dear Mark Brown,
Thanks for your detailed feedback. I'm so late in replying.
COVID-19 is going around again in Korea. I also suffered from COVID-19 this time.
Please take care of yourself from COVID-19.
Anyway, I proceeded with the update again by referring to your review. And I added my opinion to the following. Please check the below.
If there is anything wrong, please give me a feedback again.
Thank you for your patience and cooperation.
Best regards,
Ki Seok Jo
Iron Device Corporation
Tel.: +82-2-541-2896
Mobile: +82-10-3320-0376
Email: kiseok.jo@xxxxxxxxxxxxxx
The information in this email and any attachment is confidential and may be legally protected. It is intended solely for the addressee. Access to this email by anyone else is unauthorised. If you are not the intended recipient, any disclosure or actions taken as a result of the information in this email is prohibited and may be unlawful. If you are not the intended recipient, please notify us immediately and then delete this e-mail and any attachment.
-----Original Message-----
From: Mark Brown <broonie@xxxxxxxxxx>
Sent: Wednesday, August 17, 2022 9:09 PM
To: Ki-Seok Jo <kiseok.jo@xxxxxxxxxxxxxx>
Cc: Gyu-Hwa Park <gyuhwa.park@xxxxxxxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx; Suk-Min Kang <sukmin.kang@xxxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] ASoC: sma1303: Add driver for Iron Device SMA1303 Amp
On Wed, Aug 17, 2022 at 12:29:37PM +0900, Kiseok Jo wrote:
I got part way through reviewing this driver. The top level feedback here is that the driver appears to be exposing everything in the device to userspace as enumeration controls rather than using standard features like DAPM, or appropriate other control types where there's something that's a better fit than an enum. If there's a strong reason for the driver to do something so unusual then theere should be something in here which explains what's going on.
# To solve the any issue by changing the value whenever there is a problem during use, we registered to control all functions.
# Since the verification was completed by mass production with the corresponding setting, we removed the controls in the userspace except for a few things as you mentioned.
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* sma1303.c -- sma1303 ALSA SoC Audio driver
> + *
> + * Copyright 2019 Iron Device Corporation
2019?
# I missed this point. Thank you!
# Changed 2019 to 2022.
Please make the entire block a C++ one so things look more intentional.
# I have modified the code a bit. I'm sorry, but I couldn't figure out exactly what you were talking about.
# If there is anything that needs to be corrected, please reply again.
> +static int power_up_down_control_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol) {
> + struct snd_soc_component *component =
> + snd_soc_kcontrol_component(kcontrol);
> + struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
> + int sel = (int)ucontrol->value.integer.value[0];
> +
> + if (sel && !(sma1303->force_amp_power_down))
> + sma1303_startup(component);
> + else
> + sma1303_shutdown(component);
> +
> + return 0;
> +}
This and all the other controls need to return 1 on change, please make sure a card with your driver your driver passes mixer-test cleanly - it will spot this and other problems.
# Added the return value for checking error.
# It's ok if the value is '0', and if a problem occurs, it returns to a value of less than 0.
# I have one question. All return values have to match the Linux standard error code?
However I wouldn't expect the power controls to exist at all, generally power is managed via DAPM rather than userspace.
# Removed the mixer control to control power from userspace.
# Automatically power control as DAC Event(DAPM), bias, set hw parameter and so on..
> +static int force_sdo_bypass_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol) {
> + struct snd_soc_component *component =
> + snd_soc_kcontrol_component(kcontrol);
> + struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
> + int sel = (int)ucontrol->value.integer.value[0];
> +
> + sma1303->sdo_bypass_flag = (bool)sel;
> +
> + return 0;
> +}
What is this bypassing?
# In chip, this is a function to set not to output on I2S.
# The output will always be on.
# So, We removed this.
> +static const char * const sma1303_input_format_text[] = {
> + "I2S", "LJ", "Reserved", "Reserved",
> + "RJ_16", "RJ_18", "RJ_20", "RJ_24"};
> +
> +static const struct soc_enum sma1303_input_format_enum =
> +SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(sma1303_input_format_text),
> + sma1303_input_format_text);
This should be controlled by the machine driver through set_dai_fmt() and hw_params().
# It's already included in that.
# This mixer control is an additional control for debugging.
# So, We removed this.
> +static const char * const sma1303_pcm_n_slot_text[] = {
> + "Slot_1", "Slot_2", "Slot_3", "Slot_4", "Slot_5", "Slot_6",
> + "Slot_7", "Slot_8", "Slot_9", "Slot_10", "Slot_11", "Slot_12",
> + "Slot_13", "Slot_14", "Slot_15", "Slot_16"};
> +
> +static const struct soc_enum sma1303_pcm_n_slot_enum =
> +SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(sma1303_pcm_n_slot_text),
> + sma1303_pcm_n_slot_text);
set_tdm_slot()
# It's already included in that.
# This mixer control is also an additional control for debugging.
# So, We removed this.
> +static const char * const sma1303_port_config_text[] = {
> + "IN_Port", "Reserved", "OUT_Port", "Reserved"};
This should be DT properties, possibly using the pinctrl schema depending on what's actually going on.
# I'm sorry. I think I named it wrong.
# This control is a feature that sets whether output is enabled or not. Output means SDO, that is, data exported from the chip.
# So I modified this.
> +static const char * const sma1303_port_out_sel_text[] = {
> + "Disable", "Format_C", "Mixer_out", "After_DSP",
> + "Postscaler", "Reserved", "Reserved", "Reserved"};
> +
> +static const struct soc_enum sma1303_port_out_sel_enum =
> + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(sma1303_port_out_sel_text),
> + sma1303_port_out_sel_text);
This looks like it should be a DAPM routing control.
# That's right. I added the control in route.
> +static const char * const sma1303_spk_off_slope_text[] = {
> + "00", "01", "10", "11"};
> +
> +static const struct soc_enum sma1303_spk_off_slope_enum =
> + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(sma1303_spk_off_slope_text),
> + sma1303_spk_off_slope_text);
Why make this an enum and not just a numerical control with values 0-3?
# Yes. I missed this value.
# This function has been removed as users do not need to control it
> +static const char * const sma1303_spkmode_text[] = {
> + "Off", "Mono", "Reserved", "Reserved",
> + "Stereo", "Reserved", "Reserved", "Reserved"};
> +
> +static const struct soc_enum sma1303_spkmode_enum =
> + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(sma1303_spkmode_text),
> + sma1303_spkmode_text);
Use a value enum to eliminate the reserved values from user selection, however if this is controlling if the device has mono or stereo speakers it should be a DT property since that's unlikely to change at runtime.
# This function also includes PWM on/off.
# We have changed the enum.
> +static const char * const sma1303_input_gain_text[] = {
> + "Gain_0dB", "Gain_M6dB", "Gain_M12dB", "Gain_MInf"};
> +
> +static const struct soc_enum sma1303_input_gain_enum =
> +SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(sma1303_input_gain_text),
> + sma1303_input_gain_text);
This should just be a normal volume control.
# We have other control for normal volume.
# So, this control has been removed.
I've stopped reviewing at this point.
# Thanks for the detailed feedback.
