Thank you. Turns out my mail client crapped out on me (I'm trying to switch to using mutt for replies and accidentally deleted the thread) so I'm afraid my nice and neat reply didn't go quite as expected. I'll work on the things you mention below and resubmit this as an RFC. I'll include the devicetree bindings that I did so you can see how things are mapped.
To answer your questions:
Acknowledged on the maintainers, the signed-off tag, the pointer, and the commit message.
I don't know for sure the best way to accomplish things, I read in the documentation that the simple-audio-card has an option of setting a GPIO which should automatically register with the jack for headphone or microphone sources, that's what I'm trying to do. The specific chip I'm working with has 2 headphone ports (R and L), a single speaker port (let's say LR combined to L), and a single microphone port (L). The codec itself supports 2 channels for the DAC which either power the headphones or the speaker (or both at the same time, if you don't mind either only sending L to the speaker with a proper RL split on the headphones or RL combined to the L headphone with a proper RL combined to the speaker). There are also 2 channels on the ADC, but again only one microphone port which seems odd. Register documentation here for the codec: https://rockchip.fr/RK817%20datasheet%20V1.01.pdf As for the specific implementation I'm working with its here: https://dn.odroid.com/ODROID_GO_ADVANCE/ODROID_GO_ADVANCE_rev1.1.pdf
Here's my simple-audio-card DT node:
rk817-sound { compatible = "simple-audio-card"; simple-audio-card,format = "i2s"; simple-audio-card,name = "rockchip,rk817-codec"; simple-audio-card,mclk-fs = <256>; simple-audio-card,widgets = "Microphone", "Mic Jack", "Headphone", "Headphones", simple-audio-card,routing = "MIC", "Mic Jack", "Headphones", "HPOL", "Headphones", "HPOR", simple-audio-card,hp-det-gpio = <&gpio2 RK_PC6 GPIO_ACTIVE_LOW>; simple-audio-card,cpu { sound-dai = <&i2s1_2ch>; }; simple-audio-card,codec { sound-dai = <&rk817_codec>; }; };
I copied the rockchip code from the BSP kernel and it had some of the variables, such as the pre_path, as a long. Since it looks like it's just checking the path enums to see if there was a change, I imagine int will do just fine on the pre_path. Speaking of which, that is pre as opposed to post. In this specific step it's looking to see if we're going from off to on or on to off and setting the register values accordingly.
As for whether or not the steps are trying to bypass or reimplement something that should be done in DAPM, I'm not sure honestly; this is my first audio driver (or any driver for that matter). I tried to read the DAPM documentation and didn't understand much of it. What I can say is that the code in question is setting the register values via I2C depending upon which audio path is chosen. For example on the SPK_PATH it sets the register at address 0x38 (RK817_CODEC_DDAC_MUTE_MIXCTL) to 0x10, which tells it to switch the amplifier on and combine the channels into a single channel. The HP path likewise does the opposite, setting this same register to 0x00 which turns off the amplifier/splits it back into L+R channels. Curiously enough, this is actually a bug I found in Rockchip's BSP kernel where the channels don't get split apart again when jumping from external speaker back to headphones and your headphones start to output R to R and RL to L...
In regards to the DT bindings, I will document all of them, however that poses a dilemma. Most of the bindings I'm unable to test, as aside from mic-in-differential I'm not really using any of them on the single hardware implementation I have available to me. Should I document them based on Rockchip's BSP kernel and note that they are untested, or should I simply delete them and their associated functionality (and focus on that which I can test)?
I'll test removing the unregister_component code and see if that does anything unexpected.
Thank you again for all of your help.
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