Thanks Richard, this looks quite good.
Nit-pick for the entire patch: SoundWire (capital W).
> - Intel Soundwire host controllers have a low-power clock-stop mode that
> requires resetting all peripherals when resuming. This is not compatible
> with the plug-detect and button-detect because it will clear the
> condition that caused the wakeup before the driver can handle it. So
> clock-stop must be blocked when a snd_soc_jack handler is registered.
What do you mean by 'clock-stop must be blocked'? The peripheral cannot
prevent the host from stopping the clock. Maybe this is explained
further down in this patch, but that statement is a bit odd.
Even if the condition that caused the wakeup was cleared, presumably
when resetting the device the same condition will be raised again, no?
> +static int cs42l42_sdw_dai_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
> + struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> + struct sdw_stream_config sconfig;
> + struct sdw_port_config pconfig;
> + unsigned int pdn_mask;
> + int ret;
> +
> + if (!sdw_stream)
> + return -EINVAL;
> +
> + /* Needed for PLL configuration when we are notified of new bus config */
> + cs42l42->sample_rate = params_rate(params);
> +
> + memset(&sconfig, 0, sizeof(sconfig));
> + memset(&pconfig, 0, sizeof(pconfig));
> +
> + sconfig.frame_rate = params_rate(params);
> + sconfig.ch_count = params_channels(params);
> + sconfig.bps = snd_pcm_format_width(params_format(params));
> + pconfig.ch_mask = GENMASK(sconfig.ch_count - 1, 0);
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + sconfig.direction = SDW_DATA_DIR_RX;
> + pconfig.num = CS42L42_SDW_PLAYBACK_PORT;
> + pdn_mask = CS42L42_HP_PDN_MASK;
> + } else {
> + sconfig.direction = SDW_DATA_DIR_TX;
> + pconfig.num = CS42L42_SDW_CAPTURE_PORT;
> + pdn_mask = CS42L42_ADC_PDN_MASK;
> + }
> +
> + /*
> + * The DAI-link prepare() will trigger Soundwire DP prepare. But CS42L42
> + * DP will only prepare if the HP/ADC is already powered-up. The
> + * DAI prepare() and DAPM sequence run after DAI-link prepare() so the
> + * PDN bit must be written here.
> + */
Why not make use of the callbacks that were added precisely to let the
codec driver perform device-specific operations? You can add your own
code in pre and post operation for both prepare and bank switch. It's
not clear why you would do this in a hw_params (which can be called
multiple times per ALSA conventions).
> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
> + usleep_range(CS42L42_HP_ADC_EN_TIME_US, CS42L42_HP_ADC_EN_TIME_US + 1000);
> +
> + ret = sdw_stream_add_slave(cs42l42->sdw_peripheral, &sconfig, &pconfig, 1, sdw_stream);
> + if (ret) {
> + dev_err(dai->dev, "Failed to add sdw stream: %d\n", ret);
> + goto err;
> + }
> +
> + cs42l42_src_config(dai->component, params_rate(params));
> +
> + return 0;
> +
> +err:
> + regmap_set_bits(cs42l42->regmap, CS42L42_PWR_CTL1, pdn_mask);
> +
> + return ret;
> +}
> +
> +static int cs42l42_sdw_dai_prepare(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(dai->component);
> +
> + dev_dbg(dai->dev, "dai_prepare: sclk=%u rate=%u\n", cs42l42->sclk, cs42l42->sample_rate);
> +
> + if (!cs42l42->sclk || !cs42l42->sample_rate)
what does sclk mean in the SoundWire context?
> + return -EINVAL;
> +
> + return cs42l42_pll_config(dai->component, cs42l42->sclk, cs42l42->sample_rate);
> +}
> +
There should be a really big comment here that the following functions
implement delayed reads and writes - this was not needed for previous
codecs.
> +static int cs42l42_sdw_poll_status(struct sdw_slave *peripheral, u8 mask, u8 match)
> +{
> + int ret, sdwret;
> +
> + ret = read_poll_timeout(sdw_read_no_pm, sdwret,
> + (sdwret < 0) || ((sdwret & mask) == match),
> + CS42L42_DELAYED_READ_POLL_US, CS42L42_DELAYED_READ_TIMEOUT_US,
> + false, peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
> + if (ret == 0)
> + ret = sdwret;
> +
> + if (ret < 0)
> + dev_err(&peripheral->dev, "MEM_ACCESS_STATUS & %#x for %#x fail: %d\n",
> + mask, match, ret);
> +
> + return ret;
> +}
> +
> +static int cs42l42_sdw_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + struct sdw_slave *peripheral = context;
> + u8 data;
> + int ret;
> +
> + reg += CS42L42_SDW_ADDR_OFFSET;
> +
> + ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = sdw_read_no_pm(peripheral, reg);
> + if (ret < 0) {
> + dev_err(&peripheral->dev, "Failed to issue read @0x%x: %d\n", reg, ret);
> + return ret;
> + }
> +
> + data = (u8)ret; /* possible non-delayed read value */
> + ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_ACCESS_STATUS);
> + if (ret < 0) {
> + dev_err(&peripheral->dev, "Failed to read MEM_ACCESS_STATUS: %d\n", ret);
> + return ret;
> + }
> +
> + /* If read was not delayed we already have the result */
> + if ((ret & CS42L42_SDW_LAST_LATE) == 0) {
> + *val = data;
> + return 0;
> + }
> +
> + /* Poll for delayed read completion */
> + if ((ret & CS42L42_SDW_RDATA_RDY) == 0) {
> + ret = cs42l42_sdw_poll_status(peripheral,
> + CS42L42_SDW_RDATA_RDY, CS42L42_SDW_RDATA_RDY);
> + if (ret < 0)
> + return ret;
> + }
> +
> + ret = sdw_read_no_pm(peripheral, CS42L42_SDW_MEM_READ_DATA);
> + if (ret < 0) {
> + dev_err(&peripheral->dev, "Failed to read READ_DATA: %d\n", ret);
> + return ret;
> + }
> +
> + *val = (u8)ret;
> +
> + return 0;
> +}
> +
> +static int cs42l42_sdw_write(void *context, unsigned int reg, unsigned int val)
> +{
> + struct sdw_slave *peripheral = context;
> + int ret;
> +
> + ret = cs42l42_sdw_poll_status(peripheral, CS42L42_SDW_CMD_IN_PROGRESS, 0);
> + if (ret < 0)
> + return ret;
> +
> + return sdw_write_no_pm(peripheral, reg + CS42L42_SDW_ADDR_OFFSET, (u8)val);
> +}
> +
> +static void cs42l42_sdw_init(struct sdw_slave *peripheral)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> + int ret = 0;
> +
> + regcache_cache_only(cs42l42->regmap, false);
> +
> + ret = cs42l42_init(cs42l42);
> + if (ret < 0) {
> + regcache_cache_only(cs42l42->regmap, true);
> + return;
> + }
> +
> + /* Write out any cached changes that happened between probe and attach */
> + ret = regcache_sync(cs42l42->regmap);
> + if (ret < 0)
> + dev_warn(cs42l42->dev, "Failed to sync cache: %d\n", ret);
> +
> + /* Disable internal logic that makes clock-stop conditional */
> + regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);
> +
> + /*
> + * pm_runtime is needed to control bus manager suspend, and to
I don't think the intent is that the codec can control the manager
suspend, but that the manager cannot be suspended by the framework
unless the codec suspends first?
> + * recover from an unattach_request when the manager suspends.
> + * Autosuspend delay must be long enough to enumerate.
No, this last sentence is not correct. The enumeration can be done no
matter what pm_runtime state the Linux codec device is in. And it's
really the other way around, it's when the codec reports as ATTACHED
that the codec driver will be resumed.
> + */
> + pm_runtime_set_autosuspend_delay(cs42l42->dev, 3000);
> + pm_runtime_use_autosuspend(cs42l42->dev);
> + pm_runtime_set_active(cs42l42->dev);
> + pm_runtime_enable(cs42l42->dev);
> + pm_runtime_mark_last_busy(cs42l42->dev);
> + pm_runtime_idle(cs42l42->dev);
> +}
> +
> +static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> + struct sdw_slave_prop *prop = &peripheral->prop;
> + struct sdw_dpn_prop *ports;
> +
> + ports = devm_kcalloc(cs42l42->dev, 2, sizeof(*ports), GFP_KERNEL);
> + if (!ports)
> + return -ENOMEM;
> +
> + prop->source_ports = BIT(CS42L42_SDW_CAPTURE_PORT);
> + prop->sink_ports = BIT(CS42L42_SDW_PLAYBACK_PORT);
> + prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
> + prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
> +
> + /* DP1 - capture */
> + ports[0].num = CS42L42_SDW_CAPTURE_PORT,
> + ports[0].type = SDW_DPN_FULL,
> + ports[0].ch_prep_timeout = 10,
> + prop->src_dpn_prop = &ports[0];
> +
> + /* DP2 - playback */
> + ports[1].num = CS42L42_SDW_PLAYBACK_PORT,
> + ports[1].type = SDW_DPN_FULL,
> + ports[1].ch_prep_timeout = 10,
> + prop->sink_dpn_prop = &ports[1];
> +
> + return 0;
> +}
> +static int cs42l42_sdw_bus_config(struct sdw_slave *peripheral,
> + struct sdw_bus_params *params)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> + unsigned int new_sclk = params->curr_dr_freq / 2;
> +
> + /* The cs42l42 cannot support a glitchless SWIRE_CLK change. */
nit-pick: SoundWire
> + if ((new_sclk != cs42l42->sclk) && cs42l42->stream_use) {
> + dev_warn(cs42l42->dev, "Rejected SCLK change while audio active\n");
> + return -EBUSY;
> + }
It's good to have this test but so far we haven't changed the bus clock
on the fly so it's not tested.
> +
> + cs42l42->sclk = new_sclk;
> +
> + dev_dbg(cs42l42->dev, "bus_config: sclk=%u c=%u r=%u\n",
> + cs42l42->sclk, params->col, params->row);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,
> + enum sdw_clk_stop_mode mode,
> + enum sdw_clk_stop_type type)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> +
> + dev_dbg(cs42l42->dev, "clk_stop mode:%d type:%d\n", mode, type);
> +
> + return 0;
> +}
> +
> +static const struct sdw_slave_ops cs42l42_sdw_ops = {
> + .read_prop = cs42l42_sdw_read_prop,
> + .update_status = cs42l42_sdw_update_status,
> + .bus_config = cs42l42_sdw_bus_config,
> +#ifdef DEBUG
> + .clk_stop = cs42l42_sdw_clk_stop,
> +#endif
do you really need this wrapper?
> +};
> +
> +static int __maybe_unused cs42l42_sdw_runtime_suspend(struct device *dev)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
> +
> + dev_dbg(dev, "Runtime suspend\n");
> +
> + /* The host controller could suspend, which would mean no register access */
> + regcache_cache_only(cs42l42->regmap, true);
> +
> + return 0;
> +}
> +
> +static const struct reg_sequence __maybe_unused cs42l42_soft_reboot_seq[] = {
> + REG_SEQ0(CS42L42_SOFT_RESET_REBOOT, 0x1e),
> +};
>
> +static int __maybe_unused cs42l42_sdw_resume(struct device *dev)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
> + int ret;
> +
> + dev_dbg(dev, "System resume\n");
> +
> + /* Power-up so it can re-enumerate */
??
You cannot power-up with the bus, did you mean that side channels are
used for powering up?
> + ret = cs42l42_resume(dev);
> + if (ret)
> + return ret;
> +
> + /* Wait for re-attach */
> + ret = cs42l42_sdw_handle_unattach(cs42l42);
> + if (ret < 0)
> + return ret;
> +
> + cs42l42_resume_restore(dev);
> +
> + return 0;
> +}
> +
> +static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_device_id *id)
> +{
> + struct device *dev = &peripheral->dev;
> + struct cs42l42_private *cs42l42;
> + struct regmap_config *regmap_conf;
> + struct regmap *regmap;
> + struct snd_soc_component_driver *component_drv;
> + int irq, ret;
> +
> + cs42l42 = devm_kzalloc(dev, sizeof(*cs42l42), GFP_KERNEL);
> + if (!cs42l42)
> + return -ENOMEM;
> +
> + if (has_acpi_companion(dev))
> + irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> + else
> + irq = of_irq_get(dev->of_node, 0);
why do you need an interrupt when SoundWire provides an in-band one? You
need a lot more explanations on the requirement and what you intend to
do with this interrupt?
> +
> + if (irq == -ENOENT)
> + irq = 0;
> + else if (irq < 0)
> + return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> +
> + regmap_conf = devm_kmemdup(dev, &cs42l42_regmap, sizeof(cs42l42_regmap), GFP_KERNEL);
> + if (!regmap_conf)
> + return -ENOMEM;
> + regmap_conf->reg_bits = 16;
> + regmap_conf->num_ranges = 0;
> + regmap_conf->reg_read = cs42l42_sdw_read;
> + regmap_conf->reg_write = cs42l42_sdw_write;
> +
> + regmap = devm_regmap_init(dev, NULL, peripheral, regmap_conf);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to allocate register map\n");
> +
> + /* Start in cache-only until device is enumerated */
> + regcache_cache_only(regmap, true);
> +
> + component_drv = devm_kmemdup(dev,
> + &cs42l42_soc_component,
> + sizeof(cs42l42_soc_component),
> + GFP_KERNEL);
> + if (!component_drv)
> + return -ENOMEM;
> +
> + component_drv->dapm_routes = cs42l42_sdw_audio_map;
> + component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);
> +
> + cs42l42->dev = dev;
> + cs42l42->regmap = regmap;
> + cs42l42->sdw_peripheral = peripheral;
> + cs42l42->irq = irq;
> +
> + ret = cs42l42_common_probe(cs42l42, component_drv, &cs42l42_sdw_dai);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int cs42l42_sdw_remove(struct sdw_slave *peripheral)
> +{
> + struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
> +
> + /* Resume so that cs42l42_remove() can access registers */
> + pm_runtime_get_sync(cs42l42->dev);
you need to test if this resume succeeded, and possibly use
pm_resume_resume_and_get() to avoid issues.
> + cs42l42_common_remove(cs42l42);
> + pm_runtime_put(cs42l42->dev);
> + pm_runtime_disable(cs42l42->dev);
> +
> + return 0;
> +}
> static const struct snd_soc_dapm_route cs42l42_audio_map[] = {
> @@ -559,6 +564,20 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
> {
> struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
>
> + /*
> + * If the Soundwire controller issues bus reset when coming out of
> + * clock-stop it will erase the jack state. This can lose button press
> + * events, and plug/unplug interrupt bits take between 125ms and 1500ms
> + * before they are valid again.
> + * Prevent this by holding our pm_runtime to block clock-stop.
> + */
> + if (cs42l42->sdw_peripheral) {
> + if (jk)
> + pm_runtime_get_sync(cs42l42->dev);
> + else
> + pm_runtime_put_autosuspend(cs42l42->dev);
> + }
> +
I *really* don't understand this sequence.
The bus will be suspended when ALL devices have been idle for some time.
If the user presses a button AFTER the bus is suspended, the device can
still use the in-band wake and resume.
Granted the button press will be lost but the plug/unplug status will
still be handled with a delay.
> /* Prevent race with interrupt handler */
> mutex_lock(&cs42l42->irq_lock);
> cs42l42->jack = jk;
> @@ -1645,9 +1664,11 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
> unsigned int current_button_status;
> unsigned int i;
>
> + pm_runtime_get_sync(cs42l42->dev);
> mutex_lock(&cs42l42->irq_lock);
> if (cs42l42->suspended || !cs42l42->init_done) {
> mutex_unlock(&cs42l42->irq_lock);
> + pm_runtime_put_autosuspend(cs42l42->dev);
> return IRQ_NONE;
> }
>
> @@ -1750,6 +1771,8 @@ irqreturn_t cs42l42_irq_thread(int irq, void *data)
> }
>
> mutex_unlock(&cs42l42->irq_lock);
> + pm_runtime_mark_last_busy(cs42l42->dev);
> + pm_runtime_put_autosuspend(cs42l42->dev);
>
> return IRQ_HANDLED;
Again in SoundWire more you should not use a dedicated interrupt.
There's something missing in the explanations on why this thread is
required.
