On Wed, Mar 10, 2021 at 05:44:16PM -0600, Chris Morgan wrote:
At a very high level this doesn't really look like it integrates with the framework as much as it should - the way this driver is written doesn't really have much resemblance to how other ASoC drivers are written beyond a very superficial level. Please take a look at what other drivers are doing and try to ensure that this is working with the framework in an idiomatic way.
index 000000000000..6d4e05440dae --- /dev/null +++ b/Documentation/devicetree/bindings/sound/rockchip,rk817-codec.txt @@ -0,0 +1,39 @@ +* Rockchip rk817 codec
New DT bindings should be in YAML format.
diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c index ad923dd4e007..adb8a7da29db 100644 --- a/drivers/mfd/rk808.c +++ b/drivers/mfd/rk808.c @@ -163,6 +163,12 @@ static const struct mfd_cell rk817s[] = { .num_resources = ARRAY_SIZE(rk817_rtc_resources), .resources = &rk817_rtc_resources[0], }, +#ifdef CONFIG_SND_SOC_RK817
- {
.name = "rk817-codec",.of_compatible = "rockchip,rk817-codec",- },
+#endif };
This should be a separate patch and should be sent via the MFD maintainers. You shouldn't add an of_compatible here, obviously ever RK817 has the CODEC functionality so this is just describing how Linux chooses to split the device up rather than any property of the hardware.
+static int rk817_reset(struct snd_soc_component *component) +{
- snd_soc_component_write(component, RK817_CODEC_DTOP_LPT_SRST, 0x40);
- snd_soc_component_write(component, RK817_CODEC_DDAC_POPD_DACST, 0x02);
- snd_soc_component_write(component, RK817_CODEC_DTOP_DIGEN_CLKE, 0x0f);
- snd_soc_component_write(component, RK817_CODEC_APLL_CFG0, 0x04);
- snd_soc_component_write(component, RK817_CODEC_APLL_CFG1, 0x58);
- snd_soc_component_write(component, RK817_CODEC_APLL_CFG2, 0x2d);
- snd_soc_component_write(component, RK817_CODEC_APLL_CFG3, 0x0c);
- snd_soc_component_write(component, RK817_CODEC_APLL_CFG4, 0xa5);
- snd_soc_component_write(component, RK817_CODEC_APLL_CFG5, 0x00);
- snd_soc_component_write(component, RK817_CODEC_DTOP_DIGEN_CLKE, 0x00);
This appears to be configuring the CODEC in ways that I'd expect to be configured via internal APIs and/or controls, especially the PLL setup there looks like it's probably platform specific.
+static struct rk817_reg_val_typ playback_power_up_list[] = {
- {RK817_CODEC_AREF_RTCFG1, 0x40},
- {RK817_CODEC_DDAC_POPD_DACST, 0x02},
- {RK817_CODEC_DDAC_SR_LMT0, 0x02},
Similarly this looks like it's a hard coded register write sequence for a specific platform.
+/* For tiny alsa playback/capture path*/ +static const char * const rk817_playback_path_mode[] = {
- "OFF", "SPK", "HP", "SPK_HP"};
+static const char * const rk817_capture_path_mode[] = {
- "MIC OFF", "MIC"};
The routing within the device should be mapped using DAPM rather than with some custom control with hard coded paths and sequences.
+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),
+};
It's weird that there's a regmap configuration here rather than the regmap being shared with the rest of the MFD.