Date:   Thu, 23 Dec 2021 12:29:16 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Bard Liao <yung-chuan.liao@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        tiwai@...e.de, broonie@...nel.org, gregkh@...uxfoundation.org,
        srinivas.kandagatla@...aro.org,
        pierre-louis.bossart@...ux.intel.com, sanyog.r.kale@...el.com,
        bard.liao@...el.com
Subject: Re: [PATCH 7/7] soundwire: intel: remove PDM support

On 13-12-21, 13:46, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> 
> While the hardware supports PDM streams, this capability has never
> been tested or enabled on any product, so this is dead-code. Let's
> remove all this.

So no plans to test and enable this? Do the DMICs not use PDM?

Again this should not be in this series...

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@...ux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@...el.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@...ux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c |  36 +--------
>  drivers/soundwire/cadence_master.h |  12 +--
>  drivers/soundwire/intel.c          | 123 +++++++----------------------
>  3 files changed, 31 insertions(+), 140 deletions(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 4fcc3ba93004..558390af44b6 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -1178,9 +1178,6 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>  	cdns->pcm.num_bd = config.pcm_bd;
>  	cdns->pcm.num_in = config.pcm_in;
>  	cdns->pcm.num_out = config.pcm_out;
> -	cdns->pdm.num_bd = config.pdm_bd;
> -	cdns->pdm.num_in = config.pdm_in;
> -	cdns->pdm.num_out = config.pdm_out;
>  
>  	/* Allocate PDIs for PCMs */
>  	stream = &cdns->pcm;
> @@ -1211,32 +1208,6 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>  	stream->num_pdi = stream->num_bd + stream->num_in + stream->num_out;
>  	cdns->num_ports = stream->num_pdi;
>  
> -	/* Allocate PDIs for PDMs */
> -	stream = &cdns->pdm;
> -	ret = cdns_allocate_pdi(cdns, &stream->bd,
> -				stream->num_bd, offset);
> -	if (ret)
> -		return ret;
> -
> -	offset += stream->num_bd;
> -
> -	ret = cdns_allocate_pdi(cdns, &stream->in,
> -				stream->num_in, offset);
> -	if (ret)
> -		return ret;
> -
> -	offset += stream->num_in;
> -
> -	ret = cdns_allocate_pdi(cdns, &stream->out,
> -				stream->num_out, offset);
> -
> -	if (ret)
> -		return ret;
> -
> -	/* Update total number of PDM PDIs */
> -	stream->num_pdi = stream->num_bd + stream->num_in + stream->num_out;
> -	cdns->num_ports += stream->num_pdi;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(sdw_cdns_pdi_init);
> @@ -1681,7 +1652,7 @@ int sdw_cdns_probe(struct sdw_cdns *cdns)
>  EXPORT_SYMBOL(sdw_cdns_probe);
>  
>  int cdns_set_sdw_stream(struct snd_soc_dai *dai,
> -			void *stream, bool pcm, int direction)
> +			void *stream, int direction)
>  {
>  	struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
>  	struct sdw_cdns_dma_data *dma;
> @@ -1705,10 +1676,7 @@ int cdns_set_sdw_stream(struct snd_soc_dai *dai,
>  		if (!dma)
>  			return -ENOMEM;
>  
> -		if (pcm)
> -			dma->stream_type = SDW_STREAM_PCM;
> -		else
> -			dma->stream_type = SDW_STREAM_PDM;
> +		dma->stream_type = SDW_STREAM_PCM;
>  
>  		dma->bus = &cdns->bus;
>  		dma->link_id = cdns->instance;
> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
> index aa4b9b0eb2a8..595d72c15d97 100644
> --- a/drivers/soundwire/cadence_master.h
> +++ b/drivers/soundwire/cadence_master.h
> @@ -17,7 +17,7 @@
>   * @h_ch_num: high channel for PDI
>   * @ch_count: total channel count for PDI
>   * @dir: data direction
> - * @type: stream type, PDM or PCM
> + * @type: stream type, (only PCM supported)
>   */
>  struct sdw_cdns_pdi {
>  	int num;
> @@ -62,17 +62,11 @@ struct sdw_cdns_streams {
>   * @pcm_bd: number of bidirectional PCM streams supported
>   * @pcm_in: number of input PCM streams supported
>   * @pcm_out: number of output PCM streams supported
> - * @pdm_bd: number of bidirectional PDM streams supported
> - * @pdm_in: number of input PDM streams supported
> - * @pdm_out: number of output PDM streams supported
>   */
>  struct sdw_cdns_stream_config {
>  	unsigned int pcm_bd;
>  	unsigned int pcm_in;
>  	unsigned int pcm_out;
> -	unsigned int pdm_bd;
> -	unsigned int pdm_in;
> -	unsigned int pdm_out;
>  };
>  
>  /**
> @@ -111,7 +105,6 @@ struct sdw_cdns_dma_data {
>   * @ports: Data ports
>   * @num_ports: Total number of data ports
>   * @pcm: PCM streams
> - * @pdm: PDM streams
>   * @registers: Cadence registers
>   * @link_up: Link status
>   * @msg_count: Messages sent on bus
> @@ -129,7 +122,6 @@ struct sdw_cdns {
>  	int num_ports;
>  
>  	struct sdw_cdns_streams pcm;
> -	struct sdw_cdns_streams pdm;
>  
>  	int pdi_loopback_source;
>  	int pdi_loopback_target;
> @@ -188,7 +180,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,
>  int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params);
>  
>  int cdns_set_sdw_stream(struct snd_soc_dai *dai,
> -			void *stream, bool pcm, int direction);
> +			void *stream, int direction);
>  
>  void sdw_cdns_check_self_clearing_bits(struct sdw_cdns *cdns, const char *string,
>  				       bool initial_delay, int reset_iterations);
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 45ea55a7d0c8..79ba0e3f6dac 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -564,7 +564,7 @@ static void intel_pdi_init(struct sdw_intel *sdw,
>  {
>  	void __iomem *shim = sdw->link_res->shim;
>  	unsigned int link_id = sdw->instance;
> -	int pcm_cap, pdm_cap;
> +	int pcm_cap;
>  
>  	/* PCM Stream Capability */
>  	pcm_cap = intel_readw(shim, SDW_SHIM_PCMSCAP(link_id));
> @@ -575,41 +575,25 @@ static void intel_pdi_init(struct sdw_intel *sdw,
>  
>  	dev_dbg(sdw->cdns.dev, "PCM cap bd:%d in:%d out:%d\n",
>  		config->pcm_bd, config->pcm_in, config->pcm_out);
> -
> -	/* PDM Stream Capability */
> -	pdm_cap = intel_readw(shim, SDW_SHIM_PDMSCAP(link_id));
> -
> -	config->pdm_bd = FIELD_GET(SDW_SHIM_PDMSCAP_BSS, pdm_cap);
> -	config->pdm_in = FIELD_GET(SDW_SHIM_PDMSCAP_ISS, pdm_cap);
> -	config->pdm_out = FIELD_GET(SDW_SHIM_PDMSCAP_OSS, pdm_cap);
> -
> -	dev_dbg(sdw->cdns.dev, "PDM cap bd:%d in:%d out:%d\n",
> -		config->pdm_bd, config->pdm_in, config->pdm_out);
>  }
>  
>  static int
> -intel_pdi_get_ch_cap(struct sdw_intel *sdw, unsigned int pdi_num, bool pcm)
> +intel_pdi_get_ch_cap(struct sdw_intel *sdw, unsigned int pdi_num)
>  {
>  	void __iomem *shim = sdw->link_res->shim;
>  	unsigned int link_id = sdw->instance;
>  	int count;
>  
> -	if (pcm) {
> -		count = intel_readw(shim, SDW_SHIM_PCMSYCHC(link_id, pdi_num));
> +	count = intel_readw(shim, SDW_SHIM_PCMSYCHC(link_id, pdi_num));
>  
> -		/*
> -		 * WORKAROUND: on all existing Intel controllers, pdi
> -		 * number 2 reports channel count as 1 even though it
> -		 * supports 8 channels. Performing hardcoding for pdi
> -		 * number 2.
> -		 */
> -		if (pdi_num == 2)
> -			count = 7;
> -
> -	} else {
> -		count = intel_readw(shim, SDW_SHIM_PDMSCAP(link_id));
> -		count = FIELD_GET(SDW_SHIM_PDMSCAP_CPSS, count);
> -	}
> +	/*
> +	 * WORKAROUND: on all existing Intel controllers, pdi
> +	 * number 2 reports channel count as 1 even though it
> +	 * supports 8 channels. Performing hardcoding for pdi
> +	 * number 2.
> +	 */
> +	if (pdi_num == 2)
> +		count = 7;
>  
>  	/* zero based values for channel count in register */
>  	count++;
> @@ -620,12 +604,12 @@ intel_pdi_get_ch_cap(struct sdw_intel *sdw, unsigned int pdi_num, bool pcm)
>  static int intel_pdi_get_ch_update(struct sdw_intel *sdw,
>  				   struct sdw_cdns_pdi *pdi,
>  				   unsigned int num_pdi,
> -				   unsigned int *num_ch, bool pcm)
> +				   unsigned int *num_ch)
>  {
>  	int i, ch_count = 0;
>  
>  	for (i = 0; i < num_pdi; i++) {
> -		pdi->ch_count = intel_pdi_get_ch_cap(sdw, pdi->num, pcm);
> +		pdi->ch_count = intel_pdi_get_ch_cap(sdw, pdi->num);
>  		ch_count += pdi->ch_count;
>  		pdi++;
>  	}
> @@ -635,25 +619,23 @@ static int intel_pdi_get_ch_update(struct sdw_intel *sdw,
>  }
>  
>  static int intel_pdi_stream_ch_update(struct sdw_intel *sdw,
> -				      struct sdw_cdns_streams *stream, bool pcm)
> +				      struct sdw_cdns_streams *stream)
>  {
>  	intel_pdi_get_ch_update(sdw, stream->bd, stream->num_bd,
> -				&stream->num_ch_bd, pcm);
> +				&stream->num_ch_bd);
>  
>  	intel_pdi_get_ch_update(sdw, stream->in, stream->num_in,
> -				&stream->num_ch_in, pcm);
> +				&stream->num_ch_in);
>  
>  	intel_pdi_get_ch_update(sdw, stream->out, stream->num_out,
> -				&stream->num_ch_out, pcm);
> +				&stream->num_ch_out);
>  
>  	return 0;
>  }
>  
>  static int intel_pdi_ch_update(struct sdw_intel *sdw)
>  {
> -	/* First update PCM streams followed by PDM streams */
> -	intel_pdi_stream_ch_update(sdw, &sdw->cdns.pcm, true);
> -	intel_pdi_stream_ch_update(sdw, &sdw->cdns.pdm, false);
> +	intel_pdi_stream_ch_update(sdw, &sdw->cdns.pcm);
>  
>  	return 0;
>  }
> @@ -840,7 +822,6 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
>  	struct sdw_port_config *pconfig;
>  	int ch, dir;
>  	int ret;
> -	bool pcm = true;
>  
>  	dma = snd_soc_dai_get_dma_data(dai, substream);
>  	if (!dma)
> @@ -852,13 +833,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
>  	else
>  		dir = SDW_DATA_DIR_TX;
>  
> -	if (dma->stream_type == SDW_STREAM_PDM)
> -		pcm = false;
> -
> -	if (pcm)
> -		pdi = sdw_cdns_alloc_pdi(cdns, &cdns->pcm, ch, dir, dai->id);
> -	else
> -		pdi = sdw_cdns_alloc_pdi(cdns, &cdns->pdm, ch, dir, dai->id);
> +	pdi = sdw_cdns_alloc_pdi(cdns, &cdns->pcm, ch, dir, dai->id);
>  
>  	if (!pdi) {
>  		ret = -EINVAL;
> @@ -888,12 +863,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
>  	sconfig.frame_rate = params_rate(params);
>  	sconfig.type = dma->stream_type;
>  
> -	if (dma->stream_type == SDW_STREAM_PDM) {
> -		sconfig.frame_rate *= 50;
> -		sconfig.bps = 1;
> -	} else {
> -		sconfig.bps = snd_pcm_format_width(params_format(params));
> -	}
> +	sconfig.bps = snd_pcm_format_width(params_format(params));
>  
>  	/* Port configuration */
>  	pconfig = kzalloc(sizeof(*pconfig), GFP_KERNEL);
> @@ -1012,13 +982,7 @@ static void intel_shutdown(struct snd_pcm_substream *substream,
>  static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai,
>  				    void *stream, int direction)
>  {
> -	return cdns_set_sdw_stream(dai, stream, true, direction);
> -}
> -
> -static int intel_pdm_set_sdw_stream(struct snd_soc_dai *dai,
> -				    void *stream, int direction)
> -{
> -	return cdns_set_sdw_stream(dai, stream, false, direction);
> +	return cdns_set_sdw_stream(dai, stream, direction);
>  }
>  
>  static void *intel_get_sdw_stream(struct snd_soc_dai *dai,
> @@ -1133,16 +1097,6 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
>  	.get_stream = intel_get_sdw_stream,
>  };
>  
> -static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
> -	.startup = intel_startup,
> -	.hw_params = intel_hw_params,
> -	.prepare = intel_prepare,
> -	.hw_free = intel_hw_free,
> -	.shutdown = intel_shutdown,
> -	.set_sdw_stream = intel_pdm_set_sdw_stream,
> -	.get_sdw_stream = intel_get_sdw_stream,
> -};
> -
>  static const struct snd_soc_component_driver dai_component = {
>  	.name           = "soundwire",
>  	.suspend	= intel_component_dais_suspend
> @@ -1151,7 +1105,7 @@ static const struct snd_soc_component_driver dai_component = {
>  static int intel_create_dai(struct sdw_cdns *cdns,
>  			    struct snd_soc_dai_driver *dais,
>  			    enum intel_pdi_type type,
> -			    u32 num, u32 off, u32 max_ch, bool pcm)
> +			    u32 num, u32 off, u32 max_ch)
>  {
>  	int i;
>  
> @@ -1180,10 +1134,7 @@ static int intel_create_dai(struct sdw_cdns *cdns,
>  			dais[i].capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
>  		}
>  
> -		if (pcm)
> -			dais[i].ops = &intel_pcm_dai_ops;
> -		else
> -			dais[i].ops = &intel_pdm_dai_ops;
> +		dais[i].ops = &intel_pcm_dai_ops;
>  	}
>  
>  	return 0;
> @@ -1197,7 +1148,7 @@ static int intel_register_dai(struct sdw_intel *sdw)
>  	int num_dai, ret, off = 0;
>  
>  	/* DAIs are created based on total number of PDIs supported */
> -	num_dai = cdns->pcm.num_pdi + cdns->pdm.num_pdi;
> +	num_dai = cdns->pcm.num_pdi;
>  
>  	dais = devm_kcalloc(cdns->dev, num_dai, sizeof(*dais), GFP_KERNEL);
>  	if (!dais)
> @@ -1207,39 +1158,19 @@ static int intel_register_dai(struct sdw_intel *sdw)
>  	stream = &cdns->pcm;
>  
>  	ret = intel_create_dai(cdns, dais, INTEL_PDI_IN, cdns->pcm.num_in,
> -			       off, stream->num_ch_in, true);
> +			       off, stream->num_ch_in);
>  	if (ret)
>  		return ret;
>  
>  	off += cdns->pcm.num_in;
>  	ret = intel_create_dai(cdns, dais, INTEL_PDI_OUT, cdns->pcm.num_out,
> -			       off, stream->num_ch_out, true);
> +			       off, stream->num_ch_out);
>  	if (ret)
>  		return ret;
>  
>  	off += cdns->pcm.num_out;
>  	ret = intel_create_dai(cdns, dais, INTEL_PDI_BD, cdns->pcm.num_bd,
> -			       off, stream->num_ch_bd, true);
> -	if (ret)
> -		return ret;
> -
> -	/* Create PDM DAIs */
> -	stream = &cdns->pdm;
> -	off += cdns->pcm.num_bd;
> -	ret = intel_create_dai(cdns, dais, INTEL_PDI_IN, cdns->pdm.num_in,
> -			       off, stream->num_ch_in, false);
> -	if (ret)
> -		return ret;
> -
> -	off += cdns->pdm.num_in;
> -	ret = intel_create_dai(cdns, dais, INTEL_PDI_OUT, cdns->pdm.num_out,
> -			       off, stream->num_ch_out, false);
> -	if (ret)
> -		return ret;
> -
> -	off += cdns->pdm.num_out;
> -	ret = intel_create_dai(cdns, dais, INTEL_PDI_BD, cdns->pdm.num_bd,
> -			       off, stream->num_ch_bd, false);
> +			       off, stream->num_ch_bd);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.17.1

-- 
~Vinod

Powered by blists - more mailing lists