Re: [PATCH] ASoc: wm8580: Add the wm8581 codec to the driver

[Matt Flax <flatmax@flatmax.org>]

On 19/10/16 20:22, Charles Keepax wrote:
> On Wed, Oct 19, 2016 at 05:24:35AM +1100, Matt Flax wrote:
>> This patch adds support for the wm8581 codec to the wm8580 driver.
>> The wm8581 codec hardware adds a fourth DAC and otherwise is
>> compatible with the wm8580 codec.
>>
>> of_device_id data is used to allow the driver to select the
>> suitable DAC count specified in the device tree codec selection.
>> The wm8580_driver_data struct is used to store the number of DACs.
>>
>> The snd_soc_dai_driver no longer lists the channels_max for the
>> playback substream. This variable is set during the i2c probe
>> from the of_device_id supplied wm8580_driver_data struct.
>>
>> With knowledge of the number of DACs in use, the DAC4 controls,
>> widgets and routes are added as required for DAC4.
>>
>> The device tree documentation for the wm8580 is altered to list
>> the wm8581 codec support, as is the Kconfig file.
>>
>> Signed-off-by: Matt Flax <flatmax@flatmax.org>
>> ---
> When doing a respin of a patch its usually considered good
> practice to do something like [PATCH v2] in the subject line,
> lets people clearly see that its a new version not a resend.
OK - I see that I should use in-reply-to with the message id next time.
>>   Documentation/devicetree/bindings/sound/wm8580.txt |  4 +-
>>   sound/soc/codecs/Kconfig                           |  2 +-
>>   sound/soc/codecs/wm8580.c                          | 98 ++++++++++++++++++++--
>>   3 files changed, 92 insertions(+), 12 deletions(-)
> <snip>
>> @@ -65,6 +68,8 @@
>>   #define WM8580_DIGITAL_ATTENUATION_DACR2     0x17
>>   #define WM8580_DIGITAL_ATTENUATION_DACL3     0x18
>>   #define WM8580_DIGITAL_ATTENUATION_DACR3     0x19
>> +#define WM8580_DIGITAL_ATTENUATION_DACL4     0x1A
>> +#define WM8580_DIGITAL_ATTENUATION_DACR4     0x1B
> Apologies my fault for still not being clear these two new
> defines would still be best called WM8581...
>
> <snip>
>> +static int wm8580_playback_startup(struct snd_pcm_substream *substream,
>> +			   struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_codec *codec = dai->codec;
>> +	struct wm8580_priv *wm8580 = snd_soc_codec_get_drvdata(codec);
>> +
>> +	return snd_pcm_hw_constraint_single(substream->runtime,
>> +		SNDRV_PCM_HW_PARAM_CHANNELS, wm8580->drvdata->num_dacs * 2);
> Won't this limit us to exactly num_dacs * 2 channels? I would
> have expected a contraint_minmax, I haven't checked the core
> code but would be good to know you have tested smaller numbers
> of channels and they are working. Bearing in mind that some
> user-space applications will do things like pad out to the number
> of channels the hardware claims to require. For example for aplay
> I believe -v will show you if it does any conversion and
> --disable-channels will stop it attempting to convert the number
> of channels.
> Apart from those very small comments this all looks fine to me.
OK, I will use minmax to fix this problem ... I didn't see it at first, 
thanks for pointing it out.

Matt
>
> Thanks,
> Charles