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.

Sign up for GitHub

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

Jump to bottom

drivers: sensor: mlx90394: added driver #79637

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

drivers: sensor: mlx90394: added driver #79637

wants to merge 2 commits into from
+1,220 −0

Conversation

FlorianWeber1018
Copy link
Contributor

Added driver for Melexis MLX90394 magnetometer.

@FlorianWeber1018 FlorianWeber1018 mentioned this pull request Oct 10, 2024
Open
@FlorianWeber1018 FlorianWeber1018 changed the title drivers: sensors: mlx90394: added driver drivers: sensor: mlx90394: added driver Oct 10, 2024
@FlorianWeber1018 FlorianWeber1018 marked this pull request as ready for review October 18, 2024 15:57
@zephyrbot zephyrbot added the area: Sensors Sensors label Oct 18, 2024
@FlorianWeber1018 FlorianWeber1018 marked this pull request as draft October 19, 2024 22:16
@FlorianWeber1018 FlorianWeber1018 force-pushed the melexis_mlx90394_driver branch 3 times, most recently from 216c2d3 to 4fffe4e Compare October 24, 2024 08:10
@FlorianWeber1018 FlorianWeber1018 marked this pull request as ready for review October 24, 2024 09:47
@FlorianWeber1018 FlorianWeber1018 force-pushed the melexis_mlx90394_driver branch 2 times, most recently from a66c895 to c869ee4 Compare December 5, 2024 18:51
pdgendt
pdgendt reviewed Dec 12, 2024
View reviewed changes
drivers/sensor/melexis/MLX90394/mlx90394.h Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394.c Outdated Show resolved Hide resolved
@FlorianWeber1018 FlorianWeber1018 marked this pull request as draft December 17, 2024 13:50
@FlorianWeber1018
Copy link
Contributor Author

By going through the review, I have seen that there are some things missing and not tested very well.
I have decided to go back to the draft state for correct those things first.
I will try to make that the next days.

@FlorianWeber1018 FlorianWeber1018 force-pushed the melexis_mlx90394_driver branch 2 times, most recently from 41856ff to c7c040c Compare December 17, 2024 18:07
@FlorianWeber1018 FlorianWeber1018 force-pushed the melexis_mlx90394_driver branch 2 times, most recently from fe1f6b8 to 51239c6 Compare January 10, 2025 09:19
@FlorianWeber1018 FlorianWeber1018 marked this pull request as ready for review January 10, 2025 12:50
@zephyrbot zephyrbot requested a review from asemjonovs January 10, 2025 12:51
@FlorianWeber1018
Copy link
Contributor Author

51239c6 to 51f5abd only rebase on main to pull in bugfixes

@FlorianWeber1018
Copy link
Contributor Author

@pdgendt all your comments should be adressed - can you please revisit this PR?

pdgendt
pdgendt reviewed Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

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

Thanks, some comments still.

drivers/sensor/melexis/MLX90394/mlx90394.h Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_async.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_async.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_async.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_async.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_decoder.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_decoder.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_decoder.c Outdated Show resolved Hide resolved
drivers/sensor/melexis/MLX90394/mlx90394_decoder.c Outdated Show resolved Hide resolved
tests/drivers/build_all/sensor/i2c.dtsi Show resolved Hide resolved
pdgendt
pdgendt previously approved these changes Jan 15, 2025
View reviewed changes
@FlorianWeber1018 FlorianWeber1018 force-pushed the melexis_mlx90394_driver branch 2 times, most recently from 78b9c90 to 5d6cab9 Compare January 16, 2025 13:48
@FlorianWeber1018
Copy link
Contributor Author

i have added a flag for the case that the initialization of the device fails.

pdgendt
pdgendt previously approved these changes Jan 16, 2025
View reviewed changes
MaureenHelm
MaureenHelm requested changes Jan 16, 2025
View reviewed changes
drivers/sensor/melexis/Kconfig Outdated
# SPDX-License-Identifier: Apache-2.0

# zephyr-keep-sorted-start
source "drivers/sensor/melexis/MLX90394/Kconfig"
Copy link
Member

Choose a reason for hiding this comment

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

Convention is to use lower case for directory names

Suggested change
source "drivers/sensor/melexis/MLX90394/Kconfig"
source "drivers/sensor/melexis/mlx90394/Kconfig"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

drivers/sensor/melexis/MLX90394/mlx90394.c Outdated
Comment on lines 39 to 63
static void mlx90394_update_measurement_Time_us(struct mlx90394_data *data)
{
int32_t enX = FIELD_GET(MLX90394_CTRL1_X_EN, data->ctrl_reg_values.ctrl1);
int32_t enY = FIELD_GET(MLX90394_CTRL1_Y_EN, data->ctrl_reg_values.ctrl1);
int32_t enZ = FIELD_GET(MLX90394_CTRL1_Z_EN, data->ctrl_reg_values.ctrl1);
int32_t enTemp = FIELD_GET(MLX90394_CTRL4_T_EN, data->ctrl_reg_values.ctrl4);
int32_t filterHallXY =
FIELD_GET(MLX90394_CTRL3_DIG_FILT_HALL_XY, data->ctrl_reg_values.ctrl3);
int32_t filterHallZ =
FIELD_GET(MLX90394_CTRL4_DIG_FILT_HALL_Z, data->ctrl_reg_values.ctrl4);
int32_t filterTemp = FIELD_GET(MLX90394_CTRL3_DIG_FILT_TEMP, data->ctrl_reg_values.ctrl3);
int32_t osrTemp = FIELD_GET(MLX90394_CTRL3_OSR_TEMP, data->ctrl_reg_values.ctrl3);
int32_t osrHall = FIELD_GET(MLX90394_CTRL3_OSR_HALL, data->ctrl_reg_values.ctrl3);

int32_t conversion_time_us =
(osrHall + 1) * ((enX + enY) * MLX90394_CONVERSION_TIME_US_AXIS[filterHallXY] +
enZ * MLX90394_CONVERSION_TIME_US_AXIS[filterHallZ]) +
(osrTemp + 1) * enTemp * MLX90394_CONVERSION_TIME_US_AXIS[filterTemp];
int32_t dsp_time_us = MLX90394_DSP_TIME_US[enTemp][enX + enY + enZ];

/*
* adding 5% tolerance from datasheet
*/
data->measurement_time_us = (conversion_time_us + dsp_time_us) * 105 / 100;
}
Copy link
Member

Choose a reason for hiding this comment

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

please no camel case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

drivers/sensor/melexis/MLX90394/mlx90394.c Outdated
static void mlx90394_convert_temp(struct sensor_value *val, uint8_t sample_l, uint8_t sample_h)
{
int64_t conv_val =
sys_le16_to_cpu(sample_l | (sample_h << 8)) * MLX90394_MICRO_CELSIUS_PER_BIT;
Copy link
Member

Choose a reason for hiding this comment

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

sys_le16_to_cpu isn't necessary here

Copy link
Contributor Author

@FlorianWeber1018 FlorianWeber1018 Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

that happens when starting by deriving an existing driver in tree.
Sorry for that - done. Its not just not necessary it´s wrong because it swaps the bytes on BE.

drivers/sensor/melexis/MLX90394/mlx90394_async.c Outdated
edata = (struct mlx90394_encoded_data *)buf;

/* buffered from submit */
edata->header.timestamp = data->work_ctx.timestamp;
Copy link
Member

Choose a reason for hiding this comment

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

Please use the new sensor clock API introduced in #80189

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@FlorianWeber1018
drivers: sensor: mlx90394: added driver
14bd05a 
Added driver for Melexis MLX90394 magnetometer.

Signed-off-by: Florian Weber <[email protected]>
@FlorianWeber1018
tests: drivers: build_all: sensors: added mlx90394
e51664c 
added mlx90394 to the testcase so that it is in build.

Signed-off-by: Florian Weber <[email protected]>
@FlorianWeber1018
Copy link
Contributor Author

@MaureenHelm everything you have noted should be addressed now - please revisit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaureenHelm MaureenHelm MaureenHelm requested changes

@pdgendt pdgendt pdgendt left review comments

@avisconti avisconti Awaiting requested review from avisconti

@jeppenodgaard jeppenodgaard Awaiting requested review from jeppenodgaard

@teburd teburd Awaiting requested review from teburd

@tristan-google tristan-google Awaiting requested review from tristan-google

@ubieda ubieda Awaiting requested review from ubieda

@yperess yperess Awaiting requested review from yperess

@asemjonovs asemjonovs Awaiting requested review from asemjonovs

Requested changes must be addressed to merge this pull request.

Assignees

@MaureenHelm MaureenHelm

Labels
area: Sensors Sensors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FlorianWeber1018 @pdgendt @MaureenHelm @zephyrbot