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:
signature.asc
Description: PGP signature
