Messages in this thread | | | Date | Mon, 5 Apr 2021 11:34:02 +0200 | From | Sebastian Reichel <> | Subject | Re: [PATCH v4 1/2] power: supply: PCHG: Peripheral device charger |
| |
Hi,
On Sat, Jan 23, 2021 at 10:43:23AM -0800, Gwendal Grignou wrote: > On Sat, Jan 23, 2021 at 7:53 AM Daisuke Nojiri <dnojiri@chromium.org> wrote: > > > > This patch adds a driver for PCHG (Peripheral CHarGer). PCHG is a > > framework managing power supplies for peripheral devices. > > > > This driver creates a sysfs node for each peripheral charge port: > > > > /sys/class/power_supply/PCHGn > > > > where <n> is the index of a charge port. > > > > For example, when a stylus is connected to a NFC/WLC port, the node > > prints: > > > > /sys/class/power_supply/PCHG0/ > > capacity=50 > > scope=Device > > status=Charging > > type=Battery > > > > Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org> > > --- > > v1 -> v2 > > * Separate mfd/cros_ec_dev.c > > * Make CONFIG_CHARGER_CROS_PCHG default to CONFIG_MFD_CROS_EC_DEV > > v2 -> v3 > > * Return POWER_SUPPLY_SCOPE_DEVICE for POWER_SUPPLY_PROP_SCOPE > > * POWER_SUPPLY_TYPE_WIRELESS -> POWER_SUPPLY_TYPE_BATTERY > > v3 -> v4 > > * Return NOTIFY_DONE when a non-host event is notified. > > --- > > drivers/power/supply/Kconfig | 10 + > > drivers/power/supply/Makefile | 1 + > > .../power/supply/cros_peripheral_charger.c | 350 ++++++++++++++++++ > > .../linux/platform_data/cros_ec_commands.h | 48 +++ > > 4 files changed, 409 insertions(+) > > create mode 100644 drivers/power/supply/cros_peripheral_charger.c > > > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > > index eec646c568b7be..407f9fbbc2bb50 100644 > > --- a/drivers/power/supply/Kconfig > > +++ b/drivers/power/supply/Kconfig > > @@ -714,6 +714,16 @@ config CHARGER_CROS_USBPD > > what is connected to USB PD ports from the EC and converts > > that into power_supply properties. > > > > +config CHARGER_CROS_PCHG > > + tristate "ChromeOS EC based peripheral charger" > > + depends on MFD_CROS_EC_DEV > > + default MFD_CROS_EC_DEV > > + help > > + Say Y here to enable ChromeOS EC based peripheral charge driver. > > + This driver gets various information about the devices connected to > > + the peripheral charge ports from the EC and converts that into > > + power_supply properties. > > + > > config CHARGER_SC2731 > > tristate "Spreadtrum SC2731 charger driver" > > depends on MFD_SC27XX_PMIC || COMPILE_TEST > > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > > index dd4b86318cd9bd..5263472a64809b 100644 > > --- a/drivers/power/supply/Makefile > > +++ b/drivers/power/supply/Makefile > > @@ -91,6 +91,7 @@ obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o > > obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o > > obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o > > obj-$(CONFIG_CHARGER_CROS_USBPD) += cros_usbpd-charger.o > > +obj-$(CONFIG_CHARGER_CROS_PCHG) += cros_peripheral_charger.o > > obj-$(CONFIG_CHARGER_SC2731) += sc2731_charger.o > > obj-$(CONFIG_FUEL_GAUGE_SC27XX) += sc27xx_fuel_gauge.o > > obj-$(CONFIG_CHARGER_UCS1002) += ucs1002_power.o > > diff --git a/drivers/power/supply/cros_peripheral_charger.c b/drivers/power/supply/cros_peripheral_charger.c > > new file mode 100644 > > index 00000000000000..cd7db4bd5f90d0 > > --- /dev/null > > +++ b/drivers/power/supply/cros_peripheral_charger.c > > @@ -0,0 +1,350 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Power supply driver for ChromeOS EC based Peripheral Device Charger. > > + * > > + * Copyright 2020 Google LLC. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/notifier.h> > > +#include <linux/platform_data/cros_ec_commands.h> > > +#include <linux/platform_data/cros_ec_proto.h> > Add > > > +#include <linux/platform_device.h> > > +#include <linux/power_supply.h> > > +#include <linux/slab.h> > > +#include <linux/stringify.h> > > +#include <linux/types.h> > > + > > +#define DRV_NAME "cros-ec-pchg" > > +#define PCHG_DIR_NAME "PCHG%d" > nit: to be coherent with function names, define CROS_EC_PCHG_DIR_NAME instead. > Isn't PCHG0, PCHG1, ... too generic? Looking at other driver, we have > strings like > "max1721x-%012X", "wm831x-battery.%d",
Better name gives users better chance to understand what this is. I think "peripheral%d" is already better than "PCHG%d", which is quite cryptic. Does EC know the device type (e.g. stylus)?
> > +#define PCHG_DIR_NAME_LENGTH sizeof("PCHG" __stringify(EC_PCHG_MAX_PORTS)) > > +#define PCHG_CACHE_UPDATE_DELAY msecs_to_jiffies(500) > > + > > +struct port_data { > > + int port_number; > > + char name[PCHG_DIR_NAME_LENGTH]; > > + struct power_supply *psy; > > + struct power_supply_desc psy_desc; > > + int psy_status; > > + int battery_percentage; > > + struct charger_data *charger; > > + unsigned long last_update; > > +}; > > + > > +struct charger_data { > > + struct device *dev; > > + struct cros_ec_dev *ec_dev; > > + struct cros_ec_device *ec_device; > As far as I can see, ec_device is used only once, in cros_pchg_ec_command(). > > + int num_registered_psy; > > + struct port_data *ports[EC_PCHG_MAX_PORTS]; > > + struct notifier_block notifier; > > +}; > > + > > +static enum power_supply_property cros_pchg_props[] = { > > + POWER_SUPPLY_PROP_STATUS, > > + POWER_SUPPLY_PROP_CAPACITY, > > + POWER_SUPPLY_PROP_SCOPE, > > + /* > > + * todo: Add the following. > > + * > > + * POWER_SUPPLY_PROP_TECHNOLOGY, > > + * POWER_SUPPLY_PROP_ERROR, > > + * POWER_SUPPLY_PROP_SERIAL_NUMBER, > > + * > > + * POWER_SUPPLY_PROP_ONLINE can't be used because it indicates the > > + * system is powered by AC. > > + */ > > +}; > > + > > +static int cros_pchg_ec_command(const struct charger_data *charger, > > + unsigned int version, > > + unsigned int command, > > + const void *outdata, > > + unsigned int outsize, > > + void *indata, > > + unsigned int insize) > > +{ > > + struct cros_ec_dev *ec_dev = charger->ec_dev; > > + struct cros_ec_command *msg; > > + int ret; > > + > > + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL); > > + if (!msg) > > + return -ENOMEM; > > + > > + msg->version = version; > > + msg->command = ec_dev->cmd_offset + command; > > + msg->outsize = outsize; > > + msg->insize = insize; > > + > > + if (outsize) > > + memcpy(msg->data, outdata, outsize); > > + > > + ret = cros_ec_cmd_xfer_status(charger->ec_device, msg); > > + if (ret >= 0 && insize) > > + memcpy(indata, msg->data, insize); > > + > > + kfree(msg); > > + return ret; > > +} > > + > > +static int cros_pchg_port_count(const struct charger_data *charger) > > +{ > > + struct ec_response_pchg_count rsp; > > + int ret; > > + > > + ret = cros_pchg_ec_command(charger, 0, EC_CMD_PCHG_COUNT, > > + NULL, 0, &rsp, sizeof(rsp)); > > + if (ret < 0) { > > + dev_warn(charger->dev, > > + "Unable to get number or ports (err:%d)\n", ret); > > + return ret; > > + } > > + > > + return rsp.port_count; > > +} > > + > > +static int cros_pchg_get_status(struct port_data *port) > > +{ > > + struct charger_data *charger = port->charger; > > + struct ec_params_pchg req; > > + struct ec_response_pchg rsp; > > + struct device *dev = charger->dev; > > + int ret; > > + > > + req.port = port->port_number; > > + ret = cros_pchg_ec_command(charger, 0, EC_CMD_PCHG, > > + &req, sizeof(req), &rsp, sizeof(rsp)); > > + if (ret < 0) { > > + dev_err(dev, "Unable to get port.%d status (err:%d)\n", > > + port->port_number, ret); > > + return ret; > > + } > > + > > + switch (rsp.state) { > > + case PCHG_STATE_RESET: > > + case PCHG_STATE_INITIALIZED: > > + case PCHG_STATE_ENABLED: > > + default: > > + port->psy_status = POWER_SUPPLY_STATUS_UNKNOWN; > > + port->battery_percentage = 0; > > + break; > > + case PCHG_STATE_DETECTED: > > + port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING; > > + port->battery_percentage = rsp.battery_percentage; > > + break; > > + case PCHG_STATE_CHARGING: > > + port->psy_status = POWER_SUPPLY_STATUS_CHARGING; > > + port->battery_percentage = rsp.battery_percentage; > > + break; > > + } > > + > > + dev_dbg(dev, > > + "Port %d: state=%d battery=%d%%\n", > > + port->port_number, rsp.state, rsp.battery_percentage); > > + > > + return 0; > Looking at other drivers (drivers/power/supply/ds2760_battery.c for > instance), power_supply_changed() is called when status changes. > Should we?
power_supply_changed() triggers a couple of things:
1. it takes care of updating the any LED triggers 2. it notifies userspace via uevent 3. it notifies supplied devices
I suppose in this case at least the second point is interesting. It's generally a good idea to call it when charging status changes or battery level gets critical.
> > +} > > + > > +static int cros_pchg_get_port_status(struct port_data *port, bool ratelimit) > > +{ > > + int ret; > > + > > + if (ratelimit && > > + time_is_after_jiffies(port->last_update + PCHG_CACHE_UPDATE_DELAY)) > > + return 0; > > + > > + ret = cros_pchg_get_status(port); > > + if (ret < 0) > > + return ret; > > + > > + port->last_update = jiffies; > > + > > + return ret; > > +} > > + > > +static int cros_pchg_get_prop(struct power_supply *psy, > > + enum power_supply_property psp, > > + union power_supply_propval *val) > > +{ > > + struct port_data *port = power_supply_get_drvdata(psy); > > + > > + switch (psp) { > > + case POWER_SUPPLY_PROP_STATUS: > > + case POWER_SUPPLY_PROP_CAPACITY: > > + cros_pchg_get_port_status(port, true); > > + break; > > + default: > > + break; > > + } > > + > > + switch (psp) { > > + case POWER_SUPPLY_PROP_STATUS: > > + val->intval = port->psy_status; > > + break; > > + case POWER_SUPPLY_PROP_CAPACITY: > > + val->intval = port->battery_percentage; > > + break; > > + case POWER_SUPPLY_PROP_SCOPE: > > + val->intval = POWER_SUPPLY_SCOPE_DEVICE; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static int cros_pchg_event(const struct charger_data *charger, > > + unsigned long host_event) > > +{ > > + int i; > > + > > + for (i = 0; i < charger->num_registered_psy; i++) > > + cros_pchg_get_port_status(charger->ports[i], false); > > + > > + return NOTIFY_OK; > > +} > > + > > +static u32 cros_get_device_event(const struct charger_data *charger) > > +{ > > + struct ec_params_device_event req; > > + struct ec_response_device_event rsp; > > + struct device *dev = charger->dev; > > + int ret; > > + > > + req.param = EC_DEVICE_EVENT_PARAM_GET_CURRENT_EVENTS; > > + ret = cros_pchg_ec_command(charger, 0, EC_CMD_DEVICE_EVENT, > > + &req, sizeof(req), &rsp, sizeof(rsp)); > > + if (ret < 0) { > > + dev_warn(dev, "Unable to get device events (err:%d)\n", ret); > > + return 0; > > + } > > + > > + return rsp.event_mask; > > +} > > + > > +static int cros_ec_notify(struct notifier_block *nb, > cros_pchg_notify instead to preserve naming convention. > > + unsigned long queued_during_suspend, > > + void *data) > > +{ > > + struct cros_ec_device *ec_dev = (struct cros_ec_device *)data; > > + u32 host_event = cros_ec_get_host_event(ec_dev); > > + struct charger_data *charger = > > + container_of(nb, struct charger_data, notifier); > > + u32 device_event_mask; > > + > > + if (!host_event) > > + return NOTIFY_DONE; > > + > > + if (!(host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_DEVICE))) > > + return NOTIFY_DONE; > > + > > + /* > > + * todo: Retrieve device event mask in common place > > + * (e.g. cros_ec_proto.c). > > + */ > > + device_event_mask = cros_get_device_event(charger); > > + if (!(device_event_mask & EC_DEVICE_EVENT_MASK(EC_DEVICE_EVENT_WLC))) > > + return NOTIFY_DONE; > > + > > + return cros_pchg_event(charger, host_event); > > +} > > + > > +static int cros_pchg_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent); > > + struct cros_ec_device *ec_device = ec_dev->ec_dev; > > + struct power_supply_desc *psy_desc; > psy_desc is a local variable of the for loop line 297. > > + struct charger_data *charger; > > + struct power_supply *psy; > > + struct port_data *port; > port is a local variable of the for loop line 297. > > + struct notifier_block *nb; > > + int num_ports; > > + int ret; > > + int i; > > + > > + charger = devm_kzalloc(dev, sizeof(*charger), GFP_KERNEL); > > + if (!charger) > > + return -ENOMEM; > > + > > + charger->dev = dev; > Usually, we call platform_set_drvdata(pdev, charger) to tie the > private data and the platform data together.
You need platform_set_drvdata(pdev, charger) to get charger from pdev and charger->dev to get device from charger. This look ok to me.
> > + charger->ec_dev = ec_dev; > > + charger->ec_device = ec_device; > > + > > + ret = cros_pchg_port_count(charger); > > + if (ret <= 0) { > > + /* > > + * This feature is enabled by the EC and the kernel driver is > > + * included by default for CrOS devices. Don't need to be loud > > + * since this error can be normal. > > + */ > There is the same problem with iio sensors: we export > "cros_ec_get_sensor_count()" and use it in ec_device_probe(). To be > coherent, we can either export cros_pchg_port_count or call the sensor > stack even when there are no sensors reported. I prefer the former, > more generic. > > + dev_info(dev, "No peripheral charge ports (err:%d)\n", ret); > > + return -ENODEV; > > + } > > + > > + num_ports = ret; > > + if (num_ports > EC_PCHG_MAX_PORTS) { > > + dev_err(dev, "Too many peripheral charge ports (%d)\n", > > + num_ports); > > + return -ENOBUFS; > > + } > > + > > + dev_info(dev, "%d peripheral charge ports found\n", num_ports); > > + > > + for (i = 0; i < num_ports; i++) { > > + struct power_supply_config psy_cfg = {}; > > + > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); > > + if (!port) > > + return -ENOMEM; > nit: shall we fail probing the whole device or just the power supply > port? Here we fail the whole device, later just the port.
For memory errors it's definitive fine to fail everything. Kernel is in huge problems anyways.
> > + port->charger = charger; > > + port->port_number = i; > > + snprintf(port->name, sizeof(port->name), PCHG_DIR_NAME, i); > > + > > + psy_desc = &port->psy_desc; > > + psy_desc->name = port->name; > > + psy_desc->type = POWER_SUPPLY_TYPE_BATTERY; > > + psy_desc->get_property = cros_pchg_get_prop; > > + psy_desc->external_power_changed = NULL; > > + psy_desc->properties = cros_pchg_props; > > + psy_desc->num_properties = ARRAY_SIZE(cros_pchg_props); > > + psy_cfg.drv_data = port; > > + > > + psy = devm_power_supply_register_no_ws(dev, psy_desc, &psy_cfg); > > + if (IS_ERR(psy)) { > > + dev_err(dev, "Failed to register power supply\n"); > > + continue; > > + }
I suppose we might also just bail out here. Most common problems for this function are:
-ENOMEM -> if kernel runs oom during boot I would not expect other supplies to be functional. -EINVAL -> some power-supply properties are invalid. Since you register the same ones for all supplies all of them would fail to register anyways -EPROBE_DEFER -> no relevant for this driver
So maybe just call
return dev_err_probe(dev, ERR_PTR(psy), "Failed to register power supply");
> > + port->psy = psy; > > + > > + charger->ports[charger->num_registered_psy++] = port; > > + } > > + > > + if (!charger->num_registered_psy) > > + return -ENODEV; > > + > > + nb = &charger->notifier; > > + nb->notifier_call = cros_ec_notify; > > + ret = blocking_notifier_chain_register(&ec_dev->ec_dev->event_notifier, > > + nb); > > + if (ret < 0) > > + dev_err(dev, "Failed to register notifier (err:%d)\n", ret); > > + > > + return 0; > > +} > > + > > +static struct platform_driver cros_pchg_driver = { > > + .driver = { > > + .name = DRV_NAME, > > + }, > > + .probe = cros_pchg_probe > > +}; > > + > > +module_platform_driver(cros_pchg_driver); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_DESCRIPTION("ChromeOS EC peripheral device charger"); > > +MODULE_ALIAS("platform:" DRV_NAME); > > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h > > index 5a3ccf8968968e..6a82a9ad99eadf 100644 > > --- a/include/linux/platform_data/cros_ec_commands.h > > +++ b/include/linux/platform_data/cros_ec_commands.h > > @@ -4249,6 +4249,7 @@ enum ec_device_event { > > EC_DEVICE_EVENT_TRACKPAD, > > EC_DEVICE_EVENT_DSP, > > EC_DEVICE_EVENT_WIFI, > > + EC_DEVICE_EVENT_WLC, > > }; > > > > enum ec_device_event_param { > > @@ -5480,6 +5481,53 @@ struct ec_response_rollback_info { > > /* Issue AP reset */ > > #define EC_CMD_AP_RESET 0x0125 > > > > +/** > > + * Get the number of peripheral charge ports > > + */ > > +#define EC_CMD_PCHG_COUNT 0x0134 > > + > > +#define EC_PCHG_MAX_PORTS 8 > > + > > +struct ec_response_pchg_count { > > + uint8_t port_count; > > +} __ec_align1; > > + > > +/** > > + * Get the status of a peripheral charge port > > + */ > > +#define EC_CMD_PCHG 0x0135 > > + > > +struct ec_params_pchg { > > + uint8_t port; > > +} __ec_align1; > > + > > +struct ec_response_pchg { > > + uint32_t error; /* enum pchg_error */ > > + uint8_t state; /* enum pchg_state state */ > > + uint8_t battery_percentage; > > +} __ec_align2; > > + > > +enum pchg_state { > > + /* Charger is reset and not initialized. */ > > + PCHG_STATE_RESET = 0, > > + /* Charger is initialized or disabled. */ > > + PCHG_STATE_INITIALIZED, > > + /* Charger is enabled and ready to detect a device. */ > > + PCHG_STATE_ENABLED, > > + /* Device is detected in proximity. */ > > + PCHG_STATE_DETECTED, > > + /* Device is being charged. */ > > + PCHG_STATE_CHARGING, > > +}; > > + > > +#define EC_PCHG_STATE_TEXT { \ > > + [PCHG_STATE_RESET] = "RESET", \ > > + [PCHG_STATE_INITIALIZED] = "INITIALIZED", \ > > + [PCHG_STATE_ENABLED] = "ENABLED", \ > > + [PCHG_STATE_DETECTED] = "DETECTED", \ > > + [PCHG_STATE_CHARGING] = "CHARGING", \ > > + } > > + > > /*****************************************************************************/ > > /* Locate peripheral chips > > * > > -- > > 2.30.0.280.ga3ce27912f-goog > >
-- Sebastian [unhandled content-type:application/pgp-signature] | |