drivers: regulator: implement active discharge api #61056

XenuIsWatching · 2023-08-02T05:10:13Z

Add an active discharge api for regulators. This uses the already existing but previously unused regulator-active-discharge property.

This implements it within the pca9420.

I am currently working on my own PMIC where I need this api but I do not have the pca9420 to test with and verify it... but this seems like a trivial addition

TODO:

Add test cases
Add shell cmd
only allow bool param

gmarull

before adding new ops to regulators:

Is this feature present across multiple regulator devices? Is this related with e.g. ADP5360 adi,enable-output-discharge?
Is this meant to be set/unset at runtime? For example, ADP5360 driver just takes it from DT at init time
It may be better to introduce a more generic set/get props API. I fear more properties will pop up, and we can't keep increasing ops table for each one...

XenuIsWatching · 2023-08-10T20:15:08Z

Is this feature present across multiple regulator devices? Is this related with e.g. ADP5360 adi,enable-output-discharge?

This is also present in other regulator devices. I am working on the MAX77643. This api is also implemented in the linux regulator driver which this was modeled after.
https://github.com/torvalds/linux/blob/25aa0bebba72b318e71fe205bfd1236550cc9534/include/linux/regulator/driver.h#L186
It looks the ADP5360 is it's own 'vendor' prop when there is already one in the regulator.yaml which is the one I'm using.

Is this meant to be set/unset at runtime? For example, ADP5360 driver just takes it from DT at init time

yes. the ADP5360 may want to have this api implemented as well.... but I do not have that part to test out.

It may be better to introduce a more generic set/get props API. I fear more properties will pop up, and we can't keep increasing ops table for each one...

I understand your concerns there...I'm not sure what you mean by a 'more generic set/get prop api`, but this active discharge setting appears to be very generic and is within a lot of regulators. Just not implemented in some of the already existing drivers.

gmarull

lgtm, just a few comments, will actually migrate adp driver to this new API.

gmarull · 2023-08-11T10:03:03Z

include/zephyr/drivers/regulator.h

+ * @retval -errno In case of any other error.
+ */
+static inline int regulator_set_active_discharge(const struct device *dev,
+				     uint8_t active_discharge)


Suggested change

uint8_t active_discharge)

bool enable)

my concern with making it a bool would be handle the issue of it not being defined where it just gets set to the max value of a UINT8

I think bool would be better if we can assume that active_discharge is always disabled by default. If this is not a safe assumption I think it is OK like this.

From an API client perspective, this function will always be called with either 0 or 1 (bool), am I wrong?

yeah. I updated to be a bool

gmarull · 2023-08-11T10:04:18Z

include/zephyr/drivers/regulator.h

+ * @retval -errno In case of any other error.
+ */
+static inline int regulator_get_active_discharge(const struct device *dev,
+				     uint8_t *active_discharge)


Suggested change

uint8_t *active_discharge)

bool *enabled)

note: Linux doesn't seem to offer a getter, is it really needed?

correct, but just because Linux doesn't do it doesn't mean we also shouldn't do it. I can't really think of a reason not to have it.

Linux is usually right, Zephyr... not so much ;-)

gmarull · 2023-08-11T10:05:46Z

tests/drivers/regulator/api/src/main.c

@@ -611,6 +611,58 @@ ZTEST(regulator_api, test_get_mode_not_implemented)
 	zassert_equal(ret, -ENOSYS);
 }

+ZTEST(regulator_api, test_set_active_discharge_not_implemented)


please update test_common_config test as well, to cover correct capture of dt info

gmarull · 2023-08-11T10:07:34Z

Is this feature present across multiple regulator devices? Is this related with e.g. ADP5360 adi,enable-output-discharge?

This is also present in other regulator devices. I am working on the MAX77643. This api is also implemented in the linux regulator driver which this was modeled after. https://github.com/torvalds/linux/blob/25aa0bebba72b318e71fe205bfd1236550cc9534/include/linux/regulator/driver.h#L186 It looks the ADP5360 is it's own 'vendor' prop when there is already one in the regulator.yaml which is the one I'm using.

Is this meant to be set/unset at runtime? For example, ADP5360 driver just takes it from DT at init time

yes. the ADP5360 may want to have this api implemented as well.... but I do not have that part to test out.

It may be better to introduce a more generic set/get props API. I fear more properties will pop up, and we can't keep increasing ops table for each one...

I understand your concerns there...I'm not sure what you mean by a 'more generic set/get prop api`, but this active discharge setting appears to be very generic and is within a lot of regulators. Just not implemented in some of the already existing drivers.

thanks for this info, make sense

carlescufi · 2023-09-13T14:30:27Z

@gmarull @aasinclair could you please take a look?

MaureenHelm · 2023-10-05T15:49:31Z

@gmarull @aasinclair could you please take a look?

ping

gmarull · 2023-10-06T07:58:23Z

@gmarull @aasinclair could you please take a look?

ping

this PR has unresolved questions, please check.

Add an active discharge api for regulators. This uses the already existing but previously unused regulator-active-discharge property. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

Add tests for active discharge apis not being implemented. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

Add the active discharge functions that can be called from the regulator shell. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

Implement the api for controlling the active discharge setting within the pca9420. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

XenuIsWatching · 2024-01-29T06:59:38Z

@gmarull This should be ready for another review. Thanks!

XenuIsWatching force-pushed the regulator-active-discharge branch 8 times, most recently from 40021c2 to d4d35f2 Compare August 7, 2023 17:02

XenuIsWatching marked this pull request as ready for review August 7, 2023 20:16

XenuIsWatching requested review from nashif, carlescufi, galak, MaureenHelm, danieldegrasse and gmarull as code owners August 7, 2023 20:16

zephyrbot added the area: Regulators label Aug 7, 2023

zephyrbot requested a review from aasinclair August 7, 2023 20:17

zephyrbot assigned gmarull Aug 7, 2023

XenuIsWatching force-pushed the regulator-active-discharge branch 3 times, most recently from aac75a6 to 5ad2d4c Compare August 7, 2023 20:48

gmarull reviewed Aug 9, 2023

View reviewed changes

gmarull reviewed Aug 11, 2023

View reviewed changes

XenuIsWatching force-pushed the regulator-active-discharge branch 2 times, most recently from 1e25b52 to 6cb220b Compare August 26, 2023 09:03

zephyrbot added manifest-zcbor manifest-hal_wurthelektronik manifest-zscilib DNM This PR should not be merged (Do Not Merge) labels Dec 12, 2023

XenuIsWatching force-pushed the regulator-active-discharge branch 2 times, most recently from a5c9c39 to e9830b3 Compare December 13, 2023 00:33

zephyrbot removed manifest manifest-hal_openisa manifest-zscilib manifest-zcbor manifest-hal_wurthelektronik DNM This PR should not be merged (Do Not Merge) labels Dec 13, 2023

XenuIsWatching force-pushed the regulator-active-discharge branch from e9830b3 to 5db8039 Compare January 23, 2024 00:50

XenuIsWatching removed area: Build System area: C Library C Standard Library area: Toolchains Toolchains labels Jan 23, 2024

XenuIsWatching force-pushed the regulator-active-discharge branch 2 times, most recently from 2f0c05d to d6a131c Compare January 23, 2024 00:57

regulator: add active-discharge api

0453fac

Add an active discharge api for regulators. This uses the already existing but previously unused regulator-active-discharge property. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

XenuIsWatching force-pushed the regulator-active-discharge branch from d6a131c to 005eab9 Compare January 24, 2024 18:47

XenuIsWatching added 3 commits January 24, 2024 12:02

tests: drivers: regulator: add api test for active discharge

f34fac7

Add tests for active discharge apis not being implemented. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

drivers: regulator: add active discharge api to regulator shell

eb244c9

Add the active discharge functions that can be called from the regulator shell. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

drivers: regulator: pca9420: implement active discharge api

8e52da6

Implement the api for controlling the active discharge setting within the pca9420. Signed-off-by: Ryan McClelland <ryanmcclelland@meta.com>

XenuIsWatching force-pushed the regulator-active-discharge branch from 005eab9 to 8e52da6 Compare January 24, 2024 20:03

XenuIsWatching requested a review from gmarull January 25, 2024 18:53

gmarull approved these changes Jan 29, 2024

View reviewed changes

aasinclair approved these changes Jan 29, 2024

View reviewed changes

henrikbrixandersen approved these changes Jan 29, 2024

View reviewed changes

henrikbrixandersen merged commit 4fb4f71 into zephyrproject-rtos:main Jan 29, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: regulator: implement active discharge api #61056

drivers: regulator: implement active discharge api #61056

XenuIsWatching commented Aug 2, 2023 •

edited

Loading

gmarull left a comment

XenuIsWatching commented Aug 10, 2023 •

edited

Loading

gmarull left a comment

gmarull Aug 11, 2023

XenuIsWatching Aug 26, 2023

aasinclair Sep 14, 2023

gmarull Sep 14, 2023

XenuIsWatching Dec 10, 2023

gmarull Aug 11, 2023

XenuIsWatching Aug 26, 2023

gmarull Sep 14, 2023

gmarull Aug 11, 2023

gmarull commented Aug 11, 2023

carlescufi commented Sep 13, 2023

MaureenHelm commented Oct 5, 2023

gmarull commented Oct 6, 2023

XenuIsWatching commented Jan 29, 2024

drivers: regulator: implement active discharge api #61056

drivers: regulator: implement active discharge api #61056

Conversation

XenuIsWatching commented Aug 2, 2023 • edited Loading

gmarull left a comment

Choose a reason for hiding this comment

XenuIsWatching commented Aug 10, 2023 • edited Loading

gmarull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull commented Aug 11, 2023

carlescufi commented Sep 13, 2023

MaureenHelm commented Oct 5, 2023

gmarull commented Oct 6, 2023

XenuIsWatching commented Jan 29, 2024

XenuIsWatching commented Aug 2, 2023 •

edited

Loading

XenuIsWatching commented Aug 10, 2023 •

edited

Loading