On Mon, Dec 13, 2021 at 04:59:12PM +0100, Vincent Knecht wrote:
AK4375 is a 32-bit stereo DAC with headphones amplifier. There's no documentation for it on akm.com, and only a brief datasheet can be found floating on the internets [1].
This driver looks relatively clean but it's in *serious* need of modernisation, there's issues here that haven't been present upstream for a very long time. Fortunately they're mostly style things so it should be relatively easy to handle.
- struct ak4375_priv *ak4375 = snd_soc_component_get_drvdata(component);
- int value = gpiod_get_value_cansleep(ak4375->pdn_gpiod);
- if (value < 0)
dev_err(ak4375->dev, "unable to get pdn gpiod: %d\n", value);
You should cache the value set on the GPIO, there's no guarantee that reads are supported when in output mode or that the value will correspond to the value present on the pin.
+static const struct soc_enum ak4375_dac_enum[] = {
- SOC_ENUM_SINGLE(AK4375_0B_LCH_OUTPUT_VOLUME, 7,
ARRAY_SIZE(ak4375_ovolcn_select_texts), ak4375_ovolcn_select_texts),
Magic indexes into an array of enums are error prone and unreasonably hard to read. Declare individual variables for each enum like other CODEC drivers do.
+static const char * const pdn_on_select[] = { "OFF", "ON" };
+static const struct soc_enum ak4375_pdn_enum =
- SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(pdn_on_select), pdn_on_select);
Given that the driver is actively managing this GPIO at runtime with no coordination with this control this just shouldn't be exposed to userspace at all, I can't see how userspace can use this control without breaking the operation of the driver and any time the driver code kicks in it will just overwrite the current state.
+static const struct snd_kcontrol_new ak4375_snd_controls[] = {
- SOC_SINGLE_TLV("AK4375 Digital Output VolumeL",
AK4375_0B_LCH_OUTPUT_VOLUME, 0, 0x1f, 0, ovl_tlv),- SOC_SINGLE_TLV("AK4375 Digital Output VolumeR",
AK4375_0C_RCH_OUTPUT_VOLUME, 0, 0x1f, 0, ovr_tlv),
These should be a standard stereo control - SOC_DOUBLE_R_TLV.
- SOC_SINGLE_TLV("AK4375 HP-Amp Analog Volume",
AK4375_0D_HP_VOLUME_CONTROL, 0, 0x1f, 0, hpg_tlv),
As with other CODEC drivers there is no need to put the name of the CODEC in every control name.
- SOC_ENUM("AK4375 HPL Power-down Resistor", ak4375_dac_enum[6]),
- SOC_ENUM("AK4375 HPR Power-down Resistor", ak4375_dac_enum[7]),
These don't sound like things that would vary at runtime, should this be controlled via DT?
+static const struct snd_kcontrol_new ak4375_hpl_mixer_controls[] = {
- SOC_DAPM_SINGLE("LDACL", AK4375_07_DAC_MONO_MIXING, 0, 1, 0),
- SOC_DAPM_SINGLE("RDACL", AK4375_07_DAC_MONO_MIXING, 1, 1, 0),
+};
The names of on/off switches should end with Switch - see control-names.rst.
+static int ak4375_set_pllblock(struct snd_soc_component *component, int fs) +{
- struct ak4375_priv *ak4375 = snd_soc_component_get_drvdata(component);
- int mclk, pllclk, refclk, pldbit, plmbit, mdivbit, divbit;
- u8 mode;
This should be being exposed as a set_pll() operation, or the input to the PLL configured using set_sysclk(). The input clock to the PLL is going to be system dependent.
+static const unsigned int ak4375_rates[] = {
- 8000, 11025, 16000, 22050,
- 32000, 44100, 48000, 88200,
- 96000, 176400, 192000,
+};
+static const struct snd_pcm_hw_constraint_list ak4375_rate_constraints = {
- .count = ARRAY_SIZE(ak4375_rates),
- .list = ak4375_rates,
+};
+static int ak4375_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- return snd_pcm_hw_constraint_list(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_RATE,&ak4375_rate_constraints);+}
These are all standard rates, just set the supported rates in the DAI description rather than using _KNOT.
+static void ak4375_power_off(struct ak4375_priv *ak4375) +{
- gpiod_set_value_cansleep(ak4375->pdn_gpiod, 0);
- usleep_range(1000, 2000);
- if (!IS_ERR(ak4375->avdd_supply))
regulator_disable(ak4375->avdd_supply);
Unless the device is capable of operating without AVDD which doesn't seem entirely likely use of AVDD should be unconditional. If that were the case I'd expect to see some configuration of the device for operation without the supply which I don't see.
Probably also worth using the _bulk APIs unless the ordering is important here.
- regulator_disable(ak4375->tvdd_supply);
- usleep_range(3000, 4000);
What is this sleep doing?
- ak4375->pdn_gpiod = devm_gpiod_get_optional(ak4375->dev, "pdn", GPIOD_OUT_LOW);
- if (IS_ERR(ak4375->pdn_gpiod))
return PTR_ERR(ak4375->pdn_gpiod);- ret = ak4375_power_on(ak4375);
- if (ret < 0)
return ret;
We never unwind this power on - why not just remove the component level probe/remove?
- ret = devm_snd_soc_register_component(ak4375->dev, drvdata->comp_drv,
drvdata->dai_drv, 1);- if (ret < 0) {
dev_err(ak4375->dev, "Failed to register CODEC: %d\n", ret);return ret;- }
- pm_runtime_enable(&i2c->dev);
Enable runtime PM before probing the component, a card may be instantiated during component probe and userpace could start using the card.