All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Ryan Lee <ryans.lee@maximintegrated.com>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org, mark.rutland@arm.com,
	perex@perex.cz, tiwai@suse.com, kuninori.morimoto.gx@renesas.com,
	arnd@arndb.de, ckeepax@opensource.wolfsonmicro.com,
	lars@metafoo.de, bardliao@realtek.com, nh6z@nh6z.net,
	KCHSU0@nuvoton.com, axel.lin@ingics.com,
	romain.perier@collabora.com, srinivas.kandagatla@linaro.org,
	oder_chiou@realtek.com, Paul.Handrigan@cirrus.com,
	alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, dgreid@google.com,
	ryan.lee.maxim@gmail.com
Subject: Re: [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier
Date: Wed, 29 Mar 2017 15:23:04 +0100	[thread overview]
Message-ID: <20170329142304.qovnyqd6izackny2@sirena.org.uk> (raw)
In-Reply-To: <1490748828-31701-1-git-send-email-ryans.lee@maximintegrated.com>

[-- Attachment #1: Type: text/plain, Size: 3776 bytes --]

On Wed, Mar 29, 2017 at 09:53:48AM +0900, Ryan Lee wrote:

> +	case SND_SOC_DAIFMT_CBS_CFM:
> +		mode = MAX98927_PCM_MASTER_MODE_HYBRID;
> +	default:
> +		dev_err(codec->dev, "DAI clock mode unsupported");

Either we don't support _CBS_CFM or there's a missing break there.

> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		max98927->iface |= SND_SOC_DAIFMT_I2S;
> +
> +	break;

Please use the kernel coding style as you do in the rest of this
function.

> +	int reg = MAX98927_R0022_PCM_CLK_SETUP;
> +	int mask = MAX98927_PCM_CLK_SETUP_BSEL_MASK;

> +	regmap_update_bits(max98927->regmap, reg, mask, value);

reg and mask have exactly one user (as you'd expect), just use the
constants directly here to make things clearer.

> +	switch (snd_pcm_format_width(params_format(params))) {
> +	case 16:
> +		chan_sz = MAX98927_PCM_MODE_CFG_CHANSZ_16;
> +		max98927->ch_size = 16;

You could just assign ch_size directly.

> +	/* sampling rate configuration */
> +	switch (params_rate(params)) {
> +	case 8000:
> +		sampling_rate |= MAX98927_PCM_SR_SET1_SR_8000;

sampling_rate is only ever set to a real value in this switch statement,
you're not oring with anything else so you can just use an assignment
which would be a lot clearer (it's not obvious what else might go in
there) and the initial assignment to 0 for this and chan_sz can only
mask errors.

> +	/* set sampling rate of IV */
> +	if (max98927->interleave_mode &&
> +		sampling_rate > MAX98927_PCM_SR_SET1_SR_16000)
> +		regmap_update_bits(max98927->regmap,

Please use the kernel coding style, indent the second line of the if
with the ( so that it doesn't look like part of the conditional code.

> +	switch (reg) {
> +	case MAX98927_R0001_INT_RAW1 ... MAX98927_R0028_ICC_RX_EN_B:


> +		return true;
> +	}
> +	return false;

It'd be clearer to put the return false in the switch statement like the
return true, and also consistent with the _volatile_reg() function.

> +static const char * const max98927_speaker_source_text[] = {
> +	"i2s", "reserved", "tone", "pdm"
> +};

This looks like it should be a DAPM control.

> +static const char * const max98927_monomix_output_text[] = {
> +	"ch_0", "ch_1", "ch_1_2_div"
> +};

Similarly here.

> +	SOC_SINGLE_TLV("Digital Gain", MAX98927_R0036_AMP_VOL_CTRL,
> +		0, (1<<MAX98927_AMP_VOL_WIDTH)-1, 0,
> +		max98927_digital_tlv),

All volume controls should end with Volume as per control-names.rst.

> +	SOC_SINGLE("Amp DSP Enable", MAX98927_R0052_BROWNOUT_EN,
> +		MAX98927_BROWNOUT_DSP_SHIFT, 1, 0),

All on/off switches should end with Switch as per control-names.rst.

> +static int max98927_probe(struct snd_soc_codec *codec)
> +{

> +       /* Check Revision ID */
> +       ret = regmap_read(max98927->regmap,
> +               MAX98927_R01FF_REV_ID, &reg);

Basic device identification and setup should be done at the chip level
probe not at the CODEC level so that if there are problems we fail as
early as possible and so that diagnostic information is available to
users as soon as possible, even if there's no sound card for the device
in the system.

> +	/* Set inital volume (+13dB) */

As with all other CODEC drivers you should leave the hardware defaults
alone, what makes sense for your system may not make sense for other
systems and the hardware defaults are a fixed thing.

> +	/* Boost Output Voltage & Current limit */
> +	regmap_write(max98927->regmap,
> +		MAX98927_R0040_BOOST_CTRL0,
> +		0x1C);
> +	regmap_write(max98927->regmap,
> +		MAX98927_R0042_BOOST_CTRL1,
> +		0x3E);

This should be system specific, these values might be unsafe in some
systems.

> +err:
> +	if (max98927)
> +		devm_kfree(&i2c->dev, max98927);

There is no need to explicitly free devm_ allocated memory, that's the
point of devm.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

WARNING: multiple messages have this Message-ID
From: Mark Brown <broonie@kernel.org>
To: Ryan Lee <ryans.lee@maximintegrated.com>
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
	kuninori.morimoto.gx@renesas.com, lgirdwood@gmail.com,
	tiwai@suse.com, srinivas.kandagatla@linaro.org,
	romain.perier@collabora.com, bardliao@realtek.com,
	lars@metafoo.de, axel.lin@ingics.com, Paul.Handrigan@cirrus.com,
	devicetree@vger.kernel.org, arnd@arndb.de, nh6z@nh6z.net,
	robh+dt@kernel.org, ckeepax@opensource.wolfsonmicro.com,
	dgreid@google.com, oder_chiou@realtek.com,
	ryan.lee.maxim@gmail.com, KCHSU0@nuvoton.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier
Date: Wed, 29 Mar 2017 15:23:04 +0100	[thread overview]
Message-ID: <20170329142304.qovnyqd6izackny2@sirena.org.uk> (raw)
In-Reply-To: <1490748828-31701-1-git-send-email-ryans.lee@maximintegrated.com>


[-- Attachment #1.1: Type: text/plain, Size: 3776 bytes --]

On Wed, Mar 29, 2017 at 09:53:48AM +0900, Ryan Lee wrote:

> +	case SND_SOC_DAIFMT_CBS_CFM:
> +		mode = MAX98927_PCM_MASTER_MODE_HYBRID;
> +	default:
> +		dev_err(codec->dev, "DAI clock mode unsupported");

Either we don't support _CBS_CFM or there's a missing break there.

> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		max98927->iface |= SND_SOC_DAIFMT_I2S;
> +
> +	break;

Please use the kernel coding style as you do in the rest of this
function.

> +	int reg = MAX98927_R0022_PCM_CLK_SETUP;
> +	int mask = MAX98927_PCM_CLK_SETUP_BSEL_MASK;

> +	regmap_update_bits(max98927->regmap, reg, mask, value);

reg and mask have exactly one user (as you'd expect), just use the
constants directly here to make things clearer.

> +	switch (snd_pcm_format_width(params_format(params))) {
> +	case 16:
> +		chan_sz = MAX98927_PCM_MODE_CFG_CHANSZ_16;
> +		max98927->ch_size = 16;

You could just assign ch_size directly.

> +	/* sampling rate configuration */
> +	switch (params_rate(params)) {
> +	case 8000:
> +		sampling_rate |= MAX98927_PCM_SR_SET1_SR_8000;

sampling_rate is only ever set to a real value in this switch statement,
you're not oring with anything else so you can just use an assignment
which would be a lot clearer (it's not obvious what else might go in
there) and the initial assignment to 0 for this and chan_sz can only
mask errors.

> +	/* set sampling rate of IV */
> +	if (max98927->interleave_mode &&
> +		sampling_rate > MAX98927_PCM_SR_SET1_SR_16000)
> +		regmap_update_bits(max98927->regmap,

Please use the kernel coding style, indent the second line of the if
with the ( so that it doesn't look like part of the conditional code.

> +	switch (reg) {
> +	case MAX98927_R0001_INT_RAW1 ... MAX98927_R0028_ICC_RX_EN_B:


> +		return true;
> +	}
> +	return false;

It'd be clearer to put the return false in the switch statement like the
return true, and also consistent with the _volatile_reg() function.

> +static const char * const max98927_speaker_source_text[] = {
> +	"i2s", "reserved", "tone", "pdm"
> +};

This looks like it should be a DAPM control.

> +static const char * const max98927_monomix_output_text[] = {
> +	"ch_0", "ch_1", "ch_1_2_div"
> +};

Similarly here.

> +	SOC_SINGLE_TLV("Digital Gain", MAX98927_R0036_AMP_VOL_CTRL,
> +		0, (1<<MAX98927_AMP_VOL_WIDTH)-1, 0,
> +		max98927_digital_tlv),

All volume controls should end with Volume as per control-names.rst.

> +	SOC_SINGLE("Amp DSP Enable", MAX98927_R0052_BROWNOUT_EN,
> +		MAX98927_BROWNOUT_DSP_SHIFT, 1, 0),

All on/off switches should end with Switch as per control-names.rst.

> +static int max98927_probe(struct snd_soc_codec *codec)
> +{

> +       /* Check Revision ID */
> +       ret = regmap_read(max98927->regmap,
> +               MAX98927_R01FF_REV_ID, &reg);

Basic device identification and setup should be done at the chip level
probe not at the CODEC level so that if there are problems we fail as
early as possible and so that diagnostic information is available to
users as soon as possible, even if there's no sound card for the device
in the system.

> +	/* Set inital volume (+13dB) */

As with all other CODEC drivers you should leave the hardware defaults
alone, what makes sense for your system may not make sense for other
systems and the hardware defaults are a fixed thing.

> +	/* Boost Output Voltage & Current limit */
> +	regmap_write(max98927->regmap,
> +		MAX98927_R0040_BOOST_CTRL0,
> +		0x1C);
> +	regmap_write(max98927->regmap,
> +		MAX98927_R0042_BOOST_CTRL1,
> +		0x3E);

This should be system specific, these values might be unsafe in some
systems.

> +err:
> +	if (max98927)
> +		devm_kfree(&i2c->dev, max98927);

There is no need to explicitly free devm_ allocated memory, that's the
point of devm.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



  reply	other threads:[~2017-03-29 14:24 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-29  0:53 [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier Ryan Lee
2017-03-29  0:53 ` Ryan Lee
2017-03-29 14:23 ` Mark Brown [this message]
2017-03-29 14:23   ` Mark Brown
2017-03-31  1:18   ` Ryan Lee
2017-03-31  1:18     ` Ryan Lee
  -- strict thread matches above, loose matches on Subject: below --
2017-03-24 15:26 [PATCH] " Rob Herring
2017-03-28 15:32 ` [PATCH v3] " Ryan Lee
2017-03-28 15:32   ` Ryan Lee
2017-03-28 16:39   ` Mark Brown
2017-03-28 16:39     ` Mark Brown
2017-03-29  0:49     ` Ryan Lee
2017-03-29  0:49       ` Ryan Lee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170329142304.qovnyqd6izackny2@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=KCHSU0@nuvoton.com \
    --cc=Paul.Handrigan@cirrus.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=arnd@arndb.de \
    --cc=axel.lin@ingics.com \
    --cc=bardliao@realtek.com \
    --cc=ckeepax@opensource.wolfsonmicro.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dgreid@google.com \
    --cc=kuninori.morimoto.gx@renesas.com \
    --cc=lars@metafoo.de \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=nh6z@nh6z.net \
    --cc=oder_chiou@realtek.com \
    --cc=perex@perex.cz \
    --cc=robh+dt@kernel.org \
    --cc=romain.perier@collabora.com \
    --cc=ryan.lee.maxim@gmail.com \
    --cc=ryans.lee@maximintegrated.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=tiwai@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.