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

Sensor: driver: TI TMAG5273 3D Hall Sensor #53429

Merged

Conversation

deveritec-rosc
Copy link
Contributor

Product Homepage:
https://www.ti.com/product/TMAG5273

DataSheet:
https://www.ti.com/lit/ds/symlink/tmag5273.pdf

Tested on a custom hardware with nRF52840.

Signed-off-by: Robert Schulze [email protected]

@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding platform: TI Texas Instruments area: Sensors Sensors labels Jan 3, 2023
@deveritec-rosc
Copy link
Contributor Author

The compliance tests fail with

a326f05b93cbaa4927eddca90036fb263c0e54f7 tests: logging: log_api: Fix test_log_arguments case
Traceback (most recent call last):
  File "./scripts/ci/check_compliance.py", line 20, in <module>
    from yamllint import config, linter
ModuleNotFoundError: No module named 'yamllint'

Could somebody please tell me why this is happening so I have a chance to fix this?

@deveritec-rosc deveritec-rosc force-pushed the sensor_driver_tmag5273 branch from 29efddd to c04c937 Compare January 6, 2023 15:24
@cfriedt
Copy link
Member

cfriedt commented Jan 10, 2023

The compliance tests fail with

a326f05b93cbaa4927eddca90036fb263c0e54f7 tests: logging: log_api: Fix test_log_arguments case
Traceback (most recent call last):
  File "./scripts/ci/check_compliance.py", line 20, in <module>
    from yamllint import config, linter
ModuleNotFoundError: No module named 'yamllint'

Could somebody please tell me why this is happening so I have a chance to fix this?

@deveritec-rosc - it's possible you may need to rebase, do a west update, and then also re-install dependencies with pip -r scripts/requirements.txt

See the Getting Started Guide for more info.

https://docs.zephyrproject.org/latest/develop/getting_started/index.html

Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.

There are a lot of code style issues that need to be addressed. Please review the details in the failed compliance check and fix by amending the original commit rather than adding a fixup commit.

drivers/sensor/tmag5273/Kconfig Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/Kconfig Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/Kconfig Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273.c Outdated Show resolved Hide resolved
@deveritec-rosc
Copy link
Contributor Author

Hopefully everything done so far. Kind of tricky coming from a project with different styling rules, hope everything is resolved :)

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Some minor nits, but also, I think a lot of the Kconfig choices should probably be converted to Device Tree bindings. The main advantage there is that they are compile time const but not global, and can therefore be set on a per-device-instance basis.

Kconfig for configuring software, Device Tree for configuring software.

drivers/sensor/tmag5273/Kconfig Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/Kconfig Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/Kconfig Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273_trigger.c Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273_trigger.c Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273_trigger.c Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273_trigger.c Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273_trigger.c Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273_trigger.c Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273_trigger.c Outdated Show resolved Hide resolved
@deveritec-rosc deveritec-rosc force-pushed the sensor_driver_tmag5273 branch 2 times, most recently from 4e0c92e to 2eb2a7f Compare January 11, 2023 12:31
@deveritec-rosc deveritec-rosc force-pushed the sensor_driver_tmag5273 branch 3 times, most recently from 93c7145 to e5a2600 Compare January 12, 2023 08:39
@MaureenHelm
Copy link
Member

Please clean up the code style issues, move instance configuration options to devicetree properties, and see if you can reuse the built-in CRC functions in include/zephyr/sys/crc.h

@deveritec-rosc
Copy link
Contributor Author

Updated the code so that all hardware specific options are now located in the dts-file.
Will take a look at the CRC and remaining issues after the weekend.

@fabiobaltieri
Copy link
Member

I just finished the "pseudo-simultaneous" feature and would love to add it today, would this be okay, or better a new PR?

Your call, if you have it ready, you may as well.

@deveritec-rosc deveritec-rosc force-pushed the sensor_driver_tmag5273 branch 2 times, most recently from 314af7a to f61174a Compare February 1, 2024 14:49
dts/bindings/sensor/ti,tmag5273.yaml Outdated Show resolved Hide resolved
dts/bindings/sensor/ti,tmag5273.yaml Outdated Show resolved Hide resolved
drivers/sensor/tmag5273/tmag5273.c Outdated Show resolved Hide resolved
@deveritec-rosc deveritec-rosc force-pushed the sensor_driver_tmag5273 branch 2 times, most recently from cb7df5e to c546764 Compare February 2, 2024 07:35
kartben
kartben previously approved these changes Feb 2, 2024
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

pretty cool stuff :)

drivers/sensor/tmag5273/tmag5273.c Outdated Show resolved Hide resolved
kartben
kartben previously approved these changes Feb 2, 2024
fabiobaltieri
fabiobaltieri previously approved these changes Feb 2, 2024
@kartben
Copy link
Collaborator

kartben commented Feb 2, 2024

@MaureenHelm will you be able to have another look? I am pretty confident that your previous review can safely be dismissed, but of course it would be better if you can do a quick last past :)

MaureenHelm
MaureenHelm previously approved these changes Feb 2, 2024
@dleach02
Copy link
Member

dleach02 commented Feb 2, 2024

@deveritec-rosc please address the merge conflicts

kartben
kartben previously approved these changes Feb 5, 2024
@kartben
Copy link
Collaborator

kartben commented Feb 5, 2024

@fabiobaltieri @henrikbrixandersen @MaureenHelm please help get this in for 3.6. This was approved on Friday but needed a rebase (the classical fun of I2C sensors conflicting with each other in the I2C.dtsi file)

Product Homepage:
https://www.ti.com/product/TMAG5273

Datasheet:
https://www.ti.com/lit/ds/symlink/tmag5273.pdf

Tested on a custom hardware with nRF52840.

Signed-off-by: Juliane Schulze <[email protected]>
@MaureenHelm MaureenHelm merged commit 1683f19 into zephyrproject-rtos:main Feb 5, 2024
19 checks passed
@deveritec-rosc deveritec-rosc deleted the sensor_driver_tmag5273 branch March 15, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Sensors Sensors platform: TI SimpleLink Texas Instruments SimpleLink MCU platform: TI Texas Instruments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants