Skip to content

drivers: sensor: nxp: p3t1755: impl pm device runtime #90946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+83 −8
Open

drivers: sensor: nxp: p3t1755: impl pm device runtime #90946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 81 additions & 8 deletions drivers/sensor/nxp/p3t1755/p3t1755.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include <zephyr/sys/__assert.h>
#include <zephyr/logging/log.h>
#include <stdlib.h>
#include <zephyr/pm/device.h>
#include <zephyr/pm/device_runtime.h>

LOG_MODULE_REGISTER(P3T1755, CONFIG_SENSOR_LOG_LEVEL);

Expand All @@ -29,22 +31,52 @@ static int p3t1755_i3c_write_reg(const struct device *dev, uint8_t reg, uint8_t

return i3c_burst_write(data->i3c_dev, reg, byte, len);
}

static int p3t1755_i3c_get(const struct device *dev)
{
const struct p3t1755_config *config = dev->config;

return pm_device_runtime_get(config->i3c.bus);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be happening in the i2c/i3c driver code instead, when they are being used, hence "runtime" power management?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the driver sensor gets its bus when it needs to do transfers, and puts it when done, just like a user would with any other device :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the driver sensor gets its bus when it needs to do transfers, and puts it when done, just like a user would with any other device :)

But this is not the case really and how runtime power management should work, the device itself does the get/put, not the user. Sure there are special cases, maybe with sensors, but the idea is for users not to bother with power management and have this happen at the level driver itself, not its users. Our get/put helpers are very simplified and do not deal with special cases like Linux does as can be seen here: https://docs.kernel.org/power/runtime_pm.html#runtime-pm-device-helper-functions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the driver sensor gets its bus when it needs to do transfers, and puts it when done, just like a user would with any other device :)

But this is not the case really and how runtime power management should work, the device itself does the get/put, not the user. Sure there are special cases, maybe with sensors, but the idea is for users not to bother with power management and have this happen at the level driver itself, not its users. Our get/put helpers are very simplified and do not deal with special cases like Linux does as can be seen here: https://docs.kernel.org/power/runtime_pm.html#runtime-pm-device-helper-functions

I really do not agree with this at all whatsoever, how it should work is the devices are all suspended by default and anything that knows it uses the hard needs to participate in the reference counting by calling these pm get/put APIs, and as far as I can see that includes both the client layer and the driver if it is a transactional API and needs to finish the transaction, like in the case of I2C. For example, if the client doesn't participate then you get a lot of extra suspend and resume that load the system and might not be desired since only the client has a better idea of the system load and situation, if the implementer doesn't then you potentially get issues like suspending during a bus transaction. You can't just say that one layer has the responsibility alone, because all the different components of software have different knowledge about the system, that's why reference counting is so crucial on complex computing systems.

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A funny example is nordics GPIO ports on the nrf54h20, to change the state of a pin, we need to power a power domain, which is done using IPC, so every single toggle of a pin, comes with two IPC transactions if we hide this in the gpio_pin_toggle API implementation, worse, this also makes gpio_pin_toggle impossible from ISRs since we need to send an IPC message and wait for it, twice... However, if the user gets the port before doing the toggle, its done only once and likely from thread context, after which gpio_pin_toggle becomes a single register write again :)

}

static int p3t1755_i3c_put(const struct device *dev)
{
const struct p3t1755_config *config = dev->config;

return pm_device_runtime_put(config->i3c.bus);
}
#endif

#if DT_ANY_INST_ON_BUS_STATUS_OKAY(i2c)
int p3t1755_i2c_read_reg(const struct device *dev, uint8_t reg, uint8_t *value, uint8_t len)
__maybe_unused static int p3t1755_i2c_read_reg(const struct device *dev, uint8_t reg,
uint8_t *value, uint8_t len)
{
const struct p3t1755_config *config = dev->config;

return i2c_burst_read_dt(&config->bus_cfg.i2c, reg, value, len);
}

int p3t1755_i2c_write_reg(const struct device *dev, uint8_t reg, uint8_t *byte, uint8_t len)
__maybe_unused static int p3t1755_i2c_write_reg(const struct device *dev, uint8_t reg,
uint8_t *byte, uint8_t len)
{
const struct p3t1755_config *config = dev->config;

return i2c_burst_write_dt(&config->bus_cfg.i2c, reg, byte, len);
}

__maybe_unused static int p3t1755_i2c_get(const struct device *dev)
{
const struct p3t1755_config *config = dev->config;

return pm_device_runtime_get(config->bus_cfg.i2c.bus);
}

__maybe_unused static int p3t1755_i2c_put(const struct device *dev)
{
const struct p3t1755_config *config = dev->config;

return pm_device_runtime_put(config->bus_cfg.i2c.bus);
}
#endif

static int p3t1755_sample_fetch(const struct device *dev, enum sensor_channel chan)
Expand All @@ -58,6 +90,8 @@ static int p3t1755_sample_fetch(const struct device *dev, enum sensor_channel ch
return -ENOTSUP;
}

config->ops.get(dev);

if (config->oneshot_mode) {
data->config_reg |= P3T1755_CONFIG_REG_OS;
config->ops.write(dev, P3T1755_CONFIG_REG, &data->config_reg, 1);
Expand All @@ -67,6 +101,8 @@ static int p3t1755_sample_fetch(const struct device *dev, enum sensor_channel ch

int status = config->ops.read(dev, P3T1755_TEMPERATURE_REG, raw_temp, 2);

config->ops.put(dev);

if (status) {
LOG_ERR("read return error %d", status);
return -ENOTSUP;
Expand Down Expand Up @@ -115,25 +151,31 @@ static int p3t1755_channel_get(const struct device *dev, enum sensor_channel cha
return 0;
}

static int p3t1755_init(const struct device *dev)
static int p3t1755_pm_resume(const struct device *dev)
{
const struct p3t1755_config *config = dev->config;
struct p3t1755_data *data = dev->data;
int ret = -ENODEV;

if (config->ops.get(dev)) {
LOG_ERR("Bus device get failed");
goto put_and_ret;
}

#if DT_ANY_INST_ON_BUS_STATUS_OKAY(i3c)
if (config->i3c.bus != NULL) {
data->i3c_dev = i3c_device_find(config->i3c.bus, &config->i3c.dev_id);
if (data->i3c_dev == NULL) {
LOG_ERR("Cannot find I3C device descriptor");
return -ENODEV;
goto put_and_ret;
}
}
#endif
#if DT_ANY_INST_ON_BUS_STATUS_OKAY(i2c)
if (config->inst_on_bus == P3T1755_BUS_I2C) {
if (!i2c_is_ready_dt(&config->bus_cfg.i2c)) {
LOG_ERR("I2C bus device not ready");
return -ENODEV;
goto put_and_ret;
}
}
#endif
Expand All @@ -147,9 +189,35 @@ static int p3t1755_init(const struct device *dev)
config->ops.write(dev, P3T1755_CONFIG_REG, &data->config_reg, 1);
}

LOG_DBG("Init complete");
ret = 0;

return 0;
put_and_ret:
(void)config->ops.put(dev);

return ret;
}

static int p3t1755_pm_hook(const struct device *dev, enum pm_device_action action)
{
int ret;

switch (action) {
case PM_DEVICE_ACTION_SUSPEND:
ret = 0;
break;
case PM_DEVICE_ACTION_RESUME:
ret = p3t1755_pm_resume(dev);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any power savings in this implementation other than adding latency of calling resume repeatedly on resume.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the TURN_ON action instead of RESUME?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think so, turn on will happen if power domain comes up for any reason, not necessarily because this device is being used

Copy link
Collaborator

@mmahadevan108 mmahadevan108 Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TURN_ON action is called as part of pm_device_driver_init

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you're understanding what I mean. a device resumes if it has any reference count indicating that it is actually being used. So if the sensor is resumed, it means it's being used, so it needs to use it's bus device, and therefore puts a claim on it. If the sensor is not being used, it won't be resumed. The only time you need to resume the bus device from the perspective of this sensor driver is if the sensor is resumed.

Turn on action has nothing to do with the sensor being used. Any time a domain goes up it will call turn on on all of it's children, doesn't matter if they are supposed to be used or not, it's just a notification that their power is available. There's no reason to resume the bus device if the sensor driver get's a turn on action, because it doesn't mean that we actually need to do anything. And resuming the bus device will just potentially result in another domain getting turned on and wasting a bunch of time for no reason.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for explaining.

break;
default:
ret = -ENOTSUP;
}

return ret;
}

static int p3t1755_init(const struct device *dev)
{
return pm_device_driver_init(dev, p3t1755_pm_hook);
}

static DEVICE_API(sensor, p3t1755_driver_api) = {
Expand All @@ -165,6 +233,8 @@ static DEVICE_API(sensor, p3t1755_driver_api) = {
.ops = { \
.read = p3t1755_i2c_read_reg, \
.write = p3t1755_i2c_write_reg, \
.get = p3t1755_i2c_get, \
.put = p3t1755_i2c_put, \
}, \
.inst_on_bus = P3T1755_BUS_I2C,

Expand All @@ -176,6 +246,8 @@ static DEVICE_API(sensor, p3t1755_driver_api) = {
.ops = { \
.read = p3t1755_i3c_read_reg, \
.write = p3t1755_i3c_write_reg, \
.get = p3t1755_i3c_get, \
.put = p3t1755_i3c_put, \
}, \
.inst_on_bus = P3T1755_BUS_I3C, \
.i3c.bus = DEVICE_DT_GET(DT_INST_BUS(inst)), .i3c.dev_id = I3C_DEVICE_ID_DT_INST(inst),
Expand All @@ -189,7 +261,8 @@ static DEVICE_API(sensor, p3t1755_driver_api) = {
.oneshot_mode = DT_INST_PROP(n, oneshot_mode), \
}; \
\
SENSOR_DEVICE_DT_INST_DEFINE(n, p3t1755_init, NULL, &p3t1755_data_##n, \
PM_DEVICE_DT_INST_DEFINE(n, p3t1755_pm_hook); \
SENSOR_DEVICE_DT_INST_DEFINE(n, p3t1755_init, PM_DEVICE_DT_INST_GET(n), &p3t1755_data_##n, \
&p3t1755_config_##n, POST_KERNEL, \
CONFIG_SENSOR_INIT_PRIORITY, &p3t1755_driver_api);

Expand Down
2 changes: 2 additions & 0 deletions drivers/sensor/nxp/p3t1755/p3t1755.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
struct p3t1755_io_ops {
int (*read)(const struct device *dev, uint8_t reg, uint8_t *byte, uint8_t len);
int (*write)(const struct device *dev, uint8_t reg, uint8_t *byte, uint8_t len);
int (*get)(const struct device *dev);
int (*put)(const struct device *dev);
};

union p3t1755_bus_cfg {
Expand Down