[PATCH] Add audio driver for rk817 upstream

[Pierre-Louis Bossart]

On 3/9/21 5:41 PM, Chris Morgan wrote:
> I'm wondering if you all can help me.  I'm trying to get the rk817
> codec driver working from Rockchip's BSP kernel sources (GPL per
> the license) and I'm struggling with a few parts. The first part

is to Cc: audio maintainers (Takashi Iwai and Mark Brown).

Then you need to make sure there is a Signed-off-by tag from the 
original contribution this code is based on, along with yours. It 
doesn't hurt to have a pointer to that code either in the commit message.

> is I'm not sure if I have my audio paths set up correctly. For example
> the sinks I have set up are for HPOL and HPOR, and the source is for
> MIC. While this does work (audio output seems fine) I'm having issues
> with the GPIO to detect headphone insertion. When I insert headphones
> I expect the audio to output to the headphones, and when I remove
> headphones I expect the audio to output to a speaker. Right now I
> have to manually change the output between the different paths.

You don't necessarily have to do everything at the kernel level, it's 
not uncommon to have the driver set a kcontrol for jack detection, and 
let userspace change settings on a jack detection event. PulseAudio 
relies on UCM JackControl to switch to Headphones and Headset devices.

> Additionally, while the codec "technically" has dual channel inputs
> for the microphone, I'm only using the L channel. Should I have a
> stereo mixer? Note that I'm using the simple-audio-card to set my
> paths, widgets, and the gpio pin. They all work, just not seamlessly
> together.
> 
> Basically, I'm wanting to know if and how I should set up my audio
> paths for the speaker and headphones, they use mostly the same pins
> and only really differ in setting the external amp settings.
> Additionally, once I set up my paths how do I ensure that the GPIO
> events will result in switching between the speaker/headphone path?
> 
> Any help you can provide is appreciated. This is my first attempt at
> something of this magnitude (for me it's a big step), even if it's
> just trying to facilitate in getting a vendor's code ready for upstream.

> --- /dev/null
> +++ b/sound/soc/codecs/rk817_codec.c
> @@ -0,0 +1,1148 @@
> +/*
> + * Copyright (c) 2018 Rockchip Electronics Co. Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

remove and replace by an SPDX line.

> +static int rk817_playback_path_put(struct snd_kcontrol *kcontrol,
> +				   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct rk817_codec_priv *rk817 = snd_soc_component_get_drvdata(component);
> +	long int pre_path;

it's uncommon to see this, use either int or u32/u64 if your require a 
specific size.

> +
> +	if (rk817->playback_path == ucontrol->value.integer.value[0]) {
> +		return 0;
> +	}
> +
> +	pre_path = rk817->playback_path;

'pre' as in 'previous' or 'preamp' or as opposed to 'post'?

> +	rk817->playback_path = ucontrol->value.integer.value[0];
> +
> +	if (rk817->playback_path != OFF)
> +		clk_prepare_enable(rk817->mclk);
> +	else
> +		clk_disable_unprepare(rk817->mclk);
> +
> +	switch (rk817->playback_path) {
> +	case OFF:
> +		if (pre_path != OFF && pre_path != HP_PATH) {
> +			rk817_codec_power_down(component, RK817_CODEC_PLAYBACK);
> +			if (rk817->capture_path == 0)
> +				rk817_codec_power_down(component, RK817_CODEC_ALL);
> +		}
> +		break;
> +	case SPK_PATH:
> +		if (pre_path == OFF)
> +			rk817_codec_power_up(component, RK817_CODEC_PLAYBACK);
> +		if (!rk817->use_ext_amplifier) {
> +			/* power on dac ibias/l/r */
> +			snd_soc_component_write(component, RK817_CODEC_ADAC_CFG1,
> +						PWD_DACBIAS_ON | PWD_DACD_ON |
> +						PWD_DACL_DOWN | PWD_DACR_DOWN);
> +			/* CLASS D mode, combine LR channels */
> +			snd_soc_component_write(component,
> +						RK817_CODEC_DDAC_MUTE_MIXCTL,
> +						0x10);
> +			/* CLASS D enable */
> +			snd_soc_component_write(component,
> +						RK817_CODEC_ACLASSD_CFG1,
> +						0xa5);
> +			/* restart CLASS D, OCPP/N */
> +			snd_soc_component_write(component,
> +						RK817_CODEC_ACLASSD_CFG2,
> +						0xf7);
> +		} else {
> +			/* HP_CP_EN , CP 2.3V */
> +			snd_soc_component_write(component, RK817_CODEC_AHP_CP,
> +						0x11);
> +			/* power on HP two stage opamp ,HP amplitude 0db */
> +			snd_soc_component_write(component, RK817_CODEC_AHP_CFG0,
> +						0x80);
> +			/* power on dac ibias/l/r */
> +			snd_soc_component_write(component, RK817_CODEC_ADAC_CFG1,
> +						PWD_DACBIAS_ON | PWD_DACD_DOWN |
> +						PWD_DACL_ON | PWD_DACR_ON);
> +			snd_soc_component_update_bits(component,
> +						      RK817_CODEC_DDAC_MUTE_MIXCTL,
> +						      DACMT_ENABLE, DACMT_DISABLE);
> +		}
> +		snd_soc_component_write(component, RK817_CODEC_DDAC_VOLL,
> +					rk817->spk_volume);
> +		snd_soc_component_write(component, RK817_CODEC_DDAC_VOLR,
> +					rk817->spk_volume);
> +		break;
> +	case HP_PATH:
> +		if (pre_path == OFF)
> +			rk817_codec_power_up(component, RK817_CODEC_PLAYBACK);
> +		/* HP_CP_EN , CP 2.3V */
> +		snd_soc_component_write(component, RK817_CODEC_AHP_CP, 0x11);
> +		/* power on HP two stage opamp ,HP amplitude 0db */
> +		snd_soc_component_write(component, RK817_CODEC_AHP_CFG0, 0x80);
> +		/* power on dac ibias/l/r */
> +		snd_soc_component_write(component, RK817_CODEC_ADAC_CFG1,
> +					PWD_DACBIAS_ON | PWD_DACD_DOWN |
> +					PWD_DACL_ON | PWD_DACR_ON);
> +		/* CLASS D mode disable, split LR channels */
> +		snd_soc_component_write(component,
> +					RK817_CODEC_DDAC_MUTE_MIXCTL,
> +					0x00);
> +
> +		snd_soc_component_write(component, RK817_CODEC_DDAC_VOLL,
> +					rk817->hp_volume);
> +		snd_soc_component_write(component, RK817_CODEC_DDAC_VOLR,
> +					rk817->hp_volume);
> +		break;
> +	case SPK_HP:
> +		if (pre_path == OFF)
> +			rk817_codec_power_up(component, RK817_CODEC_PLAYBACK);
> +
> +		/* HP_CP_EN , CP 2.3V  */
> +		snd_soc_component_write(component, RK817_CODEC_AHP_CP, 0x11);
> +		/* power on HP two stage opamp ,HP amplitude 0db */
> +		snd_soc_component_write(component, RK817_CODEC_AHP_CFG0, 0x80);
> +
> +		/* power on dac ibias/l/r */
> +		snd_soc_component_write(component, RK817_CODEC_ADAC_CFG1,
> +					PWD_DACBIAS_ON | PWD_DACD_ON |
> +					PWD_DACL_ON | PWD_DACR_ON);
> +
> +		if (!rk817->use_ext_amplifier) {
> +			/* CLASS D mode, combine LR channels */
> +			snd_soc_component_write(component,
> +						RK817_CODEC_DDAC_MUTE_MIXCTL,
> +						0x10);
> +			/* CLASS D enable */
> +			snd_soc_component_write(component,
> +						RK817_CODEC_ACLASSD_CFG1,
> +						0xa5);
> +			/* restart CLASS D, OCPP/N */
> +			snd_soc_component_write(component,
> +						RK817_CODEC_ACLASSD_CFG2,
> +						0xf7);
> +		}
> +
> +		snd_soc_component_write(component, RK817_CODEC_DDAC_VOLL,
> +					rk817->hp_volume);
> +		snd_soc_component_write(component, RK817_CODEC_DDAC_VOLR,
> +					rk817->hp_volume);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rk817_capture_path_get(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct rk817_codec_priv *rk817 = snd_soc_component_get_drvdata(component);
> +
> +	dev_dbg(component->dev, "%s:capture_path %ld\n", __func__, rk817->capture_path);
> +	ucontrol->value.integer.value[0] = rk817->capture_path;
> +	return 0;
> +}
> +
> +static int rk817_capture_path_put(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct rk817_codec_priv *rk817 = snd_soc_component_get_drvdata(component);
> +	long int pre_path;
> +
> +	if (rk817->capture_path == ucontrol->value.integer.value[0]) {
> +		dev_dbg(component->dev, "%s:capture_path is not changed!\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	pre_path = rk817->capture_path;
> +	rk817->capture_path = ucontrol->value.integer.value[0];
> +
> +	if (rk817->capture_path != MIC_OFF)
> +		clk_prepare_enable(rk817->mclk);
> +	else
> +		clk_disable_unprepare(rk817->mclk);
> +
> +	switch (rk817->capture_path) {
> +	case MIC_OFF:
> +		if (pre_path != MIC_OFF)
> +			rk817_codec_power_down(component, RK817_CODEC_CAPTURE);
> +		break;
> +	case MIC:
> +		if (pre_path == MIC_OFF)
> +			rk817_codec_power_up(component, RK817_CODEC_CAPTURE);

these sequences look like trying to bypass DAPM or re-invent it with 
custom state machines.

> +
> +		if (rk817->adc_for_loopback) {
> +			/* don't need to gain when adc use for loopback */
> +			snd_soc_component_update_bits(component,
> +						      RK817_CODEC_AMIC_CFG0,
> +						      0xf,
> +						      0x0);
> +			snd_soc_component_write(component,
> +						RK817_CODEC_DMIC_PGA_GAIN,
> +						0x66);
> +			snd_soc_component_write(component,
> +						RK817_CODEC_DADC_VOLL,
> +						0x00);
> +			snd_soc_component_write(component,
> +						RK817_CODEC_DADC_VOLR,
> +						0x00);
> +			break;
> +		}
> +		if (!rk817->mic_in_differential) {
> +			snd_soc_component_write(component,
> +						RK817_CODEC_DADC_VOLR,
> +						0xff);
> +			snd_soc_component_update_bits(component,
> +						      RK817_CODEC_AADC_CFG0,
> +						      ADC_R_PWD_MASK,
> +						      ADC_R_PWD_EN);
> +			snd_soc_component_update_bits(component,
> +						      RK817_CODEC_AMIC_CFG0,
> +						      PWD_PGA_R_MASK,
> +						      PWD_PGA_R_EN);
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
[...]

> +static int rk817_codec_parse_dt_property(struct device *dev,
> +					 struct rk817_codec_priv *rk817)
> +{
> +	struct device_node *node = dev->parent->of_node;
> +	int ret;
> +
> +	if (!node) {
> +		dev_err(dev, "%s() dev->parent->of_node is NULL\n",
> +			__func__);
> +		return -ENODEV;
> +	}
> +
> +	node = of_get_child_by_name(dev->parent->of_node, "codec");
> +	if (!node) {
> +		dev_err(dev, "%s() Can not get child: codec\n",
> +			__func__);
> +		return -ENODEV;
> +	}
> +
> +	rk817->hp_ctl_gpio = devm_gpiod_get_optional(dev, "hp-ctl",
> +						  GPIOD_OUT_LOW);
> +
> +	rk817->spk_ctl_gpio = devm_gpiod_get_optional(dev, "spk-ctl",
> +						  GPIOD_OUT_LOW);
> +
> +	ret = of_property_read_u32(node, "spk-mute-delay-ms",
> +				   &rk817->spk_mute_delay);
> +	if (ret < 0) {
> +		rk817->spk_mute_delay = 0;
> +	}
> +
> +	ret = of_property_read_u32(node, "hp-mute-delay-ms",
> +				   &rk817->hp_mute_delay);
> +	if (ret < 0) {
> +		rk817->hp_mute_delay = 0;
> +	}
> +
> +	ret = of_property_read_u32(node, "spk-volume", &rk817->spk_volume);
> +	if (ret < 0) {
> +		rk817->spk_volume = OUT_VOLUME;
> +	}
> +	if (rk817->spk_volume < 3)
> +		rk817->spk_volume = 3;
> +
> +	ret = of_property_read_u32(node, "hp-volume",
> +				   &rk817->hp_volume);
> +	if (ret < 0) {
> +		rk817->hp_volume = OUT_VOLUME;
> +	}
> +	if (rk817->hp_volume < 3)
> +		rk817->hp_volume = 3;
> +
> +	ret = of_property_read_u32(node, "capture-volume",
> +				   &rk817->capture_volume);
> +	if (ret < 0) {
> +		rk817->capture_volume = CAPTURE_VOLUME;
> +	}
> +
> +	rk817->mic_in_differential =
> +			of_property_read_bool(node, "mic-in-differential");
> +
> +	rk817->pdmdata_out_enable =
> +			of_property_read_bool(node, "pdmdata-out-enable");
> +
> +	rk817->use_ext_amplifier =
> +			of_property_read_bool(node, "use-ext-amplifier");
> +
> +	rk817->adc_for_loopback =
> +			of_property_read_bool(node, "adc-for-loopback");

you will need DT bindings for all these properties.

> +
> +	return 0;
> +}
> +
> +static const struct regmap_config rk817_codec_regmap_config = {
> +	.name = "rk817-codec",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.reg_stride = 1,
> +	.max_register = 0x4f,
> +	.cache_type = REGCACHE_NONE,
> +	.volatile_reg = rk817_volatile_register,
> +	.writeable_reg = rk817_codec_register,
> +	.readable_reg = rk817_codec_register,
> +	.reg_defaults = rk817_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(rk817_reg_defaults),
> +};
> +
> +static int rk817_platform_probe(struct platform_device *pdev)
> +{
> +	struct rk808 *rk817 = dev_get_drvdata(pdev->dev.parent);
> +	struct rk817_codec_priv *rk817_codec_data;
> +	int ret;
> +
> +	if (!rk817) {
> +		dev_err(&pdev->dev, "%s : rk817 is NULL\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	rk817_codec_data = devm_kzalloc(&pdev->dev,
> +					sizeof(struct rk817_codec_priv),
> +					GFP_KERNEL);
> +	if (!rk817_codec_data)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, rk817_codec_data);
> +
> +	ret = rk817_codec_parse_dt_property(&pdev->dev, rk817_codec_data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "%s() parse device tree property error %d\n",
> +			__func__, ret);
> +		goto err_;
> +	}
> +
> +	rk817_codec_data->regmap = devm_regmap_init_i2c(rk817->i2c,
> +					    &rk817_codec_regmap_config);
> +	if (IS_ERR(rk817_codec_data->regmap)) {
> +		ret = PTR_ERR(rk817_codec_data->regmap);
> +		dev_err(&pdev->dev, "failed to allocate register map: %d\n",
> +			ret);
> +		goto err_;
> +	}
> +
> +	rk817_codec_data->mclk = devm_clk_get(&pdev->dev, "mclk");
> +	if (IS_ERR(rk817_codec_data->mclk)) {
> +		dev_err(&pdev->dev, "Unable to get mclk\n");
> +		ret = -ENXIO;
> +		goto err_;
> +	}
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev, &soc_codec_dev_rk817,
> +					      rk817_dai, ARRAY_SIZE(rk817_dai));
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "%s() register codec error %d\n",
> +			__func__, ret);
> +		goto err_;
> +	}
> +
> +	return 0;
> +err_:
> +
> +	return ret;
> +}
> +
> +static int rk817_platform_remove(struct platform_device *pdev)
> +{
> +	snd_soc_unregister_component(&pdev->dev);

humm, that looks like a bug. If you used devm_soc_register_component() 
in the probe, you don't need to release it here manually?


> +++ b/sound/soc/codecs/rk817_codec.h
> @@ -0,0 +1,197 @@
> +/*
> + * Copyright (c) 2018 Rockchip Electronics Co. Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

remove and use SPDX