> -----Original Message-----
> From: Chen Guangyu-B42378
> Sent: Monday, August 19, 2013 8:38 AM
> To: Bhushan Bharat-R65777
> Cc: broonie@xxxxxxxxxx; lars@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; mark.rutland@xxxxxxx; devicetree@xxxxxxxxxxxxxxx; alsa-
> devel@xxxxxxxxxxxxxxxx; swarren@xxxxxxxxxxxxx; festevam@xxxxxxxxx;
> timur@xxxxxxxx; rob.herring@xxxxxxxxxxx; tomasz.figa@xxxxxxxxx;
> shawn.guo@xxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
