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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Chen Guangyu-B42378
> Sent: Monday, August 19, 2013 8:38 AM
> To: Bhushan Bharat-R65777
> Cc: broonie at kernel.org; lars at metafoo.de; p.zabel at pengutronix.de;
> s.hauer at pengutronix.de; mark.rutland at arm.com; devicetree at vger.kernel.org; alsa-
> devel at alsa-project.org; swarren at wwwdotorg.org; festevam at gmail.com;
> timur at tabi.org; rob.herring at calxeda.com; tomasz.figa at gmail.com;
> shawn.guo at linaro.org; linuxppc-dev at 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:
> > > 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?

> 
> 
> > > +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.
> 
> 
> > > +/* 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.

> 
> > > +	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

> 
> 
> > > +/* 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.

> 
> 
> > > +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.
> 
> 
> > > +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.
> 
> 
> > > +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