linux-kernel - Re: [PATCH] ALSA SoC MAX98927 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20161123154619.6kw5devrzsw7fapm@sirena.org.uk>
Date:   Wed, 23 Nov 2016 15:46:19 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Ryan Lee <RyanS.Lee@...imintegrated.com>
Cc:     lgirdwood@...il.com, robh+dt@...nel.org, mark.rutland@....com,
        perex@...ex.cz, tiwai@...e.com, arnd@...db.de,
        michael@...rulasolutions.com, oder_chiou@...ltek.com,
        yesanishhere@...il.com, jacob@...nage.engineering,
        Damien.Horsley@...tec.com, bardliao@...ltek.com,
        kuninori.morimoto.gx@...esas.com, petr@...ix.com, lars@...afoo.de,
        nh6z@...z.net, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ALSA SoC MAX98927 driver - Initial release

On Wed, Nov 23, 2016 at 01:57:06PM +0900, Ryan Lee wrote:

> +static struct reg_default max98927_reg_map[] = {
> +	{0x0014,  0x78},
> +	{0x0015,  0xFF},
> +	{0x0043,  0x04},
> +	{0x0017,  0x55},
> +	/* For mono driver we are just enabling one channel*/

If this table contains anything other than the physical defaults the
device has it is broken.

> +	{MAX98927_PCM_Rx_Enables_A,  0x03},
> +	{MAX98927_PCM_Tx_HiZ_Control_A, 0xfc},
> +	{MAX98927_PCM_Tx_HiZ_Control_B, 0xff},
> +	{MAX98927_PCM_Tx_Channel_Sources_A, 0x01},
> +	{MAX98927_PCM_Tx_Channel_Sources_B, 0x01},
> +	{MAX98927_Measurement_DSP_Config, 0xf7},
> +	{0x0025,  0x80},

This random mix of strangely formatted #defines and numbers isn't great
- can we please be consistent and ideally use normal style defines?

> +void max98927_wrapper_write(struct max98927_priv *max98927,
> +	unsigned int reg, unsigned int val)
> +{
> +	if (max98927->regmap)
> +		regmap_write(max98927->regmap, reg, val);
> +	if (max98927->sub_regmap)
> +		regmap_write(max98927->sub_regmap, reg, val);
> +}

I don't really know what this is doing but it looks very confused.
Having multiple regmaps is a bit worrying but even more so is having
some of those regmaps be optional.  If the device does sensibly have
multiple register maps I'd really not expect to see them appearing and
disappearing at runtime.  Whatever this is doing it at least needs to be
documented.

> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		max98927_wrap_update_bits(max98927, MAX98927_PCM_Master_Mode,
> +		MAX98927_PCM_Master_Mode_PCM_MSTR_MODE_Mask,
> +		MAX98927_PCM_Master_Mode_PCM_MSTR_MODE_SLAVE);
> +		break;

Please use a normal kernel coding style, I can't think of any coding
style where it's normal to indent continuation lines in a multi line
statement like this.  There are severe coding style problems throughout
the driver which make it hard to read, it doesn't visually resemble
normal Linux kernel code.

> +	case SND_SOC_DAIFMT_IB_NF:
> +		invert = MAX98927_PCM_Mode_Config_PCM_BCLKEDGE;
> +		break;

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

The indentation of the break statements isn't consistent within this
function :(  The former is the normal kernel coding style.

> +		if (i == ARRAY_SIZE(rate_table)) {
> +			pr_err("%s couldn't get the MCLK to match codec\n",
> +				__func__);

dev_err() and I'm not sure anyone's going to be able to understand that
error message...

> +static void max98927_handle_pdata(struct snd_soc_codec *codec)
> +{
> +	struct max98927_priv *max98927 = snd_soc_codec_get_drvdata(codec);
> +	struct max98927_reg_default *regInfo;
> +	int cfg_size = 0;
> +	int x;
> +
> +	if (max98927->regcfg != NULL)
> +		cfg_size = max98927->regcfg_sz / sizeof(uint32_t);
> +
> +	if (cfg_size <= 0) {
> +		dev_dbg(codec->dev,
> +			"Register configuration is not required.\n");
> +		return;
> +	}
> +
> +	/* direct configuration from device tree */
> +	for (x = 0; x < cfg_size; x += 3) {
> +		regInfo = (struct max98927_reg_default *)&max98927->regcfg[x];
> +		dev_info(codec->dev, "CH:%d, reg:0x%02x, value:0x%02x\n",
> +			be32_to_cpu(regInfo->ch),
> +			be32_to_cpu(regInfo->reg),
> +			be32_to_cpu(regInfo->def));
> +		if (be32_to_cpu(regInfo->ch) == PRI_MAX98927
> +			&& max98927->regmap)
> +			regmap_write(max98927->regmap,
> +				be32_to_cpu(regInfo->reg),
> +				be32_to_cpu(regInfo->def));
> +		else if (be32_to_cpu(regInfo->ch) == SEC_MAX98927
> +			&& max98927->sub_regmap)
> +			regmap_write(max98927->sub_regmap,
> +				be32_to_cpu(regInfo->reg),
> +				be32_to_cpu(regInfo->def));
> +	}
> +}

This also looks like it probably shouldn't be doing whatever it is doing
but needs some documentation.

I've stopped here.  In general it seems like this driver needs a *lot*
of work to work with the kernel interfaces in a normal style.  Aside
from the coding style issues (which really get in the way) the bulk of
the code appears to be coming from unusual and undocumented ways of
working with kernel APIs.  I'd strongly recommend taking a look at other
drivers for similar hardware and making sure that your driver looks like
them textually and structurally.  If there are things about your
hardware that mean it needs something unusual then it should be clear to
someone reading the code what's going on.

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)