| 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
| ||
|
Message-ID: <s5htv4yfpnt.wl-tiwai@suse.de>
Date: Tue, 14 Jan 2020 08:44:22 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Jeff Chang <richtek.jeff.chang@...il.com>
Cc: lgirdwood@...il.com, broonie@...nel.org, perex@...ex.cz,
tiwai@...e.com, matthias.bgg@...il.com,
alsa-devel@...a-project.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, jeff_chang@...htek.com
Subject: Re: [PATCH v6] ASoC: Add MediaTek MT6660 Speaker Amp Driver
On Tue, 14 Jan 2020 03:22:06 +0100,
Jeff Chang wrote:
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 229cc89..f135fbb 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -1465,6 +1466,16 @@ config SND_SOC_MT6358
> Enable support for the platform which uses MT6358 as
> external codec device.
>
> +config SND_SOC_MT6660
> + tristate "Mediatek MT6660 Speaker Amplifier"
> + depends on I2C
> + help
> + MediaTek MT6660 is a smart power amplifier which contain
> + speaker protection, multi-band DRC, equalizer functions.
> + Select N if you don't have MT6660 on board.
> + Select M to build this as module.
> +
> +
One blank line too much here.
> --- /dev/null
> +++ b/sound/soc/codecs/mt6660.c
> @@ -0,0 +1,533 @@
> +// SPDX-License-Identifier: GPL-2.0 //
> +
> +// Copyright (c) 2019 MediaTek Inc.
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/version.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>
> +#include <sound/pcm_params.h>
> +#include <linux/debugfs.h>
Move linux/*.h above sound/*.h inclusion.
> +
> +#include "mt6660.h"
> +
> +#pragma pack(push, 1)
Actually packing makes little sense for those use cases.
As I mentioned earlier, packing is useful only for either saving some
memory (e.g. for a large array) or a strict size definition like ABI.
> +struct codec_reg_val {
> + u32 addr;
> + u32 mask;
> + u32 data;
> +};
Is this struct used anywhere? If not, kill it.
> +static struct regmap_config mt6660_regmap_config = {
This can be const.
> +static int mt6660_codec_dac_event(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> +
A superfluous blank line.
> +static int mt6660_component_get_volsw(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component =
> + snd_soc_kcontrol_component(kcontrol);
> + struct mt6660_chip *chip = (struct mt6660_chip *)
> + snd_soc_component_get_drvdata(component);
> + int ret = -EINVAL;
> +
> + if (!strcmp(kcontrol->id.name, "Chip Rev")) {
> + ucontrol->value.integer.value[0] = chip->chip_rev & 0x0f;
> + ret = 0;
> + }
> + return ret;
So, "T0 SEL" control gets always an error when reading?
Then can't we pass simply NULL for get ops instead?
> +static int _mt6660_chip_power_on(struct mt6660_chip *chip, int on_off)
> +{
> + u8 reg_data;
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(chip->i2c, MT6660_REG_SYSTEM_CTRL);
> + if (ret < 0)
> + return ret;
> + reg_data = (u8)ret;
> + if (on_off)
> + reg_data &= (~0x01);
> + else
> + reg_data |= 0x01;
> + return regmap_write(chip->regmap, MT6660_REG_SYSTEM_CTRL, reg_data);
Hm, this looks like an open-code of forced update bits via regmap.
But interestingly there is no corresponding standard helper for that.
Essentially it should be regmap_update_bits_base() with force=1.
Mark?
> +static int mt6660_component_aif_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai)
> +{
> + int word_len = params_physical_width(hw_params);
> + int aud_bit = params_width(hw_params);
....
> + switch (aud_bit) {
> + case 16:
> + reg_data = 3;
> + break;
> + case 18:
> + reg_data = 2;
> + break;
> + case 20:
> + reg_data = 1;
> + break;
> + case 24:
> + case 32:
> + reg_data = 0;
> + break;
So here both 24 and 32 bits data are handled equally, and...
....
> + ret = snd_soc_component_update_bits(dai->component,
> + MT6660_REG_TDM_CFG3, 0x3f0, word_len << 4);
... word_len is same for both S32 and S24 formats, so there can be no
difference between S24 and S32 format handling in the code.
Meanwhile, the supported formats are:
> +#define STUB_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> + SNDRV_PCM_FMTBIT_U16_LE | \
> + SNDRV_PCM_FMTBIT_S24_LE | \
> + SNDRV_PCM_FMTBIT_U24_LE | \
> + SNDRV_PCM_FMTBIT_S32_LE | \
> + SNDRV_PCM_FMTBIT_U32_LE)
Are you sure that S24_* formats really work properly?
Also, the code has no check / setup of the format signedness.
Do unsigned formats (U16, U24, etc) really work as expected, too?
> +static inline int _mt6660_chip_id_check(struct mt6660_chip *chip)
Drop unnecessary inline (here and other places).
Compiler optimizes well by itself.
> +static inline int _mt6660_chip_sw_reset(struct mt6660_chip *chip)
> +{
> + int ret;
> +
> + /* turn on main pll first, then trigger reset */
> + ret = regmap_write(chip->regmap, 0x03, 0x00);
It's MT6660_REG_SYSTEM_CTRL, right?
> + if (ret < 0)
> + return ret;
> + ret = regmap_write(chip->regmap, MT6660_REG_SYSTEM_CTRL, 0x80);
> + if (ret < 0)
> + return ret;
> + msleep(30);
> + return 0;
> +}
> +
> +static inline int _mt6660_read_chip_revision(struct mt6660_chip *chip)
> +{
> + u8 reg_data[2];
> + int ret;
> +
> + ret = i2c_smbus_read_i2c_block_data(
> + chip->i2c, MT6660_REG_DEVID, 2, reg_data);
Why avoiding regmap here? This and chip_id_check() use the raw access
by some reason...
thanks,
Takashi
Powered by blists - more mailing lists