[RFCv2] ASoC: Add Rockchip rk817 audio CODEC support

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20210311/7cb9b72a/attachment-0001.sig>