Re: [PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module
Hi,
On 4/5/21 11:32 PM, Sebastian Reichel wrote:
[...]
+static void spwr_battery_unregister(struct spwr_battery_device *bat)
+{
+ ssam_notifier_unregister(bat->sdev->ctrl, >notif);
+ cancel_delayed_work_sync(>update_work);
+ device_remove_file(>psy->dev, _attr);
+ power_supply_unregister(bat->psy);
power_supply_unregister being the last function call is a clear
sign, that devm_power_supply_register can be used instead.
Right, that works here. I normally try to not mix devm code with
non-devm code (apart from maybe allocations).
well allocations are usually done first and free'd last making
them the first targets in the conversion and pretty much a no
brainer.
Next merge window it's possible to easily go to full devm by
using devm_delayed_work_autocancel(), which has been merged
by Greg two weeks ago. Then last but not least do the ssam
notifier unregister via devm_add_action_or_reset and its fully
converted :)
Neat, I'll have a look at maybe adding some devm versions for the
SSAM notifiers. Should help in at least one other driver apart from
these two.
Thanks,
Max
Re: [PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module
Hi,
On Mon, Apr 05, 2021 at 09:07:55PM +0200, Maximilian Luz wrote:
> [...]
> > > +static int spwr_battery_recheck_adapter(struct spwr_battery_device *bat)
> > > +{
> > > + /*
> > > + * Handle battery update quirk: When the battery is fully charged (or
> > > + * charged up to the limit imposed by the UEFI battery limit) and the
> > > + * adapter is plugged in or removed, the EC does not send a separate
> > > + * event for the state (charging/discharging) change. Furthermore it
> > > + * may take some time until the state is updated on the battery.
> > > + * Schedule an update to solve this.
> > > + */
> >
> > As long as the adapter plug event is being sent you can just add
> > .external_power_changed() hook in this driver and update the battery
> > status there instead of using this hack :)
> >
> > This requires populating .supplied_to in the charger driver, so that
> > it will notify the battery device when power_supply_changed() is called
> > for the charger. I will point this out when reviewing PATCH 2.
>
> I'll switch this to the .external_power_changed() callback, thanks for
> pointing that out.
>
> I still need the delay though. The event for the charger is the same
> event that we rely on here, so the charging/not-charging flag in the BST
> data may still not be updated yet. So unfortunately still a bit of a
> hack required.
Ah, too bad.
> > > + schedule_delayed_work(>update_work, SPWR_AC_BAT_UPDATE_DELAY);
> > > + return 0;
> > > +}
> [...]
> > > +static void spwr_battery_update_bst_workfn(struct work_struct *work)
> > > +{
> > > + struct delayed_work *dwork = to_delayed_work(work);
> > > + struct spwr_battery_device *bat;
> > > + int status;
> > > +
> > > + bat = container_of(dwork, struct spwr_battery_device, update_work);
> > > +
> > > + status = spwr_battery_update_bst(bat, false);
> > > + if (!status)
> > > + power_supply_changed(bat->psy);
> >
> > power_supply_changed should only be changed for 'important' changes
> > (e.g. charging status changes, temperature or capacity threshold reached),
> > not every 5 seconds.
>
> This work struct will only be scheduled when we receive an adapter event
> and is required to handle the quirk above, so this should be an
> important change (state changing from charging to not-charging or back)
> and shouldn't repeat too often, or rather only when the user
> plugs/unplugs the charger.
Ack.
> [...]
> > > +/* -- Alarm attribute.
> > > -- */
> > > +
> > > +static ssize_t spwr_battery_alarm_show(struct device *dev, struct
> > > device_attribute *attr, char *buf)
> > > +{
> > > + struct power_supply *psy = dev_get_drvdata(dev);
> > > + struct spwr_battery_device *bat = power_supply_get_drvdata(psy);
> > > + int status;
> > > +
> > > + mutex_lock(>lock);
> > > + status = sysfs_emit(buf, "%d\n", bat->alarm * 1000);
> > > + mutex_unlock(>lock);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static ssize_t spwr_battery_alarm_store(struct device *dev, struct
> > > device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct power_supply *psy = dev_get_drvdata(dev);
> > > + struct spwr_battery_device *bat = power_supply_get_drvdata(psy);
> > > + unsigned long value;
> > > + int status;
> > > +
> > > + status = kstrtoul(buf, 0, );
> > > + if (status)
> > > + return status;
> > > +
> > > + mutex_lock(>lock);
> > > +
> > > + if (!spwr_battery_present(bat)) {
> > > + mutex_unlock(>lock);
> > > + return -ENODEV;
> > > + }
> > > +
> > > + status = spwr_battery_set_alarm_unlocked(bat, value / 1000);
> > > + if (status) {
> > > + mutex_unlock(>lock);
> > > + return status;
> > > + }
> > > +
> > > + mutex_unlock(>lock);
> > > + return count;
> > > +}
> > > +
> > > +static const struct device_attribute alarm_attr = {
> > > + .attr = {.name = "alarm", .mode = 0644},
> > > + .show = spwr_battery_alarm_show,
> > > + .store = spwr_battery_alarm_store,
> > > +};
> >
> > DEVICE_ATTR_RW()
> >
> > custom property needs to be documented in
> >
> > Documentation/ABI/testing/sysfs-class-power-surface
> >
> > Also I'm not sure what is being stored here, but it looks like you
> > can just use POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN?
>
> This (and other handling of the alarm value) has essentially been copied
> from drivers/acpi/battery.c and corresponds to ACPI _BTP/battery trip
> point (the whole interface of this EC is essentially modeled after the
> ACPI spec).
>
> The alarm value isn't strictly required to be a lower threshold, but is
> (according to ACPI spec) a trip point that causes an event to be sent
> when it is crossed in either direction. So I don't think we can directly
> map this to POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN as that seems to imply
> a lower threshold only.
>
> I'll add documentation for this if that's allright.
Ack.
> [...]
> > > +static void spwr_battery_unregister(struct
Re: [PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module
Hi,
On 4/5/21 5:37 PM, Sebastian Reichel wrote:
Hi,
On Tue, Mar 09, 2021 at 01:05:29AM +0100, Maximilian Luz wrote:
On newer Microsoft Surface models (specifically 7th-generation, i.e.
Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go),
battery and AC status/information is no longer handled via standard ACPI
devices, but instead directly via the Surface System Aggregator Module
(SSAM), i.e. the embedded controller on those devices.
While on previous generation models, battery status is also handled via
SSAM, an ACPI shim was present to translate the standard ACPI battery
interface to SSAM requests. The SSAM interface itself, which is modeled
closely after the ACPI interface, has not changed.
This commit introduces a new SSAM client device driver to support
battery status/information via the aforementioned interface on said
Surface models. It is in parts based on the standard ACPI battery
driver.
Signed-off-by: Maximilian Luz
---
Note: This patch depends on the
platform/surface: Add Surface Aggregator device registry
series. More specifically patch
platform/surface: Set up Surface Aggregator device registry
The full series has been merged into the for-next branch of the
platform-drivers-x86 tree [1]. The commit in question can be found at
[2].
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde
---
MAINTAINERS| 7 +
drivers/power/supply/Kconfig | 16 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/surface_battery.c | 901 +
4 files changed, 925 insertions(+)
create mode 100644 drivers/power/supply/surface_battery.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d756b9ec236d..f44521abe8bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11861,6 +11861,13 @@ F: drivers/scsi/smartpqi/smartpqi*.[ch]
F:include/linux/cciss*.h
F:include/uapi/linux/cciss*.h
+MICROSOFT SURFACE BATTERY AND AC DRIVERS
+M: Maximilian Luz
+L: linux...@vger.kernel.org
+L: platform-driver-...@vger.kernel.org
+S: Maintained
+F: drivers/power/supply/surface_battery.c
+
MICROSOFT SURFACE GPE LID SUPPORT DRIVER
M:Maximilian Luz
L:platform-driver-...@vger.kernel.org
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 006b95eca673..cebeff10d543 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -801,4 +801,20 @@ config BATTERY_ACER_A500
help
Say Y to include support for Acer Iconia Tab A500 battery fuel gauge.
+config BATTERY_SURFACE
+ tristate "Battery driver for 7th-generation Microsoft Surface devices"
+ depends on SURFACE_AGGREGATOR_REGISTRY
+ help
+ Driver for battery devices connected via/managed by the Surface System
+ Aggregator Module (SSAM).
+
+ This driver provides battery-information and -status support for
+ Surface devices where said data is not exposed via the standard ACPI
+ devices. On those models (7th-generation), battery-information is
+ instead handled directly via SSAM client devices and this driver.
+
+ Say M or Y here to include battery status support for 7th-generation
+ Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3,
+ Surface Book 3, and Surface Laptop Go.
+
endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 5e5fdbbef531..134041538d2c 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -101,3 +101,4 @@ obj-$(CONFIG_CHARGER_BD99954) += bd99954-charger.o
obj-$(CONFIG_CHARGER_WILCO) += wilco-charger.o
obj-$(CONFIG_RN5T618_POWER) += rn5t618_power.o
obj-$(CONFIG_BATTERY_ACER_A500) += acer_a500_battery.o
+obj-$(CONFIG_BATTERY_SURFACE) += surface_battery.o
diff --git a/drivers/power/supply/surface_battery.c
b/drivers/power/supply/surface_battery.c
new file mode 100644
index ..b93a4f556b5c
--- /dev/null
+++ b/drivers/power/supply/surface_battery.c
@@ -0,0 +1,901 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Battery driver for 7th-generation Microsoft Surface devices via Surface
+ * System Aggregator Module (SSAM).
+ *
+ * Copyright (C) 2019-2021 Maximilian Luz
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+
+/* -- SAM interface.
*/
+
+enum sam_event_cid_bat {
+ SAM_EVENT_CID_BAT_BIX = 0x15,
+ SAM_EVENT_CID_BAT_BST = 0x16,
+ SAM_EVENT_CID_BAT_ADP = 0x17,
+ SAM_EVENT_CID_BAT_PROT= 0x18,
+ SAM_EVENT_CID_BAT_DPTF= 0x53,
+};
+
+enum
Re: [PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module
Hi,
On Tue, Mar 09, 2021 at 01:05:29AM +0100, Maximilian Luz wrote:
> On newer Microsoft Surface models (specifically 7th-generation, i.e.
> Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go),
> battery and AC status/information is no longer handled via standard ACPI
> devices, but instead directly via the Surface System Aggregator Module
> (SSAM), i.e. the embedded controller on those devices.
>
> While on previous generation models, battery status is also handled via
> SSAM, an ACPI shim was present to translate the standard ACPI battery
> interface to SSAM requests. The SSAM interface itself, which is modeled
> closely after the ACPI interface, has not changed.
>
> This commit introduces a new SSAM client device driver to support
> battery status/information via the aforementioned interface on said
> Surface models. It is in parts based on the standard ACPI battery
> driver.
>
> Signed-off-by: Maximilian Luz
> ---
>
> Note: This patch depends on the
>
> platform/surface: Add Surface Aggregator device registry
>
> series. More specifically patch
>
> platform/surface: Set up Surface Aggregator device registry
>
> The full series has been merged into the for-next branch of the
> platform-drivers-x86 tree [1]. The commit in question can be found at
> [2].
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde
>
> ---
> MAINTAINERS| 7 +
> drivers/power/supply/Kconfig | 16 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/surface_battery.c | 901 +
> 4 files changed, 925 insertions(+)
> create mode 100644 drivers/power/supply/surface_battery.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d756b9ec236d..f44521abe8bf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11861,6 +11861,13 @@ F: drivers/scsi/smartpqi/smartpqi*.[ch]
> F: include/linux/cciss*.h
> F: include/uapi/linux/cciss*.h
>
> +MICROSOFT SURFACE BATTERY AND AC DRIVERS
> +M: Maximilian Luz
> +L: linux...@vger.kernel.org
> +L: platform-driver-...@vger.kernel.org
> +S: Maintained
> +F: drivers/power/supply/surface_battery.c
> +
> MICROSOFT SURFACE GPE LID SUPPORT DRIVER
> M: Maximilian Luz
> L: platform-driver-...@vger.kernel.org
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 006b95eca673..cebeff10d543 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -801,4 +801,20 @@ config BATTERY_ACER_A500
> help
> Say Y to include support for Acer Iconia Tab A500 battery fuel gauge.
>
> +config BATTERY_SURFACE
> + tristate "Battery driver for 7th-generation Microsoft Surface devices"
> + depends on SURFACE_AGGREGATOR_REGISTRY
> + help
> + Driver for battery devices connected via/managed by the Surface System
> + Aggregator Module (SSAM).
> +
> + This driver provides battery-information and -status support for
> + Surface devices where said data is not exposed via the standard ACPI
> + devices. On those models (7th-generation), battery-information is
> + instead handled directly via SSAM client devices and this driver.
> +
> + Say M or Y here to include battery status support for 7th-generation
> + Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3,
> + Surface Book 3, and Surface Laptop Go.
> +
> endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 5e5fdbbef531..134041538d2c 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -101,3 +101,4 @@ obj-$(CONFIG_CHARGER_BD99954) += bd99954-charger.o
> obj-$(CONFIG_CHARGER_WILCO) += wilco-charger.o
> obj-$(CONFIG_RN5T618_POWER) += rn5t618_power.o
> obj-$(CONFIG_BATTERY_ACER_A500) += acer_a500_battery.o
> +obj-$(CONFIG_BATTERY_SURFACE)+= surface_battery.o
> diff --git a/drivers/power/supply/surface_battery.c
> b/drivers/power/supply/surface_battery.c
> new file mode 100644
> index ..b93a4f556b5c
> --- /dev/null
> +++ b/drivers/power/supply/surface_battery.c
> @@ -0,0 +1,901 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Battery driver for 7th-generation Microsoft Surface devices via Surface
> + * System Aggregator Module (SSAM).
> + *
> + * Copyright (C) 2019-2021 Maximilian Luz
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +
> +/* -- SAM interface.
> */
> +
> +enum sam_event_cid_bat {
> + SAM_EVENT_CID_BAT_BIX = 0x15,
> + SAM_EVENT_CID_BAT_BST = 0x16,
> +
