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

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

[Nicolin Chen]

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.


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

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


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


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

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


Best regards,
Nicolin Chen

RE: [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@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:
> > > 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

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

[Nicolin Chen]

Hi Bhushan,

   I'll revise some as you suggest. Just a few replies here.

On Mon, Aug 19, 2013 at 12:38:11PM +0800, Bhushan Bharat-R65777 wrote:
> > 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.

If it is done in 8 cycles, 1000-cycle will be surely a safe value for it.
As long as it finished in 8 cycles, it would quit anyway. Why against?


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

regmap has a regcache for all the mapped registers. Set the regsiters as
volatile will allow the driver to sync the regcache with physical memory
each time when using regmap_read/write/update_bits().

But I think I can try to use the regcache_bypass instead.


Thank you,
Nicolin Chen

RE: [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 11:55 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,
> 
>    I'll revise some as you suggest. Just a few replies here.
> 
> On Mon, Aug 19, 2013 at 12:38:11PM +0800, Bhushan Bharat-R65777 wrote:
> > > 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.
> 
> If it is done in 8 cycles, 1000-cycle will be surely a safe value for it.
> As long as it finished in 8 cycles, it would quit anyway. Why against?

I am not against, I am saying why it was not 200 or 50 or 20 etc. I am saying that write a comment saying this much is sufficient as per specification and so keep 1000/etc as preservative.

-Bharat

> 
> 
> > > > > +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?
> 
> regmap has a regcache for all the mapped registers. Set the regsiters as
> volatile will allow the driver to sync the regcache with physical memory each
> time when using regmap_read/write/update_bits().
> 
> But I think I can try to use the regcache_bypass instead.
> 
> 
> Thank you,
> Nicolin Chen

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

[Nicolin Chen]

On Mon, Aug 19, 2013 at 02:31:47PM +0800, Bhushan Bharat-R65777 wrote:
> > > > 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.
> > 
> > If it is done in 8 cycles, 1000-cycle will be surely a safe value for it.
> > As long as it finished in 8 cycles, it would quit anyway. Why against?
> 
> I am not against, I am saying why it was not 200 or 50 or 20 etc. I am saying that write a comment saying this much is sufficient as per specification and so keep 1000/etc as preservative.

I did't mean that. The 'against' is from

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

Well, if you insist this extra comment for easy-understand, I'll add them

Thank you.
Nicolin Chen