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 -- 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
