drivers: dai: add ALH dai driver #45104

abonislawski · 2022-04-25T08:00:00Z

The ALH is an intermediary device, which acts as a hub and provides an
abstracted support for numerous sound interfaces (e.g. SoundWire).

abonislawski · 2022-05-10T10:58:28Z

@marcinszkudlinski @nashif please review

drivers/dai/CMakeLists.txt

drivers/dai/intel/alh/CMakeLists.txt

include/zephyr/drivers/dai.h

marcinszkudlinski · 2022-05-11T08:05:45Z

dts/bindings/alh/intel,cavs-alh-dai.yaml

label is also required, pls add

Can I ask why label is required?

somehow we need to bind to the device.

drivers/dai/intel/alh/Kconfig.alh

drivers/dai/intel/alh/alh.c

abonislawski · 2022-05-23T15:25:54Z

Updated with unified version (cavs & ace supported)

dcpleung · 2022-05-25T16:39:59Z

Since you are adding SET_BITS() in sys/util_macros.h, it conflicts with the one in drivers/audio/intel_dmic.c

Extend dai properties with stream ID parameter Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

abonislawski · 2022-05-25T16:48:08Z

Since you are adding SET_BITS() in sys/util_macros.h, it conflicts with the one in drivers/audio/intel_dmic.c

Yes, I see in the logs, hopefully this time it will pass every check

EDIT: sorry, reverted to original form with copy in alh.h, there are tons of SET_BITS definitions

juimonen

CI looks green now and dai properties is a separate commit. I can't test the actual driver myself now, but I trust you have tested it. kudos for cavs25 dtsi -> this will be tested a lot for sure (other than fpga). so LGTM.

drivers/dai/intel/alh/alh.c

MaureenHelm · 2022-05-25T20:04:25Z

drivers/dai/intel/alh/alh.c

Why is this hard coded to instance 0? Will this driver work for multiple instances?

They are the same for every instance so it doesnt matter but how to do it in more clean way?

For starters, it is recommended to comment usage of a hardcode.

MaureenHelm · 2022-05-25T20:05:41Z

drivers/dai/intel/alh/alh.c

Same question about hard coded instance

drivers/dai/intel/alh/alh.c

MaureenHelm · 2022-05-25T20:46:15Z

drivers/dai/intel/alh/alh.c

Please stay with this approach

drivers/dai/intel/alh/alh.h

MaureenHelm · 2022-05-25T20:48:18Z

drivers/dai/intel/alh/alh.h

These might be better as devicetree properties

cc: @laurenmurphyx64

MaureenHelm · 2022-05-25T20:51:25Z

dts/xtensa/intel/intel_cavs25.dtsi

The first value here needs to match the value in line 262; otherwise you will get a build warning. Twister converts build warnings to errors but this PR somehow passes CI, so is there a sample or test application configuration that builds with this new driver?

@MaureenHelm is there a "clean" way to create several instances of a driver with the same set of registers?

Changed both of them to alh@24400, surprisingly it builds without warnings (previously there was a warning as you said but it was not treated as error in cavs25 CI), is that correct?

Maybe twister only converts compiler warnings to errors, and not devicetree warnings too? What caught my attention in your previous version was the odd base address ending in 1. Also, is the second value here an address? It looks like an address, but it should be the size of the memory region.

Why are there multiple instances of the driver at the same address? How do you decide how many driver instances there should be? Is it a property of the hardware?

Maybe twister only converts compiler warnings to errors, and not devicetree warnings too?

I remember I had to fix two issues in #41706 to stop -Wa,--fatal-warnings from being ignored. I see --edtlib-Werror in the same place but maybe it's being ignored too because of some other bug?

If it's not tested then it does not work and error handling is rarely ever tested. It took a number of coincidences to notice that -Wa,--fatal-warnings was being ignored.

dcpleung · 2022-05-26T15:50:27Z

drivers/dai/intel/alh/Kconfig.alh

If you add this before config,

DT_COMPAT_INTEL_ALH_DAI := intel,alh-dai

and this inside config

default $(dt_compat_enabled,$(DT_COMPAT_INTEL_ALH_DAI))

This will allow the driver to be enabled whenever a devicetree node has the compatible value is enabled. This will allow CI to build this driver for cAVS 2.5

Sure, we can do it later but for now @juimonen requested to not enabling this driver until sof can use it (changes in review)

No, it needs to be enabled somewhere before we can merge it. Otherwise, it will not be included in CI and we won't know if it even builds.

@abonislawski @juimonen I think we can proceed with a) a test case, b) enable it for cAVS2.5 SOF build, c) add a new board file using this.

@dcpleung @MaureenHelm driver enabled again as you requested, hopefully it will pass CI :)

Thanks @abonislawski

MaureenHelm · 2022-05-26T19:20:09Z

drivers/dai/intel/alh/Kconfig.alh

No, it needs to be enabled somewhere before we can merge it. Otherwise, it will not be included in CI and we won't know if it even builds.

MaureenHelm · 2022-05-26T19:33:06Z

dts/xtensa/intel/intel_cavs25.dtsi

Maybe twister only converts compiler warnings to errors, and not devicetree warnings too? What caught my attention in your previous version was the odd base address ending in 1. Also, is the second value here an address? It looks like an address, but it should be the size of the memory region.

Why are there multiple instances of the driver at the same address? How do you decide how many driver instances there should be? Is it a property of the hardware?

The ALH is an intermediary device, which acts as a hub and provides an abstracted support for numerous sound interfaces (e.g. SoundWire). Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

MaureenHelm · 2022-05-31T19:36:03Z

drivers/dai/intel/alh/Kconfig.alh

Thanks @abonislawski

#45104 (review)

Changes addressed.

github-actions bot added area: API Changes to public APIs area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: Samples Samples labels Apr 25, 2022

abonislawski requested review from lgirdwood and marcinszkudlinski April 25, 2022 13:35

abonislawski marked this pull request as ready for review April 26, 2022 06:38

abonislawski requested review from MaureenHelm, carlescufi, dcpleung, galak and nashif as code owners April 26, 2022 06:38

zephyrbot added area: Audio area: Xtensa Xtensa Architecture labels Apr 26, 2022

zephyrbot requested review from andyross, kv2019i, lyakh, marc-hb, mbolivar-nordic and mengxianglinx April 26, 2022 20:39

marc-hb removed their request for review April 26, 2022 21:19

marcinszkudlinski requested changes May 11, 2022

View reviewed changes

abonislawski force-pushed the zephyr_dai_alh_upstream branch from 984a5f6 to 106feef Compare May 23, 2022 15:23

abonislawski requested a review from marcinszkudlinski May 23, 2022 15:26

abonislawski force-pushed the zephyr_dai_alh_upstream branch from 106feef to 483c20e Compare May 23, 2022 15:28

marcinszkudlinski previously approved these changes May 24, 2022

View reviewed changes

kv2019i added the platform: Intel ADSP Intel Audio platforms label May 24, 2022

kv2019i requested a review from juimonen May 24, 2022 10:46

abonislawski requested review from dcpleung and mwasko and removed request for dcpleung and marcinszkudlinski May 25, 2022 15:47

abonislawski force-pushed the zephyr_dai_alh_upstream branch from c3f47bb to 504b128 Compare May 25, 2022 16:44

abonislawski requested a review from tbursztyka as a code owner May 25, 2022 16:44

abonislawski force-pushed the zephyr_dai_alh_upstream branch from 504b128 to e37bb46 Compare May 25, 2022 16:46

drivers: dai: extend dai properties

25244c7

Extend dai properties with stream ID parameter Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

abonislawski force-pushed the zephyr_dai_alh_upstream branch from e37bb46 to cbfd056 Compare May 25, 2022 17:15

juimonen previously approved these changes May 25, 2022

View reviewed changes

dcpleung previously approved these changes May 25, 2022

View reviewed changes

MaureenHelm reviewed May 25, 2022

View reviewed changes

abonislawski dismissed stale reviews from dcpleung and juimonen via 2329316 May 26, 2022 07:28

abonislawski force-pushed the zephyr_dai_alh_upstream branch from cbfd056 to 2329316 Compare May 26, 2022 07:28

dcpleung reviewed May 26, 2022

View reviewed changes

MaureenHelm requested changes May 26, 2022

View reviewed changes

abonislawski force-pushed the zephyr_dai_alh_upstream branch from 2329316 to 2c25f09 Compare May 31, 2022 06:39

drivers: dai: add ALH dai driver

2c25f09

The ALH is an intermediary device, which acts as a hub and provides an abstracted support for numerous sound interfaces (e.g. SoundWire). Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

dcpleung approved these changes May 31, 2022

View reviewed changes

rljordan assigned MaureenHelm May 31, 2022

MaureenHelm approved these changes May 31, 2022

View reviewed changes

drivers/dai/intel/alh/Kconfig.alh Outdated

Copy link

Member

MaureenHelm May 31, 2022

Choose a reason for hiding this comment

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

Thanks @abonislawski

ceolin approved these changes May 31, 2022

View reviewed changes

marcinszkudlinski approved these changes Jun 1, 2022

View reviewed changes

carlescufi merged commit 638cfbb into zephyrproject-rtos:main Jun 5, 2022

drivers: dai: add ALH dai driver #45104

drivers: dai: add ALH dai driver #45104

Uh oh!

Conversation

abonislawski commented Apr 25, 2022

Uh oh!

abonislawski commented May 10, 2022

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

abonislawski commented May 23, 2022

Uh oh!

dcpleung commented May 25, 2022

Uh oh!

abonislawski commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juimonen left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

abonislawski May 26, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

abonislawski May 26, 2022 • 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!

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!

Reviewers

Assignees

abonislawski commented May 25, 2022 •

edited

Loading

abonislawski May 26, 2022 •

edited

Loading

abonislawski May 26, 2022 •

edited

Loading