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@xxxxxx> ---
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; +} +
...