[PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
From: Viorel Suman
XCVR (Audio Transceiver) is a on-chip functional module found
on i.MX8MP. It support HDMI2.1 eARC, HDMI1.4 ARC and SPDIF.
Signed-off-by: Viorel Suman
---
sound/soc/fsl/Kconfig| 10 +
sound/soc/fsl/Makefile |2 +
sound/soc/fsl/fsl_xcvr.c | 1352 ++
sound/soc/fsl/fsl_xcvr.h | 266 +
4 files changed, 1630 insertions(+)
create mode 100644 sound/soc/fsl/fsl_xcvr.c
create mode 100644 sound/soc/fsl/fsl_xcvr.h
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index 3f76ff7..d04b64d 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -95,6 +95,16 @@ config SND_SOC_FSL_EASRC
destination sample rate. It is a new design module compare with the
old ASRC.
+config SND_SOC_FSL_XCVR
+ tristate "NXP Audio Transceiver (XCVR) module support"
+ select REGMAP_MMIO
+ select SND_SOC_IMX_PCM_DMA if SND_IMX_SOC != n
+ select SND_SOC_GENERIC_DMAENGINE_PCM
+ help
+ Say Y if you want to add Audio Transceiver (XCVR) support for NXP
+ iMX CPUs. XCVR is a digital module that supports HDMI2.1 eARC,
+ HDMI1.4 ARC and SPDIF.
+
config SND_SOC_FSL_UTILS
tristate
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
index b835eeb..1d2231f 100644
--- a/sound/soc/fsl/Makefile
+++ b/sound/soc/fsl/Makefile
@@ -25,6 +25,7 @@ snd-soc-fsl-utils-objs := fsl_utils.o
snd-soc-fsl-dma-objs := fsl_dma.o
snd-soc-fsl-mqs-objs := fsl_mqs.o
snd-soc-fsl-easrc-objs := fsl_easrc.o
+snd-soc-fsl-xcvr-objs := fsl_xcvr.o
obj-$(CONFIG_SND_SOC_FSL_AUDMIX) += snd-soc-fsl-audmix.o
obj-$(CONFIG_SND_SOC_FSL_ASOC_CARD) += snd-soc-fsl-asoc-card.o
@@ -38,6 +39,7 @@ obj-$(CONFIG_SND_SOC_FSL_UTILS) += snd-soc-fsl-utils.o
obj-$(CONFIG_SND_SOC_FSL_MQS) += snd-soc-fsl-mqs.o
obj-$(CONFIG_SND_SOC_FSL_EASRC) += snd-soc-fsl-easrc.o
obj-$(CONFIG_SND_SOC_POWERPC_DMA) += snd-soc-fsl-dma.o
+obj-$(CONFIG_SND_SOC_FSL_XCVR) += snd-soc-fsl-xcvr.o
# MPC5200 Platform Support
obj-$(CONFIG_SND_MPC52xx_DMA) += mpc5200_dma.o
diff --git a/sound/soc/fsl/fsl_xcvr.c b/sound/soc/fsl/fsl_xcvr.c
new file mode 100644
index ..2e66f22
--- /dev/null
+++ b/sound/soc/fsl/fsl_xcvr.c
@@ -0,0 +1,1352 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2019 NXP
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "fsl_xcvr.h"
+#include "imx-pcm.h"
+
+#define FSL_XCVR_CAPDS_SIZE256
+
+struct fsl_xcvr {
+ struct platform_device *pdev;
+ struct regmap *regmap;
+ struct clk *ipg_clk;
+ struct clk *pll_ipg_clk;
+ struct clk *phy_clk;
+ struct clk *spba_clk;
+ struct reset_control *reset;
+ const char *fw_name;
+ u8 streams;
+ u32 mode;
+ u32 arc_mode;
+ void __iomem *ram_addr;
+ struct snd_dmaengine_dai_dma_data dma_prms_rx;
+ struct snd_dmaengine_dai_dma_data dma_prms_tx;
+ struct snd_aes_iec958 rx_iec958;
+ struct snd_aes_iec958 tx_iec958;
+ u8 cap_ds[FSL_XCVR_CAPDS_SIZE];
+};
+
+static const struct fsl_xcvr_pll_conf {
+ u8 mfi; /* min=0x18, max=0x38 */
+ u32 mfn; /* signed int, 2's compl., min=0x3FFF, max=0x0001 */
+ u32 mfd; /* unsigned int */
+ u32 fout; /* Fout = Fref*(MFI + MFN/MFD), Fref is 24MHz */
+} fsl_xcvr_pll_cfg[] = {
+ { .mfi = 54, .mfn = 1, .mfd = 6, .fout = 13, }, /* 1.3 GHz */
+ { .mfi = 32, .mfn = 96, .mfd = 125, .fout = 786432000, }, /* 8000 Hz */
+ { .mfi = 30, .mfn = 66, .mfd = 625, .fout = 722534400, }, /* 11025 Hz
*/
+ { .mfi = 29, .mfn = 1, .mfd = 6, .fout = 7, }, /* 700 MHz */
+};
+
+static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /* one bit
6, 12 ? */
+static const struct snd_pcm_hw_constraint_list fsl_xcvr_earc_channels_constr =
{
+ .count = ARRAY_SIZE(fsl_xcvr_earc_channels),
+ .list = fsl_xcvr_earc_channels,
+};
+
+static const u32 fsl_xcvr_earc_rates[] = {
+ 32000, 44100, 48000, 64000, 88200, 96000,
+ 128000, 176400, 192000, 256000, 352800, 384000,
+ 512000, 705600, 768000, 1024000, 1411200, 1536000,
+};
+static const struct snd_pcm_hw_constraint_list fsl_xcvr_earc_rates_constr = {
+ .count = ARRAY_SIZE(fsl_xcvr_earc_rates),
+ .list = fsl_xcvr_earc_rates,
+};
+
+static const u32 fsl_xcvr_spdif_channels[] = { 2, };
+static const struct snd_pcm_hw_constraint_list fsl_xcvr_spdif_channels_constr
= {
+ .count = ARRAY_SIZE(fsl_xcvr_spdif_channels),
+ .list = fsl_xcvr_spdif_channels,
+};
+
+static const u32 fsl_xcvr_spdif_rates[] = {
+ 32000, 44100, 48000, 88200, 96000, 176400, 192000,
+};
+static const struct snd_pcm_hw_constraint_list fsl_xcvr_spdif_rates_constr = {
+ .count = ARRAY_SIZE(fsl_xcvr_spdif_rates),
+ .list = fsl_xcvr_spdif_rates,
+};
+
+static int fsl_xcvr_arc_mode_put(struct snd_kc
Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
Hi Viorel,
It looks pretty clean to me, though some small comments inline.
On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
> From: Viorel Suman
>
> XCVR (Audio Transceiver) is a on-chip functional module found
> on i.MX8MP. It support HDMI2.1 eARC, HDMI1.4 ARC and SPDIF.
>
> Signed-off-by: Viorel Suman
> +static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /* one bit
> 6, 12 ? */
What's the meaning of the comments?
> +static const int fsl_xcvr_phy_arc_cfg[] = {
> + FSL_XCVR_PHY_CTRL_ARC_MODE_SE_EN, FSL_XCVR_PHY_CTRL_ARC_MODE_CM_EN,
> +};
Nit: better be u32 vs. int?
> +/** phy: true => phy, false => pll */
> +static int fsl_xcvr_ai_write(struct fsl_xcvr *xcvr, u8 reg, u32 data, bool
> phy)
> +{
> + u32 val, idx, tidx;
> +
> + idx = BIT(phy ? 26 : 24);
> + tidx = BIT(phy ? 27 : 25);
> +
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_CLR, 0xFF);
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_SET, reg);
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_WDATA, data);
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_TOG, idx);
> +
> + do {
> + regmap_read(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL, &val);
> + } while ((val & idx) != ((val & tidx) >> 1));
Might regmap_read_poll_timeout() be better? And it seems to poll
intentionally with no sleep nor timeout -- would be nice to have
a line of comments to explain why.
> > +static int fsl_xcvr_runtime_resume(struct device *dev)
> +{
> + struct fsl_xcvr *xcvr = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(xcvr->ipg_clk);
> + if (ret) {
> + dev_err(dev, "failed to start IPG clock.\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(xcvr->pll_ipg_clk);
> + if (ret) {
> + dev_err(dev, "failed to start PLL IPG clock.\n");
Should it disable ipg_clk?
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(xcvr->phy_clk);
> + if (ret) {
> + dev_err(dev, "failed to start PHY clock: %d\n", ret);
> + clk_disable_unprepare(xcvr->ipg_clk);
Should it disable pll_ipg_clk?
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(xcvr->spba_clk);
> + if (ret) {
> + dev_err(dev, "failed to start SPBA clock.\n");
> + clk_disable_unprepare(xcvr->phy_clk);
> + clk_disable_unprepare(xcvr->ipg_clk);
Ditto
> + return ret;
> + }
> +
> + regcache_cache_only(xcvr->regmap, false);
> + regcache_mark_dirty(xcvr->regmap);
> + ret = regcache_sync(xcvr->regmap);
> +
> + if (ret) {
> + dev_err(dev, "failed to sync regcache.\n");
> + return ret;
What about those clocks? Probably better to have some error-out
labels at the end of the function?
> + }
> +
> + reset_control_assert(xcvr->reset);
> + reset_control_deassert(xcvr->reset);
> +
> + ret = fsl_xcvr_load_firmware(xcvr);
> + if (ret) {
> + dev_err(dev, "failed to load firmware.\n");
> + return ret;
Ditto
> + }
> +
> + /* Release M0+ reset */
> + ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_CTRL,
> + FSL_XCVR_EXT_CTRL_CORE_RESET, 0);
> + if (ret < 0) {
> + dev_err(dev, "M0+ core release failed: %d\n", ret);
> + return ret;
Ditto
> + }
> + mdelay(50);
Any reason to use mdelay over msleep for a 50ms wait? May add a
line of comments if mdelay is a must?
Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
This looks mostly good, a few smallish things below but nothing major.
> +static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr)
> +{
> + struct device *dev = &xcvr->pdev->dev;
> + const struct firmware *fw;
> + int ret = 0, rem, off, out, page = 0, size = FSL_XCVR_REG_OFFSET;
> + u32 mask, val;
> +
> + ret = request_firmware(&fw, xcvr->fw_name, dev);
> + if (ret) {
> + dev_err(dev, "failed to request firmware.\n");
> + return ret;
> + }
> +
> + rem = fw->size;
It would be good to see some explicit validation of the image size, at
least printing an error message if the image is bigger than can be
loaded. The code should be safe in that it won't overflow the device
region it's writing to but it feels like it'd be better to tell people
if we spot a problem rather than just silently truncating the file.
> + /* RAM is 20KiB => max 10 pages 2KiB each */
> + for (page = 0; page < 10; page++) {
> + ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_CTRL,
> + FSL_XCVR_EXT_CTRL_PAGE_MASK,
> + FSL_XCVR_EXT_CTRL_PAGE(page));
regmap does have paging support, though given that this is currently the
only place where paging is used this probably doesn't matter too much.
> +static irqreturn_t irq0_isr(int irq, void *devid)
> +{
> + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> + struct device *dev = &xcvr->pdev->dev;
> + struct regmap *regmap = xcvr->regmap;
> + void __iomem *reg_ctrl, *reg_buff;
> + u32 isr, val, i;
> +
> + regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr);
> + regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr);
This will unconditionally clear any interrupts, even those we don't
understand - it might be better to only clear bits that are supported so
the IRQ core can complain if there's something unexpected showing up.
> + if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR)
> + dev_dbg(dev, "RX/TX FIFO full/empty\n");
Should this be dev_err()?
> +static irqreturn_t irq1_isr(int irq, void *devid)
> +{
> + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> + struct device *dev = &xcvr->pdev->dev;
> +
> + dev_dbg(dev, "irq[1]: %d\n", irq);
> +
> + return IRQ_HANDLED;
> +}
Is there any value in even requesting this and irq2 given the lack of
meaningful handling?
signature.asc
Description: PGP signature
RE: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
Hi Nicolin,
Thank you for your review.
> > +static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /*
> > +one bit 6, 12 ? */
>
> What's the meaning of the comments?
Just a thought noted as comment. HDMI2.1 spec defines 6- and 12-channels layout
when
one bit audio stream is transmitted - I was wandering how can this be enforced.
Is a @todo like of comment.
>
> > +static const int fsl_xcvr_phy_arc_cfg[] = {
> > + FSL_XCVR_PHY_CTRL_ARC_MODE_SE_EN,
> FSL_XCVR_PHY_CTRL_ARC_MODE_CM_EN,
> > +};
>
> Nit: better be u32 vs. int?
Yes, will fix it in v2.
>
> > +/** phy: true => phy, false => pll */ static int
> > +fsl_xcvr_ai_write(struct fsl_xcvr *xcvr, u8 reg, u32 data, bool phy)
> > +{
> > + u32 val, idx, tidx;
> > +
> > + idx = BIT(phy ? 26 : 24);
> > + tidx = BIT(phy ? 27 : 25);
> > +
> > + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_CLR, 0xFF);
> > + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_SET, reg);
> > + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_WDATA, data);
> > + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_TOG, idx);
> > +
> > + do {
> > + regmap_read(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL, &val);
> > + } while ((val & idx) != ((val & tidx) >> 1));
>
> Might regmap_read_poll_timeout() be better? And it seems to poll intentionally
> with no sleep nor timeout -- would be nice to have a line of comments to
> explain
> why.
No particular reason to do it with no sleep or timeout here, will check and fix
it in v2.
>
> > > +static int fsl_xcvr_runtime_resume(struct device *dev)
> > +{
> > + struct fsl_xcvr *xcvr = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(xcvr->ipg_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to start IPG clock.\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(xcvr->pll_ipg_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to start PLL IPG clock.\n");
>
> Should it disable ipg_clk?
Yes, thank you, will fix in v2.
>
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(xcvr->phy_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to start PHY clock: %d\n", ret);
> > + clk_disable_unprepare(xcvr->ipg_clk);
>
> Should it disable pll_ipg_clk?
Yes, will fix in v2.
>
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(xcvr->spba_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to start SPBA clock.\n");
> > + clk_disable_unprepare(xcvr->phy_clk);
> > + clk_disable_unprepare(xcvr->ipg_clk);
>
> Ditto
Ok.
>
> > + return ret;
> > + }
> > +
> > + regcache_cache_only(xcvr->regmap, false);
> > + regcache_mark_dirty(xcvr->regmap);
> > + ret = regcache_sync(xcvr->regmap);
> > +
> > + if (ret) {
> > + dev_err(dev, "failed to sync regcache.\n");
> > + return ret;
>
> What about those clocks? Probably better to have some error-out labels at the
> end of the function?
Make sense, will fix in v2.
>
> > + }
> > +
> > + reset_control_assert(xcvr->reset);
> > + reset_control_deassert(xcvr->reset);
> > +
> > + ret = fsl_xcvr_load_firmware(xcvr);
> > + if (ret) {
> > + dev_err(dev, "failed to load firmware.\n");
> > + return ret;
>
> Ditto
>
> > + }
> > +
> > + /* Release M0+ reset */
> > + ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_CTRL,
> > +FSL_XCVR_EXT_CTRL_CORE_RESET, 0);
> > + if (ret < 0) {
> > + dev_err(dev, "M0+ core release failed: %d\n", ret);
> > + return ret;
>
> Ditto
>
> > + }
> > + mdelay(50);
>
> Any reason to use mdelay over msleep for a 50ms wait? May add a line of
> comments if mdelay is a must?
No particular reason, will fix it in v2.
Thank you,
Viorel
RE: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
Hi Mark,
Thank you for your review.
> On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
> > +static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr) {
> > + struct device *dev = &xcvr->pdev->dev;
> > + const struct firmware *fw;
> > + int ret = 0, rem, off, out, page = 0, size = FSL_XCVR_REG_OFFSET;
> > + u32 mask, val;
> > +
> > + ret = request_firmware(&fw, xcvr->fw_name, dev);
> > + if (ret) {
> > + dev_err(dev, "failed to request firmware.\n");
> > + return ret;
> > + }
> > +
> > + rem = fw->size;
>
> It would be good to see some explicit validation of the image size, at least
> printing an error message if the image is bigger than can be loaded. The code
> should be safe in that it won't overflow the device region it's writing to
> but it
> feels like it'd be better to tell people if we spot a problem rather than
> just silently
> truncating the file.
Make sense, will improve this part in the next version.
> > +static irqreturn_t irq0_isr(int irq, void *devid) {
> > + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> > + struct device *dev = &xcvr->pdev->dev;
> > + struct regmap *regmap = xcvr->regmap;
> > + void __iomem *reg_ctrl, *reg_buff;
> > + u32 isr, val, i;
> > +
> > + regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr);
> > + regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr);
>
> This will unconditionally clear any interrupts, even those we don't
> understand - it
> might be better to only clear bits that are supported so the IRQ core can
> complain if there's something unexpected showing up.
The ARM core registers itself in "fsl_xcvr_prepare" (the code below) just for a
subset of all supported interrupts:
=
ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0,
FSL_XCVR_IRQ_EARC_ALL, FSL_XCVR_IRQ_EARC_ALL);
=
FSL_XCVR_IRQ_EARC_ALL - this mask represents all the interrupts we are
interested in and we handle in interrupt handler,
But this is just a subset of all interrupts the M0+ core is able to assert. Not
very intuitive, I think I need to reword it somehow.
> > + if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR)
> > + dev_dbg(dev, "RX/TX FIFO full/empty\n");
>
> Should this be dev_err()?
The interrupt may be asserted right before DMA starts to fill the TX FIFO if I
recall correctly.
I've added it just to debug the IP behavior, will check and change it to err it
in next version if it is the case.
> > +static irqreturn_t irq1_isr(int irq, void *devid) {
> > + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> > + struct device *dev = &xcvr->pdev->dev;
> > +
> > + dev_dbg(dev, "irq[1]: %d\n", irq);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> Is there any value in even requesting this and irq2 given the lack of
> meaningful
> handling?
No, will remove it in v2.
Thank you,
Viorel
Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
On Fri, Sep 18, 2020 at 03:02:39PM +, Viorel Suman (OSS) wrote: Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > > > + regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr); > > > + regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr); > > This will unconditionally clear any interrupts, even those we don't > > understand - it > > might be better to only clear bits that are supported so the IRQ core can > > complain if there's something unexpected showing up. > The ARM core registers itself in "fsl_xcvr_prepare" (the code below) just for > a subset of all supported interrupts: > = > ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0, >FSL_XCVR_IRQ_EARC_ALL, FSL_XCVR_IRQ_EARC_ALL); > = > FSL_XCVR_IRQ_EARC_ALL - this mask represents all the interrupts we are > interested in and we handle in interrupt handler, > But this is just a subset of all interrupts the M0+ core is able to assert. > Not very intuitive, I think I need to reword it somehow. That's not the issue, the issue is that if we get into the ISR we just ack all the bits that are flagged by the hardware regardless of if we actually handled them. This won't work if there are ever systems that share the interrupt and it works against safety/debugging features that the interrupt has in case something goes wrong and we get spurious interrupts. > > > + if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR) > > > + dev_dbg(dev, "RX/TX FIFO full/empty\n"); > > Should this be dev_err()? > The interrupt may be asserted right before DMA starts to fill the TX FIFO if > I recall correctly. > I've added it just to debug the IP behavior, will check and change it to err > it in next version if it is the case. If it does come up normally then a comment or something to explain why this happens normally would probably be good. signature.asc Description: PGP signature
RE: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
> On Fri, Sep 18, 2020 at 03:02:39PM +, Viorel Suman (OSS) wrote: > > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier > to read and reply to. > My bad, will do. > > > > + regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr); > > > > + regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr); > > > > This will unconditionally clear any interrupts, even those we don't > > > understand - it might be better to only clear bits that are > > > supported so the IRQ core can complain if there's something unexpected > showing up. > > > The ARM core registers itself in "fsl_xcvr_prepare" (the code below) just > > for a > subset of all supported interrupts: > > = > > ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0, > > FSL_XCVR_IRQ_EARC_ALL, > FSL_XCVR_IRQ_EARC_ALL); = > > FSL_XCVR_IRQ_EARC_ALL - this mask represents all the interrupts we are > > interested in and we handle in interrupt handler, But this is just a subset > > of all > interrupts the M0+ core is able to assert. Not very intuitive, I think I need > to > reword it somehow. > > That's not the issue, the issue is that if we get into the ISR we just ack > all the bits > that are flagged by the hardware regardless of if we actually handled them. > This > won't work if there are ever systems that share the interrupt and it works > against safety/debugging features that the interrupt has in case something > goes > wrong and we get spurious interrupts. > Thank you for explanation, will fix it in next version. > > > > + if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR) > > > > + dev_dbg(dev, "RX/TX FIFO full/empty\n"); > > > > Should this be dev_err()? > > > The interrupt may be asserted right before DMA starts to fill the TX FIFO > > if I > recall correctly. > > I've added it just to debug the IP behavior, will check and change it to > > err it in > next version if it is the case. > > If it does come up normally then a comment or something to explain why this > happens normally would probably be good. Sure, ok.
Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
On 9/18/20 9:21 AM, Viorel Suman (OSS) wrote:
+static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /*
+one bit 6, 12 ? */
What's the meaning of the comments?
Just a thought noted as comment. HDMI2.1 spec defines 6- and 12-channels layout
when
one bit audio stream is transmitted - I was wandering how can this be enforced.
Is a @todo like of comment.
Please don't add comments that other developers could never understand.
The text that you just wrote here would be a great starting point for a
better comment.
