[RFC] Comparator API

[bp-amitchone]

Introduction

This RFC is intended to discuss the introduction of a new API targeting analogue comparators on a number of microcontrollers. Below are some examples of microcontrollers that contain this functionality and that are supported by Zephyr (not an exhaustive list):

• STM32G4 series
• nRF52832

I've completed a barebones implementation for the STM32G4 in a downstream repository and would eventually like to upstream it; but prior to this an API needs to be specified and agreed upon.

Problem description

At the moment there is no Zephyr API for this peripheral.

Proposed change

Agree upon a driver API specification for Comparator peripherals and documentation with a view to (in a separate PR) introducing the first implementation and set of tests.

Detailed RFC

Proposed change (Detailed)

I propose a very minimal API that only supports configuration via Devicetree during initialisation. Firstly, here is the proposed driver API struct:

struct comp_driver_api
{
    comp_api_enable enable;
    comp_api_disable disable;

#if (defined(CONFIG_COMP_STM32))
    comp_api_lock lock;
#endif(defined(CONFIG_COMP_STM32))
};

The API is straightforward and simply allows the user to either enable or disable the peripheral. On the STM32G4 comparators it is possible to lock the comparator such that configuration is immutable until a hardware device reset, thus, this feature is dependent on the Kconfig value CONFIG_COMP_STM32.

I propose that each function should return an integer value to enable propagation of errors and facilitate debugging. On the STM32G4 microcontroller the associated LL API functions are all void types, however I believe an integer return value is useful in the case of trying to call the API before the clock is initialised, or, for example on an STM32G4, trying to perform an operation whilst the peripheral is locked.

As such, these would be the function pointer definitions:

/**
 * @brief See comp_enable() for argument descriptions
 */
typedef int (*comp_api_enable)(const struct device *dev);

/**
 * @brief See comp_disable() for argument descriptions
 */
typedef int (*comp_api_disable)(const struct device *dev);

#if (defined(CONFIG_COMP_STM32))
/**
 * @brief See comp_lock() for argument descriptions
 */
typedef int (*comp_api_lock)(const struct device *dev);
#endif // (defined(CONFIG_COMP_STM32))

And the API function definitions:

/**
 * @brief Enables the comparator
 * @param dev Pointer to the device structure for the driver instance
 * @retval 0 on success or -ENODEV if clock is not ready
 */
__syscall int comp_enable(const struct device *dev)
{
    const struct comp_driver_api *api = (const struct comp_driver_api *)dev->api;

    return api->enable(dev);
}

/**
 * @brief Disable the comparator
 * @param dev Pointer to the device structure for the driver instance
 * @retval 0 on success, -ENODEV if clock is not ready or -EBUSY if 
 *         device is locked
 */
__syscall int comp_disable(const struct device *dev)
{
    const struct comp_driver_api *api = (const struct comp_driver_api *)dev->api;

    return api->disable(dev);
}

#if (defined(CONFIG_COMP_STM32))
/**
 * @brief Prevent the comparator configuration from being modified after
 *             locking the device
 * @param dev Pointer to the device structure for the driver instance
 * @retval 0 on success, -ENODEV is clock is not ready or -EBUSY if 
 *         device is already locked
 */
__syscall int comp_lock(const struct device *dev)
{
    const struct comp_driver_api *api = (const struct comp_driver_api *)dev->api;

    return api->lock(dev);
}
#endif // (defined(CONFIG_COMP_STM32))

As for the _data structure, I propose something similar to this as a starting point:

struct comp_data
{
    uint32_t input_plus;
    uint32_t input_minus;
    uint32_t input_hysteresis;
    uint32_t output_polarity;
    uint32_t output_blanking_source;
};

The _cfg structure will be implementation dependent so as to contain to appropriate types to enable interaction with the underlying vendor API.

Dependencies

I don't believe that this is dependent on anything, but it would be great to hear if other conversations around this topic are currently taking place.

Concerns and Unresolved Questions

• The API is very simplistic and relies on configuration during initialisation via Devicetree. Whilst this has been perfectly amenable to my use case, other users may require the capability to perform runtime configuration and, as such, other function calls would need to be introduced.
• The comp_lock function is great, but I couldn't find similar functionality in other silicon (though I only did a very quick cursory search) - would a software implementation of this be useful for devices that don't have the functionality in hardware? If so this would change the macro guard to CONFIG_COMP_LOCK - omitting the STM32 suffix.
• The comp_data structure is currently very STM32-focussed, so it would be great to get input from other users as to how this could become more generic.

Lastly, I am happy to volunteer to maintain this driver in the future, however this will be my first contribution to the project so I can understand if other members would be hesitant for me to step in to the role.

Thanks for taking the time to read this, Adam

[henrikbrixandersen]

FWIW, I have previously written a driver for the Analog Comparator (ACMP) present in some of the NXP series MCUs using the Zephyr sensor driver framework: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/sensor/mcux_acmp/mcux_acmp.c

[ottojo]

Hi! I also think a comparator API would be useful! I used the comparators in an STM32F072 to debug USB-PD signals with logic level below the threshold of my logic analyzer, and wrote a driver focusing on static configuration through devicetree (with no runtime functionality at all): https://github.com/ottojo/zephyr_stm32_comp

Pinctrl integration works as well, but requires some changes to the hal_stm32: https://github.com/ottojo/hal_stm32/commit/374236373074d46b9f7f7750268ca4ecd7a5348a

@bp-amitchone is your driver available somewhere? I think an STM32 COMP driver would be nice to have upstream even without a generic API (although the proposed functions seem generic enough to not cause any issues, to me at least...)

[nono313]

Any update on this RFC? I would be very interested to see a analog comparator driver API in Zephyr.