Re: [PATCH 1/3] ASoC: codec: cpcap: new codec

[Mark Brown]

On Fri, Jul 07, 2017 at 06:42:27PM +0200, Sebastian Reichel wrote:

>  snd-soc-cq93vc-objs := cq93vc.o
> +snd-soc-cpcap-objs := cpcap.o

Please keep Kconfig and Makefile lexically sorted.

> +static int cpcap_audio_write(struct cpcap_audio *cpcap,
> +			     u16 reg, u16 mask, u16 val)
> +{
> +	struct snd_soc_codec *codec = cpcap->codec;
> +	struct device *dev = codec->dev;
> +	int err;
> +
> +	err = regmap_update_bits(cpcap->regmap, reg, mask, val);
> +	if (err)
> +		dev_err(dev, "write failed: reg=%04x mask=%04x val=%04x err=%d",
> +			reg, mask, val, err);
> +
> +	return err;
> +}

If we're going to have wrappers for the regmap functions it'd be good if
they were named in a similar way to the functions that they wrap, just
for clarity.  They're also not used consistently (and TBH I'm not sure
what they buy but if you want them that's fine).

> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		err += regmap_write(cpcap->regmap, CPCAP_REG_TEST,
> +				    STM_STDAC_EN_TEST_PRE);
> +		err += regmap_write(cpcap->regmap, CPCAP_REG_ST_TEST1,
> +				    STM_STDAC_EN_ST_TEST1_PRE);
> +		return err;

This'll return a nonsense error code if both error out, better to do
something like

	if (!err)
		err = regmap_write(...

> +static const char * const cpcap_onoff_texts[] = {
> +	"Off", "On"
> +};
> +static const SOC_ENUM_SINGLE_DECL(cpcap_ext_cap_l_enum,
> +	CPCAP_REG_TXI, CPCAP_BIT_RX_L_ENCODE, cpcap_onoff_texts);
> +static const SOC_ENUM_SINGLE_DECL(cpcap_ext_cap_r_enum,
> +	CPCAP_REG_TXI, CPCAP_BIT_RX_R_ENCODE, cpcap_onoff_texts);
> +static const struct snd_kcontrol_new cpcap_extr_cap_control =
> +	SOC_DAPM_ENUM("Ext Right Capture", cpcap_ext_cap_r_enum);
> +static const struct snd_kcontrol_new cpcap_extl_cap_control =
> +	SOC_DAPM_ENUM("Ext Left Capture", cpcap_ext_cap_l_enum);

Why are these enums and not simple Switch controls?  They appear to be
muxes with only one arm connected.

> +static int cpcap_hifi_set_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct cpcap_audio *cpcap = snd_soc_codec_get_drvdata(codec);
> +	static const u16 reg = CPCAP_REG_RXSDOA;
> +	static const u16 mask = BIT(CPCAP_BIT_ST_DAC_SW);
> +	u16 val = mute ? 0 : BIT(CPCAP_BIT_ST_DAC_SW);

Please write a normal if statement, this was a bit confusing at first
glance.

> +#ifdef CONFIG_OF
> +static const struct of_device_id cpcap_audio_of_match[] = {
> +	{ .compatible = "motorola,cpcap-audio-codec", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, cpcap_audio_of_match);
> +#endif

If this is part of a MFD shouldn't the parent device register it without
it needing to be in the DT?

Attachment: signature.asc
Description: PGP signature