Skip to content

dai-zephyr: refactor dai params#7345

Closed
juimonen wants to merge 1 commit intothesofproject:mainfrom
juimonen:dai_params_refactor
Closed

dai-zephyr: refactor dai params#7345
juimonen wants to merge 1 commit intothesofproject:mainfrom
juimonen:dai_params_refactor

Conversation

@juimonen
Copy link

@juimonen juimonen commented Mar 29, 2023

Refactor dai params into more feasible functions and remove duplicate code. No functional change.

@juimonen juimonen force-pushed the dai_params_refactor branch from 96965b2 to bdf303c Compare March 29, 2023 11:19
lgirdwood
lgirdwood approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of code removals !
@abonislawski @softwarecki pls review.

@sys-pt1s
Copy link

sys-pt1s commented Apr 6, 2023

Can one of the admins verify this patch?

@lgirdwood
Copy link
Member

lgirdwood commented Apr 14, 2023

@juimonen can you check the CI, mayeb be worth a rebase as IIUC as west update has been merged now.

lyakh
lyakh reviewed Apr 17, 2023
View reviewed changes
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to move dai_set_dma_buffer() as proposed to see if the git diff generated by GH also becomes smaller?

src/audio/dai-zephyr.c Outdated
Copy link
Collaborator

@lyakh lyakh Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd leave these alone - if you want to make lines shorter, which would be good here, just break it at some of the commas, but one warning should be less intrusive than 3. Same in line 672 / 629 below

src/audio/dai-zephyr.c Outdated
Copy link
Collaborator

@lyakh lyakh Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you just move this function to right above dai_params() the diffstat becomes some 40 lines smaller and easier to review

@juimonen
Copy link
Author

juimonen commented Apr 18, 2023

@lyakh the github diff looks just crazy with this... the line length comments I understand, but most other comments just don't seem right.... maybe look at the whole file?

@juimonen juimonen force-pushed the dai_params_refactor branch from bdf303c to 29ab8a8 Compare April 19, 2023 07:46
@lyakh
Copy link
Collaborator

lyakh commented Apr 19, 2023

@lyakh the github diff looks just crazy with this... the line length comments I understand, but most other comments just don't seem right.... maybe look at the whole file?

indeed you're right... See, how important it is to try and make a diff look small and logical! ;-) Yes, I resolved most comments, just if we could merge dev_warn() calls back, that would be better IMHO

@juimonen juimonen force-pushed the dai_params_refactor branch 2 times, most recently from 234ee69 to 0c00eba Compare April 19, 2023 13:04
lyakh
lyakh reviewed Apr 20, 2023
View reviewed changes
src/audio/dai-zephyr.c Outdated
Copy link
Collaborator

@lyakh lyakh Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a long time ago we had a limitation of up to 4 parameters after the format in all prints. I think it was in XTOS, in Zephyr there isn't such a limitation, right? So this should be ok?

src/audio/dai-zephyr.c Outdated
Copy link
Collaborator

@lyakh lyakh Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now this string is wrong - you moved it back to dai_params(). Same below.

src/audio/dai-zephyr.c Outdated
Copy link
Collaborator

@lyakh lyakh Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now, this time I'm not dreaming, am I? Is this intentional - moving the above two checks before calls to ipc_dai_data_config() and dai_verify_params()?

@juimonen juimonen force-pushed the dai_params_refactor branch from 0c00eba to 2707d9a Compare April 20, 2023 08:51
@btian1
Copy link
Contributor

btian1 commented Apr 20, 2023

I am also changing this part with #7229, seems this patch will merge first, I have to rebase #7229 based on this, this is fine.
I noticed code reduction is remarkable, just make sure 100% CI pass before merge to avoid introduce new regression.

@lgirdwood
Copy link
Member

lgirdwood commented Apr 24, 2023

@juimonen can you check the CI. Thanks !

@juimonen juimonen force-pushed the dai_params_refactor branch 2 times, most recently from 45b504c to 86bd6f9 Compare April 26, 2023 06:20
@juimonen
Copy link
Author

juimonen commented Apr 26, 2023

SOFCI TEST

@juimonen juimonen force-pushed the dai_params_refactor branch 2 times, most recently from 56632bc to b36133d Compare April 27, 2023 06:02
@kv2019i
Copy link
Collaborator

kv2019i commented Apr 27, 2023

@juimonen Please check the quickbuild logs, this failed again...

@juimonen juimonen force-pushed the dai_params_refactor branch from b36133d to b461ac5 Compare May 24, 2023 06:31
@juimonen juimonen force-pushed the dai_params_refactor branch from 95462fe to 625ff4c Compare May 29, 2023 07:39
@juimonen juimonen force-pushed the dai_params_refactor branch 2 times, most recently from c413c94 to 4c1b487 Compare June 19, 2023 17:18
btian1
btian1 reviewed Jun 20, 2023
View reviewed changes
Copy link
Contributor

@btian1 btian1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still need fight with QB results, my PR also got QB failures.

@juimonen
Copy link
Author

juimonen commented Jun 20, 2023

@kv2019i rebased. don't think the CI issues are related....

@lgirdwood lgirdwood added this to the v2.7 milestone Jul 3, 2023
@btian1
Copy link
Contributor

btian1 commented Jul 18, 2023

@juimonen , please update and check CI results.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 24, 2023

@juimonen ping

@juimonen juimonen force-pushed the dai_params_refactor branch from 4c1b487 to c19af15 Compare July 26, 2023 06:14
@juimonen
Copy link
Author

juimonen commented Jul 26, 2023

@lgirdwood updated, not sure are we having now some unrelated CI issues...

@lgirdwood
Copy link
Member

lgirdwood commented Jul 26, 2023

SOFCI TEST

@lgirdwood
Copy link
Member

lgirdwood commented Jul 26, 2023

@lgirdwood updated, not sure are we having now some unrelated CI issues...

There was a regression which has now been reverted.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 27, 2023

@juimonen can you try a rebase and force push to unblock CI west failure ?

@juimonen juimonen force-pushed the dai_params_refactor branch from c19af15 to 9916b12 Compare July 28, 2023 05:04
@lgirdwood
Copy link
Member

lgirdwood commented Jul 31, 2023

@juimonen can you rebase and force push agian, just merged a west update.

dai-zephyr: refactor dai params
7a39a61 
Refactor dai params into more feasible functions and remove duplicate
code. No functional change.

Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>
@juimonen juimonen force-pushed the dai_params_refactor branch from 9916b12 to 7a39a61 Compare July 31, 2023 12:01
@lgirdwood
Copy link
Member

lgirdwood commented Jul 31, 2023

@juimonen can you check internal CI result. Thanks !

@lgirdwood
Copy link
Member

lgirdwood commented Aug 18, 2023

@ujfalusi are you able to take this over ?

@alex-cri alex-cri modified the milestones: v2.7, v2.8 Sep 7, 2023
@kv2019i
Copy link
Collaborator

kv2019i commented Sep 13, 2023

@lgirdwood @ujfalusi I'll move this to a draft so it doesn't clutter our PR review lists.

@kv2019i kv2019i marked this pull request as draft September 13, 2023 09:10
@sofci
Copy link
Collaborator

sofci commented Sep 15, 2023

Can one of the admins verify this patch?

reply test this please to run this test once

@RanderWang RanderWang assigned RanderWang and unassigned ujfalusi Oct 30, 2023
@RanderWang RanderWang mentioned this pull request Oct 30, 2023
Merged
@kv2019i
Copy link
Collaborator

kv2019i commented Nov 16, 2023

Merged in #8415

@kv2019i kv2019i closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@btian1 btian1 btian1 left review comments

@lyakh lyakh lyakh approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@softwarecki softwarecki softwarecki approved these changes

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@kv2019i kv2019i Awaiting requested review from kv2019i kv2019i will be requested when the pull request is marked ready for review kv2019i is a code owner

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@RanderWang RanderWang

Labels

None yet

Projects

None yet

Milestone

v2.8

Development

Successfully merging this pull request may close these issues.