diff mbox series

[v3] power: supply: test-power: implement charge_behaviour property

Message ID 20240306-power_supply-charge_behaviour_prop-v3-1-d04cf1f5f0af@weissschuh.net
State New
Headers show
Series [v3] power: supply: test-power: implement charge_behaviour property | expand

Commit Message

Thomas Weißschuh March 6, 2024, 7:37 p.m. UTC 
To validate the special formatting of the "charge_behaviour" sysfs
property add it to the example driver.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
The original submission of the charge_behaviour property did not
implement proper formatting in the default formatting handler in
power_supply_sysfs.c.

At that time it was not a problem because the only provider of the UAPI,
thinkpad_acpi, did its own formatting.

Now there is an in-tree driver, mm8013, and out-of-tree driver which use
the normal power-supply properties and are affected by the wrong
formatting.
In this revision the handling of CHARGE_BEHAVIOUR in mm8013 is dropped
as it is not the correct API for it to use.
That change was not tested by me as I don't have the hardware.
---
Changes in v3:
- Drop already applied patches
- Validate value in test_power_set_battery_property
- Link to v2: https://lore.kernel.org/r/20240303-power_supply-charge_behaviour_prop-v2-0-8ebb0a7c2409@weissschuh.net

Changes in v2:
- Simplify the backwards-compatibility logic (adds a preparatory patch)
- Extend test-power to also handle writing of charge_behaviour
- Drop incorrect CHARGE_BEHAVIOUR from mm8013 driver
- Replace special CHARGE_BEHAVIOUR_AVAILABLE property with bitmask in
  struct power_supply_desc
- Link to v1: https://lore.kernel.org/r/20240204-power_supply-charge_behaviour_prop-v1-0-06a20c958f96@weissschuh.net
---
 drivers/power/supply/test_power.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)


---
base-commit: 4e61f1e9d58fb0765f59f47d4d1f318b36c14d95
change-id: 20230929-power_supply-charge_behaviour_prop-10ccfd96a666

Best regards,

Comments

Sebastian Reichel March 26, 2024, 7:50 p.m. UTC | #1 
On Wed, 06 Mar 2024 20:37:04 +0100, Thomas Weißschuh wrote:
> To validate the special formatting of the "charge_behaviour" sysfs
> property add it to the example driver.
> 
> 

Applied, thanks!

[1/1] power: supply: test-power: implement charge_behaviour property
      commit: 070c1470ae24317e7b19bd3882b300b6d69922a4

Best regards,
Hans de Goede March 27, 2024, 10:36 a.m. UTC | #2 
Hi Sebastian,

On 3/26/24 8:50 PM, Sebastian Reichel wrote:
> 
> On Wed, 06 Mar 2024 20:37:04 +0100, Thomas Weißschuh wrote:
>> To validate the special formatting of the "charge_behaviour" sysfs
>> property add it to the example driver.
>>
>>
> 
> Applied, thanks!
> 
> [1/1] power: supply: test-power: implement charge_behaviour property
>       commit: 070c1470ae24317e7b19bd3882b300b6d69922a4

Does this mean that you've also applied patches 1-3 of:
"[PATCH v2 0/4] power: supply: core: align charge_behaviour format with docs" ?

Because this is a new version of 4/4 of that series and I think
that the new test may depend on the fixes from patches 1-3
of that series (which I'm reviewing now).

Regards,

Hans
Hans de Goede March 27, 2024, 10:44 a.m. UTC | #3 
Hi,

On 3/27/24 11:36 AM, Hans de Goede wrote:
> Hi Sebastian,
> 
> On 3/26/24 8:50 PM, Sebastian Reichel wrote:
>>
>> On Wed, 06 Mar 2024 20:37:04 +0100, Thomas Weißschuh wrote:
>>> To validate the special formatting of the "charge_behaviour" sysfs
>>> property add it to the example driver.
>>>
>>>
>>
>> Applied, thanks!
>>
>> [1/1] power: supply: test-power: implement charge_behaviour property
>>       commit: 070c1470ae24317e7b19bd3882b300b6d69922a4
> 
> Does this mean that you've also applied patches 1-3 of:
> "[PATCH v2 0/4] power: supply: core: align charge_behaviour format with docs" ?
> 
> Because this is a new version of 4/4 of that series and I think
> that the new test may depend on the fixes from patches 1-3
> of that series (which I'm reviewing now).

Ok, I have some not entirely trivial comments on patch 3/4 of that series.
I guess you (Sebastian) could address those while merging, or wait for
a v3 of the series.

Regards,

Hans
Sebastian Reichel March 27, 2024, 1:25 p.m. UTC | #4 
Hello Hans,

On Wed, Mar 27, 2024 at 11:44:41AM +0100, Hans de Goede wrote:
> On 3/27/24 11:36 AM, Hans de Goede wrote:
> > On 3/26/24 8:50 PM, Sebastian Reichel wrote:
> >> On Wed, 06 Mar 2024 20:37:04 +0100, Thomas Weißschuh wrote:
> >>> To validate the special formatting of the "charge_behaviour" sysfs
> >>> property add it to the example driver.
> >>
> >> Applied, thanks!
> >>
> >> [1/1] power: supply: test-power: implement charge_behaviour property
> >>       commit: 070c1470ae24317e7b19bd3882b300b6d69922a4
> > 
> > Does this mean that you've also applied patches 1-3 of:
> > "[PATCH v2 0/4] power: supply: core: align charge_behaviour format with docs" ?
> > 
> > Because this is a new version of 4/4 of that series and I think
> > that the new test may depend on the fixes from patches 1-3
> > of that series (which I'm reviewing now).
> 
> Ok, I have some not entirely trivial comments on patch 3/4 of that series.
> I guess you (Sebastian) could address those while merging, or wait for
> a v3 of the series.

I can't. Patches 1-3 are already in 6.9-rc1. It looks you did not
get my replies, but they certainly have been captured by lore and
obviously Thomas got them since he send a v3 with just the last
patch:

https://lore.kernel.org/all/20240303-power_supply-charge_behaviour_prop-v2-0-8ebb0a7c2409@weissschuh.net/

Anyways, I think your suggestions for further simplifications in
patch 3 are sensible. They just require doing an extra patch now
instead of being squashed.

Greetings,

-- Sebastian
Hans de Goede March 27, 2024, 1:34 p.m. UTC | #5 
Hi,

On 3/27/24 2:25 PM, Sebastian Reichel wrote:
> Hello Hans,
> 
> On Wed, Mar 27, 2024 at 11:44:41AM +0100, Hans de Goede wrote:
>> On 3/27/24 11:36 AM, Hans de Goede wrote:
>>> On 3/26/24 8:50 PM, Sebastian Reichel wrote:
>>>> On Wed, 06 Mar 2024 20:37:04 +0100, Thomas Weißschuh wrote:
>>>>> To validate the special formatting of the "charge_behaviour" sysfs
>>>>> property add it to the example driver.
>>>>
>>>> Applied, thanks!
>>>>
>>>> [1/1] power: supply: test-power: implement charge_behaviour property
>>>>       commit: 070c1470ae24317e7b19bd3882b300b6d69922a4
>>>
>>> Does this mean that you've also applied patches 1-3 of:
>>> "[PATCH v2 0/4] power: supply: core: align charge_behaviour format with docs" ?
>>>
>>> Because this is a new version of 4/4 of that series and I think
>>> that the new test may depend on the fixes from patches 1-3
>>> of that series (which I'm reviewing now).
>>
>> Ok, I have some not entirely trivial comments on patch 3/4 of that series.
>> I guess you (Sebastian) could address those while merging, or wait for
>> a v3 of the series.
> 
> I can't. Patches 1-3 are already in 6.9-rc1. It looks you did not
> get my replies, but they certainly have been captured by lore and
> obviously Thomas got them since he send a v3 with just the last
> patch:
> 
> https://lore.kernel.org/all/20240303-power_supply-charge_behaviour_prop-v2-0-8ebb0a7c2409@weissschuh.net/
> 
> Anyways, I think your suggestions for further simplifications in
> patch 3 are sensible. They just require doing an extra patch now
> instead of being squashed.

Ah I see that is fine too :)

Thomas, can you do a follow-up patch with the simplifications
which I suggested in my review of patch v2 3/4 ?

Regards,

Hans
Thomas Weißschuh March 27, 2024, 6:30 p.m. UTC | #6 
Hi,

On 2024-03-27 14:34:00+0100, Hans de Goede wrote:
> On 3/27/24 2:25 PM, Sebastian Reichel wrote:
> > Hello Hans,
> > 
> > On Wed, Mar 27, 2024 at 11:44:41AM +0100, Hans de Goede wrote:
> >> On 3/27/24 11:36 AM, Hans de Goede wrote:
> >>> On 3/26/24 8:50 PM, Sebastian Reichel wrote:
> >>>> On Wed, 06 Mar 2024 20:37:04 +0100, Thomas Weißschuh wrote:
> >>>>> To validate the special formatting of the "charge_behaviour" sysfs
> >>>>> property add it to the example driver.
> >>>>
> >>>> Applied, thanks!
> >>>>
> >>>> [1/1] power: supply: test-power: implement charge_behaviour property
> >>>>       commit: 070c1470ae24317e7b19bd3882b300b6d69922a4
> >>>
> >>> Does this mean that you've also applied patches 1-3 of:
> >>> "[PATCH v2 0/4] power: supply: core: align charge_behaviour format with docs" ?
> >>>
> >>> Because this is a new version of 4/4 of that series and I think
> >>> that the new test may depend on the fixes from patches 1-3
> >>> of that series (which I'm reviewing now).
> >>
> >> Ok, I have some not entirely trivial comments on patch 3/4 of that series.
> >> I guess you (Sebastian) could address those while merging, or wait for
> >> a v3 of the series.
> > 
> > I can't. Patches 1-3 are already in 6.9-rc1. It looks you did not
> > get my replies, but they certainly have been captured by lore and
> > obviously Thomas got them since he send a v3 with just the last
> > patch:
> > 
> > https://lore.kernel.org/all/20240303-power_supply-charge_behaviour_prop-v2-0-8ebb0a7c2409@weissschuh.net/
> > 
> > Anyways, I think your suggestions for further simplifications in
> > patch 3 are sensible. They just require doing an extra patch now
> > instead of being squashed.
> 
> Ah I see that is fine too :)
> 
> Thomas, can you do a follow-up patch with the simplifications
> which I suggested in my review of patch v2 3/4 ?

Will do!


Thomas
Sebastian Reichel March 27, 2024, 7:13 p.m. UTC | #7 
Hi,

On Wed, Mar 27, 2024 at 07:30:03PM +0100, Thomas Weißschuh wrote:
> On 2024-03-27 14:34:00+0100, Hans de Goede wrote:
> > Thomas, can you do a follow-up patch with the simplifications
> > which I suggested in my review of patch v2 3/4?
> 
> Will do!

Thanks for taking care of it!

-- Sebastian
diff mbox series

Patch

diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 0d0a77584c5d..442ceb7795e1 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -35,6 +35,8 @@  static int battery_capacity		= 50;
 static int battery_voltage		= 3300;
 static int battery_charge_counter	= -1000;
 static int battery_current		= -1600;
+static enum power_supply_charge_behaviour battery_charge_behaviour =
+	POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
 
 static bool module_initialized;
 
@@ -123,6 +125,9 @@  static int test_power_get_battery_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		val->intval = battery_current;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		val->intval = battery_charge_behaviour;
+		break;
 	default:
 		pr_info("%s: some properties deliberately report errors.\n",
 			__func__);
@@ -131,6 +136,31 @@  static int test_power_get_battery_property(struct power_supply *psy,
 	return 0;
 }
 
+static int test_power_battery_property_is_writeable(struct power_supply *psy,
+						    enum power_supply_property psp)
+{
+	return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR;
+}
+
+static int test_power_set_battery_property(struct power_supply *psy,
+					   enum power_supply_property psp,
+					   const union power_supply_propval *val)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		if (val->intval < 0 ||
+		    val->intval >= BITS_PER_TYPE(typeof(psy->desc->charge_behaviours)) ||
+		    !(BIT(val->intval) & psy->desc->charge_behaviours)) {
+			return -EINVAL;
+		}
+		battery_charge_behaviour = val->intval;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static enum power_supply_property test_power_ac_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 };
@@ -156,6 +186,7 @@  static enum power_supply_property test_power_battery_props[] = {
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_AVG,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
 };
 
 static char *test_power_ac_supplied_to[] = {
@@ -178,6 +209,11 @@  static const struct power_supply_desc test_power_desc[] = {
 		.properties = test_power_battery_props,
 		.num_properties = ARRAY_SIZE(test_power_battery_props),
 		.get_property = test_power_get_battery_property,
+		.set_property = test_power_set_battery_property,
+		.property_is_writeable = test_power_battery_property_is_writeable,
+		.charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
+				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
+				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
 	},
 	[TEST_USB] = {
 		.name = "test_usb",