Feature Request: API should use peripheral instance index instead of peripheral base address

[danielholala]

Hi there,

Every time I use a standard SDK driver I wonder why I have to put in the register base address of the peripheral instance which the driver is using.

In my opinion it would be much more convenient if one just had to enter the index of the peripheral instance (or 0 if the peripheral has only one instance).

For example when using PWM (see "fsl_ctimer.h"), I have to call CTIMER_SetupPwm() function to setup PWM:

status_t CTIMER_SetupPwm(CTIMER_Type *base,
                         const ctimer_match_t pwmPeriodChannel,
                         ctimer_match_t matchChannel,
                         uint8_t dutyCyclePercent,
                         uint32_t pwmFreq_Hz,
                         uint32_t srcClock_Hz,
                         bool enableInt)
{
    assert(pwmFreq_Hz > 0U);

    uint32_t reg;
    uint32_t period, pulsePeriod = 0;
    uint32_t timerClock = srcClock_Hz / (base->PR + 1U);
    uint32_t index      = CTIMER_GetInstance(base);

From the signature you can tell that I have to provide the base address of the timer instance, i.e., CTIMER0, CTIMER1, etc. (see the register definition file of the specific MCU).

CTIMER_SetupPwm()  then calls CTIMER_GetInstance(base) to identify the numerical index of the timer instance. To do this is loops through all base addresses s_ctimerBases[] of available timer instances and checks whether it matches with the provided base address.

This seems cumbersome and superfluous.

Suggestion:

API functions should take the index of the peripheral instance as first parameter (instead of register base address).

This would get rid of the for-loop and would improve usability of the driver as one would not have to look up the name of the base address but could just enter the index of the peripheral instance.

In the example:

status_t CTIMER_SetupPwm(uint32_t index,
                         const ctimer_match_t pwmPeriodChannel,
                         ctimer_match_t matchChannel,
                         uint8_t dutyCyclePercent,
                         uint32_t pwmFreq_Hz,
                         uint32_t srcClock_Hz,
                         bool enableInt) {

CTIMER_Type *base = s_ctimerBases[index];

That would do it.

Does that make sense?

Cheers
Daniel

[jay_heng]

As far as i know, there are historical reasons for this.

on some devices, peripheral instance starts from 0, but on other devices, peripheral instance starts from 1, this is due to device header file differences. as driver is common for all devices, that's why you see all the APIs take base address instead of instance here.

[danielholala]

Dear @jay_heng ,

Thank you for your reply. If those inconsistencies are a historical residue, why not start to clean up the API in order to improve consistency? 

I just don't see a reason why some API calls require the index of a peripheral instance, e.g.,

/* Get FLEXCOMM input clock */
/*! brief Return Frequency of flexcomm input clock
* param id : flexcomm instance id
* return Frequency value
*/
uint32_t CLOCK_GetFlexCommInputClock(uint32_t id);

while other require the base address of the peripheral instance, e.g.,

/*! @brief Initializes FLEXCOMM and selects peripheral mode according to the second parameter. */
status_t FLEXCOMM_Init(void *base, FLEXCOMM_PERIPH_T periph);

As a start, you could always add a second function with same functionality but different signature that uses the index instead of base address as first parameter.

Just saying...
Daniel

[jay_heng]

Using Base address as first parameter is also common practice of API, you can see the competitor's SDK (eg, STM StdPeriph_Driver )