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@xxxxxxxxxx>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
