Skip to content

include: ipc: Add compress_params interface file#5590

Merged
dbaluta merged 3 commits into
thesofproject:mainfrom
dbaluta:add_compr
Apr 12, 2022
Merged

include: ipc: Add compress_params interface file#5590
dbaluta merged 3 commits into
thesofproject:mainfrom
dbaluta:add_compr

Conversation

@dbaluta

@dbaluta dbaluta commented Mar 23, 2022

Copy link
Copy Markdown
Collaborator

This file is copied 'as is' from the Linux kernel repo
and will be used to have a common interface for passing
compressed stream params.

Signed-off-by: Daniel Baluta [email protected]

@dbaluta

dbaluta commented Mar 23, 2022

Copy link
Copy Markdown
Collaborator Author

@lgirdwood @plbossart I kept the license intact from the original Linux kernel file.

RanderWang
RanderWang reviewed Mar 24, 2022
View reviewed changes

@RanderWang RanderWang 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.

why we copy it to sof ? can we leave it in linux kernel and include it ?

@dbaluta

dbaluta commented Mar 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

why we copy it to sof ? can we leave it in linux kernel and include it ?

How does this inclusion will work? Linux kernel and SOF are separate repos.

@dbaluta

dbaluta commented Mar 24, 2022

Copy link
Copy Markdown
Collaborator Author

@plbossart @lgirdwood I wonder if the license is compatbile with SOF. it should be, but i'm not experimented in this area.

@lgirdwood

lgirdwood commented Mar 24, 2022

Copy link
Copy Markdown
Member

@plbossart @lgirdwood I wonder if the license is compatbile with SOF. it should be, but i'm not experimented in this area.

MIT license is permissive, and is already used in xtos/HAL files so no issue there.

However, the GPL part should not be selected when using dual licensed files with SOF. I'm not sure what's the correct way to import dual licensed files and say we use the MIT license for SOF usage ? Maybe a big comment at the top of the file ?

@plbossart any inputs ?

@paulstelian97

paulstelian97 commented Mar 25, 2022

Copy link
Copy Markdown
Collaborator

I believe that in dual-licensed files if only one of the licenses is appropriate for a given usage it applies. So in MIT/GPL licensed files inside SOF firmware we have MIT applying, as an example.

I'm no legal expert so double check, but this is my understanding. No special need to highlight one of the two licenses.

@lgirdwood

lgirdwood commented Mar 28, 2022

Copy link
Copy Markdown
Member

I believe that in dual-licensed files if only one of the licenses is appropriate for a given usage it applies. So in MIT/GPL licensed files inside SOF firmware we have MIT applying, as an example.

I'm no legal expert so double check, but this is my understanding. No special need to highlight one of the two licenses.

Yep, that's my take too, the kernel takes dual license but the LICENSE files clarifies that kernel selects the GPL license.
@dbaluta are you OK to add a statement at the top of the SOF LICENSE file that states
"SOF reuses some dual licensed header files where interfaces must be maintained between SOF and other software. Where a dual licensed file contains the GPL (any version) as a license option, then SOF will always use the permissive license option (e.g. BSD, MIT or Apache or another permissive license) instead of the GPL option."

@dbaluta dbaluta requested review from lbetlej and mmaka1 as code owners March 28, 2022 11:39
@dbaluta

dbaluta commented Mar 28, 2022

Copy link
Copy Markdown
Collaborator Author

@lgirdwood i think your suggestion is very good. I added it in a separate patch.

lgirdwood
lgirdwood approved these changes Mar 28, 2022
View reviewed changes

@lgirdwood lgirdwood 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.

Lets get inputs from @plbossart as the compressed API author.

@dbaluta dbaluta mentioned this pull request Mar 28, 2022
Merged
@lgirdwood

lgirdwood commented Mar 30, 2022

Copy link
Copy Markdown
Member

@dbaluta can you check doxygen report, could be a typo in the docs.

@dbaluta

dbaluta commented Mar 30, 2022

Copy link
Copy Markdown
Collaborator Author

@lgirdwood the warning looks very strange.

 /home/runner/work/sof/sof/src/include/ipc/compress_params.h:374: error: Found unknown command '\max_ch' (warning treated as error, aborting now)

Couldn't spot any \ and that line. Investigating ...

@dbaluta

dbaluta commented Mar 30, 2022

Copy link
Copy Markdown
Collaborator Author

SOFCI TEST

@lgirdwood

lgirdwood commented Apr 5, 2022

Copy link
Copy Markdown
Member

@lgirdwood the warning looks very strange.

 /home/runner/work/sof/sof/src/include/ipc/compress_params.h:374: error: Found unknown command '\max_ch' (warning treated as error, aborting now)

Couldn't spot any \ and that line. Investigating ...

Tabs or spaces issue ?

@dbaluta dbaluta force-pushed the add_compr branch 6 times, most recently from 01e97bf to 916dce4 Compare April 6, 2022 15:44
dbaluta added 3 commits April 6, 2022 18:58
@dbaluta
include: ipc: Add compress_params interface file
76b9e47 
This file is copied 'as is' from the Linux kernel repo
and will be used to have a common interface for passing
compressed stream params.

Notice that Doxygen doesn't recognize markup style for structure
commenting so we had to escape @ symbols.

Signed-off-by: Daniel Baluta <[email protected]>
@dbaluta
LICENCE: Add clarification for dual license files
bfce25e 
We often need to include interface files from other projects. When the
projects are dual licensed we always go for the more permissive license.

Suggested-by: Liam Girdwood <[email protected]>
Signed-off-by: Daniel Baluta <[email protected]>
@dbaluta
LICENCE: Add Khronos Group Inc license header
7b52d3f 
This is introduced with compress_params.h file.

Signed-off-by: Daniel Baluta <[email protected]>
@dbaluta

dbaluta commented Apr 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@lgirdwood i fixed the doxygen problems. The problem is that markup style for documenting structures uses @ which is a marker for doxygen commands.

I escaped @ and it works now. I think this is ready to be merged.

@lgirdwood

lgirdwood commented Apr 6, 2022

Copy link
Copy Markdown
Member

SOFCI TEST

@dbaluta dbaluta merged commit cf80889 into thesofproject:main Apr 12, 2022
@dbaluta dbaluta deleted the add_compr branch April 12, 2022 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulstelian97 paulstelian97 paulstelian97 approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@marcinszkudlinski marcinszkudlinski Awaiting requested review from marcinszkudlinski

@ranj063 ranj063 Awaiting requested review from ranj063

@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

+1 more reviewer

@RanderWang RanderWang RanderWang left review comments

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.

4 participants

@dbaluta @lgirdwood @paulstelian97 @RanderWang