Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1684552

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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?

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/3] Motorola Droid 4 Audio Support Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-07 18:50 +0200
  [PATCH 3/3] ARM: dts: omap4-droid4: add soundcard Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-07 18:50 +0200
    Re: [PATCH 3/3] ARM: dts: omap4-droid4: add soundcard Mark Brown <broonie@kernel.org> - 2017-07-10 14:30 +0200
  Re: [PATCH 0/3] Motorola Droid 4 Audio Support Tony Lindgren <tony@atomide.com> - 2017-07-08 07:30 +0200
    Re: [PATCH 0/3] Motorola Droid 4 Audio Support Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-08 13:40 +0200
      Re: [PATCH 0/3] Motorola Droid 4 Audio Support Tony Lindgren <tony@atomide.com> - 2017-07-09 07:40 +0200
  Re: [PATCH 1/3] ASoC: codec: cpcap: new codec Tony Lindgren <tony@atomide.com> - 2017-07-09 07:40 +0200
  Re: [PATCH 1/3] ASoC: codec: cpcap: new codec Rob Herring <robh@kernel.org> - 2017-07-10 17:30 +0200
  Re: [PATCH 1/3] ASoC: codec: cpcap: new codec Mark Brown <broonie@kernel.org> - 2017-07-10 20:00 +0200
    Re: [PATCH 1/3] ASoC: codec: cpcap: new codec Tony Lindgren <tony@atomide.com> - 2017-07-11 07:20 +0200
      Re: [PATCH 1/3] ASoC: codec: cpcap: new codec Mark Brown <broonie@kernel.org> - 2017-07-11 12:50 +0200
        Re: [PATCH 1/3] ASoC: codec: cpcap: new codec Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-11 14:20 +0200
          Re: [PATCH 1/3] ASoC: codec: cpcap: new codec Mark Brown <broonie@kernel.org> - 2017-07-11 15:40 +0200

csiph-web