Re: [PATCH v3 2/2] ASoC: mchp-spdiftx: add driver for S/PDIF TX Controller

[Codrin.Ciubotariu]

On 03.08.2020 20:11, Claudiu Beznea - M18063 wrote:
> 
> 
> On 03.08.2020 19:11, Codrin Ciubotariu - M19940 wrote:
>> 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@xxxxxxxxxxxxx>
>>>> ---
>>>>
>>>> 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@xxxxxxxxxxxxx>
>>>> +
>>>> +#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.
> 
> Can mchp_spdiftx_hw_params() be suspended right after this regmap_read()
> then mchp_spdiftx_trigger() to be scheduled, to set the enable bit, then
> mchp_spdiftx_hw_params() to be rescheduled and after this the enable bit to
> be actually set in the registers but mchp_spdiftx_hw_params() to work with
> the old value? Would it be an issue if this could happen?

Thanks to [1], mchp_spdiftx_hw_params() can be called only if 
be->dpcm[stream].state is SND_SOC_DPCM_STATE_OPEN, 
SND_SOC_DPCM_STATE_HW_PARAMS or SND_SOC_DPCM_STATE_HW_FREE . If 
hw_params() gets interrupted, mchp_spdiftx_trigger() can't be called 
because of [2] .

Best regards,
Codrin

[1] https://elixir.bootlin.com/linux/latest/source/sound/soc/soc-pcm.c#L2213
[2] https://elixir.bootlin.com/linux/latest/source/sound/soc/soc-pcm.c#L2324
> 
>> 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@xxxxxxxxxxxxx>");
>>>> +MODULE_DESCRIPTION("Microchip S/PDIF TX Controller Driver");
>>>> +MODULE_LICENSE("GPL v2");