Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 10 Feb 2020 09:50:17 -0500
From:   Adam Serbinski <adam@serbinski.com>
To:     Mark Brown <broonie@kernel.org>
Cc:     Srini Kandagatla <srinivas.kandagatla@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Patrick Lai <plai@codeaurora.org>,
        Banajit Goswami <bgoswami@codeaurora.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/8] ASoC: qdsp6: q6afe: add support to pcm ports
In-Reply-To: <20200210133143.GG7685@sirena.org.uk>
References: <20200207205013.12274-1-adam@serbinski.com>
 <20200209154748.3015-1-adam@serbinski.com>
 <20200209154748.3015-3-adam@serbinski.com>
 <20200210133143.GG7685@sirena.org.uk>
User-Agent: Roundcube Webmail/1.4-beta
Message-ID: <4df03448f7919187a8a056d3f10415ab@serbinski.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-02-10 08:31, Mark Brown wrote:
> On Sun, Feb 09, 2020 at 10:47:42AM -0500, Adam Serbinski wrote:
> 
>> 
>> +#define AFE_API_VERSION_PCM_CONFIG	0x1
>> +/* Enumeration for the auxiliary PCM synchronization signal
>> + * provided by an external source.
>> + */
>> +
>> +#define AFE_PORT_PCM_SYNC_SRC_EXTERNAL 0x0
>> +/*	Enumeration for the auxiliary PCM synchronization signal
>> + * provided by an internal source.
>> + */
> 
> This is a *weird* commenting style for these #defines and it's not
> consistent within the block, I'm seeing at least 3 different styles.

I will clean up the commenting.


>> +	default:
>> +		break;
>> +	}
> 
> Why is this not returning an error on unsupported values?

Only to be consistent with the pre-existing implementation for i2s 
ports.
I will add an error return.

> 
>> +
>> +	switch (cfg->sample_rate) {
>> +	case 8000:
>> +		pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_128;
>> +		break;
>> +	case 16000:
>> +		pcfg->pcm_cfg.frame_setting = AFE_PORT_PCM_BITS_PER_FRAME_64;
>> +		break;
>> +	}
> 
> Same here.

I will also add the error return here.