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

[Damien Horsley <Damien.Horsley@imgtec.com>]

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