Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.

Lori Hikichi
2015-04-02T18:47:18Z

On 15-03-30 11:42 PM, Mark Brown wrote:
> On Mon, Mar 30, 2015 at 08:16:24PM -0700, Scott Branden wrote:
>
> The CC list for this patch is pretty wide - please look at who you're
> sending this to and try to send to only relevant people (for example I'm
> not sure the Raspberry Pi people need to review this). People working
> upstream get a lot of mail so it's helpful to avoid filling their
> inboxes with random irrelevant stuff.
>
>> sound/soc/bcm/Kconfig | 11 +
>> sound/soc/bcm/Makefile | 5 +-
>> sound/soc/bcm/cygnus-pcm.c | 918 +++++++++++++++++++++++++
>> sound/soc/bcm/cygnus-pcm.h | 45 ++
>> sound/soc/bcm/cygnus-ssp.c | 1613 ++++++++++++++++++++++++++++++++++++++++++++
>> sound/soc/bcm/cygnus-ssp.h | 84 +++
>> 6 files changed, 2675 insertions(+), 1 deletion(-)
>
> Send the DMA and DAI drivers as separate patches please, it makes review
> easier.
>
>> +config SND_SOC_CYGNUS
>> + tristate "SoC platform audio for Broadcom Cygnus chips"
>> + depends on ARCH_BCM_CYGNUS || COMPILE_TEST
>> + default ARCH_BCM_CYGNUS
>
Okay.

> Remove the default setting here - we don't do this for other drivers, we
> shouldn't do it here.
>
>> +/*
>> + * PERIOD_BYTES_MIN is the number of bytes to at which the interrupt will tick.
>> + * This number should be a multiple of 256
>> + */
>> +#define PERIOD_BYTES_MIN 0x100
>
> This sounds like it's a setting rather than actually the minimum?
>
It is a bad comment. I will update. This is the minimum period (in
bytes) at which the interrupt can tick. Other possible value for the
period must be multiple of this value.

>> +static const struct snd_pcm_hardware cygnus_pcm_hw = {
>> + .info = SNDRV_PCM_INFO_MMAP |
>> + SNDRV_PCM_INFO_MMAP_VALID |
>> + SNDRV_PCM_INFO_INTERLEAVED,
>
> The DMA controller is integrated into the IP?
>
Yes, it is dedicated for the audio driver.

>> +static int enable_count;
>
> This looks bogus - why is this a global variable not part of the device
> struct and if it does need to be global why does it need no locking?
>
I will fix.

>> + if (aio->portnum == 0)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(0);
>> + else if (aio->portnum == 1)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(2);
>> + else if (aio->portnum == 2)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(4);
>> + else if (aio->portnum == 3)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(6); /*SPDIF */
>> + else
>> + status = -EINVAL;
>
> This looks like a switch statement, there are many places in the code
> where you're writing switch statements as chains of ifs.
>
No problem. Will use switch statements.

>> +static void ringbuf_inc(void __iomem *audio_io, bool is_playback,
>> + const struct ringbuf_regs *p_rbuf)
>> +{
>> + u32 regval, endval, active_ptr;
>> +
>> + if (is_playback)
>> + active_ptr = p_rbuf->wraddr;
>> + else
>> + active_ptr = p_rbuf->rdaddr;
>> +
>> + endval = readl(audio_io + p_rbuf->endaddr);
>> + regval = readl(audio_io + active_ptr);
>> + regval = regval + p_rbuf->period_bytes;
>> + if (regval > endval)
>> + regval -= p_rbuf->buf_size;
>> +
>> + writel(regval, audio_io + active_ptr);
>> +}
>
> So it looks like we're getting an interrupt per period and we have to
> manually advance to the next one?
>
Yes.

>> +static irqreturn_t cygnus_dma_irq(int irq, void *data)
>> +{
>> + u32 r5_status;
>> + struct cygnus_audio *cygaud;
>> +
>> + cygaud = (struct cygnus_audio *)data;
>
> If you need to cast away from void something is very wrong.
>
Okay, will fix.

>> + /*
>> + * R5 status bits Description
>> + * 0 ESR0 (playback FIFO interrupt)
>> + * 1 ESR1 (playback rbuf interrupt)
>> + * 2 ESR2 (capture rbuf interrupt)
>> + * 3 ESR3 (Freemark play. interrupt)
>> + * 4 ESR4 (Fullmark capt. interrupt)
>> + */
>> + r5_status = readl(cygaud->audio + INTH_R5F_STATUS_OFFSET);
>> +
>> + /* If playback interrupt happened */
>> + if (ANY_PLAYBACK_IRQ & r5_status)
>> + handle_playback_irq(cygaud);
>> +
>> + /* If capture interrupt happened */
>> + if (ANY_CAPTURE_IRQ & r5_status)
>> + handle_capture_irq(cygaud);
>> +
>> + /*
>> + * clear r5 interrupts after servicing them to avoid overwriting
>> + * esr_status
>> + */
>> + writel(r5_status, cygaud->audio + INTH_R5F_CLEAR_OFFSET);
>
> This feels racy especially given that we seem to need every interrupt
> delivering. What if another period happens during the servicing? I
> don't understand what "overwriting esr_status" means here.
>
Let me review this handler and I will enhance as needed.

>> + return IRQ_HANDLED;
>
> If neither playback nor capture interrupts were flagged we should return
> IRQ_NONE.
>
Okay, will fix.

>> +/*
>> + * This code is identical to what is done by the framework, when we do not
>> + * supply a 'copy' function. Having our own copy hook in place allows for
>> + * us to easily add some diagnotics when needed.
>> + */
>> +int cygnus_pcm_copy(struct snd_pcm_substream *substream, int channel,
>> + snd_pcm_uframes_t pos,
>> + void __user *buf, snd_pcm_uframes_t count)
>
> This is obviously not suitable for mainline - "let's just cut'n'paste
> the shared code in case we want to add diagnostics in future" does not
> scale, you can always add local diagnostics in the core code.
>
Okay, will remove.

>> +};
>> +/*
>
> Blank line between these two please. Missing blanks between bits of
> code seem to be a common issue in this driver.
>
Okay, will fix.

>> +static int audio_ssp_in_enable(struct cygnus_aio_port *aio, bool enable)
>> +{
>> + u32 value;
>> +
>> + if (enable) {
>
>> + } else {
>
> Why not just write two functions?
>
Okay, will change.

>> +static int audio_ssp_source_bitres(struct cygnus_aio_port *aio, unsigned bits)
>> +{
>> + u32 mask = 0x1f;
>> + u32 value = 0;
>> +
>> + if ((bits == 8) || (bits == 16) || (bits == 32)) {
>> + value = readl(aio->audio + aio->regs.bf_sourcech_cfg);
>> + value &= ~(mask << BF_SRC_CFGX_BIT_RES);
>> +
>> + /* 32 bit mode is coded as 0 */
>> + if ((bits == 8) || (bits == 16))
>> + value |= (bits << BF_SRC_CFGX_BIT_RES);
>> +
>> + writel(value, aio->audio + aio->regs.bf_sourcech_cfg);
>> + return 0;
>> + }
>> +
>> + pr_err("Bit resolution not supported %d\n", bits);
>> + return -EINVAL;
>
> dev_err() (this applies throughout the driver).
>
Okay, will convert pr_err to dev_err.

> It's not clear why this is a function and not just written as a single
> statement in the one place that it's used (there are multiple calls but
> they're all together and could just be inlined).
>
I can integrate it with the calling code.

>> + if (!aio->bitrate) {
>> + pr_err("%s Use .set_clkdiv() to set bitrate\n", __func__);
>> + return -EINVAL;
>> + }
>
> This seems bogus - why are we enforcing the use of set_clkdiv()? Can't
> we figure out something esneible?
>
Yes, I believe I can set this via "set_tdm_slot".

>> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>> + /* Set the SSP up as slave */
>> + case SND_SOC_DAIFMT_CBM_CFM:
>
> The comments here look odd due to the indentation and really aren't
> adding much anyway.
>
Okay, will remove.

>> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>> + case SND_SOC_DAIFMT_RIGHT_J:
>> + case SND_SOC_DAIFMT_LEFT_J:
>> + return -EINVAL;
>
> These are just eaten by the default case.
>
Okay, will remove.

>> + case SND_SOC_DAIFMT_DSP_A:
>> + case SND_SOC_DAIFMT_DSP_B:
>> + ssp_newcfg |= BIT(I2S_OUT_CFGX_TDM_MODE);
>> +
>> + /* DSP_A = data after FS, DSP_B = data during FS */
>> + if (SND_SOC_DAIFMT_DSP_A)
>> + ssp_newcfg |= BIT(I2S_OUT_CFGX_DATA_ALIGNMENT);
>
> That if statement isn't doing what was intended...
>
Yikes. I will fix that.

>> + } else {
>> +
>> + switch (cmd) {
>> + case SNDRV_PCM_TRIGGER_START:
>> + audio_ssp_in_enable(aio, 1);
>> + break;
>> +
>> + case SNDRV_PCM_TRIGGER_STOP:
>> + audio_ssp_in_enable(aio, 0);
>> + break;
>> + }
>
> We can just ignore other triggers? It's not clear why this is different
> to the playback case.
>
I will review this behaviour and fix it up.

>> +int cygnus_ssp_get_mode(struct snd_soc_dai *cpu_dai)
>> +{
>> + struct cygnus_aio_port *aio = cygnus_dai_get_portinfo(cpu_dai);
>> +
>> + return aio->mode;
>> +}
>> +EXPORT_SYMBOL(cygnus_ssp_get_mode);
>
> What is this for, it's setting off alarm bells? Note also that ASoC is
> _GPL() only.
>
Okay, will remove. It is not needed if I set the port mode from the
machine file (current done via device tree).

>> +static const struct snd_kcontrol_new pll_tweak_controls[] = {
>> + SOC_SINGLE_EXT("PLL Tweak", 0, 0, PLL_NDIV_FRACT_MAX, 0,
>> + pll_tweak_get, pll_tweak_put),
>> +};
>
> Whatever this control is doing it should be a separate patch (as I think
> we discussed in person a ELC?), it's clearly something that's unusual
> and is likely to block the rest of the code as a result. At a high
> level this needs at least some documentation.
>
Okay, will remove.

>> +int cygnus_ssp_add_pll_tweak_controls(struct snd_soc_pcm_runtime *rtd)
>> +{
>> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +
>> + return snd_soc_add_dai_controls(cpu_dai,
>> + pll_tweak_controls,
>> + ARRAY_SIZE(pll_tweak_controls));
>> +}
>> +EXPORT_SYMBOL(cygnus_ssp_add_pll_tweak_controls);
>
> Again, why is this being exported and why is it not _GPL? If the driver
> is adding controls I'd expect it to just add its controls itself.
>
Okay, will remove.

>> +static int cygnus_ssp_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *child_node;
>> + struct resource *res = pdev->resource;
>> + struct cygnus_audio *cygaud;
>> + int err = -EINVAL;
>> + int node_count;
>> + int active_port_count;
>> +
>> + if (!of_match_device(cygnus_ssp_of_match, dev)) {
>> + dev_err(dev, "Failed to find ssp controller\n");
>> + return -ENODEV;
>> + }
>
> This error message is misleading - you mean to say that the driver got
> loaded for a device it doesn't understand.
>
Okay, will remove.

>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + cygaud->audio = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(cygaud->audio)) {
>> + dev_err(dev, "audio_io ioremap failed\n");
>> + return PTR_ERR(cygaud->audio);
>
> devm_ioremap_resource() will complain for you, and in general you should
> be printing error codes.
>
Okay. Will remove and rely on devm_ipremap message.

>> + node_count = 0;
>
> This doesn't seem to be needed?
>
Okay, will clean up.

>> + node_count = of_get_child_count(pdev->dev.of_node);
>> + if ((node_count < 1) || (node_count > CYGNUS_MAX_PORTS)) {
>> + dev_err(dev, "Incorrct number of child nodes\n");
>> + return -EINVAL;
>
> Spelling mistake there and it would be helpful to the user to tell them
> what we parsed.
>
Okay, will fix.