about lps22df sensor driver https://github.com/zephyrproject-rtos/zephyr/pull/62028

[zafersn]

Is your enhancement proposal related to a problem? Please describe. I came across with this sensor driver, https://github.com/zephyrproject-rtos/zephyr/pull/62028 here is the discussion. it seems this sensor driver is not comprehensive for the lps22df and doesn't allow a user to use it fully functional. The user needs to rewrite some parts of the code. Well, of course, it really good starter to see pressure and temp data coming out to the terminal screen but I think it would be better

I have a couple of questions for you if you don't mind to understand your approach to how a zephyros driver should be comprehensive.

question 1:

    if (ON_I3C_BUS(cfg)) {
            lps22df_bus_mode_t bus_mode;

            /* Select bus interface */
            bus_mode.filter = LPS22DF_AUTO;
            bus_mode.interface = LPS22DF_SEL_BY_HW;
            lps22df_bus_mode_set(ctx, &bus_mode);

    }

here is your bus mode setting, may I ask why you restrict bus_mode_settings with only the i3c bus interface? Because this bus mode setting is not only for i3c but also is valid for I²C with antispike filter as well, if you are running this code for i3c you should run it for the i2c bus too.

look at the lps22df.pdf page 28, paragraph: 7.4.2:

Overview of antispike filter management The device acts as a standard I²C target as long as it has an I²C static address. The device is capable of detecting and disabling the I²C antispike filter after detecting the broadcast address (7'h7E/W). In order to guarantee proper behavior of the device, the I3C master must emit the first START, 7'h7E/W at open-drain speed using I²C fast mode plus reference timing. After detecting the broadcast address, the device can receive the I3C dynamic address following the I3C pushpull timing. If the device is not assigned a dynamic address, then the device continues to operate as an I²C device with no antispike filter. For the case in which the host decides to keep the device as I²C with antispike filter, there is a configuration required to keep the antispike filter active. This configuration is done by writing the ASF_ON bit to 1 in the I3C_IF_CTRL_ADD (19h) register. This configuration forces the antispike filter to always be turned on instead of being managed by the communication on the bus.

#if DT_ANY_INST_ON_BUS_STATUS_OKAY(i3c)
        #define ON_I3C_BUS(cfg) (cfg->i3c.bus != NULL)
#else
        #define ON_I3C_BUS(cfg) (false)
#endif

but look at your configuration it is only valid if the bus interface is i3c.

so would it not be better to remove if(ON_I3C_BUS(cfg)) statement?

question 2:

/* set sensor default odr */
    LOG_DBG("%s: odr: %d", dev->name, cfg->odr);
    ret = lps22df_set_odr_raw(dev, cfg->odr);
    if (ret < 0) {
        LOG_ERR("%s: Failed to set odr %d", dev->name, cfg->odr);
        return ret;
    }

may I ask why you call this function set_odr or set only odr setting here? Because the function lps22df_mode_set you used in the lps22df_set_odr_raw requires also cfg->avg and cfg->lpf configurations which are really important together in terms of power consumption,(see page 33, paragraph 9.6 ) so would be better to give these parameters together into the function you defined (lps22df_set_odr_raw) and rename it. I see you call this function in the lps22df_attr_set to change sampling frequency but the avg(resolution ) config is also very important. I would've added the SENSOR_ATTR_OVERSAMPLING factor with the SENSOR_ATTR_SAMPLING_FREQUENCY in the lps22df_attr_set function to make this sensor fully functional.

it seems you took the lps22hh driver as a sample, that's why you have limited configuration here but lps22hh sensor supports only the odr setting as a sensor attribute but this lps22df sensor has some more attributes

question 3

should not it be better to have this tries value as a config definition? As far as I understand here this one is the timeout counter and waits until the reset is completed. if the reset is not completed, it is an error. so you defined the tries variable and its value as 10 in the init function in the source file

do {
        if (!--tries) {
            LOG_DBG("sw reset timed out");
            return -ETIMEDOUT;
        }
        k_usleep(LPS22DF_SWRESET_WAIT_TIME);

        if (lps22df_status_get(ctx, &status) < 0) {
            return -EIO;
        }
    } while (status.sw_reset);

question 4:

we have 2 additional interrupt configs for the lps22df sensor. would it not be better to show these configurations here to a user of this driver?

uint8_t int_latched  : 1; /* int events are: int on threshold, FIFO */
uint8_t active_low   : 1; /* 1 = active low / 0 = active high */

/* enable drdy in pulsed/latched mode */
    LOG_DBG("drdy_pulsed is %d", (int)cfg->drdy_pulsed);
    mode.drdy_latched = ~cfg->drdy_pulsed;
    if (lps22df_interrupt_mode_set(ctx, &mode) < 0) {
        return -EIO;
    }

thanks for reading my questions patiently. and I'll appreciate it if you can reply to me

[fabiobaltieri]

cc @erwango @FRASTM @gautierg-st