Message-ID: <d415817f-d32c-07bb-4d9a-41649290a559@microchip.com>
Date:   Mon, 3 Aug 2020 16:11:54 +0000
From:   <Codrin.Ciubotariu@...rochip.com>
To:     <Claudiu.Beznea@...rochip.com>, <alsa-devel@...a-project.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
CC:     <alexandre.belloni@...tlin.com>, <lgirdwood@...il.com>,
        <robh+dt@...nel.org>, <tiwai@...e.com>,
        <Ludovic.Desroches@...rochip.com>, <broonie@...nel.org>,
        <perex@...ex.cz>
Subject: Re: [PATCH v3 2/2] ASoC: mchp-spdiftx: add driver for S/PDIF TX
 Controller

On 03.08.2020 16:06, Claudiu Beznea - M18063 wrote:
> 
> 
> On 03.08.2020 11:18, Codrin Ciubotariu wrote:
>> The new SPDIF TX controller is a serial port compliant with the IEC-
>> 60958 standard. It also supports programmable User Data and Channel
>> Status fields.
>>
>> This IP is embedded in Microchip's sama7g5 SoC.
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>
>> ---
>>
>> Changes in v2, v3:
>>   - none;
>>
>>   sound/soc/atmel/Kconfig        |  12 +
>>   sound/soc/atmel/Makefile       |   2 +
>>   sound/soc/atmel/mchp-spdiftx.c | 864 +++++++++++++++++++++++++++++++++
>>   3 files changed, 878 insertions(+)
>>   create mode 100644 sound/soc/atmel/mchp-spdiftx.c
>>
>> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
>> index 71f2d42188c4..93beb7d670a3 100644
>> --- a/sound/soc/atmel/Kconfig
>> +++ b/sound/soc/atmel/Kconfig
>> @@ -132,4 +132,16 @@ config SND_MCHP_SOC_I2S_MCC
>>   	  and supports a Time Division Multiplexed (TDM) interface with
>>   	  external multi-channel audio codecs.
>>   
>> +config SND_MCHP_SOC_SPDIFTX
>> +	tristate "Microchip ASoC driver for boards using S/PDIF TX"
>> +	depends on OF && (ARCH_AT91 || COMPILE_TEST)
>> +	select SND_SOC_GENERIC_DMAENGINE_PCM
>> +	select REGMAP_MMIO
>> +	help
>> +	  Say Y or M if you want to add support for Microchip S/PDIF TX ASoc
>> +	  driver on the following Microchip platforms:
>> +	  - sama7g5
>> +
>> +	  This S/PDIF TX driver is compliant with IEC-60958 standard and
>> +	  includes programable User Data and Channel Status fields.
>>   endif
>> diff --git a/sound/soc/atmel/Makefile b/sound/soc/atmel/Makefile
>> index c7d2989791be..3fd89a0063df 100644
>> --- a/sound/soc/atmel/Makefile
>> +++ b/sound/soc/atmel/Makefile
>> @@ -5,6 +5,7 @@ snd-soc-atmel-pcm-dma-objs := atmel-pcm-dma.o
>>   snd-soc-atmel_ssc_dai-objs := atmel_ssc_dai.o
>>   snd-soc-atmel-i2s-objs := atmel-i2s.o
>>   snd-soc-mchp-i2s-mcc-objs := mchp-i2s-mcc.o
>> +snd-soc-mchp-spdiftx-objs := mchp-spdiftx.o
>>   
>>   # pdc and dma need to both be built-in if any user of
>>   # ssc is built-in.
>> @@ -17,6 +18,7 @@ endif
>>   obj-$(CONFIG_SND_ATMEL_SOC_SSC) += snd-soc-atmel_ssc_dai.o
>>   obj-$(CONFIG_SND_ATMEL_SOC_I2S) += snd-soc-atmel-i2s.o
>>   obj-$(CONFIG_SND_MCHP_SOC_I2S_MCC) += snd-soc-mchp-i2s-mcc.o
>> +obj-$(CONFIG_SND_MCHP_SOC_SPDIFTX) += snd-soc-mchp-spdiftx.o
>>   
>>   # AT91 Machine Support
>>   snd-soc-sam9g20-wm8731-objs := sam9g20_wm8731.o
>> diff --git a/sound/soc/atmel/mchp-spdiftx.c b/sound/soc/atmel/mchp-spdiftx.c
>> new file mode 100644
>> index 000000000000..738f6788212e
>> --- /dev/null
>> +++ b/sound/soc/atmel/mchp-spdiftx.c
>> @@ -0,0 +1,864 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Driver for Microchip S/PDIF TX Controller
>> +//
>> +// Copyright (C) 2020 Microchip Technology Inc. and its subsidiaries
>> +//
>> +// Author: Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/spinlock.h>
>> +
>> +#include <sound/asoundef.h>
>> +#include <sound/dmaengine_pcm.h>
>> +#include <sound/pcm_params.h>
>> +#include <sound/soc.h>
>> +
>> +/*
>> + * ---- S/PDIF Transmitter Controller Register map ----
>> + */
>> +#define SPDIFTX_CR			0x00	/* Control Register */
>> +#define SPDIFTX_MR			0x04	/* Mode Register */
> 
> This register is read/write either in atomic and non-atomic contextes but
> not protected everywhere the same way.

I only need the TXEN bit from this register in an atomic context, this 
is why there are also non-atomic contexts.

> 
>> +#define SPDIFTX_CDR			0x0C	/* Common Data Register */
>> +
>> +#define SPDIFTX_IER			0x14	/* Interrupt Enable Register */
>> +#define SPDIFTX_IDR			0x18	/* Interrupt Disable Register */
>> +#define SPDIFTX_IMR			0x1C	/* Interrupt Mask Register */
>> +#define SPDIFTX_ISR			0x20	/* Interrupt Status Register */
>> +
>> +#define SPDIFTX_CH1UD(reg)	(0x50 + (reg) * 4)	/* User Data 1 Register x */
>> +#define SPDIFTX_CH1S(reg)	(0x80 + (reg) * 4)	/* Channel Status 1 Register x */
>> +
>> +#define SPDIFTX_VERSION			0xF0
>> +
>> +/*
>> + * ---- Control Register (Write-only) ----
>> + */
>> +#define SPDIFTX_CR_SWRST		BIT(0)	/* Software Reset */
>> +#define SPDIFTX_CR_FCLR			BIT(1)	/* FIFO clear */
>> +
>> +/*
>> + * ---- Mode Register (Read/Write) ----
>> + */
>> +/* Transmit Enable */
>> +#define SPDIFTX_MR_TXEN_MASK		GENMASK(0, 0)
>> +#define SPDIFTX_MR_TXEN_DISABLE		(0 << 0)
>> +#define SPDIFTX_MR_TXEN_ENABLE		(1 << 0)
>> +
>> +/* Multichannel Transfer */
>> +#define SPDIFTX_MR_MULTICH_MASK		GENAMSK(1, 1)
>> +#define SPDIFTX_MR_MULTICH_MONO		(0 << 1)
>> +#define SPDIFTX_MR_MULTICH_DUAL		(1 << 1)
>> +
>> +/* Data Word Endian Mode */
>> +#define SPDIFTX_MR_ENDIAN_MASK		GENMASK(2, 2)
>> +#define SPDIFTX_MR_ENDIAN_LITTLE	(0 << 2)
>> +#define SPDIFTX_MR_ENDIAN_BIG		(1 << 2)
>> +
>> +/* Data Justification */
>> +#define SPDIFTX_MR_JUSTIFY_MASK		GENMASK(3, 3)
>> +#define SPDIFTX_MR_JUSTIFY_LSB		(0 << 3)
>> +#define SPDIFTX_MR_JUSTIFY_MSB		(1 << 3)
>> +
>> +/* Common Audio Register Transfer Mode */
>> +#define SPDIFTX_MR_CMODE_MASK			GENMASK(5, 4)
>> +#define SPDIFTX_MR_CMODE_INDEX_ACCESS		(0 << 4)
>> +#define SPDIFTX_MR_CMODE_TOGGLE_ACCESS		(1 << 4)
>> +#define SPDIFTX_MR_CMODE_INTERLVD_ACCESS	(2 << 4)
>> +
>> +/* Valid Bits per Sample */
>> +#define SPDIFTX_MR_VBPS_MASK		GENMASK(13, 8)
>> +#define SPDIFTX_MR_VBPS(bps)		(((bps) << 8) & SPDIFTX_MR_VBPS_MASK)
>> +
>> +/* Chunk Size */
>> +#define SPDIFTX_MR_CHUNK_MASK		GENMASK(19, 16)
>> +#define SPDIFTX_MR_CHUNK(size)		(((size) << 16) & SPDIFTX_MR_CHUNK_MASK)
>> +
>> +/* Validity Bits for Channels 1 and 2 */
>> +#define SPDIFTX_MR_VALID1			BIT(24)
>> +#define SPDIFTX_MR_VALID2			BIT(25)
>> +
>> +/* Disable Null Frame on underrrun */
>> +#define SPDIFTX_MR_DNFR_MASK		GENMASK(27, 27)
>> +#define SPDIFTX_MR_DNFR_INVALID		(0 << 27)
>> +#define SPDIFTX_MR_DNFR_VALID		(1 << 27)
>> +
>> +/* Bytes per Sample */
>> +#define SPDIFTX_MR_BPS_MASK		GENMASK(29, 28)
>> +#define SPDIFTX_MR_BPS(bytes) \
>> +	((((bytes) - 1) << 28) & SPDIFTX_MR_BPS_MASK)
>> +
>> +/*
>> + * ---- Interrupt Enable/Disable/Mask/Status Register (Write/Read-only) ----
>> + */
>> +#define SPDIFTX_IR_TXRDY		BIT(0)
>> +#define SPDIFTX_IR_TXEMPTY		BIT(1)
>> +#define SPDIFTX_IR_TXFULL		BIT(2)
>> +#define SPDIFTX_IR_TXCHUNK		BIT(3)
>> +#define SPDIFTX_IR_TXUDR		BIT(4)
>> +#define SPDIFTX_IR_TXOVR		BIT(5)
>> +#define SPDIFTX_IR_CSRDY		BIT(6)
>> +#define SPDIFTX_IR_UDRDY		BIT(7)
>> +#define SPDIFTX_IR_TXRDYCH(ch)		BIT((ch) + 8)
>> +#define SPDIFTX_IR_SECE			BIT(10)
>> +#define SPDIFTX_IR_TXUDRCH(ch)		BIT((ch) + 11)
>> +#define SPDIFTX_IR_BEND			BIT(13)
>> +
>> +static bool mchp_spdiftx_readable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	switch (reg) {
>> +	case SPDIFTX_MR:
>> +	case SPDIFTX_IMR:
>> +	case SPDIFTX_ISR:
>> +	case SPDIFTX_CH1UD(0):
>> +	case SPDIFTX_CH1UD(1):
>> +	case SPDIFTX_CH1UD(2):
>> +	case SPDIFTX_CH1UD(3):
>> +	case SPDIFTX_CH1UD(4):
>> +	case SPDIFTX_CH1UD(5):
>> +	case SPDIFTX_CH1S(0):
>> +	case SPDIFTX_CH1S(1):
>> +	case SPDIFTX_CH1S(2):
>> +	case SPDIFTX_CH1S(3):
>> +	case SPDIFTX_CH1S(4):
>> +	case SPDIFTX_CH1S(5):
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static bool mchp_spdiftx_writeable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	switch (reg) {
>> +	case SPDIFTX_CR:
>> +	case SPDIFTX_MR:
>> +	case SPDIFTX_CDR:
>> +	case SPDIFTX_IER:
>> +	case SPDIFTX_IDR:
>> +	case SPDIFTX_CH1UD(0):
>> +	case SPDIFTX_CH1UD(1):
>> +	case SPDIFTX_CH1UD(2):
>> +	case SPDIFTX_CH1UD(3):
>> +	case SPDIFTX_CH1UD(4):
>> +	case SPDIFTX_CH1UD(5):
>> +	case SPDIFTX_CH1S(0):
>> +	case SPDIFTX_CH1S(1):
>> +	case SPDIFTX_CH1S(2):
>> +	case SPDIFTX_CH1S(3):
>> +	case SPDIFTX_CH1S(4):
>> +	case SPDIFTX_CH1S(5):
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static bool mchp_spdiftx_precious_reg(struct device *dev, unsigned int reg)
>> +{
>> +	switch (reg) {
>> +	case SPDIFTX_CDR:
>> +	case SPDIFTX_ISR:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static const struct regmap_config mchp_spdiftx_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
>> +	.max_register = SPDIFTX_VERSION,
>> +	.readable_reg = mchp_spdiftx_readable_reg,
>> +	.writeable_reg = mchp_spdiftx_writeable_reg,
>> +	.precious_reg = mchp_spdiftx_precious_reg,
>> +};
>> +
>> +#define SPDIFTX_GCLK_RATIO	128
>> +
>> +#define SPDIFTX_CS_BITS		192
>> +#define SPDIFTX_UD_BITS		192
>> +
>> +struct mchp_spdiftx_mixer_control {
>> +	unsigned char				ch_stat[SPDIFTX_CS_BITS / 8];
>> +	unsigned char				user_data[SPDIFTX_UD_BITS / 8];
>> +	spinlock_t				lock;
>> +};
>> +
>> +struct mchp_spdiftx_dev {
>> +	struct mchp_spdiftx_mixer_control	control;
>> +	struct snd_dmaengine_dai_dma_data	playback;
>> +	struct device				*dev;
>> +	struct regmap				*regmap;
>> +	struct clk				*pclk;
>> +	struct clk				*gclk;
>> +	unsigned int				fmt;
>> +	const struct mchp_i2s_caps		*caps;
>> +	int					gclk_enabled:1;
>> +};
>> +
>> +static inline int mchp_spdiftx_is_running(struct mchp_spdiftx_dev *dev)
>> +{
>> +	u32 mr;
>> +
>> +	regmap_read(dev->regmap, SPDIFTX_MR, &mr);
>> +	return !!(mr & SPDIFTX_MR_TXEN_ENABLE);
>> +}
>> +
>> +static void mchp_spdiftx_channel_status_write(struct mchp_spdiftx_dev *dev)
>> +{
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	u32 val;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ctrl->ch_stat) / 4; i++) {
>> +		val = (ctrl->ch_stat[(i * 4) + 0] << 0) |
>> +		      (ctrl->ch_stat[(i * 4) + 1] << 8) |
>> +		      (ctrl->ch_stat[(i * 4) + 2] << 16) |
>> +		      (ctrl->ch_stat[(i * 4) + 3] << 24);
>> +
>> +		regmap_write(dev->regmap, SPDIFTX_CH1S(i), val);
>> +	}
>> +}
>> +
>> +static void mchp_spdiftx_user_data_write(struct mchp_spdiftx_dev *dev)
>> +{
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	u32 val;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ctrl->user_data) / 4; i++) {
>> +		val = (ctrl->user_data[(i * 4) + 0] << 0) |
>> +		      (ctrl->user_data[(i * 4) + 1] << 8) |
>> +		      (ctrl->user_data[(i * 4) + 2] << 16) |
>> +		      (ctrl->user_data[(i * 4) + 3] << 24);
>> +
>> +		regmap_write(dev->regmap, SPDIFTX_CH1UD(i), val);
>> +	}
>> +}
>> +
>> +static irqreturn_t mchp_spdiftx_interrupt(int irq, void *dev_id)
>> +{
>> +	struct mchp_spdiftx_dev *dev = dev_id;
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	u32 sr, imr, pending, idr = 0;
>> +
>> +	regmap_read(dev->regmap, SPDIFTX_ISR, &sr);
>> +	regmap_read(dev->regmap, SPDIFTX_IMR, &imr);
>> +	pending = sr & imr;
>> +
>> +	if (!pending)
>> +		return IRQ_NONE;
>> +
>> +	if (pending & SPDIFTX_IR_TXUDR) {
>> +		dev_warn(dev->dev, "underflow detected\n");
>> +		idr |= SPDIFTX_IR_TXUDR;
>> +	}
>> +
>> +	if (pending & SPDIFTX_IR_TXOVR) {
>> +		dev_warn(dev->dev, "overflow detected\n");
>> +		idr |= SPDIFTX_IR_TXOVR;
>> +	}
>> +
>> +	if (pending & SPDIFTX_IR_UDRDY) {
>> +		spin_lock(&ctrl->lock);
>> +		mchp_spdiftx_user_data_write(dev);
>> +		spin_unlock(&ctrl->lock);
>> +		idr |= SPDIFTX_IR_UDRDY;
>> +	}
>> +
>> +	if (pending & SPDIFTX_IR_CSRDY) {
>> +		spin_lock(&ctrl->lock);
>> +		mchp_spdiftx_channel_status_write(dev);
>> +		spin_unlock(&ctrl->lock);
>> +		idr |= SPDIFTX_IR_CSRDY;
>> +	}
>> +
>> +	regmap_write(dev->regmap, SPDIFTX_IDR, idr);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int mchp_spdiftx_dai_startup(struct snd_pcm_substream *substream,
>> +				    struct snd_soc_dai *dai)
>> +{
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +	int err;
>> +
>> +	err = clk_prepare_enable(dev->pclk);
>> +	if (err) {
>> +		dev_err(dev->dev,
>> +			"failed to enable the peripheral clock: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* Software reset the IP */
>> +	regmap_write(dev->regmap, SPDIFTX_CR,
>> +		     SPDIFTX_CR_SWRST | SPDIFTX_CR_FCLR);
>> +
>> +	return 0;
>> +}
>> +
>> +static void mchp_spdiftx_dai_shutdown(struct snd_pcm_substream *substream,
>> +				      struct snd_soc_dai *dai)
>> +{
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +
>> +	/* Disable interrupts */
>> +	regmap_write(dev->regmap, SPDIFTX_IDR, 0xffffffff);
>> +
>> +	clk_disable_unprepare(dev->pclk);
>> +}
>> +
>> +static int mchp_spdiftx_trigger(struct snd_pcm_substream *substream, int cmd,
>> +				struct snd_soc_dai *dai)
>> +{
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	u32 mr;
>> +	int running;
>> +	int ret;
>> +
>> +	/* do not start/stop while channel status or user data is updated */
>> +	spin_lock(&ctrl->lock);
>> +	regmap_read(dev->regmap, SPDIFTX_MR, &mr);
> 
> Here, atomic, for instance.

This is where I check and change for TXEN. The IP must not be started 
while the userspace updates the SPDIFTX_CH1UDx or SPDIFTX_CH1Sx registers.

> 
>> +	running = !!(mr & SPDIFTX_MR_TXEN_ENABLE);
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +		if (!running) {
>> +			mr &= ~SPDIFTX_MR_TXEN_MASK;
>> +			mr |= SPDIFTX_MR_TXEN_ENABLE;
>> +		}
>> +		break;
>> +	case SNDRV_PCM_TRIGGER_STOP:
>> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> +		if (running) {
>> +			mr &= ~SPDIFTX_MR_TXEN_MASK;
>> +			mr |= SPDIFTX_MR_TXEN_DISABLE;
>> +		}
>> +		break;
>> +	default:
>> +		spin_unlock(&ctrl->lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = regmap_write(dev->regmap, SPDIFTX_MR, mr);
>> +	spin_unlock(&ctrl->lock);
>> +	if (ret) {
>> +		dev_err(dev->dev, "unable to disable TX: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_spdiftx_hw_params(struct snd_pcm_substream *substream,
>> +				  struct snd_pcm_hw_params *params,
>> +				  struct snd_soc_dai *dai)
>> +{
>> +	unsigned long flags;
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	u32 mr;
>> +	unsigned int bps = params_physical_width(params) / 8;
>> +	int ret;
>> +
>> +	dev_dbg(dev->dev, "%s() rate=%u format=%#x width=%u channels=%u\n",
>> +		__func__, params_rate(params), params_format(params),
>> +		params_width(params), params_channels(params));
>> +
>> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
>> +		dev_err(dev->dev, "Capture is not supported\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	regmap_read(dev->regmap, SPDIFTX_MR, &mr);
> 
> Here non-atomic.

TXEN is not toutched here and hw_params() and trigger() callbacks can't 
be called at the same time. The concurency can be only with the controls 
functions, who don't touch the SPDIFTX_MR register at all.

> 
>> +
>> +	if (mr & SPDIFTX_MR_TXEN_ENABLE) {
>> +		dev_err(dev->dev, "PCM already running\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	/* Defaults: Toggle mode, justify to LSB, chunksize 1 */
>> +	mr = SPDIFTX_MR_CMODE_TOGGLE_ACCESS | SPDIFTX_MR_JUSTIFY_LSB;
>> +	dev->playback.maxburst = 1;
>> +	switch (params_channels(params)) {
>> +	case 1:
>> +		mr |= SPDIFTX_MR_MULTICH_MONO;
>> +		break;
>> +	case 2:
>> +		mr |= SPDIFTX_MR_MULTICH_DUAL;
>> +		if (bps > 2)
>> +			dev->playback.maxburst = 2;
>> +		break;
>> +	default:
>> +		dev_err(dev->dev, "unsupported number of channels: %d\n",
>> +			params_channels(params));
>> +		return -EINVAL;
>> +	}
>> +	mr |= SPDIFTX_MR_CHUNK(dev->playback.maxburst);
>> +
>> +	switch (params_format(params)) {
>> +	case SNDRV_PCM_FORMAT_S8:
>> +		mr |= SPDIFTX_MR_VBPS(8);
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S16_BE:
>> +		mr |= SPDIFTX_MR_ENDIAN_BIG;
>> +		fallthrough;
>> +	case SNDRV_PCM_FORMAT_S16_LE:
>> +		mr |= SPDIFTX_MR_VBPS(16);
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S18_3BE:
>> +		mr |= SPDIFTX_MR_ENDIAN_BIG;
>> +		fallthrough;
>> +	case SNDRV_PCM_FORMAT_S18_3LE:
>> +		mr |= SPDIFTX_MR_VBPS(18);
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S20_3BE:
>> +		mr |= SPDIFTX_MR_ENDIAN_BIG;
>> +		fallthrough;
>> +	case SNDRV_PCM_FORMAT_S20_3LE:
>> +		mr |= SPDIFTX_MR_VBPS(20);
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S24_3BE:
>> +		mr |= SPDIFTX_MR_ENDIAN_BIG;
>> +		fallthrough;
>> +	case SNDRV_PCM_FORMAT_S24_3LE:
>> +		mr |= SPDIFTX_MR_VBPS(24);
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S24_BE:
>> +		mr |= SPDIFTX_MR_ENDIAN_BIG;
>> +		fallthrough;
>> +	case SNDRV_PCM_FORMAT_S24_LE:
>> +		mr |= SPDIFTX_MR_VBPS(24);
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S32_BE:
>> +		mr |= SPDIFTX_MR_ENDIAN_BIG;
>> +		fallthrough;
>> +	case SNDRV_PCM_FORMAT_S32_LE:
>> +		mr |= SPDIFTX_MR_VBPS(32);
>> +		break;
>> +	default:
>> +		dev_err(dev->dev, "unsupported PCM format: %d\n",
>> +			params_format(params));
>> +		return -EINVAL;
>> +	}
>> +
>> +	mr |= SPDIFTX_MR_BPS(bps);
>> +
>> +	spin_lock_irqsave(&ctrl->lock, flags);
>> +	ctrl->ch_stat[3] &= ~IEC958_AES3_CON_FS;
>> +	switch (params_rate(params)) {
>> +	case 22050:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_22050;
>> +		break;
>> +	case 24000:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_24000;
>> +		break;
>> +	case 32000:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_32000;
>> +		break;
>> +	case 44100:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_44100;
>> +		break;
>> +	case 48000:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_48000;
>> +		break;
>> +	case 88200:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_88200;
>> +		break;
>> +	case 96000:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_96000;
>> +		break;
>> +	case 176400:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_176400;
>> +		break;
>> +	case 192000:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_192000;
>> +		break;
>> +	case 8000:
>> +	case 11025:
>> +	case 16000:
>> +	case 64000:
>> +		ctrl->ch_stat[3] |= IEC958_AES3_CON_FS_NOTID;
>> +		break;
>> +	default:
>> +		dev_err(dev->dev, "unsupported sample frequency: %u\n",
>> +			params_rate(params));
>> +		spin_unlock_irqrestore(&ctrl->lock, flags);
>> +		return -EINVAL;
>> +	}
>> +	mchp_spdiftx_channel_status_write(dev);
>> +	spin_unlock_irqrestore(&ctrl->lock, flags);
>> +	mr |= SPDIFTX_MR_VALID1 | SPDIFTX_MR_VALID2;
>> +
>> +	if (dev->gclk_enabled) {
>> +		clk_disable_unprepare(dev->gclk);
>> +		dev->gclk_enabled = 0;
>> +	}
>> +	ret = clk_set_rate(dev->gclk, params_rate(params) *
>> +				      SPDIFTX_GCLK_RATIO);
>> +	if (ret) {
>> +		dev_err(dev->dev,
>> +			"unable to change gclk rate to: rate %u * ratio %u\n",
>> +			params_rate(params), SPDIFTX_GCLK_RATIO);
>> +		return ret;
>> +	}
>> +	ret = clk_prepare_enable(dev->gclk);
>> +	if (ret) {
>> +		dev_err(dev->dev, "unable to enable gclk: %d\n", ret);
>> +		return ret;
>> +	}
>> +	dev->gclk_enabled = 1;
>> +	dev_dbg(dev->dev, "%s(): GCLK set to %d\n", __func__,
>> +		params_rate(params) * SPDIFTX_GCLK_RATIO);
>> +
>> +	/* Enable interrupts */
>> +	regmap_write(dev->regmap, SPDIFTX_IER,
>> +		     SPDIFTX_IR_TXUDR | SPDIFTX_IR_TXOVR);
>> +
>> +	regmap_write(dev->regmap, SPDIFTX_MR, mr);
> 
> Same here.

I explained above. Even if MR register is changed here, the TXEN bit is 
not changed and the previous value is kept.

The purpose of this lock is to assure that the IP won't change its state 
(TXEN changed) while SPDIFTX_CH1UDx or SPDIFTX_CH1Sx registers are 
updated. The lock is not to protect all the values from the MR register. 
If you found a case in which this is not achieved, please let me know.

Thank you for your review!

Best regards,
Codrin

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_spdiftx_hw_free(struct snd_pcm_substream *substream,
>> +				struct snd_soc_dai *dai)
>> +{
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +
>> +	regmap_write(dev->regmap, SPDIFTX_IDR,
>> +		     SPDIFTX_IR_TXUDR | SPDIFTX_IR_TXOVR);
>> +	if (dev->gclk_enabled) {
>> +		clk_disable_unprepare(dev->gclk);
>> +		dev->gclk_enabled = 0;
>> +	}
>> +
>> +	return regmap_write(dev->regmap, SPDIFTX_CR,
>> +			    SPDIFTX_CR_SWRST | SPDIFTX_CR_FCLR);
>> +}
>> +
>> +
>> +static const struct snd_soc_dai_ops mchp_spdiftx_dai_ops = {
>> +	.startup	= mchp_spdiftx_dai_startup,
>> +	.shutdown	= mchp_spdiftx_dai_shutdown,
>> +	.trigger	= mchp_spdiftx_trigger,
>> +	.hw_params	= mchp_spdiftx_hw_params,
>> +	.hw_free	= mchp_spdiftx_hw_free,
>> +};
>> +
>> +#define MCHP_SPDIFTX_RATES	SNDRV_PCM_RATE_8000_192000
>> +
>> +#define MCHP_SPDIFTX_FORMATS	(SNDRV_PCM_FMTBIT_S8 |		\
>> +				 SNDRV_PCM_FMTBIT_S16_LE |	\
>> +				 SNDRV_PCM_FMTBIT_U16_BE |	\
>> +				 SNDRV_PCM_FMTBIT_S18_3LE |	\
>> +				 SNDRV_PCM_FMTBIT_S18_3BE |	\
>> +				 SNDRV_PCM_FMTBIT_S20_3LE |	\
>> +				 SNDRV_PCM_FMTBIT_S20_3BE |	\
>> +				 SNDRV_PCM_FMTBIT_S24_3LE |	\
>> +				 SNDRV_PCM_FMTBIT_S24_3BE |	\
>> +				 SNDRV_PCM_FMTBIT_S24_LE |	\
>> +				 SNDRV_PCM_FMTBIT_S24_BE |	\
>> +				 SNDRV_PCM_FMTBIT_S32_LE |	\
>> +				 SNDRV_PCM_FMTBIT_S32_BE	\
>> +				 )
>> +
>> +static int mchp_spdiftx_info(struct snd_kcontrol *kcontrol,
>> +			     struct snd_ctl_elem_info *uinfo)
>> +{
>> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
>> +	uinfo->count = 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_spdiftx_cs_get(struct snd_kcontrol *kcontrol,
>> +			       struct snd_ctl_elem_value *uvalue)
>> +{
>> +	unsigned long flags;
>> +	struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol);
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +
>> +	spin_lock_irqsave(&ctrl->lock, flags);
>> +	memcpy(uvalue->value.iec958.status, ctrl->ch_stat,
>> +	       sizeof(ctrl->ch_stat));
>> +	spin_unlock_irqrestore(&ctrl->lock, flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_spdiftx_cs_put(struct snd_kcontrol *kcontrol,
>> +			       struct snd_ctl_elem_value *uvalue)
>> +{
>> +	unsigned long flags;
>> +	struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol);
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	int changed = 0;
>> +	int i;
>> +
>> +	spin_lock_irqsave(&ctrl->lock, flags);
>> +	for (i = 0; i < ARRAY_SIZE(ctrl->ch_stat); i++) {
>> +		if (ctrl->ch_stat[i] != uvalue->value.iec958.status[i])
>> +			changed = 1;
>> +		ctrl->ch_stat[i] = uvalue->value.iec958.status[i];
>> +	}
>> +
>> +	if (changed) {
>> +		/* don't enable IP while we copy the channel status */
>> +		if (mchp_spdiftx_is_running(dev)) {
>> +			/*
>> +			 * if SPDIF is running, wait for interrupt to write
>> +			 * channel status
>> +			 */
>> +			regmap_write(dev->regmap, SPDIFTX_IER,
>> +				     SPDIFTX_IR_CSRDY);
>> +		} else {
>> +			mchp_spdiftx_channel_status_write(dev);
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&ctrl->lock, flags);
>> +
>> +	return changed;
>> +}
>> +
>> +static int mchp_spdiftx_cs_mask(struct snd_kcontrol *kcontrol,
>> +				struct snd_ctl_elem_value *uvalue)
>> +{
>> +	memset(uvalue->value.iec958.status, 0xff,
>> +	       sizeof(uvalue->value.iec958.status));
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_spdiftx_subcode_get(struct snd_kcontrol *kcontrol,
>> +				    struct snd_ctl_elem_value *uvalue)
>> +{
>> +	struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol);
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ctrl->lock, flags);
>> +	memcpy(uvalue->value.iec958.subcode, ctrl->user_data,
>> +	       sizeof(ctrl->user_data));
>> +	spin_unlock_irqrestore(&ctrl->lock, flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mchp_spdiftx_subcode_put(struct snd_kcontrol *kcontrol,
>> +				    struct snd_ctl_elem_value *uvalue)
>> +{
>> +	unsigned long flags;
>> +	struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol);
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +	struct mchp_spdiftx_mixer_control *ctrl = &dev->control;
>> +	int changed = 0;
>> +	int i;
>> +
>> +	spin_lock_irqsave(&ctrl->lock, flags);
>> +	for (i = 0; i < ARRAY_SIZE(ctrl->user_data); i++) {
>> +		if (ctrl->user_data[i] != uvalue->value.iec958.subcode[i])
>> +			changed = 1;
>> +
>> +		ctrl->user_data[i] = uvalue->value.iec958.subcode[i];
>> +	}
>> +	if (changed) {
>> +		if (mchp_spdiftx_is_running(dev)) {
>> +			/*
>> +			 * if SPDIF is running, wait for interrupt to write
>> +			 * user data
>> +			 */
>> +			regmap_write(dev->regmap, SPDIFTX_IER,
>> +				     SPDIFTX_IR_UDRDY);
>> +		} else {
>> +			mchp_spdiftx_user_data_write(dev);
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&ctrl->lock, flags);
>> +
>> +	return changed;
>> +}
>> +
>> +static struct snd_kcontrol_new mchp_spdiftx_ctrls[] = {
>> +	/* Channel status controller */
>> +	{
>> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
>> +		.name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT),
>> +		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
>> +			SNDRV_CTL_ELEM_ACCESS_VOLATILE,
>> +		.info = mchp_spdiftx_info,
>> +		.get = mchp_spdiftx_cs_get,
>> +		.put = mchp_spdiftx_cs_put,
>> +	},
>> +	{
>> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
>> +		.name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, MASK),
>> +		.access = SNDRV_CTL_ELEM_ACCESS_READ,
>> +			SNDRV_CTL_ELEM_ACCESS_VOLATILE,
>> +		.info = mchp_spdiftx_info,
>> +		.get = mchp_spdiftx_cs_mask,
>> +	},
>> +	/* User bits controller */
>> +	{
>> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
>> +		.name = "IEC958 Subcode Playback Default",
>> +		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
>> +		.info = mchp_spdiftx_info,
>> +		.get = mchp_spdiftx_subcode_get,
>> +		.put = mchp_spdiftx_subcode_put,
>> +	},
>> +};
>> +
>> +static int mchp_spdiftx_dai_probe(struct snd_soc_dai *dai)
>> +{
>> +	struct mchp_spdiftx_dev *dev = snd_soc_dai_get_drvdata(dai);
>> +
>> +	snd_soc_dai_init_dma_data(dai, &dev->playback, NULL);
>> +
>> +	/* Add controls */
>> +	snd_soc_add_dai_controls(dai, mchp_spdiftx_ctrls,
>> +				 ARRAY_SIZE(mchp_spdiftx_ctrls));
>> +
>> +	return 0;
>> +}
>> +
>> +static struct snd_soc_dai_driver mchp_spdiftx_dai = {
>> +	.name = "mchp-spdiftx",
>> +	.probe	= mchp_spdiftx_dai_probe,
>> +	.playback = {
>> +		.stream_name = "S/PDIF TX Playback",
>> +		.channels_min = 1,
>> +		.channels_max = 2,
>> +		.rates = MCHP_SPDIFTX_RATES,
>> +		.formats = MCHP_SPDIFTX_FORMATS,
>> +	},
>> +	.ops = &mchp_spdiftx_dai_ops,
>> +};
>> +
>> +static const struct snd_soc_component_driver mchp_spdiftx_component = {
>> +	.name		= "mchp-spdiftx",
>> +};
>> +
>> +static const struct of_device_id mchp_spdiftx_dt_ids[] = {
>> +	{
>> +		.compatible = "microchip,sama7g5-spdiftx",
>> +	},
>> +	{ /* sentinel */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, mchp_spdiftx_dt_ids);
>> +static int mchp_spdiftx_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	const struct of_device_id *match;
>> +	struct mchp_spdiftx_dev *dev;
>> +	struct resource *mem;
>> +	struct regmap *regmap;
>> +	void __iomem *base;
>> +	struct mchp_spdiftx_mixer_control *ctrl;
>> +	int irq;
>> +	int err;
>> +
>> +	/* Get memory for driver data. */
>> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> +	if (!dev)
>> +		return -ENOMEM;
>> +
>> +	/* Get hardware capabilities. */
>> +	match = of_match_node(mchp_spdiftx_dt_ids, np);
>> +	if (match)
>> +		dev->caps = match->data;
>> +
>> +	/* Map I/O registers. */
>> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> +				       &mchp_spdiftx_regmap_config);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	/* Request IRQ */
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	err = devm_request_irq(&pdev->dev, irq, mchp_spdiftx_interrupt, 0,
>> +			       dev_name(&pdev->dev), dev);
>> +	if (err)
>> +		return err;
>> +
>> +	/* Get the peripheral clock */
>> +	dev->pclk = devm_clk_get(&pdev->dev, "pclk");
>> +	if (IS_ERR(dev->pclk)) {
>> +		err = PTR_ERR(dev->pclk);
>> +		dev_err(&pdev->dev,
>> +			"failed to get the peripheral clock: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* Get the generic clock */
>> +	dev->gclk = devm_clk_get(&pdev->dev, "gclk");
>> +	if (IS_ERR(dev->gclk)) {
>> +		err = PTR_ERR(dev->gclk);
>> +		dev_err(&pdev->dev,
>> +			"failed to get the PMC generic clock: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	ctrl = &dev->control;
>> +	spin_lock_init(&ctrl->lock);
>> +
>> +	/* Init channel status */
>> +	ctrl->ch_stat[0] = IEC958_AES0_CON_NOT_COPYRIGHT |
>> +			   IEC958_AES0_CON_EMPHASIS_NONE;
>> +
>> +	dev->dev = &pdev->dev;
>> +	dev->regmap = regmap;
>> +	platform_set_drvdata(pdev, dev);
>> +
>> +	dev->playback.addr = (dma_addr_t)mem->start + SPDIFTX_CDR;
>> +	dev->playback.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +
>> +	err = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to register PMC: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	err = devm_snd_soc_register_component(&pdev->dev,
>> +					      &mchp_spdiftx_component,
>> +					      &mchp_spdiftx_dai, 1);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to register component: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver mchp_spdiftx_driver = {
>> +	.probe	= mchp_spdiftx_probe,
>> +	.driver	= {
>> +		.name	= "mchp_spdiftx",
>> +		.of_match_table = of_match_ptr(mchp_spdiftx_dt_ids),
>> +	},
>> +};
>> +
>> +module_platform_driver(mchp_spdiftx_driver);
>> +
>> +MODULE_AUTHOR("Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>");
>> +MODULE_DESCRIPTION("Microchip S/PDIF TX Controller Driver");
>> +MODULE_LICENSE("GPL v2");

Powered by blists - more mailing lists