Add dts yaml support for same sensor on i2c or spi #11854

galak · 2018-12-04T23:00:05Z

Update how we handle the YAML files in the extract script to support the same compatible for a sensor being on either I2C or SPI. We still have unique YAML files, but the DTS compatible can now be the same.

codecov-io · 2018-12-04T23:47:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c9e5a27). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #11854   +/-   ##
=========================================
  Coverage          ?   48.17%           
=========================================
  Files             ?      281           
  Lines             ?    43360           
  Branches          ?    10392           
=========================================
  Hits              ?    20887           
  Misses            ?    18318           
  Partials          ?     4155

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 c9e5a27...867c33e. Read the comment docs.

ulfalizer

Just some random comments. Not very familiar with this code.

scripts/dts/extract/globals.py

scripts/dts/extract_dts_includes.py

b0661

I would prefer to stop using the term yaml and start replacing it by bindings where appropriate.

scripts/dts/extract/globals.py

scripts/dts/extract_dts_includes.py

avisconti

Seems fine, but I will have to review some of the changes I have done (am doing) on sensor part.
So, basically there will still be two yaml file (say lsm6dsl-spi.yaml and lsm6dsl-i2c.yaml) using
same compatible string inside.

erwango

Clean up is good but I'm still not convinced with the main idea of the change.
At the very least it misses a way to instruct how to build the sensor driver with the ad-hoc structs, since the info is no more carried by "compatible".

erwango · 2018-12-05T09:41:16Z

boards/arm/96b_argonkey/96b_argonkey.dts

@@ -60,7 +60,7 @@

 	/* ST Microelectronics LSM6DSL accel/gyro sensor */
 	lsm6dsl@1 {
-		compatible = "st,lsm6dsl-spi";
+		compatible = "st,lsm6dsl";
 		reg = <1>;
 		spi-max-frequency = <1000000>;


IIUC, this whole change allows to have these bus related properties documented by the binding depending on the bus. Is that compatible with device tree usage in eg Linux?

I'm still not convinced how this would work at driver level. To me we should be able to build matching sensor driver using "compatible".
When "compatible" carried SPI/I2C inforamtion, we could have used #idef DT_COMPAT_ST_LSM6DL_SPI / DT_COMPAT_ST_LSM6DL_I2C to know if driver should be compiled with SPI or I2C fields.
Now this information is no more present we'll need to have a new piece of info to know how to build the driver with fields matching the bus.

Actually in both cases the driver (e.g. LSM6DSL in this case) needs an extra configuration
parameter, say LSM6DSL_BUS_TYPE, to select spi/i2c bus case. The changes introduced
by @galak are somehow a cleaner solution, but what we would need instead is a way to
extract the bus type from the dts w/o the need for such configuration parameter.

but then it should be available as a compilation input, not as a configuration input.

The bus type can be inferred by looking at the parent node, and the tooling generate the appropriate flags. The BME280 supports SPI and I2C, and the Linux binding documentation doesn't represent which is being used: it just provides two drivers, each of which are compatible with bosch,bme280.

The inference would be simplified by marking all I2C bus nodes as compatible with zephyr,i2c and similarly for other generic Zephyr drivers. This is something I'd like done regardless.

IIUC, this whole change allows to have these bus related properties documented by the binding depending on the bus. Is that compatible with device tree usage in eg Linux?

Yes, if you look at how various sensor drivers in linux you'll see they use the same compatible regardless of what bus the device is on.

The inference would be simplified by marking all I2C bus nodes as compatible with zephyr,i2c and similarly for other generic Zephyr drivers. This is something I'd like done regardless.

I don't see us ever doing this, as we should be aiming for the device tree's to have less zephyr specific details not more. The intent would be to know that a bus is I2C or SPI via the binding YAML info.

galak · 2018-12-05T15:05:29Z

Pushed an update to fix up comments from @ulfalizer and @b0661

pabigot

I still haven't grasped the design goals/constraints of all this tooling, but within those limits I agree with the purpose of this commit and have no concerns about the changes.

pabigot · 2018-12-05T15:28:57Z

scripts/dts/extract/clocks.py

-                clock_provider_compat = get_compat(clock_provider_node_address)
-                clock_provider_bindings = yaml[clock_provider_compat]
+                clock_provider_bindings = get_binding(
+                                            clock_provider_node_address)


Off topic but this sort of thing strengthens my belief that a strict 80-character limit on line length is decreasing code clarity more than increasing it. Especially when EOL escapes used to meet the requirement aren't consistent (see next line).

The 80 char limit is not enforced. checkpatch will emit a warning, but CI will not fail. Contributors are free to exceed the limit when it makes sense to do so.

Move the handling of 'inherits' in the YAML to right when we load. This allows us to remove yaml_collapse function and will allow us to look into the yaml right after we load it for things like if the yaml is for a child bus. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Various functions like add_prop_aliases, extract_single_prop, extract_single, reg.extract, compatible.extract, default.extract, and flash.extract don't use the yaml argument that is passed to them. Remove passing the argument, this is the first step in making access to the yaml node information go through an accessor function. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Add accessor functions get_binding and get_binding_compats to encapsulate access to binding database info. Cleanup all direct access to binding info and replace with calls to get_binding() and get_binding_compats() Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Change get_binding to take the node address instead of the compatiable. This is in prep for having get_binding be able to look at the parent of the node and determine which bus specific binding to utilize (for cases like sensors that support both I2C & SPI). Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Have the yaml_list now have 'node', 'bus', and 'compat' information instead of just the 'node' info. For now we have all YAML nodes under yaml_list['node'], however this will change to support sensors that can be on I2C or SPI, since they will have the same compat, but different YAML nodes. This also introduces yaml_list['bus'] which has YAML nodes for any compatiables on a given 'parent' bus (ie, 'i2c', or 'spi'). Finally, we also have yaml_list['compat'] with all the compatiables that we parsed and matched to a YAML. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

There are a number of cases in which we a sensor can be either connected on I2C or SPI. We've been treating these cases as different compatiable properties, but they really should use the same compatiable and just determine the info based on the parent bus in the device tree. We can now support having two different binding files for the same compatiable to handle the case of a sensor supporting either I2C or SPI as how its connected. We put "sensor" nodes in a bus specific dict bus_bindings[bus] that we can than lookup later based on the DTS and determining the bus type that the "sensor" node is on. Fixes zephyrproject-rtos#11375 Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

For devices on buses like I2C or SPI its possible that a sensor might support being on either bus type. In the dts we can tell this by looking at the parent bus of the device. Its useful for a driver to know this so can we build support into the driver accordingly. For example if the LSM6DSL sensor ("st,lsm6dsl") is in the dts on a spi bus we now generate: #define DT_ST_LPS22HB_PRESS_BUS_SPI 1 Its possible that a system exists in which multiple of the same sensor exist but on different busses, so drivers should handle that case accordingly. For the LSM6DSL example we'd end up with: #define DT_ST_LPS22HB_PRESS_BUS_I2C 1 #define DT_ST_LPS22HB_PRESS_BUS_SPI 1 Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Now that we can support the same compatible but different bus types, update the LSM6DSL support to utilize the same compatible for either I2C or SPI. We rename the i2c binding file to st,lsm6dsl-i2c.yaml just to be a bit more clear. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

galak added area: SPI SPI bus area: I2C area: Sensors Sensors area: Devicetree labels Dec 4, 2018

galak requested review from avisconti, ulfalizer, b0661, anangl, erwango and MaureenHelm December 4, 2018 23:00

ulfalizer reviewed Dec 5, 2018

View reviewed changes

scripts/dts/extract/globals.py Outdated Show resolved Hide resolved

scripts/dts/extract_dts_includes.py Outdated Show resolved Hide resolved

scripts/dts/extract_dts_includes.py Show resolved Hide resolved

scripts/dts/extract_dts_includes.py Outdated Show resolved Hide resolved

b0661 reviewed Dec 5, 2018

View reviewed changes

scripts/dts/extract/globals.py Outdated Show resolved Hide resolved

scripts/dts/extract/globals.py Outdated Show resolved Hide resolved

scripts/dts/extract_dts_includes.py Outdated Show resolved Hide resolved

scripts/dts/extract_dts_includes.py Show resolved Hide resolved

avisconti mentioned this pull request Dec 5, 2018

Add lsm6dsl specific samples + change its dts binding #11848

Merged

avisconti reviewed Dec 5, 2018

View reviewed changes

erwango reviewed Dec 5, 2018

View reviewed changes

carlescufi requested a review from pabigot December 5, 2018 10:45

galak force-pushed the dts-fix-11375 branch from 6e70ffa to 28fcee0 Compare December 5, 2018 15:05

pabigot reviewed Dec 5, 2018

View reviewed changes

carlescufi requested a review from Mierunski December 5, 2018 15:37

erwango approved these changes Dec 7, 2018

View reviewed changes

galak added 8 commits December 7, 2018 07:58

galak force-pushed the dts-fix-11375 branch from 28fcee0 to 867c33e Compare December 7, 2018 13:58

galak merged commit a24770b into zephyrproject-rtos:master Dec 7, 2018

galak deleted the dts-fix-11375 branch December 7, 2018 15:08

erwango mentioned this pull request Dec 19, 2018

samples/96b_argonkey: divided test into sensor and microphone #11188

Merged

galak mentioned this pull request Jan 24, 2019

dts - support a binding file being on more than one bus #7937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dts yaml support for same sensor on i2c or spi #11854

Add dts yaml support for same sensor on i2c or spi #11854

galak commented Dec 4, 2018

codecov-io commented Dec 4, 2018 •

edited

Loading

ulfalizer left a comment

b0661 left a comment

avisconti left a comment

erwango left a comment

erwango Dec 5, 2018

avisconti Dec 5, 2018 •

edited

Loading

erwango Dec 5, 2018 •

edited

Loading

pabigot Dec 5, 2018

galak Dec 5, 2018

galak Dec 5, 2018

galak commented Dec 5, 2018

pabigot left a comment

pabigot Dec 5, 2018

carlescufi Dec 5, 2018

Add dts yaml support for same sensor on i2c or spi #11854

Add dts yaml support for same sensor on i2c or spi #11854

Conversation

galak commented Dec 4, 2018

codecov-io commented Dec 4, 2018 • edited Loading

Codecov Report

ulfalizer left a comment

Choose a reason for hiding this comment

b0661 left a comment

Choose a reason for hiding this comment

avisconti left a comment

Choose a reason for hiding this comment

erwango left a comment

Choose a reason for hiding this comment

erwango Dec 5, 2018

Choose a reason for hiding this comment

avisconti Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

erwango Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

pabigot Dec 5, 2018

Choose a reason for hiding this comment

galak Dec 5, 2018

Choose a reason for hiding this comment

galak Dec 5, 2018

Choose a reason for hiding this comment

galak commented Dec 5, 2018

pabigot left a comment

Choose a reason for hiding this comment

pabigot Dec 5, 2018

Choose a reason for hiding this comment

carlescufi Dec 5, 2018

Choose a reason for hiding this comment

codecov-io commented Dec 4, 2018 •

edited

Loading

avisconti Dec 5, 2018 •

edited

Loading

erwango Dec 5, 2018 •

edited

Loading