Skip to content

Conversation

@avisconti
Copy link
Contributor

update to the new GPIO API

@zephyrbot
Copy link

zephyrbot commented Dec 6, 2019

All checks passed.

checkpatch (informational only, not a failure)

-:37: WARNING:LONG_LINE: line over 80 characters
#37: FILE: boards/arm/sensortile_box/sensortile_box.dts:106:
+		irq-gpios = <&gpioc 13 GPIO_ACTIVE_HIGH>, <&gpioe 6 GPIO_ACTIVE_HIGH>;

- total: 0 errors, 1 warnings, 370 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@avisconti
Copy link
Contributor Author

I have re-pushed handling also the drivers for the sensors on the board

@avisconti
Copy link
Contributor Author

Repushed cleaning out the sensor polarity setting.
Instead, the sensor will be left to its default polarity (ACTIVE_HIGH for both lis2dw12 and lps22hh). The default is captured in the dts drdy configuration for the sensor, and gpio is configured accordingly.

@pabigot
Copy link
Contributor

pabigot commented Dec 9, 2019

Could you update the binding YAML for these sensors to add text like what's in the hts221 yaml so the sensor active level is documented there?

@avisconti
Copy link
Contributor Author

Could you update the binding YAML for these sensors to add text like what's in the hts221 yaml so the sensor active level is documented there?

Sure, I forgot it.
Done for both i2c/spi lis2dw12/lps22hh

Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

Tested these on nrf52_pca10040 + x-nucleo-iks01a3. As far as basic functionality it seems to work.

The board does not properly handle a reset:

Could not get LSM6DSO device
[00:00:00.007,446] <dbg> LIS2DW12.lis2dw12_set_power_mode: Apply default Power Mode
[00:00:00.025,115] <inf> LSM6DSO: chip id 0x6c
[00:00:00.026,031] <err> i2c_nrfx_twi: Error 195952642 occurred for message 1
[00:00:00.026,031] <dbg> LSM6DSO.lsm6dso_init: failed to initialize chip
[00:00:00.026,031] <dbg> temp_nrf5.temp_nrf5_init: 

I think we exchanged some comments about that somewhere; my recollection is that if you follow the pattern suggested in #20017 (comment) and also demonstrated in #19839 that would be fixed: i.e. check the signal level and handle the interrupt when it's detected by level rather than let it be missed.

There are a couple binding yaml errors noted below; they produce build-time errors. It would be nice if the active level of lsm6dso was also provided in that binding file.

You could also update the x-nucleo-iks01a3 shield overlay GPIO entries for the three sensors present on it (all active high).

Minimum for approval is fixing the broken binding yamls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Needs this inserted:

      description: DRDY pin

Copy link
Contributor

Choose a reason for hiding this comment

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

Also here (insert description keyword)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, this should have been on lis2dw12-i2c.yaml.

@avisconti
Copy link
Contributor Author

Tested these on nrf52_pca10040 + x-nucleo-iks01a3. As far as basic functionality it seems to work.

The board does not properly handle a reset:

Could not get LSM6DSO device
[00:00:00.007,446] <dbg> LIS2DW12.lis2dw12_set_power_mode: Apply default Power Mode
[00:00:00.025,115] <inf> LSM6DSO: chip id 0x6c
[00:00:00.026,031] <err> i2c_nrfx_twi: Error 195952642 occurred for message 1
[00:00:00.026,031] <dbg> LSM6DSO.lsm6dso_init: failed to initialize chip
[00:00:00.026,031] <dbg> temp_nrf5.temp_nrf5_init: 

I think we exchanged some comments about that somewhere; my recollection is that if you follow the pattern suggested in #20017 (comment) and also demonstrated in #19839 that would be fixed: i.e. check the signal level and handle the interrupt when it's detected by level rather than let it be missed.

yes, there was a discussion about this that was handled in #20913 .

There are a couple binding yaml errors noted below; they produce build-time errors.

Ok, I'll fix the bindings for i2c/spi lis2dw12.

It would be nice if the active level of lsm6dso was also provided in that binding file.

I'll add the same description in i2c/spi lsm6dso bindings

You could also update the x-nucleo-iks01a3 shield overlay GPIO entries for the three sensors present on it (all active high).

Yes, sure.
I'll do it in a separate commit in this PR only.

Minimum for approval is fixing the broken binding yamls.

@avisconti
Copy link
Contributor Author

@pabigot
Concerning 1dbfe22, I just changed lps22hh/lis2dw12/lsm6dso.
The lis2mdl and stts751 sensors are still to be done and in my basket. So the plan is to change them later on.

@pabigot
Copy link
Contributor

pabigot commented Dec 10, 2019

The lis2mdl and stts751 sensors are still to be done and in my basket. So the plan is to change them later on.

Agreed, only the sensors that have been updated should be changed in the overlay: makes it more clear that something's left undone.

FWIW I suspect #20913 doesn't solve the general case and that you really do need to check the level of the interrupt and process it if it's already set. That's a rule for any edge-triggered detection on a level signal that changes asynchronously: it can happen after startup too. Changing it isn't in scope for this PR, though.

@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Dec 10, 2019
Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

LGTM with a minor comment.

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment.

Get rid of all the deprecated functions and definitions
replacing them with the new ones.

Signed-off-by: Armando Visconti <[email protected]>
Get rid of all the deprecated functions and definitions
replacing them with the new ones.

Signed-off-by: Armando Visconti <[email protected]>
Get rid of all the deprecated functions and definitions
replacing them with the new ones.

Signed-off-by: Armando Visconti <[email protected]>
Get rid of all the deprecated functions and definitions
replacing them with the new ones.

Signed-off-by: Armando Visconti <[email protected]>
Get rid of all the deprecated functions and definitions
replacing them with the new ones.

Signed-off-by: Armando Visconti <[email protected]>
Use GPIO_ACTIVE_HIGH instead of '0' for drdy/irq flags in overlay
files.

Signed-off-by: Armando Visconti <[email protected]>
@carlescufi carlescufi merged commit ffba0ab into zephyrproject-rtos:topic-gpio Dec 18, 2019
@avisconti avisconti deleted the stile-new-gpio-api branch January 9, 2020 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards area: Devicetree area: GPIO area: Samples Samples area: Sensors Sensors area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants