michaelh/zephyr
1
0
Fork 0
Code Releases Activity

drivers: regulator: drop async enable

Browse source 
Drop the async enable function. This feature is rarely/never used,
complicates driver design, and doesn't really follow the sync/async API
design/naming used in other areas. In the future we can introduce
regulator_enable_async if needed, with support from the driver class (no
onoff). Note that drivers like PCA9420 did not implement any
asynchronous behavior. regulator-fixed implemented in the past
asynchronous behavior using work queues, an overkill for most GPIO
driven regulators. Let's keep things simple for now and extend the API
when needed, based on specific usecases.

In the current implementation, reference counting is managed by the
driver class. \isr-ok attribute is dropped, since calls are potentially
blocking. Note that drivers like PCA9420 already violated such rule.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
This commit is contained in:
Gerard Marull-Paretas 2022-11-29 16:00:06 +01:00 • committed by Carles Cufí
commit a29bdc262c
7 changed files with 132 additions and 180 deletions
Download patch file Download diff file Expand all files Collapse all files

1
drivers/regulator/CMakeLists.txt
View file

@ -3,6 +3,7 @@
zephyr_library() zephyr_library()
zephyr_library_sources(regulator_common.c)
zephyr_library_sources_ifdef(CONFIG_REGULATOR_FIXED regulator_fixed.c) zephyr_library_sources_ifdef(CONFIG_REGULATOR_FIXED regulator_fixed.c)
zephyr_library_sources_ifdef(CONFIG_REGULATOR_PCA9420 regulator_pca9420.c) zephyr_library_sources_ifdef(CONFIG_REGULATOR_PCA9420 regulator_pca9420.c)
zephyr_library_sources_ifdef(CONFIG_REGULATOR_SHELL regulator_shell.c) zephyr_library_sources_ifdef(CONFIG_REGULATOR_SHELL regulator_shell.c)

65
drivers/regulator/regulator_common.c Normal file
View file

@ -0,0 +1,65 @@
/*
* Copyright 2022 Nordic Semiconductor ASA
* SPDX-License-Identifier: Apache-2.0
*/
#include <zephyr/drivers/regulator.h>
void regulator_common_data_init(const struct device *dev)
{
struct regulator_common_data *data =
(struct regulator_common_data *)dev->data;
(void)k_mutex_init(&data->lock);
data->refcnt = 0;
}
int regulator_enable(const struct device *dev)
{
struct regulator_common_data *data =
(struct regulator_common_data *)dev->data;
int ret = 0;
(void)k_mutex_lock(&data->lock, K_FOREVER);
data->refcnt++;
if (data->refcnt == 1) {
const struct regulator_driver_api *api =
(const struct regulator_driver_api *)dev->api;
ret = api->enable(dev);
if (ret < 0) {
data->refcnt--;
}
}
k_mutex_unlock(&data->lock);
return ret;
}
int regulator_disable(const struct device *dev)
{
struct regulator_common_data *data =
(struct regulator_common_data *)dev->data;
int ret = 0;
(void)k_mutex_lock(&data->lock, K_FOREVER);
data->refcnt--;
if (data->refcnt == 0) {
const struct regulator_driver_api *api =
(const struct regulator_driver_api *)dev->api;
ret = api->disable(dev);
if (ret < 0) {
data->refcnt++;
}
}
k_mutex_unlock(&data->lock);
return ret;
}

39
drivers/regulator/regulator_fixed.c
View file

@ -28,62 +28,39 @@ struct regulator_fixed_config {
}; };
struct regulator_fixed_data { struct regulator_fixed_data {
struct onoff_sync_service srv; struct regulator_common_data common;
}; };
static int regulator_fixed_enable(const struct device *dev, static int regulator_fixed_enable(const struct device *dev)
struct onoff_client *cli)
{ {
const struct regulator_fixed_config *cfg = dev->config; const struct regulator_fixed_config *cfg = dev->config;
struct regulator_fixed_data *data = dev->data;
k_spinlock_key_t key;
int ret; int ret;
if ((cfg->options & OPTION_ALWAYS_ON) != 0) { if ((cfg->options & OPTION_ALWAYS_ON) != 0) {
return 0; return 0;
} }
ret = onoff_sync_lock(&data->srv, &key);
if (ret > 0) {
goto finalize;
}
ret = gpio_pin_set_dt(&cfg->enable, 1); ret = gpio_pin_set_dt(&cfg->enable, 1);
if (ret < 0) { if (ret < 0) {
goto finalize; return ret;
} }
if (cfg->off_on_delay_us > 0U) { if (cfg->off_on_delay_us > 0U) {
k_sleep(K_USEC(cfg->off_on_delay_us)); k_sleep(K_USEC(cfg->off_on_delay_us));
} }
finalize: return 0;
return onoff_sync_finalize(&data->srv, key, cli, ret, true);
} }
static int regulator_fixed_disable(const struct device *dev) static int regulator_fixed_disable(const struct device *dev)
{ {
const struct regulator_fixed_config *cfg = dev->config; const struct regulator_fixed_config *cfg = dev->config;
struct regulator_fixed_data *data = dev->data;
k_spinlock_key_t key;
int ret;
if ((cfg->options & OPTION_ALWAYS_ON) != 0) { if ((cfg->options & OPTION_ALWAYS_ON) != 0) {
return 0; return 0;
} }
ret = onoff_sync_lock(&data->srv, &key); return gpio_pin_set_dt(&cfg->enable, 0);
if (ret != 1) {
goto finalize;
}
ret = gpio_pin_set_dt(&cfg->enable, 0);
if (ret < 0) {
return ret;
}
finalize:
return onoff_sync_finalize(&data->srv, key, NULL, ret, false);
} }
static const struct regulator_driver_api regulator_fixed_api = { static const struct regulator_driver_api regulator_fixed_api = {
@ -96,6 +73,8 @@ static int regulator_fixed_init(const struct device *dev)
const struct regulator_fixed_config *cfg = dev->config; const struct regulator_fixed_config *cfg = dev->config;
int ret; int ret;
regulator_common_data_init(dev);
if (!device_is_ready(cfg->enable.port)) { if (!device_is_ready(cfg->enable.port)) {
LOG_ERR("GPIO port: %s not ready", cfg->enable.port->name); LOG_ERR("GPIO port: %s not ready", cfg->enable.port->name);
return -ENODEV; return -ENODEV;
@ -119,6 +98,8 @@ static int regulator_fixed_init(const struct device *dev)
} }
#define REGULATOR_FIXED_DEFINE(inst) \ #define REGULATOR_FIXED_DEFINE(inst) \
static struct regulator_fixed_data data##inst; \
\
static const struct regulator_fixed_config config##inst = { \ static const struct regulator_fixed_config config##inst = { \
.startup_delay_us = DT_INST_PROP(inst, startup_delay_us), \ .startup_delay_us = DT_INST_PROP(inst, startup_delay_us), \
.off_on_delay_us = DT_INST_PROP(inst, off_on_delay_us), \ .off_on_delay_us = DT_INST_PROP(inst, off_on_delay_us), \
@ -129,7 +110,7 @@ static int regulator_fixed_init(const struct device *dev)
<< OPTION_ALWAYS_ON_POS), \ << OPTION_ALWAYS_ON_POS), \
}; \ }; \
\ \
DEVICE_DT_INST_DEFINE(inst, regulator_fixed_init, NULL, NULL, \ DEVICE_DT_INST_DEFINE(inst, regulator_fixed_init, NULL, &data##inst, \
&config##inst, POST_KERNEL, \ &config##inst, POST_KERNEL, \
CONFIG_REGULATOR_FIXED_INIT_PRIORITY, \ CONFIG_REGULATOR_FIXED_INIT_PRIORITY, \
&regulator_fixed_api); &regulator_fixed_api);

59
drivers/regulator/regulator_pca9420.c
View file

@ -89,10 +89,6 @@ struct regulator_pca9420_desc {
const struct linear_range *ranges; const struct linear_range *ranges;
}; };
struct regulator_pca9420_data {
struct onoff_sync_service srv;
};
struct regulator_pca9420_common_config { struct regulator_pca9420_common_config {
struct i2c_dt_spec i2c; struct i2c_dt_spec i2c;
int32_t vin_ilim_ua; int32_t vin_ilim_ua;
@ -111,6 +107,10 @@ struct regulator_pca9420_config {
const struct device *parent; const struct device *parent;
}; };
struct regulator_pca9420_data {
struct regulator_common_data data;
};
static const struct linear_range buck1_ranges[] = { static const struct linear_range buck1_ranges[] = {
LINEAR_RANGE_INIT(500000, 25000U, 0x0U, 0x28U), LINEAR_RANGE_INIT(500000, 25000U, 0x0U, 0x28U),
LINEAR_RANGE_INIT(1500000, 0U, 0x29U, 0x3E), LINEAR_RANGE_INIT(1500000, 0U, 0x29U, 0x3E),
@ -558,55 +558,30 @@ static int regulator_pca9420_set_mode(const struct device *dev, uint32_t mode)
return rc; return rc;
} }
static int regulator_pca9420_enable(const struct device *dev, static int regulator_pca9420_enable(const struct device *dev)
struct onoff_client *cli)
{ {
k_spinlock_key_t key;
int rc;
uint8_t en_val;
struct regulator_pca9420_data *data = dev->data;
const struct regulator_pca9420_config *config = dev->config; const struct regulator_pca9420_config *config = dev->config;
const struct regulator_pca9420_common_config *cconfig = config->parent->config; const struct regulator_pca9420_common_config *cconfig = config->parent->config;
uint8_t en_val;
LOG_DBG("Enabling regulator");
rc = onoff_sync_lock(&data->srv, &key);
if (rc) {
/* Request has already enabled PMIC */
return onoff_sync_finalize(&data->srv, key, cli, rc, true);
}
en_val = config->enable_inverted ? 0 : config->desc->enable_val; en_val = config->enable_inverted ? 0 : config->desc->enable_val;
rc = regulator_pca9420_modify_register(&cconfig->i2c, return regulator_pca9420_modify_register(&cconfig->i2c,
config->desc->enable_reg, config->desc->enable_reg,
config->desc->enable_mask, config->desc->enable_mask,
en_val); en_val);
if (rc != 0) {
return onoff_sync_finalize(&data->srv, key, NULL, rc, false);
}
return onoff_sync_finalize(&data->srv, key, cli, rc, true);
} }
static int regulator_pca9420_disable(const struct device *dev) static int regulator_pca9420_disable(const struct device *dev)
{ {
struct regulator_pca9420_data *data = dev->data;
const struct regulator_pca9420_config *config = dev->config; const struct regulator_pca9420_config *config = dev->config;
const struct regulator_pca9420_common_config *cconfig = config->parent->config; const struct regulator_pca9420_common_config *cconfig = config->parent->config;
k_spinlock_key_t key;
uint8_t dis_val; uint8_t dis_val;
int rc;
LOG_DBG("Disabling regulator"); dis_val = config->enable_inverted ? config->desc->enable_val : 0;
rc = onoff_sync_lock(&data->srv, &key); return regulator_pca9420_modify_register(&cconfig->i2c,
if (rc == 0) { config->desc->enable_reg,
rc = -EINVAL; config->desc->enable_mask,
return onoff_sync_finalize(&data->srv, key, NULL, rc, false); dis_val);
} else if (rc == 1) {
/* Disable regulator */
dis_val = config->enable_inverted ? config->desc->enable_val : 0;
rc = regulator_pca9420_modify_register(
&cconfig->i2c, config->desc->enable_reg,
config->desc->enable_mask, dis_val);
}
return onoff_sync_finalize(&data->srv, key, NULL, rc, false);
} }
static int regulator_pca9420_init(const struct device *dev) static int regulator_pca9420_init(const struct device *dev)
@ -615,12 +590,14 @@ static int regulator_pca9420_init(const struct device *dev)
const struct regulator_pca9420_common_config *cconfig = config->parent->config; const struct regulator_pca9420_common_config *cconfig = config->parent->config;
int rc = 0; int rc = 0;
regulator_common_data_init(dev);
if (!device_is_ready(config->parent)) { if (!device_is_ready(config->parent)) {
return -ENODEV; return -ENODEV;
} }
if (config->boot_on) { if (config->boot_on) {
rc = regulator_pca9420_enable(dev, NULL); rc = regulator_pca9420_enable(dev);
} }
if (cconfig->initial_mode) { if (cconfig->initial_mode) {
rc = regulator_pca9420_set_mode(dev, cconfig->initial_mode); rc = regulator_pca9420_set_mode(dev, cconfig->initial_mode);

3
drivers/regulator/regulator_shell.c
View file

@ -14,7 +14,6 @@ LOG_MODULE_REGISTER(regulator_shell, CONFIG_REGULATOR_LOG_LEVEL);
static int cmd_reg_en(const struct shell *sh, size_t argc, char **argv) static int cmd_reg_en(const struct shell *sh, size_t argc, char **argv)
{ {
struct onoff_client cli;
const struct device *reg_dev; const struct device *reg_dev;
int ret; int ret;
@ -24,7 +23,7 @@ static int cmd_reg_en(const struct shell *sh, size_t argc, char **argv)
return -ENODEV; return -ENODEV;
} }
ret = regulator_enable(reg_dev, &cli); ret = regulator_enable(reg_dev);
if (ret < 0) { if (ret < 0) {
shell_error(sh, "failed to enable regulator, error %d", ret); shell_error(sh, "failed to enable regulator, error %d", ret);
return ret; return ret;

65
include/zephyr/drivers/regulator.h
View file

@ -19,7 +19,7 @@
#include <stdint.h> #include <stdint.h>
#include <zephyr/device.h> #include <zephyr/device.h>
#include <zephyr/sys/onoff.h> #include <zephyr/kernel.h>
#ifdef __cplusplus #ifdef __cplusplus
extern "C" { extern "C" {
@ -29,7 +29,7 @@ extern "C" {
/** @brief Driver-specific API functions to support regulator control. */ /** @brief Driver-specific API functions to support regulator control. */
__subsystem struct regulator_driver_api { __subsystem struct regulator_driver_api {
int (*enable)(const struct device *dev, struct onoff_client *cli); int (*enable)(const struct device *dev);
int (*disable)(const struct device *dev); int (*disable)(const struct device *dev);
int (*count_voltages)(const struct device *dev); int (*count_voltages)(const struct device *dev);
int (*count_modes)(const struct device *dev); int (*count_modes)(const struct device *dev);
@ -51,35 +51,41 @@ __subsystem struct regulator_driver_api {
int (*mode_enable)(const struct device *dev, uint32_t mode); int (*mode_enable)(const struct device *dev, uint32_t mode);
}; };
/**
* @brief Common regulator data.
*
* This structure **must** be placed first in the driver's data structure.
*/
struct regulator_common_data {
/** Lock */
struct k_mutex lock;
/** Reference count */
int refcnt;
};
/**
* @brief Initialize common regulator data.
*
* This function **must** be called when driver is initialized.
*
* @param dev Regulator device instance.
*/
void regulator_common_data_init(const struct device *dev);
/** @endcond */ /** @endcond */
/** /**
* @brief Enable a regulator. * @brief Enable a regulator.
* *
* Reference-counted request that a regulator be turned on. This is an * Reference-counted request that a regulator be turned on. A regulator is
* asynchronous operation; if successfully initiated the result will be * considered "on" when it has reached a stable/usable state.
* communicated through the @p cli parameter.
*
* A regulator is considered "on" when it has reached a stable/usable state.
*
* @funcprops \isr_ok \pre_kernel_ok
* *
* @param dev Regulator device instance * @param dev Regulator device instance
* @param cli On-off client instance. This is used to notify the caller when the
* attempt to turn on the regulator has completed (can be `NULL`).
* *
* @retval 0 If enable request has been successfully initiated. * @retval 0 If regulator has been successfully enabled.
* @retval -errno Negative errno in case of failure (can be from onoff_request() * @retval -errno Negative errno in case of failure.
* or individual regulator drivers).
*/ */
static inline int regulator_enable(const struct device *dev, int regulator_enable(const struct device *dev);
struct onoff_client *cli)
{
const struct regulator_driver_api *api =
(const struct regulator_driver_api *)dev->api;
return api->enable(dev, cli);
}
/** /**
* @brief Disable a regulator. * @brief Disable a regulator.
@ -93,21 +99,12 @@ static inline int regulator_enable(const struct device *dev,
* *
* This must be invoked at most once for each successful regulator_enable(). * This must be invoked at most once for each successful regulator_enable().
* *
* @funcprops \isr_ok
*
* @param dev Regulator device instance. * @param dev Regulator device instance.
* *
* @retval 0 If enable request has been successfully initiated. * @retval 0 If regulator has been successfully disabled.
* @retval -errno Negative errno in case of failure (can be from onoff_release() * @retval -errno Negative errno in case of failure.
* or individual regulator drivers).
*/ */
static inline int regulator_disable(const struct device *dev) int regulator_disable(const struct device *dev);
{
const struct regulator_driver_api *api =
(const struct regulator_driver_api *)dev->api;
return api->disable(dev);
}
/** /**
* @brief Obtain the number of supported voltage levels. * @brief Obtain the number of supported voltage levels.

80
tests/drivers/regulator/fixed/src/main.c
View file

@ -47,47 +47,6 @@ static const char *const pc_errstr[] = {
[PC_OK] = "precheck OK", [PC_OK] = "precheck OK",
}; };
static struct onoff_client cli;
static struct onoff_manager *callback_srv;
static struct onoff_client *callback_cli;
static uint32_t callback_state;
static int callback_res;
static onoff_client_callback callback_fn;
static void callback(struct onoff_manager *srv,
struct onoff_client *cli,
uint32_t state,
int res)
{
onoff_client_callback cb = callback_fn;
callback_srv = srv;
callback_cli = cli;
callback_state = state;
callback_res = res;
callback_fn = NULL;
if (cb != NULL) {
cb(srv, cli, state, res);
}
}
static void reset_callback(void)
{
callback_srv = NULL;
callback_cli = NULL;
callback_state = INT_MIN;
callback_res = 0;
callback_fn = NULL;
}
static void reset_client(void)
{
cli = (struct onoff_client){};
reset_callback();
sys_notify_init_callback(&cli.notify, callback);
}
static int reg_status(void) static int reg_status(void)
{ {
return gpio_pin_get_dt(&check_gpio); return gpio_pin_get_dt(&check_gpio);
@ -197,30 +156,11 @@ ZTEST(regulator, test_basic)
"not off at boot: %d", rs); "not off at boot: %d", rs);
} }
reset_client();
/* Turn it on */ /* Turn it on */
int rc = regulator_enable(reg_dev, &cli); int rc = regulator_enable(reg_dev);
zassert_true(rc >= 0,
"first enable failed: %d", rc);
if (STARTUP_DELAY_US > 0) { zassert_equal(rc, 0,
rc = sys_notify_fetch_result(&cli.notify, &rc); "first enable failed: %d", rc);
zassert_equal(rc, -EAGAIN,
"startup notify early: %d", rc);
while (sys_notify_fetch_result(&cli.notify, &rc) == -EAGAIN) {
k_yield();
}
}
zassert_equal(callback_cli, &cli,
"callback not invoked");
zassert_equal(callback_res, 0,
"callback res: %d", callback_res);
zassert_equal(callback_state, ONOFF_STATE_ON,
"callback state: 0x%x", callback_res);
/* Make sure it's on */ /* Make sure it's on */
@ -230,18 +170,10 @@ ZTEST(regulator, test_basic)
/* Turn it on again (another client) */ /* Turn it on again (another client) */
reset_client(); rc = regulator_enable(reg_dev);
rc = regulator_enable(reg_dev, &cli); zassert_equal(rc, 0,
zassert_true(rc >= 0,
"second enable failed: %d", rc); "second enable failed: %d", rc);
zassert_equal(callback_cli, &cli,
"callback not invoked");
zassert_true(callback_res >= 0,
"callback res: %d", callback_res);
zassert_equal(callback_state, ONOFF_STATE_ON,
"callback state: 0x%x", callback_res);
/* Make sure it's still on */ /* Make sure it's still on */
rs = reg_status(); rs = reg_status();
@ -263,7 +195,7 @@ ZTEST(regulator, test_basic)
/* Turn it off again (no more clients) */ /* Turn it off again (no more clients) */
rc = regulator_disable(reg_dev); rc = regulator_disable(reg_dev);
zassert_true(rc >= 0, zassert_equal(rc, 0,
"first disable failed: %d", rc); "first disable failed: %d", rc);
/* On if and only if it can't be turned off */ /* On if and only if it can't be turned off */