Skip to content

math : Add square root function using a lookup table - #5034

Merged
lgirdwood merged 1 commit into
thesofproject:mainfrom
ShriramShastry:shastry_sqrt_dev
Dec 21, 2021
Merged

math : Add square root function using a lookup table#5034
lgirdwood merged 1 commit into
thesofproject:mainfrom
ShriramShastry:shastry_sqrt_dev

Conversation

@ShriramShastry

@ShriramShastryShriramShastry commented Nov 26, 2021

Copy link
Copy Markdown
Contributor

fix point math square function having positive number y as input and
return the positive number x multiplied by itself (squared)

Signed-off-by: MalladiSastry shastry@mherber2-ub20.ostc.intel.com

@singalsusingalsu mentioned this pull request Nov 26, 2021
Merged
singalsu
singalsu reviewed Nov 26, 2021
View reviewed changes
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/include/sof/math/sqrt_int16.h Outdated
@ShriramShastry
ShriramShastryforce-pushed the shastry_sqrt_dev branch 6 times, most recently from fc0748a to 59bfe17CompareNovember 27, 2021 14:39
@ShriramShastry
ShriramShastry marked this pull request as ready for review November 29, 2021 08:30
singalsu
singalsu reviewed Nov 29, 2021
View reviewed changes
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
singalsu
singalsu reviewed Nov 29, 2021
View reviewed changes
Comment threadtest/cmocka/src/math/arithmetics/square_root.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
@ShriramShastry
ShriramShastryforce-pushed the shastry_sqrt_dev branch 3 times, most recently from 4054b3e to 2b3433eCompareNovember 30, 2021 12:22
singalsu
singalsu reviewed Nov 30, 2021
View reviewed changes
Comment threadsrc/math/sqrt_int16.c Outdated
singalsu
singalsu reviewed Nov 30, 2021
View reviewed changes
Comment threadsrc/include/sof/math/sqrt.h Outdated
singalsu
singalsu approved these changes Dec 1, 2021
View reviewed changes

@singalsusingalsu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You have one not needed type cast (double)y in cmocka function. Otherwise OK so approve. Thanks!

lgirdwood
lgirdwood reviewed Dec 1, 2021
View reviewed changes

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just one minor correction needed.

Comment threadsrc/include/sof/math/sqrt.h Outdated
@ShriramShastry
ShriramShastryforce-pushed the shastry_sqrt_dev branch 2 times, most recently from dbf0b81 to 9814180CompareDecember 1, 2021 11:50
@marc-hb

marc-hb commented Dec 1, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

More C99 comments... can we finally turn off this warning?

EDIT: submitted in #5046

cujomalainey
cujomalainey suggested changes Dec 1, 2021
View reviewed changes
Comment threadtest/cmocka/include/sqrt_tables.h Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
@ShriramShastry
ShriramShastryforce-pushed the shastry_sqrt_dev branch 3 times, most recently from 68db25d to ca8fc26CompareDecember 2, 2021 11:15
marc-hb
marc-hb reviewed Dec 2, 2021
View reviewed changes
Comment threadsrc/math/Kconfig Outdated
Comment threadtest/cmocka/include/sqrt_tables.h Outdated
@ShriramShastry
ShriramShastryforce-pushed the shastry_sqrt_dev branch 3 times, most recently from fe4fcb6 to a3f41ceCompareDecember 15, 2021 17:23
marc-hb
marc-hb reviewed Dec 15, 2021
View reviewed changes

@marc-hbmarc-hb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some minor style issues

Comment threadsrc/include/sof/math/sqrt.h Outdated
Comment threadsrc/include/sof/math/sqrt.h Outdated
Comment threadsrc/math/Kconfig Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
Comment threadsrc/math/sqrt_int16.c Outdated
@lgirdwood

lgirdwood commented Dec 16, 2021

Copy link
Copy Markdown
Member

@cujomalainey are you happy to for this to merge ?

cujomalainey
cujomalainey approved these changes Dec 16, 2021
View reviewed changes

@cujomalaineycujomalainey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one minor comment complaint but otherwise LGTM

Comment threadsrc/math/sqrt_int16.c Outdated

@cujomalaineycujomalaineyDec 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: this comment seems out of place

@ShriramShastryShriramShastryDec 16, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I remove it.

@ShriramShastry
math : Add square root function using lookup table
1212e15
fix point math square function having positive number y as input and
return the positive number x multiplied by itself (squared)
Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>
@cujomalainey

cujomalainey commented Dec 16, 2021

Copy link
Copy Markdown
Contributor

Once CI returns I think we can merge

@singalsu

singalsu commented Dec 20, 2021

Copy link
Copy Markdown
Collaborator

@cujomalainey@lgirdwood CI issues look unrelated.

@singalsu

singalsu commented Dec 21, 2021

Copy link
Copy Markdown
Collaborator

Three approvals, would be nice to get this merged because my PR #4826 can't pass CI without this.

@lgirdwood
lgirdwood merged commit 29fa4e1 into thesofproject:mainDec 21, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lgirdwoodlgirdwoodlgirdwood approved these changes
@singalsusingalsusingalsu approved these changes
@dbalutadbalutaAwaiting requested review from dbalutadbaluta is a code owner
@lbetlejlbetlejAwaiting requested review from lbetlejlbetlej is a code owner
@mmaka1mmaka1Awaiting requested review from mmaka1mmaka1 is a code owner
@plbossartplbossartAwaiting requested review from plbossartplbossart is a code owner
@johnylin76johnylin76Awaiting requested review from johnylin76
@juimonenjuimonenAwaiting requested review from juimonen
@keyonjiekeyonjieAwaiting requested review from keyonjie
@mwaskomwaskoAwaiting requested review from mwasko
@marc-hbmarc-hbAwaiting requested review from marc-hb
+1 more reviewer
@cujomalaineycujomalaineycujomalainey approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ShriramShastry@marc-hb@lgirdwood@cujomalainey@singalsu