Re: [alsa-devel] [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Arnaud Pouliquen]

On 06/06/2018 11:47 AM, Takashi Iwai wrote:
> On Wed, 06 Jun 2018 11:31:45 +0200,
> Arnaud Pouliquen wrote:
>> 
>> 
>> 
>> On 06/05/2018 08:29 PM, Takashi Iwai wrote:
>> > On Tue, 05 Jun 2018 17:50:57 +0200,
>> > Arnaud Pouliquen wrote:
>> >> 
>> >> Hi Takashi,
>> >> 
>> >> On 04/17/2018 01:17 PM, Mark Brown wrote:
>> >> > On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:
>> >> > 
>> >> >> I guess the blocking patch in this patchset is the patch "add IEC958 
>> >> >> channel status control helper". This patch has been reviewed several 
>> >> >> times, but did not get a ack so far.
>> >> >> If you think these helpers will not be merged, I will reintegrate the 
>> >> >> corresponding code in stm driver.
>> >> >> Please let me know, if I need to prepare a v2 without helpers, or if we 
>> >> >> can go further in the review of iec helpers patch ?
>> >> > 
>> >> > I don't mind either way but you're right here, I'm waiting for Takashi
>> >> > to review the first patch. I'd probably be OK with it just integrated
>> >> > into the driver if we have to go that way though.
>> >> 
>> >> Gentlemen reminder for this patch set. We would appreciate to have your
>> >> feedback on iec helper.
>> >> From our point of view it could be useful to have a generic management
>> >> of the iec controls based on helpers instead of redefining them in DAIs.
>> >> Having the same behavior for these controls could be useful to ensure
>> >> coherence of the control management used by application (for instance
>> >> Gstreamer uses it to determine iec raw mode capability for iec61937 streams)
>> > 
>> > Oh sorry for the late reply, I've totally overlooked the thread.
>> > 
>> > And, another sorry: the patchset doesn't look convincing enough to
>> > me.
>> > 
>> > First off, the provided API definition appears somewhat
>> > unconventional, the mixture of the ops, the static data and the
>> > dynamic data.
>> Sorry i can't figure out your point. I suppose that you speak about the
>> snd_pcm_iec958_params.
>> what would be a more conventional API?
> 
> Imagine you'd want to put a const to the data passed to the API for
> hardening. The current struct is a mixture of static and dynamic
> data.
> 
> 
>> > Moreover, this is only for your driver, ATM. 
>> It is also compatible with the sound/sti driver, even if we does not
>> propose patch yet. We also plan to propose an implementation, for the
>> HDMI_codec that would need to export a control to allow none-audio mode.
>> 
>> >If it were an API that
>> > does clean up the already existing usages, I'd happily apply it. There
>> > are lots of drivers creating and controlling the IEC958 ctls even
>> > now.
>> > 
>> > Also, the patchset requires more fine-tuning, in anyways. The changes
>> > in create_iec958_consumre() are basically irrelevant, should be split
>> > off as an individual fix. it is linked to the control, as not possible in existing implementation
>> (rate and width are get from hwparam or runtime). But no problem we can
>> split it in a separate patch.
>> 
>> Also, the new function doesn't create the
>> > "XXX Mask" controls. And the byte comparison should be replaced with
>> > memcmp(), etc, etc.
>> Yes mask are missing, can be added. For the rest could you comment
>> directly in code? i suppose that you want to replace the for loops by
>> memcmp, memcpy...
> 
> Right.
> 
>> > So, please proceed rather with the open codes for now. If you can
>> > provide a patch that cleans up the *multiple* driver codes, again,
>> > I'll happily take it. But it can be done anytime later, too.
>> Not simple to clean up the other drivers as this control is a PCM
>> control, that is mainly implemented as a mixer or card control.
>> This means that it should be registered on the pcm_new in CPU DAI or in
>> the DAI codec, to be able to bind it to the PCM device.
>> Inpact is not straigthforward as this could generate regression on driver.
> 
> Yes, and that's my point. The application of API is relatively
> limited -- although the API itself has nothing to do with ASoC at
> all.
> 
>> For now We can add the clean up on the sti driver based on this helper,
>> and we are working on the HDMI_codec, we could also use this helper to
>> export the control....
>> 
>> So if you estimate that it is interesting to purchase on this helper we
>> can try to come back with a patch set that implements the helper for
>> the 3 drivers.
> 
> Right. Basically there are two cases we add a new API:
> 
> 1. It's absolutely new and nothing else can do it
> 2. API simplifies the whole tree, not only one you're trying to add.
> 
> And in this case, let's prove 2 at first, that the API *is* actually
> useful in multiple situations we already have. Then I'll happily ack
> for that. More drivers cleanup, better. At best, think of more
> range above ASoC, as you're proposing ALSA core API, not the ASoC
> one.
> 
>> The other option, is that we drop the helpers, and implement the control
>> directly in our drivers.
> 
> This is of course another, maybe easier option.
> 
>> Please just tell us if we should continue to propose the helpers or not.
> 
> I have no preference over two ways, but am only interested in the
> resulting patches :)

My tentative here was to start to introduce helpers at ALSA level (not
only ASoC) to have a generic implementation of the this generic control.
Today the snd_pcm_create_iec958_consumer_hw_params just allow to fill
AES based on runtime parameters, but not to offer a generic management
of iec control.
Now you are right i'm developing under ASoC and i have not the whole
knowledge of the ALSA drivers, an probably too limited view of the iec
controls usage.

Based on your feedback i think (at least in a first step) we will choose
the easiest option for the STM driver...

Thanks
Arnaud


> 
> 
> thanks,
> 
> Takashi