Re: [PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 6018 bytes --]

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(&bat->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(&bat->lock);
> > > +	status = sysfs_emit(buf, "%d\n", bat->alarm * 1000);
> > > +	mutex_unlock(&bat->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, &value);
> > > +	if (status)
> > > +		return status;
> > > +
> > > +	mutex_lock(&bat->lock);
> > > +
> > > +	if (!spwr_battery_present(bat)) {
> > > +		mutex_unlock(&bat->lock);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	status = spwr_battery_set_alarm_unlocked(bat, value / 1000);
> > > +	if (status) {
> > > +		mutex_unlock(&bat->lock);
> > > +		return status;
> > > +	}
> > > +
> > > +	mutex_unlock(&bat->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 spwr_battery_device *bat)
> > > +{
> > > +	ssam_notifier_unregister(bat->sdev->ctrl, &bat->notif);
> > > +	cancel_delayed_work_sync(&bat->update_work);
> > > +	device_remove_file(&bat->psy->dev, &alarm_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 :)

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]