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

bjarki-andreasen · 2025-06-02T13:19:03Z

Implement pm device runtime for p3t1755 sensor.

yperess · 2025-06-02T16:22:27Z

drivers/sensor/nxp/p3t1755/p3t1755.c

+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);
+}
+
+int p3t1755_i2c_put(const struct device *dev)


Why not static?

I'm following the pattern of the other APIs, I tried making them static and I get an "unused" warning :) outside scope of this PR

These, make them static and compiler complains

zephyr/drivers/sensor/nxp/p3t1755/p3t1755.c

Lines 35 to 47 in e0a915a

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)

{

const struct p3t1755_config *config = dev->config;

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

}

Add __maybe_unused:

__maybe_unused static int p3t1755_i2c_get(const struct device*dev)

yperess · 2025-06-02T16:25:08Z

drivers/sensor/nxp/p3t1755/p3t1755.c

+	case PM_DEVICE_ACTION_SUSPEND:
+		ret = p3t1755_pm_suspend(dev);
+		break;


Why are we calling this if it's a no-op?

~~keeping it simple following the style of the other APIs :) I can ifdef it out...~~ EDIT sorry, yeah, removed it :)

Implement pm device runtime for p3t1755 sensor. Signed-off-by: Bjarki Arge Andreasen <[email protected]>

nashif · 2025-06-02T18:33:53Z

drivers/sensor/nxp/p3t1755/p3t1755.c

+{
+	const struct p3t1755_config *config = dev->config;
+
+	return pm_device_runtime_get(config->i3c.bus);


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

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 :)

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

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.

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 :)

sonarqubecloud · 2025-06-02T18:45:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

yperess · 2025-06-02T20:25:20Z

drivers/sensor/nxp/p3t1755/p3t1755.c

+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);
+}
+
+int p3t1755_i2c_put(const struct device *dev)


Add __maybe_unused:

__maybe_unused static int p3t1755_i2c_get(const struct device*dev)

mmahadevan108 · 2025-06-02T22:31:07Z

drivers/sensor/nxp/p3t1755/p3t1755.c

+		ret = 0;
+		break;
+	case PM_DEVICE_ACTION_RESUME:
+		ret = p3t1755_pm_resume(dev);


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

Should this be in the TURN_ON action instead of RESUME?

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

TURN_ON action is called as part of pm_device_driver_init

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.

Got it, thanks for explaining.

mmahadevan108

Need information why this is needed as the implementation does not seem to be saving power..

decsny · 2025-06-05T14:47:43Z

@mmahadevan108 the point of the PR is clearly not to save power directly from the sensor, it's to claim and release the i2c bus device resource properly with the pm APIs.... because you can't use the sensor if the underlying i2c to talk to it is suspended...

decsny · 2025-06-13T16:15:59Z

@bjarki-andreasen maybe this is a stupid question, but, instead of putting all this pm resuming/getting and suspending/putting code in these drivers like you are doing here with the sensor driver, I thought this was supposed to be the point of your refactor in #89372 for the PM subsystem to handle this automatically? So couldn't you just make the bus device the power domain of the sensor device and it would have the same effect, where if you resume the sensor then the bus also would get automatically resumed?

bjarki-andreasen · 2025-06-13T17:02:26Z

@bjarki-andreasen maybe this is a stupid question, but, instead of putting all this pm resuming/getting and suspending/putting code in these drivers like you are doing here with the sensor driver, I thought this was supposed to be the point of your refactor in #89372 for the PM subsystem to handle this automatically? So couldn't you just make the bus device the power domain of the sensor device and it would have the same effect, where if you resume the sensor then the bus also would get automatically resumed?

Two things to consider:

The bus controller is not the sensors power domain, the sensor can be powered without the bus controller being powered, its entirely independent :)
To answer the real question, should the bus be powered as long as the sensor is? Not in this case. The sensor itself is the "user" of the bus, it gets the bus for the entire duration of the fetch, a write, a check and a read. Since this sensor is a low throughput temperature sensor, its optimal to get/put on every fetch in most cases. If we where dealing with a high throughput IMU or something, the bus would likely be active all the time, depending on the configured datarate, but again, the driver is the "user" here, it decides what's most optimal :)

github-actions bot added area: Sensors Sensors platform: NXP Drivers NXP Semiconductors, drivers labels Jun 2, 2025

github-actions bot requested review from asemjonovs, avisconti, dbaluta, decsny, dleach02, jeppenodgaard, manuargue, MaureenHelm, mmahadevan108, Raymond0225, teburd, tristan-google, ubieda and yperess June 2, 2025 13:20

github-actions bot assigned dleach02 and mmahadevan108 Jun 2, 2025

bjarki-andreasen mentioned this pull request Jun 2, 2025

PM_DEVICE_POWER_DOMAIN refactor #89372

Open

yperess requested changes Jun 2, 2025

View reviewed changes

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

ec3d0b6

Implement pm device runtime for p3t1755 sensor. Signed-off-by: Bjarki Arge Andreasen <[email protected]>

bjarki-andreasen force-pushed the p3t1755-impl-device-runtime branch from 67707f5 to ec3d0b6 Compare June 2, 2025 18:14

nashif reviewed Jun 2, 2025

View reviewed changes

yperess requested changes Jun 2, 2025

View reviewed changes

mmahadevan108 reviewed Jun 2, 2025

View reviewed changes

mmahadevan108 requested changes Jun 3, 2025

View reviewed changes

bjarki-andreasen mentioned this pull request Jun 14, 2025

Specify and document PM_DEVICE behavior, including PM_DEVICE=n and device_deinit() #90275

Open

mmahadevan108 approved these changes Jun 16, 2025

View reviewed changes

decsny approved these changes Jun 17, 2025

View reviewed changes

	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)
	{
	const struct p3t1755_config *config = dev->config;

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

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

Are you sure you want to change the base?

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

Conversation

bjarki-andreasen commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 2, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmahadevan108 Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmahadevan108 left a comment

Choose a reason for hiding this comment

Uh oh!

decsny commented Jun 5, 2025

Uh oh!

decsny commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarki-andreasen commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bjarki-andreasen Jun 2, 2025 •

edited

Loading

bjarki-andreasen Jun 2, 2025 •

edited

Loading

bjarki-andreasen Jun 2, 2025 •

edited

Loading

bjarki-andreasen Jun 5, 2025 •

edited

Loading

mmahadevan108 Jun 9, 2025 •

edited

Loading

decsny commented Jun 13, 2025 •

edited

Loading

bjarki-andreasen commented Jun 13, 2025 •

edited

Loading