Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/sensor/lsm6dsl: Use DT defines to select I2C/SPI bus #12105

Merged
merged 1 commit into from
Jan 8, 2019

Conversation

avisconti
Copy link
Collaborator

Remove configuration parameter CONFIG_BUS_TYPE. Now we may
make use of DT_ST_LSM6DSL_BUS_I2C and DT_ST_LSM6DSL_BUS_SPI
definition to select the bus.

Signed-off-by: Armando Visconti armando.visconti@st.com

@avisconti
Copy link
Collaborator Author

compiled/tested on both ArgonKey (LSM6DSL_SPI) and disco_l475_iot1 (LSM6DSL_I2C) boards

@codecov-io
Copy link

codecov-io commented Dec 14, 2018

Codecov Report

Merging #12105 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12105   +/-   ##
=======================================
  Coverage   48.06%   48.06%           
=======================================
  Files         284      284           
  Lines       43383    43383           
  Branches    10397    10397           
=======================================
  Hits        20853    20853           
  Misses      18389    18389           
  Partials     4141     4141

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b9cfe7...743035e. Read the comment docs.

@galak
Copy link
Collaborator

galak commented Dec 14, 2018

So I think we need avoid using DT_ symbols in CMakeLists.txt

I wondering about introducing a:

lsm6dsl_bus.c which looks like:

#ifdef DT_ST_LSM6DSL_BUS_SPI
#include "lsm6dsl_spi.c"
#elif defined(DT_ST_LSM6DSL_BUS_I2C)
#include "lsm6dsl_i2c.c"
#else 
#error "..."
#endif

@galak
Copy link
Collaborator

galak commented Dec 14, 2018

We also probably need to add:

#define DT_ST_LSM6DSL_BUS_I2C 1 in tests/drivers/build_all/dts_fixup.h

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Made a few comments on how to get sanity builds to pass.

@avisconti
Copy link
Collaborator Author

So I think we need avoid using DT_ symbols in CMakeLists.txt

This should be fine, as I was able to compile it for ArgonKey and discovery iot boards.

We also probably need to add:
#define DT_ST_LSM6DSL_BUS_I2C 1 in tests/drivers/build_all/dts_fixup.h

It might be. I can try.

@avisconti avisconti force-pushed the lsm6dsl-bus-type branch 2 times, most recently from 2b866d8 to af2962b Compare December 14, 2018 17:07
@galak
Copy link
Collaborator

galak commented Dec 14, 2018

This should be fine, as I was able to compile it for ArgonKey and discovery iot boards.

The reason I say that is because of how build the tests that are failing. Use DT_ in CMakefile.txt like this is probably best to be avoided.

@erwango
Copy link
Member

erwango commented Dec 17, 2018

@galak

So I think we need avoid using DT_ symbols in CMakeLists.txt

I understand why (lack of dependencies control and so on), but I feel this render the DT_X_BUS not easy to use. I know this seems like a proposal going in the opposite direction of all the efforts made recently, but:
What about generating a CONFIG_, as it is proposed for compats in #9406 (comment)?

@avisconti
Copy link
Collaborator Author

avisconti commented Dec 17, 2018

@galak @erwango

So I think we need avoid using DT_ symbols in CMakeLists.txt

To be honest I'm not sure I understand why we cannot do it, but I feel
you are both right

I wondering about introducing a:
lsm6dsl_bus.c which looks like:

#ifdef DT_ST_LSM6DSL_BUS_SPI
#include "lsm6dsl_spi.c"
#elif defined(DT_ST_LSM6DSL_BUS_I2C)
#include "lsm6dsl_i2c.c"
#else
#error "..."
#endif

Why not doing the following instead:

CMakeLists.txt::
...
zephyr_library_sources(lsm6dsl_spi.c)
zephyr_library_sources(lsm6dsl_i2c.c)
...

lsm6dsl_spi.c::
#ifdef DT_ST_LSM6DSL_BUS_SPI
...
#endif

lsm6dsl_i2c.c::
#ifdef DT_ST_LSM6DSL_BUS_I2C
...
#endif

Similar to @galak proposal, but without introducing a new file.

EDIT::
Implemented in this way and repushed. Now is passing the CI.

Remove configuration parameter CONFIG_BUS_TYPE. Now we may
make use of DT_ST_LSM6DSL_BUS_I2C and DT_ST_LSM6DSL_BUS_SPI
definition to select the bus.

Signed-off-by: Armando Visconti <armando.visconti@st.com>
@avisconti
Copy link
Collaborator Author

@galak
Please let me know if this way of solving CI is matching your expectation

@avisconti
Copy link
Collaborator Author

@galak
Any comment on this PR?

@galak galak merged commit f300ded into zephyrproject-rtos:master Jan 8, 2019
@avisconti avisconti deleted the lsm6dsl-bus-type branch January 8, 2019 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants