Re: [alsa-devel] [PATCH 2/2] ASoC: pcm3168a: Add driver for pcm3168a codec

Re: [alsa-devel] [PATCH 2/2] ASoC: pcm3168a: Add driver for pcm3168a codec

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH 2/2] ASoC: pcm3168a: Add driver for pcm3168a codec

[Damien Horsley]

On 27/11/15 12:54, Mark Brown wrote:
> 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.
> 

The list was generated by scripts/get_maintainer.pl when used on the
patches. I will remove some that look like they don't need to be there.

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

Ok

>> +	val = slave_mode ? 0 : ((i + 1) << shift);
> 
> Please write normal if statements unless there's a strong reason to use
> the ternery operator.
>

Ok

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

Should the value returned by snd_soc_params_to_frame_size be treated as
the minimum frame size?

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

Ok

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

Ok

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

There is nothing driver specific. I will add this to include/sound/soc.h
as SOC_DOUBLE_STS

Re: [alsa-devel] [PATCH 2/2] ASoC: pcm3168a: Add driver for pcm3168a codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On Mon, Nov 30, 2015 at 04:49:30PM +0000, Damien Horsley wrote:
> On 27/11/15 12:54, Mark Brown wrote:

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

> The list was generated by scripts/get_maintainer.pl when used on the
> patches. I will remove some that look like they don't need to be there.

You should not be using get_maintainer unfiltered, you need to think
about why everyone is there.  People often end up getting included due
to just random cleanups and getting lots of mail about drivers they have
no lasting interest in may be demotivating.

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

> Should the value returned by snd_soc_params_to_frame_size be treated as
> the minimum frame size?

Yes, in general most hardware is perfectly happy with extra clocks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]