Re: [PATCH v3] ASoC: tas2783: Add source files for tas2783 driver.
From: Christophe JAILLET
Date: Sat Oct 28 2023 - 10:40:23 EST
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;
+}
+
...