Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754058AbbDBSrj (ORCPT ); Thu, 2 Apr 2015 14:47:39 -0400 Received: from mail-gw1-out.broadcom.com ([216.31.210.62]:43377 "EHLO mail-gw1-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754039AbbDBSrc (ORCPT ); Thu, 2 Apr 2015 14:47:32 -0400 X-IronPort-AV: E=Sophos;i="5.11,512,1422950400"; d="scan'208";a="61273881" Message-ID: <551D8EB6.1050004@broadcom.com> Date: Thu, 2 Apr 2015 11:47:18 -0700 From: Lori Hikichi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0 MIME-Version: 1.0 To: Mark Brown , Scott Branden CC: Rob Herring , Pawel Moll , "Mark Rutland" , Ian Campbell , Kumar Gala , "Liam Girdwood" , Jaroslav Kysela , "Takashi Iwai" , , , , , , Dmitry Torokhov , Anatol Pomazao , , , , Subject: Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC. References: <1427771784-29950-1-git-send-email-sbranden@broadcom.com> <1427771784-29950-3-git-send-email-sbranden@broadcom.com> <20150331064209.GD2869@sirena.org.uk> In-Reply-To: <20150331064209.GD2869@sirena.org.uk> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10919 Lines: 362 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/