Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ
To:     Kirill Marinushkin <kmarinushkin@birdec.com>
CC:     Mark Brown <broonie@kernel.org>, Takashi Iwai <tiwai@suse.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Matthias Reichl <hias@horus.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>
References: <20201109212133.25869-1-kmarinushkin@birdec.com>
 <690508c7-7029-6781-a1a2-0609e37cb9e6@ti.com>
 <a3df4fb0-35cd-4757-2037-d4ff80e9f74c@birdec.com>
 <d0f76607-c3a5-9b87-dc0e-eddbce585558@ti.com>
 <6e14cdc3-2d45-d7bc-14eb-30c6ea568078@birdec.com>
From:   Peter Ujfalusi <peter.ujfalusi@ti.com>
Message-ID: <43973577-f719-b414-395c-91a4e1a3b460@ti.com>
Date:   Thu, 12 Nov 2020 10:59:07 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.2
MIME-Version: 1.0
In-Reply-To: <6e14cdc3-2d45-d7bc-14eb-30c6ea568078@birdec.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Kirill,

On 12/11/2020 9.57, Kirill Marinushkin wrote:
>> The set_fmt callback is there to set the bus format, it has nothing to
>> do (in most cases) with the sample format (hw_params). Bus coding, clock
>> source has nothing to do with hw_params.
>>
>> When you bind a link you will use set_fmt for the two sides to see if
>> they can agree, that both can support what has been asked.
>>
>> The pcm512x driver just saves the fmt and say back to that card:
>> whatever, I'm fine with it. But runtime during hw_params it can fail due
>> to unsupported bus format, which it actually acked to be ok.
>>
>> This is the difference.
>>
>> Sure, some device have constraint based on the fmt towards the hw_params
>> and it is perfectly OK to do such a checks and rejections or build
>> rules/constraints based on fmt, but failing hw_params just because
>> set_fmt did not checked that the bus format is not even supported is not
>> a nice thing to do.
> 
> Those are good arguments
> 
>>> Would you agree?
>>
>> I don't have a device to test, I'm just trying to point out what is the
>> right thing to do.
> 
> I have a device to test. I will move format checks into `pcm512x_set_fmt()`,
> ensure that it works properly, and submit as patch v3.

Thank you. I know it is slightly more work.

When you send v3 please do not use --in-reply-to, let it be itself.

Regards,
- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki