> +config SND_SOC_TAS2783 > + tristate "Texas Instruments TAS2783 speaker amplifier (sdw)" > + depends on SOUNDWIRE > + select REGMAP > + select CRC32_SARWATE > + help > + Enable support for Texas Instruments TAS2783 Smart Amplifier > + Digital input mono Class-D and DSP-inside audio power amplifiers. > + Note the TAS2783 driver implements a flexible and configurable > + algo coff setting, for one, two, even multiple TAS2783 chips. Algorithm coefficient > +#include <linux/crc32.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/efi.h> is this really needed? > +#include <linux/err.h> > +#include <linux/firmware.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/pm.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include <linux/soundwire/sdw.h> > +#include <linux/soundwire/sdw_registers.h> > +#include <linux/soundwire/sdw_type.h> > +#include <sound/pcm_params.h> > +#include <sound/sdw.h> > +#include <sound/soc.h> > +#include <sound/tlv.h> > +#include <sound/tas2781-tlv.h> > + > +#include "tas2783.h" > + > +static const unsigned int tas2783_calibration_reg[] = { > + TAS2783_CALIBRATION_RE, > + TAS2783_CALIBRATION_RE_LOW, > + TAS2783_CALIBRATION_INV_RE, > + TAS2783_CALIBRATION_POW, > + TAS2783_CALIBRATION_TLIMIT, > + 0, what is the purpose of this zero, presumably you can use ARRAY_SIZE and don't need a zero-terminated value? > +static bool tas2783_volatile_register(struct device *dev, > + unsigned int reg) > +{ > + switch (reg) { > + case 0x8001: > + // Only reset register was volatiled. > + return true; can you explain the concept of a volatile reset register? > + default: > + return false; > + } > +} > +static int tas2783_digital_getvol(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *codec > + = snd_soc_kcontrol_component(kcontrol); > + struct tasdevice_priv *tas_dev = > + snd_soc_component_get_drvdata(codec); > + struct soc_mixer_control *mc = > + (struct soc_mixer_control *)kcontrol->private_value; > + struct regmap *map = tas_dev->regmap; > + int val = 0, ret; different line when a variable is initialized? > + > + if (!map) { > + ret = -EINVAL; > + dev_err(tas_dev->dev, "%s, regmap doesn't exist.\n", > + __func__); > + goto out; > + } > + /* Read the primary device as the whole */ I can't figure out what this comment means > + ret = regmap_read(map, mc->reg, &val); > + dev_dbg(tas_dev->dev, "%s, get digital vol %d from %x with %d\n", > + __func__, val, mc->reg, ret); > + if (ret) { > + dev_err(tas_dev->dev, "%s, get digital vol error %x.\n", > + __func__, ret); > + goto out; > + } > + ucontrol->value.integer.value[0] = > + tasdevice_clamp(val, mc->max, mc->invert); > + > +out: > + return ret; > +} > +static int tas2783_amp_getvol(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *codec > + = snd_soc_kcontrol_component(kcontrol); > + struct tasdevice_priv *tas_dev = > + snd_soc_component_get_drvdata(codec); > + struct soc_mixer_control *mc = > + (struct soc_mixer_control *)kcontrol->private_value; > + struct regmap *map = tas_dev->regmap; > + unsigned char mask; > + int ret, val; > + > + if (!map) { > + dev_err(tas_dev->dev, "%s, regmap doesn't exist.\n", > + __func__); > + return -EINVAL; > + } > + /* Read the primary device */ What is a primary device? > + ret = regmap_read(map, mc->reg, &val); > + dev_dbg(tas_dev->dev, "%s, get AMP vol %d from %x with %d\n", > + __func__, val, mc->reg, 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_calibration(struct tasdevice_priv *tas_priv) > +{ > + efi_guid_t efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, > + 0x09, 0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92); > + static efi_char16_t efi_name[] = TAS2783_CALIDATA_NAME; > + struct tm *tm = &tas_priv->tm; > + unsigned int attr, crc; > + unsigned int *tmp_val; > + efi_status_t status; > + > + tas_priv->cali_data.total_sz = TAS2783_MAX_CALIDATA_SIZE; > + /* Get real size of UEFI variable */ > + status = efi.get_variable(efi_name, &efi_guid, &attr, > + &tas_priv->cali_data.total_sz, tas_priv->cali_data.data); > + dev_dbg(tas_priv->dev, "cali get %lx bytes with result : %ld\n", > + tas_priv->cali_data.total_sz, status); > + if (status == EFI_BUFFER_TOO_SMALL) { > + status = efi.get_variable(efi_name, &efi_guid, &attr, > + &tas_priv->cali_data.total_sz, > + tas_priv->cali_data.data); > + dev_dbg(tas_priv->dev, "cali get %lx bytes result:%ld\n", > + tas_priv->cali_data.total_sz, status); > + } > + /* Failed got calibration data from EFI. */ I don't get what the dependency on EFI is. First time I see a codec needing this. Please describe in details what you are trying to accomplish. Edit: there's also a dependency on firmware but the firmware name SWFT will hint at ACPI uses for anyone that has read the SDCA draft. This is beyond confusing. > + if (status != 0) { > + dev_dbg(tas_priv->dev, "cali get %lx error with:%ld\n", > + tas_priv->cali_data.total_sz, status); > + return 0; > + } > + /* Print all content of calibration data for debug. */ > + for (int i = 0; i < tas_priv->cali_data.total_sz; i += 4) { > + dev_dbg(tas_priv->dev, "cali get %02x %02x %02x %02x", > + tas_priv->cali_data.data[i], > + tas_priv->cali_data.data[i+1], > + tas_priv->cali_data.data[i+2], > + tas_priv->cali_data.data[i+3]); > + } > + > + tmp_val = (unsigned int *)tas_priv->cali_data.data; > + > + crc = crc32(~0, tas_priv->cali_data.data, 84) ^ ~0; > + dev_dbg(tas_priv->dev, "cali crc 0x%08x PK tmp_val 0x%08x\n", > + crc, tmp_val[21]); > + > + if (crc == tmp_val[21]) { > + time64_to_tm(tmp_val[20], 0, tm); > + dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n", > + tm->tm_year, tm->tm_mon, tm->tm_mday, > + tm->tm_hour, tm->tm_min, tm->tm_sec); What is this about? Why would a codec care about time? > + tas2783_apply_calib(tas_priv, tmp_val); > + } else { > + dev_dbg(tas_priv->dev, "CRC error!\n"); > + tas_priv->cali_data.total_sz = 0; > + } > + > + return 0; > +} > + > +static void tasdevice_rca_ready(const struct firmware *fmw, void *context) > +{ > + struct tasdevice_priv *tas_dev = > + (struct tasdevice_priv *) context; > + struct regmap *map = tas_dev->regmap; > + struct tas2783_firmware_node *p; > + int offset = 0, num_nodes = 0, img_sz, ret; > + unsigned char *buf; > + > + mutex_lock(&tas_dev->codec_lock); > + > + if (!fmw || !fmw->data) { > + dev_err(tas_dev->dev, > + "Failed to read %s, no side - effect on driver running\n", side-effect > + tas_dev->rca_binaryname); > + ret = -EINVAL; > + goto out; > + } > + if (!map) { > + dev_err(tas_dev->dev, "Failed to load regmap.\n"); > + ret = -EINVAL; > + goto out; > + } > + buf = (unsigned char *)fmw->data; > + > + img_sz = le32_to_cpup((__le32 *)&buf[offset]); > + dev_dbg(tas_dev->dev, "Got %x:%lx.\n", img_sz, fmw->size); > + offset += sizeof(img_sz); > + if (img_sz != fmw->size) { > + dev_err(tas_dev->dev, > + "Size not match, %d %u", (int)fmw->size, img_sz); Size does not match or size not matching > + ret = -EINVAL; > + goto out; > + } > + > + while ((offset < img_sz) && (num_nodes < TAS2783_MAX_NODES)) { > + /* Store firmware into context of driver. */ > + p = (struct tas2783_firmware_node *)(buf + offset); > + tas_dev->firmware_node[num_nodes].vendor_id = > + p->vendor_id; > + tas_dev->firmware_node[num_nodes].file_id = p->file_id; > + tas_dev->firmware_node[num_nodes].version_id = > + p->version_id; > + tas_dev->firmware_node[num_nodes].length = p->length; > + tas_dev->firmware_node[num_nodes].download_addr = > + p->download_addr; > + tas_dev->firmware_node[num_nodes].start_addr = > + ((char *)p) + sizeof(unsigned int)*5; > + > + ret = regmap_bulk_write(map, p->download_addr, > + p->start_addr, p->length); > + dev_dbg(tas_dev->dev, "Wr %d :%x:%x:%x:%x:%x:%x %d.\n", > + num_nodes, p->vendor_id, p->file_id, > + p->version_id, p->length, p->download_addr, > + p->start_addr[0], ret); > + > + offset += sizeof(unsigned int)*5 + p->length; > + num_nodes++; > + } > + > + tas2783_calibration(tas_dev); > + > +out: > + mutex_unlock(&tas_dev->codec_lock); > + if (fmw) > + release_firmware(fmw); > +} > + > +static const struct snd_soc_dapm_widget tasdevice_dapm_widgets[] = { > + SND_SOC_DAPM_AIF_IN("ASI", "ASI Playback", 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_AIF_OUT("ASI OUT", "ASI Capture", 0, SND_SOC_NOPM, > + 0, 0), > + SND_SOC_DAPM_OUTPUT("OUT"), > + SND_SOC_DAPM_INPUT("DMIC") Can you clarify what "ASI" is? Also what is the plan for DMIC - this is an amplifier, no? > +}; > + > +static const struct snd_soc_dapm_route tasdevice_audio_map[] = { > + {"OUT", NULL, "ASI"}, > + {"ASI OUT", NULL, "DMIC"} > +}; > + > +static int tasdevice_set_sdw_stream( > + struct snd_soc_dai *dai, void *sdw_stream, int direction) > +{ > + struct sdw_stream_data *stream; > + > + if (!sdw_stream) > + return 0; > + > + stream = kzalloc(sizeof(*stream), GFP_KERNEL); > + if (!stream) > + return -ENOMEM; > + > + stream->sdw_stream = sdw_stream; > + > + /* Use tx_mask or rx_mask to set dma_data */ > + snd_soc_dai_dma_data_set(dai, direction, stream); > + > + return 0; > +} this can be simplied, just look at all other existing codecs and implement the same, e.g. static int cs42l42_sdw_dai_set_sdw_stream(struct snd_soc_dai *dai, void *sdw_stream, int direction) { snd_soc_dai_dma_data_set(dai, direction, sdw_stream); return 0; } > + > +static void tasdevice_sdw_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct sdw_stream_data *stream; > + > + stream = snd_soc_dai_get_dma_data(dai, substream); > + snd_soc_dai_set_dma_data(dai, substream, NULL); > + kfree(stream); same, this can be simplified > +} > + > +static struct snd_soc_dai_driver tasdevice_dai_driver[] = { > + { > + .name = "tas2783-codec", > + .id = 0, > + .playback = { > + .stream_name = "Playback", > + .channels_min = 1, > + .channels_max = 4, > + .rates = TAS2783_DEVICE_RATES, > + .formats = TAS2783_DEVICE_FORMATS, > + }, > + .capture = { > + .stream_name = "Capture", > + .channels_min = 1, > + .channels_max = 4, > + .rates = TAS2783_DEVICE_RATES, > + .formats = TAS2783_DEVICE_FORMATS, > + }, what is the capture part? Feedback or DMIC? > + .ops = &tasdevice_dai_ops, > + .symmetric_rate = 1, > + }, > +}; > + > +static void tas2783_reset(struct tasdevice_priv *tas_dev) > +{ > + struct regmap *map = tas_dev->regmap; > + unsigned char value_sdw; > + int ret; > + > + if (!map) { > + dev_err(tas_dev->dev, "Failed to load regmap.\n"); > + return; > + } > + value_sdw = TAS2873_REG_SWRESET_RESET; > + ret = regmap_write(map, TAS2873_REG_SWRESET, value_sdw); > + dev_dbg(tas_dev->dev, "%s TAS2783 was reseted %d.\n", > + __func__, ret); > + usleep_range(1000, 1050); don't we need some sort of regmap_sync here? > +} > + > +static int tasdevice_codec_probe(struct snd_soc_component *codec) the naming is poor, this is the component probe, not the codec driver probe. > +{ > + struct tasdevice_priv *tas_dev = > + snd_soc_component_get_drvdata(codec); > + int ret; > + > + dev_dbg(tas_dev->dev, "%s called for TAS2783 start.\n", > + __func__); > + /* Codec Lock Hold */ > + mutex_lock(&tas_dev->codec_lock);> + > + tas2783_reset(tas_dev); Isn't this something you would do in a driver probe? > + > + tas_dev->codec = codec; > + > + scnprintf(tas_dev->rca_binaryname, 64, "MY_SWFT_x%01x.bin", The naming is rather controversial... You need to have some sort of identifier that is TI specific, and/or a prefix to find this file in /lib/firmware. It's really unclear to me why this is part of a component probe and not a driver probe. > + tas_dev->sdw_peripheral->id.unique_id); > + > + ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, > + tas_dev->rca_binaryname, tas_dev->dev, GFP_KERNEL, > + tas_dev, tasdevice_rca_ready); > + dev_dbg(tas_dev->dev, > + "%s: request_firmware %x open status: 0x%08x\n", > + __func__, tas_dev->sdw_peripheral->id.unique_id, ret); > + > + /* Codec Lock Release*/ > + mutex_unlock(&tas_dev->codec_lock); > + > + dev_dbg(tas_dev->dev, "%s was called end.\n", __func__); > + return ret; > +} > +static int tasdevice_io_init(struct device *dev, struct sdw_slave *slave) > +{ > + struct tasdevice_priv *tasdevice = dev_get_drvdata(dev); > + > + if (tasdevice->hw_init) > + return 0; > + > + /* PM runtime is only enabled when > + * a Slave reports as Attached > + * set autosuspend parameters > + */ > + pm_runtime_set_autosuspend_delay(&slave->dev, 3000); > + pm_runtime_use_autosuspend(&slave->dev); > + > + /* update count of parent 'active' children */ > + pm_runtime_set_active(&slave->dev); > + > + /* make sure the device does not suspend immediately */ > + pm_runtime_mark_last_busy(&slave->dev); > + > + pm_runtime_enable(&slave->dev); > + > + pm_runtime_get_noresume(&slave->dev); > + > + /* Mark Slave initialization complete */ > + tasdevice->hw_init = true; > + > + pm_runtime_mark_last_busy(&slave->dev); > + pm_runtime_put_autosuspend(&slave->dev); > + > + dev_dbg(&slave->dev, "%s hw_init complete\n", __func__); > + return 0; This is really not aligned with the latest upstream code. Intel (and specifically me myself and I) contributed a change where the pm_runtime is enabled in the driver probe, but the device status changes to active in the io_init. In addition, it's rather surprising that on attachment there is not a single regmap access? > +static void tasdevice_remove(struct tasdevice_priv *tas_dev) > +{ > + snd_soc_unregister_component(tas_dev->dev); > + > + mutex_destroy(&tas_dev->dev_lock); > + mutex_destroy(&tas_dev->codec_lock); I didn't really look into the locking parts but you will need a lot more explanations on what you are trying to protect and the concurrency issues. > +} > + > +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; > + > + dev_dbg(dev, "%s was called.\n", __func__); > + > + tas_dev = devm_kzalloc(dev, sizeof(*tas_dev), GFP_KERNEL); > + if (!tas_dev) { > + ret = -ENOMEM; > + goto out; > + } > + tas_dev->dev = &peripheral->dev; > + tas_dev->chip_id = id->driver_data; > + tas_dev->sdw_peripheral = peripheral; > + tas_dev->hw_init = false; > + > + dev_dbg(dev, "%d chip id %x for TAS2783.\n", > + peripheral->id.unique_id, tas_dev->chip_id); > + > + 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); > + goto out; > + } > + ret = tasdevice_init(tas_dev); > + > +out: > + if (ret < 0 && tas_dev != NULL) > + tasdevice_remove(tas_dev); > + > + return ret; like I said above, this is missing the pm_runtime stuff. > + > +} > + > +static int tasdevice_sdw_remove(struct sdw_slave *peripheral) > +{ > + struct tasdevice_priv *tas_dev = > + dev_get_drvdata(&peripheral->dev); > + > + if (tas_dev) > + tasdevice_remove(tas_dev); > + > + return 0; > +} > + > +static const struct sdw_device_id tasdevice_sdw_id[] = { > + SDW_SLAVE_ENTRY(0x0102, 0x0, 0), 0x0102 is the legit TI manufacturer ID, that's good. What's not so good is that the part ID is *ZERO*? Is this really intentional? > +#define TASDEVICE_REG(book, page, reg) ((book * 256 * 256) + 0x8000 +\ > + (page * 128) + reg) > + > +/*Software Reset */ /* Software