STMicroelectronics / st-mems-android-linux-drivers-iio

stm mems iio drivers for Android and Linux platforms.
GNU General Public License v2.0
24 stars 1 forks source link

LIS2DUX12/LIS2DUXS12 driver create badly named event files #20

Open lorenzodallacasa opened 5 months ago

lorenzodallacasa commented 5 months ago

Hi, I'm using LIS2DUX12 accelerometer in a Linux environment with kernel version 5.10 and I'm testing event monitoring with iio_event_monitor tool. I managed to verify the functionality of a few events (TAP, DTAP, FF and MLC) but I couldn't test the behavior of the SIGN_MOTION event because I'm having a hard time trying to enable that event from C code. This is because the file responsible to track the event enable/disable status has a particularly bad name: in my case it is in_???'L0_thresh_rising_en. This made me realize that a few other event files were also incorrect because they are named like in_(null)0_thresh_rising_en.

So I decided to take a look inside the kernel and the driver to better understand what is going on and I found out that the driver is using custom enum type values to initialize the structure struct iio_chan_spec of these sensor.

/* stm_iio_types.h */
/* Linux IIO driver custom types */
enum {
    STM_IIO_LAST = 0x3f,
    STM_IIO_SIGN_MOTION = STM_IIO_LAST - 6,
    STM_IIO_STEP_COUNTER = STM_IIO_LAST - 5,
    STM_IIO_TILT = STM_IIO_LAST - 4,
    STM_IIO_TAP = STM_IIO_LAST - 3,
    STM_IIO_TAP_TAP = STM_IIO_LAST - 2,
    STM_IIO_WRIST_TILT_GESTURE = STM_IIO_LAST - 1,
    STM_IIO_GESTURE = STM_IIO_LAST,
};
/* st_lis2duxs12.h */
#define ST_LIS2DUXS12_EVENT_CHANNEL(ctype, etype)   \
{                           \
    .type = ctype,                  \
    .modified = 0,                  \
    .scan_index = -1,               \
    .indexed = -1,                  \
    .event_spec = &st_lis2duxs12_##etype##_event,   \
    .num_event_specs = 1,               \
}
/* st_lis2duxs12_basicfunc.c */
static const struct iio_chan_spec st_lis2duxs12_ff_channels[] = {
    ST_LIS2DUXS12_EVENT_CHANNEL(STM_IIO_GESTURE, thr),
    IIO_CHAN_SOFT_TIMESTAMP(1),
};
static const struct iio_chan_spec st_lis2duxs12_tap_channels[] = {
    ST_LIS2DUXS12_EVENT_CHANNEL(STM_IIO_TAP, thr),
    IIO_CHAN_SOFT_TIMESTAMP(1),
};
static const struct iio_chan_spec st_lis2duxs12_dtap_channels[] = {
    ST_LIS2DUXS12_EVENT_CHANNEL(STM_IIO_TAP_TAP, thr),
    IIO_CHAN_SOFT_TIMESTAMP(1),
};
static const struct iio_chan_spec st_lis2duxs12_ttap_channels[] = {
    ST_LIS2DUXS12_EVENT_CHANNEL(STM_IIO_GESTURE, thr),
    IIO_CHAN_SOFT_TIMESTAMP(1),
};

For my understanding, this is a problem because when the kernel generates the event names for these sensors/devices, it tries to get a char pointer from the iio_chan_type_name_spec[] variable that has only IIO_MASSCONCENTRATION (=34) items which is less than the first STM custom type STM_IIO_SIGN_MOTION (=57). So our index/type is out of bounds.

/* industrial-core.c */
static const char * const iio_chan_type_name_spec[] = {
    [IIO_VOLTAGE] = "voltage",
    [IIO_CURRENT] = "current",
    [IIO_POWER] = "power",
    [IIO_ACCEL] = "accel",
    [IIO_ANGL_VEL] = "anglvel",
    [IIO_MAGN] = "magn",
    [IIO_LIGHT] = "illuminance",
    [IIO_INTENSITY] = "intensity",
    [IIO_PROXIMITY] = "proximity",
    [IIO_TEMP] = "temp",
    [IIO_INCLI] = "incli",
    [IIO_ROT] = "rot",
    [IIO_ANGL] = "angl",
    [IIO_TIMESTAMP] = "timestamp",
    [IIO_CAPACITANCE] = "capacitance",
    [IIO_ALTVOLTAGE] = "altvoltage",
    [IIO_CCT] = "cct",
    [IIO_PRESSURE] = "pressure",
    [IIO_HUMIDITYRELATIVE] = "humidityrelative",
    [IIO_ACTIVITY] = "activity",
    [IIO_STEPS] = "steps",
    [IIO_ENERGY] = "energy",
    [IIO_DISTANCE] = "distance",
    [IIO_VELOCITY] = "velocity",
    [IIO_CONCENTRATION] = "concentration",
    [IIO_RESISTANCE] = "resistance",
    [IIO_PH] = "ph",
    [IIO_UVINDEX] = "uvindex",
    [IIO_ELECTRICALCONDUCTIVITY] = "electricalconductivity",
    [IIO_COUNT] = "count",
    [IIO_INDEX] = "index",
    [IIO_GRAVITY]  = "gravity",
    [IIO_POSITIONRELATIVE]  = "positionrelative",
    [IIO_PHASE] = "phase",
    [IIO_MASSCONCENTRATION] = "massconcentration",
};

I think this is the point where the empty string messes things up:

/* industrial-core.c */
/* int __iio_device_attr_init() */
case IIO_SEPARATE:
    if (chan->indexed)
        name = kasprintf(GFP_KERNEL, "%s_%s%d_%s",
                iio_direction[chan->output],
                iio_chan_type_name_spec[chan->type],
                chan->channel,
                full_postfix);

Am I understanding this behavior correctly? How can we fix this?

Thanks

mariotesi commented 4 months ago

Hi,

Yes we know the problem in fact we added a message here in the Kconfig.

Unfortunately the Linux IIO framework still not support all event types that our MEMS sensors are capable to generate so in order to have this event working an IIO patch is requested for extending the iio_chan_type_name_spec array to our custom types names.