Re: [PATCH v3 2/2] ASoC: mchp-spdiftx: add driver for S/PDIF TX Controller
From: Claudiu.Beznea
Date: Mon Aug 03 2020 - 13:11:52 EST
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?
> 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");
>