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.
Attachment:
signature.asc
Description: PGP signature
