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@ti.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;

+}

...