From: Mark Brown <broonie@kernel.org> To: Damien Horsley <Damien.Horsley@imgtec.com> Cc: alsa-devel@alsa-project.org, Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Kumar Gala <galak@codeaurora.org>, Liam Girdwood <lgirdwood@gmail.com>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, Charles Keepax <ckeepax@gmail.com>, Vinod Koul <vinod.koul@intel.com>, Thomas Niederprum <niederp@physik.uni-kl.de>, Bard Liao <bardliao@realtek.com>, Richard Fitzgerald <rf@opensource.wolfsonmicro.com>, Jyri Sarha <jsarha@ti.com>, "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>, Arnd Bergmann <arnd@arndb.de>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, James Hartley <james.hartley@imgtec.com> Subject: Re: [alsa-devel] [PATCH 2/2] ASoC: pcm3168a: Add driver for pcm3168a codec Date: Fri, 27 Nov 2015 12:54:11 +0000 [thread overview] Message-ID: <20151127125411.GR1929@sirena.org.uk> (raw) In-Reply-To: <1448376224-23964-3-git-send-email-Damien.Horsley@imgtec.com> [-- Attachment #1: Type: text/plain, Size: 3571 bytes --] On Tue, Nov 24, 2015 at 02:43:44PM +0000, Damien Horsley wrote: > From: "Damien.Horsley" <Damien.Horsley@imgtec.com> > > Add driver for Texas Instruments pcm3168a codec Please try to keep your CC lists reasonable - only CC people who have an interest in the patch you're posting. The CC list you have here is far too broad, I can't figure out why a lot of the people there ended up there. If you've got 10 people that's generally a warning sign. Overall this looks good but there are a few smallish issues below: > + if ((!slave_mode || (fmt & PCM3168A_FMT_DSP_MASK)) && > + (format == SNDRV_PCM_FORMAT_S24_3LE)) { > + dev_err(codec->dev, "48-bit frames not supported in master mode or slave mode using DSP(A/B)\n"); You've got a lot of weird indentation with the second line of multi-line if conditionals massively further indented than I'd expect. These conditionals all also seem more complex than they should be - the fact that you're using so many !slave_modes makes me think that you should have a flag for master mode and... > + if ((!slave_mode || ((fmt != PCM3168A_FMT_RIGHT_J) && > + (fmt != PCM3168A_FMT_LEFT_J))) && > + (format == SNDRV_PCM_FORMAT_S16)) { ...especially with things like this the indentation makes it hard to tell what bits go together. > + val = slave_mode ? 0 : ((i + 1) << shift); Please write normal if statements unless there's a strong reason to use the ternery operator. > + /* > + * Justification has no effect for S32 and S16 as the whole frame > + * is filled with the samples, but the register field > + * must be set to a specific value for correct operation > + */ > + if ((fmt == PCM3168A_FMT_RIGHT_J) && > + (format == SNDRV_PCM_FORMAT_S32)) { > + fmt = PCM3168A_FMT_LEFT_J; > + } else if (format == SNDRV_PCM_FORMAT_S16) { > + fmt = PCM3168A_FMT_RIGHT_J_16; > + } Being in right justified mode *does* have an effect - it changes where the audio data starts relative to LRCLK. It is true that if the frame has exactly the right number of clocks left and right justified are identical but extra clocks are in general permitted so it looks like your right justified case above just isn't something the device really supports. > + pcm3168a->scki = devm_clk_get(dev, "scki"); > + if (IS_ERR(pcm3168a->scki)) { > + if (PTR_ERR(pcm3168a->scki) != -EPROBE_DEFER) > + dev_err(dev, "failed to acquire clock 'scki'\n"); Please print the error code as well, it may help people understand the problem. > + ret = snd_soc_register_codec(dev, &pcm3168a_driver, pcm3168a_dais, > + ARRAY_SIZE(pcm3168a_dais)); > + if (ret) { > + dev_err(dev, "failed to register codec: %d\n", ret); > + goto err_regulator; > + } > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_idle(dev); You should enable runtime PM before registering the CODEC since the core can do runtime PM operations and if it tries before runtime PM is enabled things will get unbalanced. > +#define PCM3168A_DOUBLE_STS(xname, reg, shift_left, shift_right, max, invert) \ > +{ \ > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \ > + .info = snd_soc_info_volsw, .get = snd_soc_get_volsw, \ > + .put = snd_soc_put_volsw, \ > + .access = SNDRV_CTL_ELEM_ACCESS_READ | \ > + SNDRV_CTL_ELEM_ACCESS_VOLATILE, \ > + .private_value = SOC_DOUBLE_VALUE(reg, shift_left, shift_right, \ > + max, invert, 0) \ > +} This doesn't look like it should be driver specific - what's driver specific about it? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org> To: Damien Horsley <Damien.Horsley@imgtec.com> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, Bard Liao <bardliao@realtek.com>, "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>, Pawel Moll <pawel.moll@arm.com>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Vinod Koul <vinod.koul@intel.com>, linux-kernel@vger.kernel.org, Takashi Iwai <tiwai@suse.com>, Liam Girdwood <lgirdwood@gmail.com>, Jyri Sarha <jsarha@ti.com>, Rob Herring <robh+dt@kernel.org>, Arnd Bergmann <arnd@arndb.de>, Kumar Gala <galak@codeaurora.org>, Charles Keepax <ckeepax@gmail.com>, Richard Fitzgerald <rf@opensource.wolfsonmicro.com>, James Hartley <james.hartley@imgtec.com>, Thomas Niederprum <niederp@physik.uni-kl.de> Subject: Re: [PATCH 2/2] ASoC: pcm3168a: Add driver for pcm3168a codec Date: Fri, 27 Nov 2015 12:54:11 +0000 [thread overview] Message-ID: <20151127125411.GR1929@sirena.org.uk> (raw) In-Reply-To: <1448376224-23964-3-git-send-email-Damien.Horsley@imgtec.com> [-- Attachment #1.1: Type: text/plain, Size: 3571 bytes --] On Tue, Nov 24, 2015 at 02:43:44PM +0000, Damien Horsley wrote: > From: "Damien.Horsley" <Damien.Horsley@imgtec.com> > > Add driver for Texas Instruments pcm3168a codec Please try to keep your CC lists reasonable - only CC people who have an interest in the patch you're posting. The CC list you have here is far too broad, I can't figure out why a lot of the people there ended up there. If you've got 10 people that's generally a warning sign. Overall this looks good but there are a few smallish issues below: > + if ((!slave_mode || (fmt & PCM3168A_FMT_DSP_MASK)) && > + (format == SNDRV_PCM_FORMAT_S24_3LE)) { > + dev_err(codec->dev, "48-bit frames not supported in master mode or slave mode using DSP(A/B)\n"); You've got a lot of weird indentation with the second line of multi-line if conditionals massively further indented than I'd expect. These conditionals all also seem more complex than they should be - the fact that you're using so many !slave_modes makes me think that you should have a flag for master mode and... > + if ((!slave_mode || ((fmt != PCM3168A_FMT_RIGHT_J) && > + (fmt != PCM3168A_FMT_LEFT_J))) && > + (format == SNDRV_PCM_FORMAT_S16)) { ...especially with things like this the indentation makes it hard to tell what bits go together. > + val = slave_mode ? 0 : ((i + 1) << shift); Please write normal if statements unless there's a strong reason to use the ternery operator. > + /* > + * Justification has no effect for S32 and S16 as the whole frame > + * is filled with the samples, but the register field > + * must be set to a specific value for correct operation > + */ > + if ((fmt == PCM3168A_FMT_RIGHT_J) && > + (format == SNDRV_PCM_FORMAT_S32)) { > + fmt = PCM3168A_FMT_LEFT_J; > + } else if (format == SNDRV_PCM_FORMAT_S16) { > + fmt = PCM3168A_FMT_RIGHT_J_16; > + } Being in right justified mode *does* have an effect - it changes where the audio data starts relative to LRCLK. It is true that if the frame has exactly the right number of clocks left and right justified are identical but extra clocks are in general permitted so it looks like your right justified case above just isn't something the device really supports. > + pcm3168a->scki = devm_clk_get(dev, "scki"); > + if (IS_ERR(pcm3168a->scki)) { > + if (PTR_ERR(pcm3168a->scki) != -EPROBE_DEFER) > + dev_err(dev, "failed to acquire clock 'scki'\n"); Please print the error code as well, it may help people understand the problem. > + ret = snd_soc_register_codec(dev, &pcm3168a_driver, pcm3168a_dais, > + ARRAY_SIZE(pcm3168a_dais)); > + if (ret) { > + dev_err(dev, "failed to register codec: %d\n", ret); > + goto err_regulator; > + } > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_idle(dev); You should enable runtime PM before registering the CODEC since the core can do runtime PM operations and if it tries before runtime PM is enabled things will get unbalanced. > +#define PCM3168A_DOUBLE_STS(xname, reg, shift_left, shift_right, max, invert) \ > +{ \ > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \ > + .info = snd_soc_info_volsw, .get = snd_soc_get_volsw, \ > + .put = snd_soc_put_volsw, \ > + .access = SNDRV_CTL_ELEM_ACCESS_READ | \ > + SNDRV_CTL_ELEM_ACCESS_VOLATILE, \ > + .private_value = SOC_DOUBLE_VALUE(reg, shift_left, shift_right, \ > + max, invert, 0) \ > +} This doesn't look like it should be driver specific - what's driver specific about it? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --]
next prev parent reply other threads:[~2015-11-27 12:54 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-11-24 14:43 [PATCH 0/2] Add support for Texas Instruments pcm3168a codec Damien Horsley 2015-11-24 14:43 ` [PATCH 1/2] ASoC: pcm3168a: Add binding document for " Damien Horsley 2015-12-12 23:07 ` Applied "ASoC: pcm3168a: Add binding document for pcm3168a codec" to the asoc tree Mark Brown 2015-11-24 14:43 ` [PATCH 2/2] ASoC: pcm3168a: Add driver for pcm3168a codec Damien Horsley 2015-11-27 12:54 ` Mark Brown [this message] 2015-11-27 12:54 ` Mark Brown 2015-11-30 16:49 ` [alsa-devel] " Damien Horsley 2015-11-30 16:49 ` Damien Horsley 2015-12-01 17:59 ` Mark Brown 2015-12-01 17:59 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151127125411.GR1929@sirena.org.uk \ --to=broonie@kernel.org \ --cc=Damien.Horsley@imgtec.com \ --cc=alsa-devel@alsa-project.org \ --cc=arnd@arndb.de \ --cc=bardliao@realtek.com \ --cc=ckeepax@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=ijc+devicetree@hellion.org.uk \ --cc=james.hartley@imgtec.com \ --cc=jsarha@ti.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mail@maciej.szmigiero.name \ --cc=mark.rutland@arm.com \ --cc=niederp@physik.uni-kl.de \ --cc=pawel.moll@arm.com \ --cc=perex@perex.cz \ --cc=rf@opensource.wolfsonmicro.com \ --cc=robh+dt@kernel.org \ --cc=tiwai@suse.com \ --cc=vinod.koul@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.