RE: [PATCH v6 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver

-----Original Message-----
From: Chen Guangyu-B42378
Sent: Monday, August 19, 2013 8:38 AM
To: Bhushan Bharat-R65777
Cc: broonie@kernel.org; lars@metafoo.de; p.zabel@pengutronix.de;
s.hauer@pengutronix.de; mark.rutland@arm.com; devicetree@vger.kernel.org; alsa-
devel@alsa-project.org; swarren@wwwdotorg.org; festevam@gmail.com;
timur@tabi.org; rob.herring@calxeda.com; tomasz.figa@gmail.com;
shawn.guo@linaro.org; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v6 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver

Hi Bhushan,

   Thank you for the comments :)
   I'll fix some in v7.

   Here is my some replies to you.

On Sat, Aug 17, 2013 at 02:24:19AM +0800, Bhushan Bharat-R65777 wrote:

quoted

quoted

This patch add S/PDIF controller driver for Freescale SoC.

Please give some more description of the driver?

I've referred some ASoC drivers, all of them seem to be brief as mine.
So I'm not sure what else information I should provide here. It's already kinda
okay to me.

Other does not have description does not mean we also should not add description here.
Please describe in few lines about this driver and devices it handles?

quoted

quoted

+struct spdif_mixer_control {
+	/* buffer ptrs for writer */
+	u32 upos;
+	u32 qpos;

They does not look like pointer?

They are more like offsets to get the correspond pointer.
But I'll change the confusing comments.

quoted

quoted

+/* U/Q Channel receive register full */ static void
+spdif_irq_uqrx_full(struct fsl_spdif_priv *spdif_priv, char name) {
+	struct spdif_mixer_control *ctrl = &spdif_priv->fsl_spdif_control;
+	struct regmap *regmap = spdif_priv->regmap;
+	struct platform_device *pdev = spdif_priv->pdev;
+	u32 *pos, size, val, reg;
+
+	switch (name) {
+	case 'U':
+		pos = &ctrl->upos;
+		size = SPDIF_UBITS_SIZE;
+		reg = REG_SPDIF_SRU;
+		break;
+	case 'Q':
+		pos = &ctrl->qpos;
+		size = SPDIF_QSUB_SIZE;
+		reg = REG_SPDIF_SRQ;
+		break;
+	default:
+		return;

Should return error.

IMHO, this should be fine. It's a void type function and being used in the
isr(). The params 'name' is totally controlled by driver itself, so basically we
don't need to worry about the default path.

Silently returning on potential error is bad. At least add a printk/BUGON or something similar which points that some unexpected parameter is passed.

quoted

quoted

+	if (*pos >= size * 2) {
+		*pos = 0;
+	} else if (unlikely((*pos % size) + 3 > size)) {
+		dev_err(&pdev->dev, "User bit receivce buffer overflow\n");
+		return;

Should return error.

Ditto, it's being used in isr(), we don't need to detect the return value, just
use dev_err() to warn users and let the driver clear the irq.

Same as above

quoted

quoted

+/* U/Q Channel framing error */
+static void spdif_irq_uq_err(struct fsl_spdif_priv *spdif_priv) {
+	struct spdif_mixer_control *ctrl = &spdif_priv->fsl_spdif_control;
+	struct regmap *regmap = spdif_priv->regmap;
+	struct platform_device *pdev = spdif_priv->pdev;
+	u32 val;
+
+	dev_dbg(&pdev->dev, "isr: U/Q Channel framing error\n");
+
+	/* read U/Q data and do buffer reset */
+	regmap_read(regmap, REG_SPDIF_SRU, &val);
+	regmap_read(regmap, REG_SPDIF_SRQ, &val);

Above prints says read u/q data and buffer reset, what is buffer reset? Is

that read on clear?

That's the behavior needed by IP, according to the reference manual:
"U Channel receive register full, can't be cleared with reg. IntClear.
To clear it, read from U Rx reg." and "Q Channel receive register full, can't be
cleared with reg. IntClear. To clear it, read from Q Rx reg."

Then please add this behavior in comment.

quoted

quoted

+static void spdif_softreset(struct fsl_spdif_priv *spdif_priv) {
+	struct regmap *regmap = spdif_priv->regmap;
+	u32 val, cycle = 1000;
+
+	regmap_write(regmap, REG_SPDIF_SCR, SCR_SOFT_RESET);
+	regcache_sync(regmap);
+
+	/* RESET bit would be cleared after finishing its reset procedure */
+	do {
+		regmap_read(regmap, REG_SPDIF_SCR, &val);
+	} while ((val & SCR_SOFT_RESET) && cycle--);

What if reset is not cleared and timeout happen?

We here suppose the reset bit would be cleared -- "The software reset will last
8 cycles." from RM, so if this happened to be a failure, the whole IP module
won't be normally working as well.

Also add a comment describing this against why cycle = 1000 is selected.

Well, but I don't mind to put here an extra failed return to make it clear.

quoted

quoted

+static u8 reverse_bits(u8 input)
+{
+	u8 tmp = input;
+
+	tmp = ((tmp & 0b10101010) >> 1) | ((tmp << 1) & 0b10101010);
+	tmp = ((tmp & 0b11001100) >> 2) | ((tmp << 2) & 0b11001100);
+	tmp = ((tmp & 0b11110000) >> 4) | ((tmp << 4) & 0b11110000);

What is this logic, can the hardcoding be removed and some description on

above calculation?

This was provided by Philipp Zabel in his review at patch v3.
It's pretty clear to me that it just reverses the bits for u8.

This is not obvious. Why not use bitrev8() ?

I don't think this logic has any problem and the mask here doesn't look like any
hardcode to me.

quoted

quoted

+static bool fsl_spdif_volatile_reg(struct device *dev, unsigned int reg)
+{
+	/* Sync all registers after reset */

Where us sync :) ?

The "return true" would do that. For volatile registers, if no "return true"
here, the whole regmap would use the value in cache, while for some bits
we need to trace its true value from the physical registers not from cache.

Where will be device registers cached? Do not we program them to be non-cacheable in core?

-Bharat

Best regards,
Nicolin Chen