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 <[email protected]> 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 <[email protected]> > > --- > > 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
signature.asc
Description: PGP signature
