[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2efb5250-25f3-465e-81fc-cb885027b481@linux.intel.com>
Date: Tue, 5 Mar 2024 17:04:57 +0100
From: Amadeusz Sławiński
<amadeuszx.slawinski@...ux.intel.com>
To: Shenghao Ding <shenghao-ding@...com>, broonie@...nel.org
Cc: andriy.shevchenko@...ux.intel.com, lgirdwood@...il.com, perex@...ex.cz,
pierre-louis.bossart@...ux.intel.com, 13916275206@....com,
alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
liam.r.girdwood@...el.com, bard.liao@...el.com, mengdong.lin@...el.com,
yung-chuan.liao@...ux.intel.com, kevin-lu@...com, tiwai@...e.de,
soyer@....hu, Baojun.Xu@....com, navada@...com
Subject: Re: [PATCH v11] ASoc: tas2783: Add tas2783 codec driver
On 3/5/2024 2:26 PM, Shenghao Ding wrote:
> The tas2783 is a smart audio amplifier with integrated MIPI SoundWire
> interface (Version 1.2.1 compliant), I2C, and I2S/TDM interfaces designed
> for portable applications. An on-chip DSP supports Texas Instruments
> SmartAmp speaker protection algorithm. The integrated speaker voltage and
> current sense provides for real-time monitoring of loudspeakers.
>
> The ASoC component provides the majority of the functionality of the
> device, all the audio functions.
>
> Signed-off-by: Shenghao Ding <shenghao-ding@...com>
>
> ---
..
> +
> +static void tas2783_apply_calibv2(struct tasdevice_priv *tas_dev,
> + unsigned int *cali_data)
> +{
> + const unsigned int arr_size = ARRAY_SIZE(tas2783_cali_reg);
> + struct regmap *map = tas_dev->regmap;
> + unsigned int dev_sum = cali_data[1], i, j, k;
> + u8 *cali_start;
> + u16 dev_info;
> + int ret;
> +
> + if (!tas_dev->sdw_peripheral) {
> + dev_err(tas_dev->dev, "%s: peripheral doesn't exist.\n",
> + __func__);
> + return;
> + }
> +
> + dev_info = tas_dev->sdw_peripheral->bus->link_id |
> + tas_dev->sdw_peripheral->id.unique_id << 16;
> +
> + /*
> + * The area saving tas2783 calibrated data is specified by its
> + * unique_id offset. cali_start is the first address of current
> + * tas2783's calibrated data.
> + */
> + cali_start = (u8 *)&cali_data[3];
> + for (i = 0; i < dev_sum; i++) {
> + k = i * (arr_size + 1) + 3;
> + if (dev_info != cali_data[k]) {
> + for (j = 0; j < arr_size; j++) {
> + k = 4 * (k + 1 + j);
> + ret = regmap_bulk_write(map,
> + tas2783_cali_reg[j],
> + &cali_start[k], 4);
> + if (ret) {
> + dev_err(tas_dev->dev,
> + "Cali failed %x:%d\n",
> + tas2783_cali_reg[j], ret);
> + break;
> + }
> + }
> + break;
> + }
> + }
This seems a bit hard to read, any chance to do some reordering to make
it more readable?
> +}
> +
..
> +
> +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;
> +
> + dev_dbg(tas_dev->dev, "%s: %d.\n", __func__, mute);
> +
> + if (mute) {
> + if (direction == SNDRV_PCM_STREAM_CAPTURE) {
> + ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> + TAS2783_REG_AEF_MASK,
> + TAS2783_REG_AEF_INACTIVE);
> + if (ret)
> + dev_err(tas_dev->dev,
> + "%s: Disable AEF failed.\n", __func__);
> + } else {
> + /* FU23 mute (0x40400108) */
> + ret = regmap_write(map,
> + SDW_SDCA_CTL(TAS2783_FUNC_TYPE_SMART_AMP,
> + TAS2783_SDCA_ENT_FU23,
> + TAS2783_SDCA_CTL_FU_MUTE, 0), 1);
> + if (ret) {
> + dev_err(tas_dev->dev,
> + "%s: FU23 mute failed.\n", __func__);
> + goto out;
> + }
> + /*
> + * Both playback and echo data will be shutdown in
> + * playback stream.
> + */
> + ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> + TAS2783_REG_PWR_MODE_MASK |
> + TAS2783_REG_AEF_MASK,
> + TAS2783_REG_PWR_MODE_ACTIVE |
> + TAS2783_REG_PWR_MODE_SW_PWD);
> + if (ret) {
> + dev_err(tas_dev->dev,
> + "%s: PWR&AEF shutdown failed.\n",
> + __func__);
> + goto out;
> + }
> + tas_dev->pstream = false;
> + }
> + } else {
> + /* FU23 Unmute, 0x40400108. */
> + if (direction == SNDRV_PCM_STREAM_PLAYBACK) {
> + ret = regmap_write(map,
> + SDW_SDCA_CTL(TAS2783_FUNC_TYPE_SMART_AMP,
> + TAS2783_SDCA_ENT_FU23,
> + TAS2783_SDCA_CTL_FU_MUTE, 0), 0);
> + if (ret) {
> + dev_err(tas_dev->dev,
> + "%s: FU23 Unmute failed.\n", __func__);
> + goto out;
> + }
> + ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> + TAS2783_REG_PWR_MODE_MASK,
> + TAS2783_REG_PWR_MODE_ACTIVE);
> + if (ret) {
> + dev_err(tas_dev->dev,
> + "%s: PWR Unmute failed.\n", __func__);
> + goto out;
> + }
> + tas_dev->pstream = true;
> + } else {
> + /* Capture stream is the echo ref data for voice.
> + * Without playback, it can't be active.
> + */
> + if (tas_dev->pstream == true) {
> + ret = regmap_update_bits(map,
> + TAS2873_REG_PWR_CTRL,
> + TAS2783_REG_AEF_MASK,
> + TAS2783_REG_AEF_ACTIVE);
> + if (ret) {
> + dev_err(tas_dev->dev,
> + "%s: AEF enable failed.\n",
> + __func__);
> + goto out;
> + }
> + } else {
> + dev_err(tas_dev->dev,
> + "%s: No playback, no AEF!", __func__);
> + ret = -EINVAL;
> + }
> + }
> + }
> +out:
> + if (ret)
> + dev_err(tas_dev->dev, "Mute or unmute %d failed %d.\n",
> + mute, ret);
> +
> + return ret;
> +}
Above function seem to be bit long, which also causes a lot of
indentation, perhaps split it into mute and unmute helpers?
..
> +
> +static int tasdevice_read_prop(struct sdw_slave *slave)
> +{
> + struct sdw_slave_prop *prop = &slave->prop;
> + struct sdw_dpn_prop *dpn;
> + unsigned long addr;
> + int nval, i, j;
> + u32 bit;
> +
> + prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
> + prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
> +
> + prop->paging_support = true;
> +
> + /* first we need to allocate memory for set bits in port lists */
> + prop->source_ports = BIT(2); /* BITMAP: 00000100 */
> + prop->sink_ports = BIT(1); /* BITMAP: 00000010 */
> +
> + nval = hweight32(prop->source_ports);
> + prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
> + sizeof(*prop->src_dpn_prop), GFP_KERNEL);
> + if (!prop->src_dpn_prop)
> + return -ENOMEM;
> +
> + i = 0;
> + dpn = prop->src_dpn_prop;
> + addr = prop->source_ports;
> + for_each_set_bit(bit, &addr, 32) {
> + dpn[i].num = bit;
> + dpn[i].type = SDW_DPN_FULL;
> + dpn[i].simple_ch_prep_sm = true;
> + dpn[i].ch_prep_timeout = 10;
> + i++;
> + }
> +
> + /* do this again for sink now */
> + nval = hweight32(prop->sink_ports);
> + prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
> + sizeof(*prop->sink_dpn_prop), GFP_KERNEL);
> + if (!prop->sink_dpn_prop)
> + return -ENOMEM;
> +
> + j = 0;
No need for separate j variable, you can reuse i here.
> + dpn = prop->sink_dpn_prop;
> + addr = prop->sink_ports;
> + for_each_set_bit(bit, &addr, 32) {
> + dpn[j].num = bit;
> + dpn[j].type = SDW_DPN_FULL;
> + dpn[j].simple_ch_prep_sm = true;
> + dpn[j].ch_prep_timeout = 10;
> + j++;
> + }
> +
> + /* set the timeout values */
> + prop->clk_stop_timeout = 20;
> +
> + return 0;
> +}
> +
..
> +
> +struct tasdevice_priv {
> + struct snd_soc_component *component;
Apart from being assigned this field seems to be unused.
> + struct sdw_slave *sdw_peripheral;
> + enum sdw_slave_status status;
This one seems to be only used in tasdevice_update_status()? Does it
really need to be kept in struct?
> + struct sdw_bus_params params;
Unused?
> + struct regmap *regmap;
> + struct device *dev;
> + unsigned char dspfw_binaryname[TAS2783_DSPFW_FILENAME_LEN];
This one also seems weird, it is mainly needed when loading FW and could
be local to tasdevice_comp_probe(), although there is one dev_warn which
uses it outside of it, but pretty sure it could be dropped.
> + unsigned char dev_name[32];
Another unused field.
> + unsigned int chip_id;
Another one that only seems to be assigned.
> + bool pstream;
> + bool hw_init;
> + bool first_hw_init;
> +};
> +
> +#endif /*__TAS2783_H__ */
Powered by blists - more mailing lists