-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: sensors: bmi08x: add initial support for bmi08x #51545
Conversation
628196d
to
1753704
Compare
I want to note that the binary blob for configuration is something we already allow in similar drivers: My recommendation is to document that this is allowed. |
* Copyright (c) 2022 Meta Platforms, Inc. and its affiliates | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source copyright and license should be preserved (Bosch, BSD-3-Clause)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.... but it appears to of broken one of the CI checks
3da925d
to
744fdb6
Compare
744fdb6
to
052009e
Compare
052009e
to
0683ed0
Compare
#endif | ||
|
||
/* set accel ints */ | ||
if (bmi08x_accel_byte_write(dev, BMI08X_REG_ACCEL_INT1_MAP, cfg->int1_map) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find it in the datasheet of bmi088 and bmi085 BMI08X_REG_ACCEL_INT1_MAP: 0x56 and BMI08X_REG_ACCEL_INT2_MAP: 0x57. the datasheet said that 0x55-0x57 is reserved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is there.... Just about every register that says "reserved" in that data actually does have a purpose (just not a very important one...) if you look in this Bosch public BMI08x bare metal driver.. you see those registers actually defined in there. I could change it to the more easily found register in the data sheet... but it's really the same thing
https://github.com/BoschSensortec/BMI08x-Sensor-API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really do not like "reserved" register, I can`t find those "reserved" register describe.
https://github.com/boschsensortec/BMI08x-Sensor-API/blob/631a966891885643f180e18e94870411b8f4c3fc/bmi08a.c#L2198
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not data ready interrupt in set_accel_data_ready_int
by writing BMI08X_REG_ACCEL_INT1_MAP and BMI08X_REG_ACCEL_INT2_MAP register.
I did a test, and the synchronization interrupt could not be generated data ready interrupt, When I change write BMI08X_REG_ACCEL_INT1_INT2_MAP_DATA register, The interrupt have active.
in datasheet:
To increase flexibility, both gyroscope and accelerometer can not only be operated individually, but tied
together for data synchronization purposes. The on-chip features comprise FIFOs for acceleration and
gyroscope data and interrupt controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I assume you're not using data sync?... I'll replace it with this
#if BMI08X_ACCEL_ANY_INST_HAS_DATA_SYNC
if (config->data_sync != 0) {
/* set accel ints */
if (bmi08x_accel_byte_write(dev, BMI08X_REG_ACCEL_INT1_MAP, cfg->int1_map) < 0) {
LOG_ERR("Failed to map interrupts.");
return -EIO;
}
if (bmi08x_accel_byte_write(dev, BMI08X_REG_ACCEL_INT2_MAP, cfg->int2_map) < 0) {
LOG_ERR("Failed to map interrupts.");
return -EIO;
}
} else
#endif
{
uint8_t map_data = ((cfg->int2_map << BMI08X_ACCEL_INT2_DRDY_POS) |
(cfg->int1_map << BMI08X_ACCEL_INT1_DRDY_POS));
if (bmi08x_accel_byte_write(dev, BMI08X_REG_ACCEL_INT1_INT2_MAP_DATA, map_data) < 0) {
LOG_ERR("Failed to map interrupts.");
return -EIO;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to set the sensor synchronization function in the attribute. What do you think?
Macros can only be selected from one of two options, and using attribute functions can be set at any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, ...but I can't tell exactly what you're saying here??
Data sync is done in the DTS so it is instanced. You can have multiple BMI08X connected and one can have data sync on while the other doesn't. With a KConfig, it's either ALL or NONE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BMI08X_ACCEL_ANY_INST_HAS_DATA_SYNC
bmi08x_attr_set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I emailed a bosch FAE about this a while ago... data sync can't easily be set at runtime (unless if you write complex code to reset it and reinitialize it...) It also must be set at initialization
I see that the config file is acceptable under 3-clause BSD
@XenuIsWatching please rebase |
rebased, but it blew away the reviews |
@MaureenHelm @carlescufi sorry to bug you all again on this, but are you able to re-approve and merge now that it's rebased? |
bdaf0f3
to
3b09279
Compare
@XenuIsWatching thanks for the fixes! Can't wait wait to get this merged in! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refreshing my +1
drivers/sensor/bmi08x/bmi08x_accel.c
Outdated
len1 = CONFIG_BMI08X_I2C_WRITE_BURST_SIZE; | ||
} | ||
if (bmi08x_stream_transfer_write_i2c(dev, index, data, len1) != 0) { | ||
return -EIO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually 3 more... I took a second look through it all
This adds support for the bosch bmi085 and bmi088. This also includes support for data sync mode. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refresh +1
Thanks @bbilas! Are you able to merge this in now @carlescufi? |
@MaureenHelm is back, so I delegate to her now. |
This adds support for the bosch bmi085 and bmi088. This also includes support for data sync mode. The binary blob in bmi08x_config_file.h is under the BSD-3 clause which comes from the github repo https://github.com/BoschSensortec/BMI08x-Sensor-API/blob/master/LICENSE
Signed-off-by: Ryan McClelland ryanmcclelland@meta.com