Introduce Charging Subsystem #56666

rriveramcrus · 2023-04-07T20:48:21Z

Introduce a Battery Charging API #56635

include/zephyr/drivers/charger.h

drivers/Kconfig

drivers/charger/Kconfig

drivers/charger/sbs_charger/emul_sbs_charger.c

drivers/charger/sbs_charger/sbs_charger.c

tests/drivers/charger/sbs_charger/testcase.yaml

tests/drivers/charger/sbs_charger/src/test_sbs_charger.c

dts/bindings/charger/charger.yaml

include/zephyr/drivers/charger.h

tests/drivers/charger/sbs_charger/boards/emulated_board.conf

gmarull · 2023-04-18T07:55:32Z

tests/drivers/charger/sbs_charger/CMakeLists.txt

@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.20.0)


please add tests into a separate commit to ease review

sure, I can split it into two commits:

Introduces the driver

Adds tests

#56666 (comment)
I don't want to mess up the existing comments by splitting up the commits. I'll split it up once this comment is resolved.

tests/drivers/charger/sbs_charger/src/test_sbs_charger.c

tests/drivers/charger/sbs_charger/testcase.yaml

include/zephyr/drivers/charger.h

drivers/charger/sbs_charger/sbs_charger.c

tests/drivers/charger/sbs_charger/testcase.yaml

drivers/charger/sbs_charger/sbs_charger.h

teburd

Needs compliance fixes but I like it

tests/drivers/charger/sbs_charger/testcase.yaml

teburd · 2023-04-27T17:50:59Z

I mean to add... just like the fuel gauge API... this could use with a notification mechanism for status changes. I believe many of the fuel gauge and charger ICs have a gpio line that can be hooked on to for catching interrupts to trigger event handling for things like charger status changes, fuel gauge level sets and such.

That can be done in subsequent work but it would be great to see a plan for it done soon for both of these APIs.

carlescufi · 2023-08-22T16:19:51Z

Many of the initial discussed changes have been dropped (why?)

You should've been at the dev-review meeting. Towards the end @aaronemassey made his case for this runtime architecture and frankly I agree. Additionally, there was a Nordic Semi representative that was asked about his thoughts on the runtime properties and he did not speak up against it.

Perhaps I wasn't clear enough when I spoke during the meeting @rriveramcrus, my mistake. I have not followed the development of this PR nor I know charging in general, so if I gave the impression of giving my acquiescence then it was the wrong impression. I just don't have an opinion one way or the other due to lack of knowledge in the area.

, and I still fail to see some additional implementations (BQ24190?) that prove this API is good enough for multiple devices.
This was also addressed at the dev-review meeting. As a compromise I did add a settable property to sbs_charger.

As it is today, the API can get and it can set. If you would like to see more properties or devices added, your contributions are welcome and encouraged.

As for BQ24190, it's not happening until after this PR has closed and that is my final decision.

It is however common practice to implement a driver from at least two different vendors to ensure that we cover the hardware diversity as best as possible.
EDIT: I actually got confused, we usually require those to transition an API from experimental to stable

fabiobaltieri

Thanks for the followup, one leftover file and one comment still open about the parameter handling, repeating here: I think that comment is relevant, though it also applies to the fuel gauge APIs so I see why you did it like that, I'll work with @aaronemassey separately on having that one fixed, I don't think it should stay in the current form.

include/zephyr/drivers/charger/charger_sbs_charger.h

fabiobaltieri · 2023-08-23T09:57:22Z

include/zephyr/drivers/charger.h

+static inline int z_impl_charger_get_prop(const struct device *dev,
+					  struct charger_get_property *prop)


Hey I think this is still an open point and I agree with Gerard that this should take a (dev, prop, &val) as an argument, having the property embedding the value is very weird, looks like an ioctl or something. I see that this mimics the fuel gauge APIs though so I understnad the motivation, I'll talk with @aaronemassey about reviewing that one as well as I think it should get the same modifications.

Apologies for throwing you off track, but in the end this is a good chance to review both APIs and get them cleaned and coherent.

fabiobaltieri · 2023-08-24T09:02:56Z

Thanks @rriveramcrus, I think my last concern is with how the get/set arguments are defined, basically @gmarull comment at 1afad57#r1205212874. I had a good chat with @aaronemassey since that's in the fuel gauge api as well and if anything the two should be aligned.

rriveramcrus · 2023-08-24T18:48:32Z

I had a good chat with @aaronemassey since that's in the fuel gauge api as well and if anything the two should be aligned.

Likewise chatted with Aaron yesterday. The recommended changes work for me, ack.

fabiobaltieri

Thanks, few more comments but I like how the get/set are organized now, @gmarull can you take a look at how the subsystem API is defined now?

drivers/charger/charger_handlers.c

drivers/charger/sbs_charger.c

Add initial charger driver API with the most basic of native_posix driver tests. Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>

Adds a sample sbs charger driver and basics tests. Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>

Adds a short stub doc as a placeholder for future documentation in the charger API. Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>

rriveramcrus · 2023-08-30T13:57:50Z

@gmarull bump, it's been a week since the get/set changes were pushed up.

I have lost track of this extremely lengthy discussion going back and forward between suggested solutions.

main concerns have been addressed

github-actions

Hi @rriveramcrus!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

rriveramcrus requested review from nashif, carlescufi, galak and MaureenHelm as code owners April 7, 2023 20:48

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Documentation labels Apr 7, 2023

zephyrbot requested a review from kartben April 7, 2023 20:48

This was referenced Apr 7, 2023

Introduce a Battery Charging API #56635

Closed

Creates Charging Subsystem CirrusLogic/zephyr-drivers#2

Closed

aaronemassey self-requested a review April 10, 2023 17:03

aaronemassey self-assigned this Apr 10, 2023

aaronemassey requested changes Apr 12, 2023

View reviewed changes

rriveramcrus force-pushed the charger branch from 811938c to b87e264 Compare April 17, 2023 20:26

rriveramcrus commented Apr 17, 2023

View reviewed changes

tests/drivers/charger/sbs_charger/src/test_sbs_charger.c Outdated Show resolved Hide resolved

rriveramcrus commented Apr 17, 2023

View reviewed changes

tests/drivers/charger/sbs_charger/src/test_sbs_charger.c Outdated Show resolved Hide resolved

rriveramcrus force-pushed the charger branch from b87e264 to db3f587 Compare April 17, 2023 21:37

aasinclair mentioned this pull request Apr 18, 2023

Npm1300 initial charger driver #56929

Merged

gmarull requested changes Apr 18, 2023

View reviewed changes

aasinclair reviewed Apr 18, 2023

View reviewed changes

include/zephyr/drivers/charger.h Outdated Show resolved Hide resolved

include/zephyr/drivers/charger.h Outdated Show resolved Hide resolved

rriveramcrus commented Apr 18, 2023

View reviewed changes

drivers/charger/sbs_charger/sbs_charger.c Outdated Show resolved Hide resolved

rriveramcrus force-pushed the charger branch 4 times, most recently from 7ea3e77 to 1484160 Compare April 20, 2023 18:01

rriveramcrus commented Apr 20, 2023

View reviewed changes

tests/drivers/charger/sbs_charger/testcase.yaml Show resolved Hide resolved

rriveramcrus requested a review from gmarull April 20, 2023 20:40

dnojiri reviewed Apr 21, 2023

View reviewed changes

drivers/charger/sbs_charger/sbs_charger.h Outdated Show resolved Hide resolved

rriveramcrus force-pushed the charger branch from 1484160 to 960dca5 Compare April 24, 2023 20:17

teburd previously approved these changes Apr 27, 2023

View reviewed changes

tests/drivers/charger/sbs_charger/testcase.yaml Show resolved Hide resolved

rriveramcrus force-pushed the charger branch from c27e6cc to 0069497 Compare August 22, 2023 20:32

rriveramcrus requested review from aaronemassey and fabiobaltieri August 22, 2023 21:09

fabiobaltieri reviewed Aug 23, 2023

View reviewed changes

rriveramcrus force-pushed the charger branch from 0069497 to 008fe5a Compare August 23, 2023 18:07

aaronemassey mentioned this pull request Aug 23, 2023

Fuel Gauge API Improvements #61818

Open

8 tasks

rriveramcrus force-pushed the charger branch from 008fe5a to 4c4d219 Compare August 24, 2023 18:46

rriveramcrus requested a review from fabiobaltieri August 24, 2023 20:00

rriveramcrus force-pushed the charger branch from 4c4d219 to c6d7e64 Compare August 24, 2023 21:20

fabiobaltieri reviewed Aug 25, 2023

View reviewed changes

drivers/charger/charger_handlers.c Outdated Show resolved Hide resolved

drivers/charger/charger_handlers.c Outdated Show resolved Hide resolved

drivers/charger/charger_handlers.c Outdated Show resolved Hide resolved

drivers/charger/sbs_charger.c Show resolved Hide resolved

rriveramcrus added 3 commits August 25, 2023 14:43

charger: Initial charger dedicated API

1b5aa11

Add initial charger driver API with the most basic of native_posix driver tests. Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>

charger: Sample sbs charger driver with tests

a9c3cde

Adds a sample sbs charger driver and basics tests. Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>

charger: Adds basic stub API doc

07ace86

Adds a short stub doc as a placeholder for future documentation in the charger API. Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>

rriveramcrus force-pushed the charger branch from c6d7e64 to 07ace86 Compare August 25, 2023 19:43

rriveramcrus requested a review from gmarull August 28, 2023 15:29

rriveramcrus requested a review from fabiobaltieri September 6, 2023 14:09

aasinclair approved these changes Sep 7, 2023

View reviewed changes

fabiobaltieri approved these changes Sep 7, 2023

View reviewed changes

teburd approved these changes Sep 7, 2023

View reviewed changes

aaronemassey approved these changes Sep 7, 2023

View reviewed changes

nashif approved these changes Sep 7, 2023

View reviewed changes

nashif merged commit a627035 into zephyrproject-rtos:main Sep 7, 2023
18 checks passed

github-actions bot reviewed Sep 7, 2023

View reviewed changes

rriveramcrus deleted the charger branch September 7, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Charging Subsystem #56666

Introduce Charging Subsystem #56666

rriveramcrus commented Apr 7, 2023 •

edited

gmarull Apr 18, 2023

rriveramcrus Apr 18, 2023

rriveramcrus Apr 20, 2023

teburd left a comment

teburd commented Apr 27, 2023 •

edited

carlescufi commented Aug 22, 2023 •

edited

fabiobaltieri left a comment

fabiobaltieri Aug 23, 2023 •

edited

fabiobaltieri commented Aug 24, 2023

rriveramcrus commented Aug 24, 2023 •

edited

fabiobaltieri left a comment

rriveramcrus commented Aug 30, 2023

github-actions bot left a comment

		@@ -0,0 +1,8 @@
		# SPDX-License-Identifier: Apache-2.0

		cmake_minimum_required(VERSION 3.20.0)

		static inline int z_impl_charger_get_prop(const struct device *dev,
		struct charger_get_property *prop)

Introduce Charging Subsystem #56666

Introduce Charging Subsystem #56666

Conversation

rriveramcrus commented Apr 7, 2023 • edited

gmarull Apr 18, 2023

Choose a reason for hiding this comment

rriveramcrus Apr 18, 2023

Choose a reason for hiding this comment

rriveramcrus Apr 20, 2023

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

teburd commented Apr 27, 2023 • edited

carlescufi commented Aug 22, 2023 • edited

fabiobaltieri left a comment

Choose a reason for hiding this comment

fabiobaltieri Aug 23, 2023 • edited

Choose a reason for hiding this comment

fabiobaltieri commented Aug 24, 2023

rriveramcrus commented Aug 24, 2023 • edited

fabiobaltieri left a comment

Choose a reason for hiding this comment

rriveramcrus commented Aug 30, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

rriveramcrus commented Apr 7, 2023 •

edited

teburd commented Apr 27, 2023 •

edited

carlescufi commented Aug 22, 2023 •

edited

fabiobaltieri Aug 23, 2023 •

edited

rriveramcrus commented Aug 24, 2023 •

edited