Add lis2du12 accelerometer driver #65734
Conversation
avisconti
added
area: Drivers
area: Boards
area: Sensors
avisconti
requested review from
teburd,
fabiobaltieri,
MaureenHelm and
erwango
zephyrbot
requested review from
galak,
kartben,
nashif,
tristan-google and
yperess
avisconti
force-pushed
the
add-lis2du12
branch
from
aac0a1b
to
8832eca
Compare
avisconti
force-pushed
the
add-lis2du12
branch
from
8832eca
to
c07e09e
Compare
|
Any feedback here? |
drivers/sensor/lis2du12/lis2du12.c
Outdated
| case SENSOR_ATTR_SAMPLING_FREQUENCY: | ||
| return lis2du12_accel_odr_set(dev, val->val1); | ||
| default: | ||
| LOG_DBG("Accel attribute not supported."); |
There was a problem hiding this comment.
should probably be a LOG_WRN and print the value of attr
There was a problem hiding this comment.
I put LOG_ERR here with attribute. Do you feel a warning would be more approriate?
There was a problem hiding this comment.
I think I would do warning here and keep errors for stuff that breaks the driver permanently, like initialization errors.
drivers/sensor/lis2du12/lis2du12.h
Outdated
| #define LIS2DU12_EN_BIT 0x01 | ||
| #define LIS2DU12_DIS_BIT 0x00 |
There was a problem hiding this comment.
the indentation looks funny, I'd sugget to just use a space
#define LIS2DU12_EN_BIT 0x01
#define LIS2DU12_DIS_BIT 0x00no need to make it fancy, same below
There was a problem hiding this comment.
mmmh, I think that maybe tabs are better. But I'd agree to shorten it a bit and align with below
There was a problem hiding this comment.
Or just a space... it's just two, I wouldn't bother.
avisconti
force-pushed
the
add-lis2du12
branch
from
c07e09e
to
ce1ccb1
Compare
drivers/sensor/lis2du12/Kconfig
Outdated
| if LIS2DU12 | ||
|
|
||
| choice LIS2DU12_TRIGGER_MODE | ||
| prompt "Trigger mode" | ||
| help | ||
| Specify the type of triggering to be used by the driver. | ||
|
|
||
| config LIS2DU12_TRIGGER_NONE | ||
| bool "No trigger" | ||
|
|
||
| config LIS2DU12_TRIGGER_GLOBAL_THREAD | ||
| bool "Use global thread" | ||
| depends on GPIO | ||
| select LIS2DU12_TRIGGER | ||
|
|
||
| config LIS2DU12_TRIGGER_OWN_THREAD | ||
| bool "Use own thread" | ||
| depends on GPIO | ||
| select LIS2DU12_TRIGGER | ||
|
|
||
| endchoice |
There was a problem hiding this comment.
unrelated to this PR but let me pitch the idea to you: this pattern of having an option for how to handle asyncronous events (thread, workqueue) is copypasted everywhere. How would you feel about having a Kconfig template to define these options (like the log one, the one you use with source "subsys/logging/Kconfig.template.log_config") and then common set of of macro to prepare and use the support data for the selected operating mode? (thread, workqueue, syncronous or none)
Haven't quite tried to do it but I think it could remove lot of duplicated code.
There was a problem hiding this comment.
Yes, looks a great idea. Let me explore more on that.
There was a problem hiding this comment.
ok, take a look just to see if I did it in the way you wanted to (not 100% sure)
There was a problem hiding this comment.
brilliant, yeah I was actually thinking about also having a set of macros to dedup the code as well but this is a brilliant start and a step towards it, thanks for doing it, let's think about the code in followups
@MaureenHelm @teburd what do you think?
There was a problem hiding this comment.
It would be great, though to be frank I'd rather see us move in the newer sensor read API approach which avoids the need for work queues/threads at all and so the entire Kconfig around work queue or thread for trigger handling goes away.
avisconti
force-pushed
the
add-lis2du12
branch
from
ce1ccb1
to
8c2c3d3
Compare
Add Kconfig.trigger_template to allow an extensive re-use of
trigger configuration inside all sensor drivers.
This template must be included as in the following example:
module = LSM6DSO
thread_priority = 10
thread_stack_size = 1024
source "drivers/sensor/Kconfig.trigger_template"
Signed-off-by: Armando Visconti <armando.visconti@st.com>
The LIS2DU12 is a linear 3-axis accelerometer with advanced digital functions whose MEMS and ASIC have been expressly designed to build an outstanding ultralow-power architecture in which the anti-aliasing filter operates with a current consumption among the lowest in the market. This driver is based on stmemsc HAL i/f v2.3 https://www.st.com/en/datasheet/lis2du12.pdf Signed-off-by: Armando Visconti <armando.visconti@st.com>
Extend sensortile_box_pro with lis2du12 accelerometer. Signed-off-by: Armando Visconti <armando.visconti@st.com>
Extend sensor sample reading also lis2du12 accelerometer data. Signed-off-by: Armando Visconti <armando.visconti@st.com>
avisconti
force-pushed
the
add-lis2du12
branch
from
8c2c3d3
to
28a9004
Compare
The LIS2DU12 is a linear ultralow power 3-axis accelerometer sensor.
This driver is based on stmemsc HAL i/f v2.3