Audio: ASRC: Code reorganization by andrula-song · Pull Request #8514 · thesofproject/sof

andrula-song · 2023-11-23T03:19:38Z

Move all ASRC related header files into src/audio/asrc folder.
split IPC version related code to asrc_ipc3.c and asrc_ipc4.c.

keqiaozhang · 2023-11-23T07:49:32Z

SOFCI TEST

RanderWang

LGTM

lgirdwood

We can make the Cmake/Kconfig changes later as there are other PRs that need to be merged 1st.

lgirdwood · 2023-11-24T13:16:12Z

@singalsu @andrula-song @btian1 These should really be in src/audio/module_name/CMakefile.txt (same rule applies to Kconfig too)

I agree, shall we use a separate pr to do the clean up once we finished all module converter.
this is because the in-complete of Cmakelist in module, some does not have, some does not used.

@lgirdwood , this change is based on search, so I request use a separate PR to track clean up src/audio/cMakeList.txt

Hmm, "asrc_sources" was not set before this PR, so why do it now. It seems we already go to the asrc subdirectory, so I don't see why we need to add individual source files here.

actually, it is first set in line 196 as "set(asrc_sources asrc/asrc.c asrc/asrc_farrow.c asrc/asrc_farrow_generic.c)". Adding here is just to keep the asrc_sources right after I refine ASRC code.

Ack @andrula-song . I still don't understand why we set the module sources list twice, but clearly this was not added in this PR, so should not block this one. Let's handle separately.

I still don't understand why we set the module sources list twice,

Same "LIBRARY" explanation as in #8595 (comment), isn't it?

andrula-song · 2023-11-27T05:32:45Z

SOFCI TEST

lgirdwood

LGTM, needs the cleanups in a different patch.

lyakh

I like it much better with the typedef!

RanderWang

Why we need to move it to ASRC folder ? Thanks!

keqiaozhang · 2023-11-30T08:49:46Z

SOFCI TEST

lgirdwood

I'm good here.

lgirdwood · 2023-11-30T17:12:46Z

This is a code change (I know, no impact), but should come in another patch.

lgirdwood · 2023-11-30T17:16:32Z

This is fine for now. We can incrementally update if needed.

lgirdwood · 2023-11-30T17:17:44Z

Fine for now, we can split this further later if needed.

Thank you

marc-hb · 2023-12-04T07:17:20Z

ALSA plugin build failure https://github.com/thesofproject/sof/actions/runs/7081524859/job/19272763291?pr=8514 discussed in #8538

btian1 · 2023-12-06T07:36:28Z

@lyakh , with typedef, there are codestyle errors, do you still insist use typedef, how about #define, or directly use # if IPC3?

lyakh · 2023-12-06T11:17:09Z

@lyakh , with typedef, there are codestyle errors, do you still insist use typedef, how about #define, or directly use # if IPC3?

@btian1 I never insisted on that, it was my proposal. I still think it's ok to ignore code style complaints in this case, but if others disagree we can also use a #define which would be my second preference.

plbossart · 2023-12-06T15:52:25Z

this isn't really a code reorganization... It's really not good practice to add such configuration changes in the middle or a code move...

This should be in a separate patch IMHO.

add a separate commit at last.

marc-hb · 2023-12-06T16:24:26Z

but if others disagree we can also use a #define which would be my second preference.

Checkpatch is not always right: https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html

Checkpatch is not always right. Your judgement takes precedence over checkpatch messages. If your code looks better with the violations, then its probably best left alone.

I wish people obsessed at least as much about actual test results as about checkpatch. "Do not add any new typedef" looks especially wrong. Maybe it makes sense in the Linux kernel but not here. The less pre-processing the better; the indirection is a nightmare to debug (recent example: zephyrproject-rtos/zephyr#65953)

kv2019i

Mostly look good, a few minor comments inline.

kv2019i · 2023-12-07T11:58:42Z

Hmm, "asrc_sources" was not set before this PR, so why do it now. It seems we already go to the asrc subdirectory, so I don't see why we need to add individual source files here.

kv2019i · 2023-12-07T12:00:20Z

C-style comments would be better

kv2019i · 2023-12-07T12:00:26Z

Move the ASRC related header files into ASRC folder. Signed-off-by: Andrula Song <andrula.song@intel.com>

Use typedef ipc_asrc_cfg to replace the splited IPC version asrc module config structure. Signed-off-by: Andrula Song <andrula.song@intel.com>

Move IPC3 related code to asrc_ipc3.c and IPC4 related code to asrc_ipc4.c. Signed-off-by: Andrula Song <andrula.song@intel.com>

Make HiFi4 share c source files with HiFi3. Signed-off-by: Andrula Song <andrula.song@intel.com>

singalsu

Looks good, thanks! With assumption Kai's findings fixed.

kv2019i · 2023-12-11T10:54:27Z

Ack @andrula-song . I still don't understand why we set the module sources list twice, but clearly this was not added in this PR, so should not block this one. Let's handle separately.

btian1 reviewed Nov 23, 2023

View reviewed changes

Comment thread src/audio/asrc/asrc_ipc3.c Resolved

andrula-song force-pushed the asrc-reorg branch from 100cd0b to 17291ee Compare November 23, 2023 08:41

andrula-song marked this pull request as ready for review November 24, 2023 01:44

andrula-song requested review from RanderWang, dbaluta, kv2019i, lbetlej, lgirdwood, marc-hb, marcinszkudlinski, mmaka1, pblaszko and plbossart as code owners November 24, 2023 01:44

RanderWang reviewed Nov 24, 2023

View reviewed changes

Comment thread src/audio/CMakeLists.txt Resolved

btian1 reviewed Nov 24, 2023

View reviewed changes

Comment thread src/audio/asrc/asrc.c Resolved

Comment thread src/audio/asrc/asrc.c Resolved

Comment thread src/audio/asrc/asrc.c Resolved

Comment thread src/audio/asrc/asrc.c Resolved

Comment thread src/audio/asrc/asrc.c Resolved

Comment thread src/audio/asrc/asrc.h Resolved

lyakh reviewed Nov 24, 2023

View reviewed changes

Comment thread src/audio/asrc/asrc.c Resolved

andrula-song force-pushed the asrc-reorg branch from 17291ee to 764500f Compare November 24, 2023 10:00

andrula-song changed the title ~~Audio: ASRC: Code clean~~ Audio: ASRC: Code reorganization Nov 24, 2023

lgirdwood reviewed Nov 24, 2023

View reviewed changes

lgirdwood reviewed Nov 27, 2023

View reviewed changes

Comment thread src/audio/asrc/asrc.c Resolved

andrula-song force-pushed the asrc-reorg branch from 764500f to 232dde8 Compare November 28, 2023 01:52

lyakh reviewed Nov 28, 2023

View reviewed changes

Comment thread src/audio/asrc/asrc_ipc4.c Resolved

andrula-song force-pushed the asrc-reorg branch from 232dde8 to 0e3f1f2 Compare November 29, 2023 01:45

marc-hb previously requested changes Nov 29, 2023

View reviewed changes

Comment thread zephyr/CMakeLists.txt Resolved

marc-hb mentioned this pull request Nov 30, 2023

Audio: multiband_drc: multiband drc code re-organize and split #8526

Merged

This comment was marked as outdated.

Sign in to view

RanderWang reviewed Nov 30, 2023

View reviewed changes

lgirdwood approved these changes Nov 30, 2023

View reviewed changes

plbossart requested changes Nov 30, 2023

View reviewed changes

Comment thread src/audio/asrc/asrc.h Resolved

andrula-song force-pushed the asrc-reorg branch from 0e3f1f2 to b91436d Compare December 1, 2023 03:30

andrula-song force-pushed the asrc-reorg branch from b91436d to 79b9546 Compare December 4, 2023 03:10

lyakh approved these changes Dec 6, 2023

View reviewed changes

plbossart reviewed Dec 6, 2023

View reviewed changes

andrula-song force-pushed the asrc-reorg branch from 79b9546 to b62e218 Compare December 7, 2023 09:32

kv2019i reviewed Dec 7, 2023

View reviewed changes

andrula-song added 4 commits December 8, 2023 09:54

Audio: ASRC: Move header files into ASRC folder

9774997

Move the ASRC related header files into ASRC folder. Signed-off-by: Andrula Song <andrula.song@intel.com>

Audio: ASRC: Use typedef to replace IPC version asrc cfg

35fc3fb

Use typedef ipc_asrc_cfg to replace the splited IPC version asrc module config structure. Signed-off-by: Andrula Song <andrula.song@intel.com>

Audio: ASRC: Split IPC version code to speclfic file

4c9e82f

Move IPC3 related code to asrc_ipc3.c and IPC4 related code to asrc_ipc4.c. Signed-off-by: Andrula Song <andrula.song@intel.com>

Audio: ASRC: Make HiFi4 share c source files with HiFi3

756314f

Make HiFi4 share c source files with HiFi3. Signed-off-by: Andrula Song <andrula.song@intel.com>

andrula-song force-pushed the asrc-reorg branch from b62e218 to 756314f Compare December 8, 2023 01:58

lgirdwood approved these changes Dec 8, 2023

View reviewed changes

plbossart approved these changes Dec 8, 2023

View reviewed changes

singalsu reviewed Dec 11, 2023

View reviewed changes

singalsu approved these changes Dec 11, 2023

View reviewed changes

kv2019i approved these changes Dec 11, 2023

View reviewed changes

kv2019i merged commit bfb6780 into thesofproject:main Dec 11, 2023

kv2019i mentioned this pull request Dec 14, 2023

[FEATURE] organize audio module source code to per-module directories #8630

Closed

Conversation

andrula-song commented Nov 23, 2023

Uh oh!

Uh oh!

keqiaozhang commented Nov 23, 2023

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrula-song Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrula-song commented Nov 27, 2023

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

keqiaozhang commented Nov 30, 2023

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marc-hb commented Dec 4, 2023

Uh oh!

btian1 commented Dec 6, 2023

Uh oh!

lyakh commented Dec 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

andrula-song Dec 8, 2023 •

edited

Loading

marc-hb commented Dec 6, 2023 •

edited

Loading

singalsu left a comment •

edited

Loading