| 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
| ||
|
Date: Sat, 28 Oct 2023 16:40:04 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Baojun Xu <baojun.xu@...com>, broonie@...nel.org,
lgirdwood@...il.com, perex@...ex.cz
Cc: pierre-louis.bossart@...ux.intel.com, alsa-devel@...a-project.org,
linux-kernel@...r.kernel.org, kevin-lu@...com,
shenghao-ding@...com, peeyush@...com, navada@...com, tiwai@...e.de
Subject: Re: [PATCH v3] ASoC: tas2783: Add source files for tas2783 driver.
Le 28/10/2023 à 11:24, Baojun Xu a écrit :
> Add source file and header file for tas2783 soundwire driver.
> Also update Kconfig and Makefile for tas2783 driver.
>
> Signed-off-by: Baojun Xu <baojun.xu@...com>
> ---
Hi,
some nit and on fix below.
CJ
...
> +static int tas2783_digital_getvol(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component
> + = snd_soc_kcontrol_component(kcontrol);
> + struct tasdevice_priv *tas_dev =
> + snd_soc_component_get_drvdata(component);
> + struct soc_mixer_control *mc =
> + (struct soc_mixer_control *)kcontrol->private_value;
> + struct regmap *map = tas_dev->regmap;
> + int val = 0, ret;
> +
> + if (!map || !ucontrol) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "%s, wrong regmap.\n", __func__);
> + return -EINVAL;
> + }
> + /* Read current volume from the device. */
> + ret = regmap_read(map, mc->reg, &val);
> + if (ret) {
> + dev_err(tas_dev->dev, "%s, get digital vol error %x.\n",
> + __func__, ret);
> + return ret;
> + }
> + ucontrol->value.integer.value[0] =
> + tasdevice_clamp(val, mc->max, mc->invert);
> +
> + return ret;
> +}
> +
> +static int tas2783_digital_putvol(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component
> + = snd_soc_kcontrol_component(kcontrol);
> + struct tasdevice_priv *tas_dev =
> + snd_soc_component_get_drvdata(component);
> + struct soc_mixer_control *mc =
> + (struct soc_mixer_control *)kcontrol->private_value;
> + struct regmap *map = tas_dev->regmap;
> + int val, ret;
> +
> + if (!map || !ucontrol) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "%s, wrong regmap.\n", __func__);
> + return -EINVAL;
> + }
> + val = tasdevice_clamp(ucontrol->value.integer.value[0],
> + mc->max, mc->invert);
> +
> + ret = regmap_write(map, mc->reg, val);
> + if (ret != 0) {
> + dev_dbg(tas_dev->dev, "%s, Put vol %d into %x %x.\n",
> + __func__, val, mc->reg, ret);
> + }
> +
> + return ret;
> +}
> +
> +static int tas2783_amp_getvol(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component
> + = snd_soc_kcontrol_component(kcontrol);
> + struct tasdevice_priv *tas_dev =
> + snd_soc_component_get_drvdata(component);
> + struct soc_mixer_control *mc =
> + (struct soc_mixer_control *)kcontrol->private_value;
> + struct regmap *map = tas_dev->regmap;
> + unsigned char mask = 0;
> + int ret = 0, val = 0;
Useless initialisation of ret.
> +
> + if (!map || !ucontrol) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "%s, wrong regmap.\n", __func__);
> + return -EINVAL;
> + }
> + /* Read current volume from the device. */
> + ret = regmap_read(map, mc->reg, &val);
> + if (ret != 0) {
> + dev_err(tas_dev->dev, "%s get AMP vol from %x with %d.\n",
> + __func__, mc->reg, ret);
> + return ret;
> + }
> +
> + mask = (1 << fls(mc->max)) - 1;
> + mask <<= mc->shift;
> + val = (val & mask) >> mc->shift;
> + ucontrol->value.integer.value[0] = tasdevice_clamp(val, mc->max,
> + mc->invert);
> +
> + return ret;
> +}
> +
> +static int tas2783_amp_putvol(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component
> + = snd_soc_kcontrol_component(kcontrol);
> + struct tasdevice_priv *tas_dev =
> + snd_soc_component_get_drvdata(component);
> + struct soc_mixer_control *mc =
> + (struct soc_mixer_control *)kcontrol->private_value;
> + struct regmap *map = tas_dev->regmap;
> + unsigned char mask;
> + int val, ret;
> +
> + if (!map || !ucontrol) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "%s, wrong regmap.\n", __func__);
> + return -EINVAL;
> + }
> + mask = (1 << fls(mc->max)) - 1;
> + mask <<= mc->shift;
> + val = tasdevice_clamp(ucontrol->value.integer.value[0], mc->max,
> + mc->invert);
> + ret = regmap_update_bits(map, mc->reg, mask, val << mc->shift);
> + if (ret != 0) {
> + dev_err(tas_dev->dev, "Write @%#x..%#x:%d\n",
> + mc->reg, val, ret);
> + }
> +
> + return ret;
> +}
...
> +static void tas2783_apply_calib(
> + struct tasdevice_priv *tas_dev, unsigned int *cali_data)
> +{
> + struct regmap *map = tas_dev->regmap;
> + u8 *reg_start;
> + int ret;
> +
> + if (!map) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "%s, wrong regmap.\n", __func__);
> + return;
> + }
> + if (!tas_dev->sdw_peripheral) {
> + dev_err(tas_dev->dev, "%s, slaver doesn't exist.\n",
> + __func__);
> + return;
> + }
> + if ((tas_dev->sdw_peripheral->id.unique_id < TAS2783_ID_MIN) ||
> + (tas_dev->sdw_peripheral->id.unique_id > TAS2783_ID_MAX))
> + return;
> + reg_start = (u8 *)(cali_data+(tas_dev->sdw_peripheral->id.unique_id
> + - TAS2783_ID_MIN)*sizeof(tas2783_cali_reg));
> + for (int i = 0; i < ARRAY_SIZE(tas2783_cali_reg); i++) {
> + ret = regmap_bulk_write(map, tas2783_cali_reg[i],
> + reg_start + i, 4);
> + if (ret != 0) {
> + dev_err(tas_dev->dev, "Cali failed %x:%d\n",
> + tas2783_cali_reg[i], ret);
> + break;
> + }
> + }
> +}
...
> +static void tasdevice_rca_ready(const struct firmware *fmw, void *context)
> +{
> + struct tasdevice_priv *tas_dev =
> + (struct tasdevice_priv *) context;
> + struct tas2783_firmware_node *p;
> + struct regmap *map = tas_dev->regmap;
> + unsigned char *buf = NULL;
> + int offset = 0, img_sz;
> + int ret, value_sdw;
> +
> + mutex_lock(&tas_dev->codec_lock);
> +
> + if (!map) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "%s, wrong regmap.\n", __func__);
> + ret = -EINVAL;
> + goto out;
> + }
> + if (!fmw || !fmw->data) {
> + /* No firmware binary, devices will work in ROM mode. */
> + dev_err(tas_dev->dev,
> + "Failed to read %s, no side-effect on driver running\n",
> + tas_dev->rca_binaryname);
> + ret = -EINVAL;
> + goto out;
> + }
> + buf = (unsigned char *)fmw->data;
> +
> + img_sz = le32_to_cpup((__le32 *)&buf[offset]);
> + offset += sizeof(img_sz);
> + if (img_sz != fmw->size) {
> + dev_err(tas_dev->dev, "Size not matching, %d %u",
> + (int)fmw->size, img_sz);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + while (offset < img_sz) {
> + p = (struct tas2783_firmware_node *)(buf + offset);
> + if (p->length > 1) {
> + ret = regmap_bulk_write(map, p->download_addr,
> + buf + offset + sizeof(unsigned int)*5, p->length);
> + } else {
> + ret = regmap_write(map, p->download_addr,
> + *(buf + offset + sizeof(unsigned int)*5));
> + }
> + if (ret != 0) {
> + dev_dbg(tas_dev->dev, "Load FW fail: %d.\n", ret);
> + goto out;
> + }
> + offset += sizeof(unsigned int)*5 + p->length;
> + }
> + /* Select left-right channel based on unique id. */
> + value_sdw = 0x1a;
> + value_sdw += ((tas_dev->sdw_peripheral->id.unique_id & 1) << 4);
> + regmap_write(map, TASDEVICE_REG(0, 0, 0x0a), value_sdw);
> +
> + tas2783_calibration(tas_dev);
> +
> +out:
> + mutex_unlock(&tas_dev->codec_lock);
> + if (fmw)
> + release_firmware(fmw);
> +}
...
> +static int tasdevice_mute(struct snd_soc_dai *dai, int mute,
> + int direction)
> +{
> + struct snd_soc_component *component = dai->component;
> + struct tasdevice_priv *tas_dev =
> + snd_soc_component_get_drvdata(component);
> + struct regmap *map = tas_dev->regmap;
> + int ret;
> +
> + if (!map) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "%s, wrong regmap.\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (mute == 0) {/* Unmute. */
> + /* FU23 Unmute, 0x40400108. */
> + ret = regmap_write(map, SDW_SDCA_CTL(1, 2, 1, 0), 0);
> + ret += regmap_write(map, TASDEVICE_REG(0, 0, 0x02), 0x0);
> + } else {/* Mute */
> + /* FU23 mute */
> + ret = regmap_write(map, SDW_SDCA_CTL(1, 2, 1, 0), 1);
> + ret += regmap_write(map, TASDEVICE_REG(0, 0, 0x02), 0x1a);
> + }
> + if (ret) {
> + dev_err(tas_dev->dev, "Mute or unmute %d failed %d.\n",
> + mute, ret);
> + }
> +
> + return ret;
> +}
...
> +static void tas2783_reset(struct tasdevice_priv *tas_dev)
> +{
> + struct regmap *map = tas_dev->regmap;
> + int ret;
> +
> + if (!map) {
'map' can't be NULL if the probe succeeds.
> + dev_err(tas_dev->dev, "Failed to load regmap.\n");
> + return;
> + }
> + ret = regmap_write(map, TAS2873_REG_SWRESET, 1);
> + if (ret) {
> + dev_err(tas_dev->dev, "Reset failed.\n");
> + return;
> + }
> + usleep_range(1000, 1050);
> +}
...
> +static void tasdevice_remove(struct tasdevice_priv *tas_dev)
> +{
> + snd_soc_unregister_component(tas_dev->dev);
Is it needed?
In tasdevice_init(), devm_snd_soc_register_component() is used.
> +
> + mutex_destroy(&tas_dev->codec_lock);
> +}
> +
> +static int tasdevice_sdw_probe(struct sdw_slave *peripheral,
> + const struct sdw_device_id *id)
> +{
> + struct device *dev = &peripheral->dev;
> + struct tasdevice_priv *tas_dev;
> + int ret;
> +
> + tas_dev = devm_kzalloc(dev, sizeof(*tas_dev), GFP_KERNEL);
> + if (!tas_dev) {
> + ret = -ENOMEM;
A direct return -ENOMEM; would be cleaner IMHO...
> + goto out;
> + }
> + tas_dev->dev = dev;
> + tas_dev->chip_id = id->driver_data;
> + tas_dev->sdw_peripheral = peripheral;
> + tas_dev->hw_init = false;
> +
> + dev_set_drvdata(dev, tas_dev);
> +
> + tas_dev->regmap = devm_regmap_init_sdw(peripheral,
> + &tasdevice_regmap);
> + if (IS_ERR(tas_dev->regmap)) {
> + ret = PTR_ERR(tas_dev->regmap);
> + dev_err(dev, "Failed devm_regmap_init: %d\n", ret);
Mater of taste, but dev_err_probe() could be used
> + goto out;
> + }
> + ret = tasdevice_init(tas_dev);
> +
> +out:
> + if (ret < 0 && tas_dev != NULL)
... it would also save the "&& tas_dev != NULL" test here.
> + tasdevice_remove(tas_dev);
> +
> + return ret;
> +}
> +
> +static int tasdevice_sdw_remove(struct sdw_slave *peripheral)
> +{
> + struct tasdevice_priv *tas_dev = dev_get_drvdata(&peripheral->dev);
> +
> + if (tas_dev) {
If I'm correct, 'tas_dev is known' to be not-NULL, if
tasdevice_sdw_remove() is called.
This test can be removed.
> + pm_runtime_disable(tas_dev->dev);
> + tasdevice_remove(tas_dev);
> + }
> +
> + return 0;
> +}
> +
...
Powered by blists - more mailing lists