| Message ID | 20240312-aspire1-ec-v4-3-bd8e3eea212f@trvn.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform: arm64: Acer Aspire 1 embedded controller | expand |
On 12/03/2024 08:42, Nikita Travkin wrote: > Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded > controller to perform a set of various functions, such as: > > - Battery and charger monitoring; > - Keyboard layout control (i.e. fn_lock settings); > - USB Type-C DP alt mode HPD notifications; > - Laptop lid status. > > Unfortunately, while all this functionality is implemented in ACPI, it's > currently not possible to use ACPI to boot Linux on such Qualcomm > devices. To allow Linux to still support the features provided by EC, > this driver reimplments the relevant ACPI parts. This allows us to boot > the laptop with Device Tree and retain all the features. > > Signed-off-by: Nikita Travkin <nikita@trvn.ru> > --- > drivers/platform/arm64/Kconfig | 16 + > drivers/platform/arm64/Makefile | 2 + > drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++ You should be listing yourself as a maintainer for a driver you contribute. Good work BTW. > 3 files changed, 573 insertions(+) > > diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig > index 644b83ede093..07d47879a9e3 100644 > --- a/drivers/platform/arm64/Kconfig > +++ b/drivers/platform/arm64/Kconfig > @@ -16,4 +16,20 @@ menuconfig ARM64_PLATFORM_DEVICES > > if ARM64_PLATFORM_DEVICES > > +config EC_ACER_ASPIRE1 > + tristate "Acer Aspire 1 Emedded Controller driver" > + depends on I2C > + depends on DRM > + depends on POWER_SUPPLY > + depends on INPUT > + help > + Say Y here to enable the EC driver for the (Snapdragon-based) > + Acer Aspire 1 laptop. The EC handles battery and charging > + monitoring as well as some misc functions like the lid sensor > + and USB Type-C DP HPD events. > + > + This driver provides battery and AC status support for the mentioned > + laptop where this information is not properly exposed via the > + standard ACPI devices. > + > endif # ARM64_PLATFORM_DEVICES > diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile > index f91cdc7155e2..4fcc9855579b 100644 > --- a/drivers/platform/arm64/Makefile > +++ b/drivers/platform/arm64/Makefile > @@ -4,3 +4,5 @@ > # > # This dir should only include drivers for EC-like devices. > # > + > +obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o > diff --git a/drivers/platform/arm64/acer-aspire1-ec.c b/drivers/platform/arm64/acer-aspire1-ec.c > new file mode 100644 > index 000000000000..3941e24c5c7c > --- /dev/null > +++ b/drivers/platform/arm64/acer-aspire1-ec.c > @@ -0,0 +1,555 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright (c) 2023, Nikita Travkin <nikita@trvn.ru> */ 2024 > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/delay.h> This list should be alphabetised. > + > +#include <linux/usb/typec_mux.h> > +#include <drm/drm_bridge.h> Zap the newline and sort all of these headers alphabetically please. > + > +#define ASPIRE_EC_EVENT 0x05 > + > +#define ASPIRE_EC_EVENT_WATCHDOG 0x20 > +#define ASPIRE_EC_EVENT_KBD_BKL_ON 0x57 > +#define ASPIRE_EC_EVENT_KBD_BKL_OFF 0x58 > +#define ASPIRE_EC_EVENT_LID_CLOSE 0x9b > +#define ASPIRE_EC_EVENT_LID_OPEN 0x9c > +#define ASPIRE_EC_EVENT_BKL_UNBLANKED 0x9d > +#define ASPIRE_EC_EVENT_BKL_BLANKED 0x9e > +#define ASPIRE_EC_EVENT_FG_INF_CHG 0x85 > +#define ASPIRE_EC_EVENT_FG_STA_CHG 0xc6 > +#define ASPIRE_EC_EVENT_HPD_DIS 0xa3 > +#define ASPIRE_EC_EVENT_HPD_CON 0xa4 > + > +#define ASPIRE_EC_FG_DYNAMIC 0x07 > +#define ASPIRE_EC_FG_STATIC 0x08 > + > +#define ASPIRE_EC_FG_FLAG_PRESENT BIT(0) > +#define ASPIRE_EC_FG_FLAG_FULL BIT(1) > +#define ASPIRE_EC_FG_FLAG_DISCHARGING BIT(2) > +#define ASPIRE_EC_FG_FLAG_CHARGING BIT(3) > + > +#define ASPIRE_EC_RAM_READ 0x20 > +#define ASPIRE_EC_RAM_WRITE 0x21 > + > +#define ASPIRE_EC_RAM_WATCHDOG 0x19 > +#define ASPIRE_EC_RAM_KBD_MODE 0x43 > + > +#define ASPIRE_EC_RAM_KBD_FN_EN BIT(0) > +#define ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP BIT(5) > +#define ASPIRE_EC_RAM_KBD_ALWAYS_SET BIT(6) > +#define ASPIRE_EC_RAM_KBD_NUM_LAYER_EN BIT(7) > + > +#define ASPIRE_EC_RAM_KBD_MODE_2 0x60 > + > +#define ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY BIT(3) > + > +#define ASPIRE_EC_RAM_HPD_STATUS 0xf4 > +#define ASPIRE_EC_HPD_CONNECTED 0x03 > + > +#define ASPIRE_EC_RAM_LID_STATUS 0x4c > +#define ASPIRE_EC_LID_OPEN BIT(6) > + > +#define ASPIRE_EC_RAM_ADP 0x40 > +#define ASPIRE_EC_AC_STATUS BIT(0) > + > +struct aspire_ec { > + struct i2c_client *client; > + struct power_supply *bat_psy; > + struct power_supply *adp_psy; > + struct input_dev *idev; > + > + bool bridge_configured; > + struct drm_bridge bridge; > + struct work_struct work; > +}; > + > +static int aspire_ec_ram_read(struct i2c_client *client, u8 off, u8 *data, u8 data_len) > +{ > + i2c_smbus_write_byte_data(client, ASPIRE_EC_RAM_READ, off); > + i2c_smbus_read_i2c_block_data(client, ASPIRE_EC_RAM_READ, data_len, data); > + return 0; > +} > + > +static int aspire_ec_ram_write(struct i2c_client *client, u8 off, u8 data) > +{ > + u8 tmp[2] = {off, data}; > + > + i2c_smbus_write_i2c_block_data(client, ASPIRE_EC_RAM_WRITE, sizeof(tmp), tmp); > + return 0; > +} > + > +static irqreturn_t aspire_ec_irq_handler(int irq, void *data) > +{ > + struct aspire_ec *ec = data; > + int id; > + u8 tmp; > + > + /* > + * The original ACPI firmware actually has a small sleep in the handler. > + * > + * It seems like in most cases it's not needed but when the device > + * just exits suspend, our i2c driver has a brief time where data > + * transfer is not possible yet. So this delay allows us to suppress > + * quite a bunch of spurious error messages in dmesg. Thus it's kept. > + */ > + usleep_range(15000, 30000); > + > + id = i2c_smbus_read_byte_data(ec->client, ASPIRE_EC_EVENT); > + if (id < 0) { > + dev_err(&ec->client->dev, "Failed to read event id: %pe\n", ERR_PTR(id)); > + return IRQ_HANDLED; > + } > + > + switch (id) { > + case 0x0: /* No event */ > + break; > + > + case ASPIRE_EC_EVENT_WATCHDOG: > + /* > + * Here acpi responds to the event and clears some bit. > + * Notify (\_SB.I2C3.BAT1, 0x81) // Information Change > + * Notify (\_SB.I2C3.ADP1, 0x80) // Status Change > + */ > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_WATCHDOG, &tmp, sizeof(tmp)); > + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_WATCHDOG, tmp & 0xbf); > + break; > + > + case ASPIRE_EC_EVENT_LID_CLOSE: > + /* Notify (\_SB.LID0, 0x80) // Status Change */ > + input_report_switch(ec->idev, SW_LID, 1); > + input_sync(ec->idev); > + break; > + > + case ASPIRE_EC_EVENT_LID_OPEN: > + /* Notify (\_SB.LID0, 0x80) // Status Change */ > + input_report_switch(ec->idev, SW_LID, 0); > + input_sync(ec->idev); > + break; > + > + case ASPIRE_EC_EVENT_FG_INF_CHG: > + /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */ fallthrough; > + case ASPIRE_EC_EVENT_FG_STA_CHG: > + /* Notify (\_SB.I2C3.BAT1, 0x80) // Status Change */ > + power_supply_changed(ec->bat_psy); > + power_supply_changed(ec->adp_psy); > + break; > + > + case ASPIRE_EC_EVENT_HPD_DIS: > + if (ec->bridge_configured) > + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); > + break; > + > + case ASPIRE_EC_EVENT_HPD_CON: > + if (ec->bridge_configured) > + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); > + break; > + > + case ASPIRE_EC_EVENT_BKL_BLANKED: > + case ASPIRE_EC_EVENT_BKL_UNBLANKED: > + /* Display backlight blanked on FN+F6. No action needed. */ > + break; > + > + case ASPIRE_EC_EVENT_KBD_BKL_ON: > + case ASPIRE_EC_EVENT_KBD_BKL_OFF: > + /* > + * There is a keyboard backlight connector on Aspire 1 that is > + * controlled by FN+F8. There is no kb backlight on the device though. > + * Seems like this is used on other devices like Acer Spin 7. > + * No action needed. > + */ > + break; > + > + default: > + dev_warn(&ec->client->dev, "Unknown event id=0x%x\n", id); > + } > + > + return IRQ_HANDLED; > +} > + > +/* > + * Power supply. > + */ > + > +struct aspire_ec_bat_psy_static_data { > + u8 unk1; > + u8 flags; > + __le16 unk2; > + __le16 voltage_design; > + __le16 capacity_full; > + __le16 unk3; > + __le16 serial; > + u8 model_id; > + u8 vendor_id; > +} __packed; > + > +static const char * const aspire_ec_bat_psy_battery_model[] = { > + "AP18C4K", > + "AP18C8K", > + "AP19B8K", > + "AP16M4J", > + "AP16M5J", > +}; > + > +static const char * const aspire_ec_bat_psy_battery_vendor[] = { > + "SANYO", > + "SONY", > + "PANASONIC", > + "SAMSUNG", > + "SIMPLO", > + "MOTOROLA", > + "CELXPERT", > + "LGC", > + "GETAC", > + "MURATA", > +}; > + > +struct aspire_ec_bat_psy_dynamic_data { > + u8 unk1; > + u8 flags; > + u8 unk2; > + __le16 capacity_now; > + __le16 voltage_now; > + __le16 current_now; > + __le16 unk3; > + __le16 unk4; > +} __packed; > + > +static int aspire_ec_bat_psy_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct aspire_ec *ec = power_supply_get_drvdata(psy); > + struct aspire_ec_bat_psy_static_data sdat; > + struct aspire_ec_bat_psy_dynamic_data ddat; > + > + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_STATIC, sizeof(sdat), (u8 *)&sdat); > + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_DYNAMIC, sizeof(ddat), (u8 *)&ddat); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > + if (ddat.flags & ASPIRE_EC_FG_FLAG_CHARGING) > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + else if (ddat.flags & ASPIRE_EC_FG_FLAG_DISCHARGING) > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + else if (ddat.flags & ASPIRE_EC_FG_FLAG_FULL) > + val->intval = POWER_SUPPLY_STATUS_FULL; > + break; > + > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = le16_to_cpu(ddat.voltage_now) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + val->intval = le16_to_cpu(sdat.voltage_design) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_CHARGE_NOW: > + val->intval = le16_to_cpu(ddat.capacity_now) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_CHARGE_FULL: > + val->intval = le16_to_cpu(sdat.capacity_full) * 1000; > + break; You could stick this "* 1000" stuff in a macro > + > + case POWER_SUPPLY_PROP_CAPACITY: > + val->intval = le16_to_cpu(ddat.capacity_now) * 100; > + val->intval /= le16_to_cpu(sdat.capacity_full); > + break; > + > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_PRESENT: > + val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT); > + break; > + > + case POWER_SUPPLY_PROP_SCOPE: > + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; > + break; > + > + case POWER_SUPPLY_PROP_MODEL_NAME: > + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model)) > + val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1]; > + else > + val->strval = "Unknown"; > + break; > + > + case POWER_SUPPLY_PROP_MANUFACTURER: > + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor)) > + val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3]; How does this -3 offset not underflow ? Seems a bit dodgy to me - can you add a comment to the code to explain ? Its not immediately obvious the -3 is OK. Also could you take an index instead of replicating the -value stepdown each time ? int myindex = sdat.model_id - 1; if (myindex < someconstraint) strval = somearry[myindex]; > + else > + val->strval = "Unknown"; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static enum power_supply_property aspire_ec_bat_psy_props[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_NOW, > + POWER_SUPPLY_PROP_CHARGE_FULL, > + POWER_SUPPLY_PROP_CAPACITY, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_PRESENT, > + POWER_SUPPLY_PROP_SCOPE, > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_MANUFACTURER, > +}; > + > +static const struct power_supply_desc aspire_ec_bat_psy_desc = { > + .name = "aspire-ec-bat", > + .type = POWER_SUPPLY_TYPE_BATTERY, > + .get_property = aspire_ec_bat_psy_get_property, > + .properties = aspire_ec_bat_psy_props, > + .num_properties = ARRAY_SIZE(aspire_ec_bat_psy_props), > +}; > + > +static int aspire_ec_adp_psy_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct aspire_ec *ec = power_supply_get_drvdata(psy); > + u8 tmp; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_ADP, &tmp, sizeof(tmp)); > + val->intval = !!(tmp & ASPIRE_EC_AC_STATUS); > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static enum power_supply_property aspire_ec_adp_psy_props[] = { > + POWER_SUPPLY_PROP_ONLINE, > +}; > + > +static const struct power_supply_desc aspire_ec_adp_psy_desc = { > + .name = "aspire-ec-adp", > + .type = POWER_SUPPLY_TYPE_MAINS, > + .get_property = aspire_ec_adp_psy_get_property, > + .properties = aspire_ec_adp_psy_props, > + .num_properties = ARRAY_SIZE(aspire_ec_adp_psy_props), > +}; > + > +/* > + * USB-C DP Alt mode HPD. > + */ > + > +static int aspire_ec_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) > +{ > + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; > +} > + > +static void aspire_ec_bridge_update_hpd_work(struct work_struct *work) > +{ > + struct aspire_ec *ec = container_of(work, struct aspire_ec, work); > + u8 tmp; > + > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_HPD_STATUS, &tmp, sizeof(tmp)); > + if (tmp == ASPIRE_EC_HPD_CONNECTED) > + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); > + else > + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); > +} > + > +static void aspire_ec_bridge_hpd_enable(struct drm_bridge *bridge) > +{ > + struct aspire_ec *ec = container_of(bridge, struct aspire_ec, bridge); > + > + schedule_work(&ec->work); > +} > + > +static const struct drm_bridge_funcs aspire_ec_bridge_funcs = { > + .hpd_enable = aspire_ec_bridge_hpd_enable, > + .attach = aspire_ec_bridge_attach, > +}; > + > +/* > + * Sysfs attributes. > + */ > + > +static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); > + u8 tmp; > + > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); > + > + return sysfs_emit(buf, "%d\n", !(tmp & ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP)); > +} > + > +static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); > + u8 tmp; > + > + bool state; > + int ret; > + > + ret = kstrtobool(buf, &state); > + if (ret) > + return ret; > + > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); > + > + if (state) > + tmp &= ~ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; > + else > + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; > + > + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_KBD_MODE, tmp); > + > + return count; > +} > + > +static DEVICE_ATTR_RW(fn_lock); > + > +static struct attribute *aspire_ec_attributes[] = { > + &dev_attr_fn_lock.attr, > + NULL > +}; > + > +static const struct attribute_group aspire_ec_attribute_group = { > + .attrs = aspire_ec_attributes > +}; > + > +static int aspire_ec_probe(struct i2c_client *client) > +{ > + struct power_supply_config psy_cfg = {0}; > + struct device *dev = &client->dev; > + struct fwnode_handle *fwnode; > + struct aspire_ec *ec; > + int ret; > + u8 tmp; > + > + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL); > + if (!ec) > + return -ENOMEM; > + > + ec->client = client; > + i2c_set_clientdata(client, ec); > + > + /* Battery status reports */ > + psy_cfg.drv_data = ec; > + ec->bat_psy = devm_power_supply_register(dev, &aspire_ec_bat_psy_desc, &psy_cfg); > + if (IS_ERR(ec->bat_psy)) > + return dev_err_probe(dev, PTR_ERR(ec->bat_psy), > + "Failed to register battery power supply\n"); > + > + ec->adp_psy = devm_power_supply_register(dev, &aspire_ec_adp_psy_desc, &psy_cfg); > + if (IS_ERR(ec->adp_psy)) > + return dev_err_probe(dev, PTR_ERR(ec->adp_psy), > + "Failed to register ac power supply\n"); > + > + /* Lid switch */ > + ec->idev = devm_input_allocate_device(dev); > + if (!ec->idev) > + return -ENOMEM; > + > + ec->idev->name = "aspire-ec"; > + ec->idev->phys = "aspire-ec/input0"; > + input_set_capability(ec->idev, EV_SW, SW_LID); > + > + ret = input_register_device(ec->idev); > + if (ret) > + return dev_err_probe(dev, ret, "Input device register failed\n"); > + > + /* Enable the keyboard fn keys */ > + tmp = ASPIRE_EC_RAM_KBD_FN_EN | ASPIRE_EC_RAM_KBD_ALWAYS_SET; > + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; > + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE, tmp); > + > + aspire_ec_ram_read(client, ASPIRE_EC_RAM_KBD_MODE_2, &tmp, sizeof(tmp)); > + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY; > + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE_2, tmp); > + > + ret = devm_device_add_group(dev, &aspire_ec_attribute_group); > + if (ret) Don't you need input_unregister_device() in this case ? suggest a goto jump label to clean up down the end of your probe() function. > + return dev_err_probe(dev, ret, "Failed to create attribute group\n"); > + > + /* External Type-C display attach reports */ > + fwnode = device_get_named_child_node(dev, "connector"); > + if (fwnode) { > + INIT_WORK(&ec->work, aspire_ec_bridge_update_hpd_work); > + ec->bridge.funcs = &aspire_ec_bridge_funcs; > + ec->bridge.of_node = to_of_node(fwnode); > + ec->bridge.ops = DRM_BRIDGE_OP_HPD; > + ec->bridge.type = DRM_MODE_CONNECTOR_USB; > + > + ret = devm_drm_bridge_add(dev, &ec->bridge); > + if (ret) { > + fwnode_handle_put(fwnode); > + return dev_err_probe(dev, ret, "Failed to register drm bridge\n"); > + } > + > + ec->bridge_configured = true; > + } > + > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > + aspire_ec_irq_handler, IRQF_ONESHOT, > + dev_name(dev), ec); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to request irq\n"); > + > + return 0; jump label here for stuff that doens't just clean up > +} > + > +static int aspire_ec_resume(struct device *dev) > +{ > + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); > + u8 tmp; > + > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_LID_STATUS, &tmp, sizeof(tmp)); > + input_report_switch(ec->idev, SW_LID, !!(tmp & ASPIRE_EC_LID_OPEN)); > + input_sync(ec->idev); > + > + return 0; > +} > + > +static const struct i2c_device_id aspire_ec_id[] = { > + { "aspire1-ec", }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, aspire_ec_id); > + > +static const struct of_device_id aspire_ec_of_match[] = { > + { .compatible = "acer,aspire1-ec", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, aspire_ec_of_match); > + > +static DEFINE_SIMPLE_DEV_PM_OPS(aspire_ec_pm_ops, NULL, aspire_ec_resume); > + > +static struct i2c_driver aspire_ec_driver = { > + .driver = { > + .name = "aspire-ec", > + .of_match_table = aspire_ec_of_match, > + .pm = pm_sleep_ptr(&aspire_ec_pm_ops), > + }, > + .probe = aspire_ec_probe, > + .id_table = aspire_ec_id, > +}; > +module_i2c_driver(aspire_ec_driver); Don't you need an exit function to clean up stuff that's not devm_ derived ? input_unregister_device() ? > + > +MODULE_DESCRIPTION("Acer Aspire 1 embedded controller"); > +MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>"); > +MODULE_LICENSE("GPL"); >
Bryan O'Donoghue писал(а) 12.03.2024 16:58: > On 12/03/2024 08:42, Nikita Travkin wrote: >> Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded >> controller to perform a set of various functions, such as: >> >> - Battery and charger monitoring; >> - Keyboard layout control (i.e. fn_lock settings); >> - USB Type-C DP alt mode HPD notifications; >> - Laptop lid status. >> >> Unfortunately, while all this functionality is implemented in ACPI, it's >> currently not possible to use ACPI to boot Linux on such Qualcomm >> devices. To allow Linux to still support the features provided by EC, >> this driver reimplments the relevant ACPI parts. This allows us to boot >> the laptop with Device Tree and retain all the features. >> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >> --- >> drivers/platform/arm64/Kconfig | 16 + >> drivers/platform/arm64/Makefile | 2 + >> drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++ > > You should be listing yourself as a maintainer for a driver you contribute. I always believed that being in the AUTHOR() at the bottom of the driver would guarantee me being in CC for patches, which so far worked great, thus I was always hesitent adding extra entries in MAINTAINERS. > > Good work BTW. > >> 3 files changed, 573 insertions(+) >> >> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig >> index 644b83ede093..07d47879a9e3 100644 >> --- a/drivers/platform/arm64/Kconfig >> +++ b/drivers/platform/arm64/Kconfig >> @@ -16,4 +16,20 @@ menuconfig ARM64_PLATFORM_DEVICES >> if ARM64_PLATFORM_DEVICES >> +config EC_ACER_ASPIRE1 >> + tristate "Acer Aspire 1 Emedded Controller driver" >> + depends on I2C >> + depends on DRM >> + depends on POWER_SUPPLY >> + depends on INPUT >> + help >> + Say Y here to enable the EC driver for the (Snapdragon-based) >> + Acer Aspire 1 laptop. The EC handles battery and charging >> + monitoring as well as some misc functions like the lid sensor >> + and USB Type-C DP HPD events. >> + >> + This driver provides battery and AC status support for the mentioned >> + laptop where this information is not properly exposed via the >> + standard ACPI devices. >> + >> endif # ARM64_PLATFORM_DEVICES >> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile >> index f91cdc7155e2..4fcc9855579b 100644 >> --- a/drivers/platform/arm64/Makefile >> +++ b/drivers/platform/arm64/Makefile >> @@ -4,3 +4,5 @@ >> # >> # This dir should only include drivers for EC-like devices. >> # >> + >> +obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o >> diff --git a/drivers/platform/arm64/acer-aspire1-ec.c b/drivers/platform/arm64/acer-aspire1-ec.c >> new file mode 100644 >> index 000000000000..3941e24c5c7c >> --- /dev/null >> +++ b/drivers/platform/arm64/acer-aspire1-ec.c >> @@ -0,0 +1,555 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* Copyright (c) 2023, Nikita Travkin <nikita@trvn.ru> */ > > 2024 > That driver was looong in the oven :D >> + >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/power_supply.h> >> +#include <linux/i2c.h> >> +#include <linux/input.h> >> +#include <linux/delay.h> > > This list should be alphabetised. > >> + >> +#include <linux/usb/typec_mux.h> >> +#include <drm/drm_bridge.h> > > Zap the newline and sort all of these headers alphabetically please. > Ack, will cleanup. >> + >> +#define ASPIRE_EC_EVENT 0x05 >> + >> +#define ASPIRE_EC_EVENT_WATCHDOG 0x20 >> +#define ASPIRE_EC_EVENT_KBD_BKL_ON 0x57 >> +#define ASPIRE_EC_EVENT_KBD_BKL_OFF 0x58 >> +#define ASPIRE_EC_EVENT_LID_CLOSE 0x9b >> +#define ASPIRE_EC_EVENT_LID_OPEN 0x9c >> +#define ASPIRE_EC_EVENT_BKL_UNBLANKED 0x9d >> +#define ASPIRE_EC_EVENT_BKL_BLANKED 0x9e >> +#define ASPIRE_EC_EVENT_FG_INF_CHG 0x85 >> +#define ASPIRE_EC_EVENT_FG_STA_CHG 0xc6 >> +#define ASPIRE_EC_EVENT_HPD_DIS 0xa3 >> +#define ASPIRE_EC_EVENT_HPD_CON 0xa4 >> + >> +#define ASPIRE_EC_FG_DYNAMIC 0x07 >> +#define ASPIRE_EC_FG_STATIC 0x08 >> + >> +#define ASPIRE_EC_FG_FLAG_PRESENT BIT(0) >> +#define ASPIRE_EC_FG_FLAG_FULL BIT(1) >> +#define ASPIRE_EC_FG_FLAG_DISCHARGING BIT(2) >> +#define ASPIRE_EC_FG_FLAG_CHARGING BIT(3) >> + >> +#define ASPIRE_EC_RAM_READ 0x20 >> +#define ASPIRE_EC_RAM_WRITE 0x21 >> + >> +#define ASPIRE_EC_RAM_WATCHDOG 0x19 >> +#define ASPIRE_EC_RAM_KBD_MODE 0x43 >> + >> +#define ASPIRE_EC_RAM_KBD_FN_EN BIT(0) >> +#define ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP BIT(5) >> +#define ASPIRE_EC_RAM_KBD_ALWAYS_SET BIT(6) >> +#define ASPIRE_EC_RAM_KBD_NUM_LAYER_EN BIT(7) >> + >> +#define ASPIRE_EC_RAM_KBD_MODE_2 0x60 >> + >> +#define ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY BIT(3) >> + >> +#define ASPIRE_EC_RAM_HPD_STATUS 0xf4 >> +#define ASPIRE_EC_HPD_CONNECTED 0x03 >> + >> +#define ASPIRE_EC_RAM_LID_STATUS 0x4c >> +#define ASPIRE_EC_LID_OPEN BIT(6) >> + >> +#define ASPIRE_EC_RAM_ADP 0x40 >> +#define ASPIRE_EC_AC_STATUS BIT(0) >> + >> +struct aspire_ec { >> + struct i2c_client *client; >> + struct power_supply *bat_psy; >> + struct power_supply *adp_psy; >> + struct input_dev *idev; >> + >> + bool bridge_configured; >> + struct drm_bridge bridge; >> + struct work_struct work; >> +}; >> + >> +static int aspire_ec_ram_read(struct i2c_client *client, u8 off, u8 *data, u8 data_len) >> +{ >> + i2c_smbus_write_byte_data(client, ASPIRE_EC_RAM_READ, off); >> + i2c_smbus_read_i2c_block_data(client, ASPIRE_EC_RAM_READ, data_len, data); >> + return 0; >> +} >> + >> +static int aspire_ec_ram_write(struct i2c_client *client, u8 off, u8 data) >> +{ >> + u8 tmp[2] = {off, data}; >> + >> + i2c_smbus_write_i2c_block_data(client, ASPIRE_EC_RAM_WRITE, sizeof(tmp), tmp); >> + return 0; >> +} >> + >> +static irqreturn_t aspire_ec_irq_handler(int irq, void *data) >> +{ >> + struct aspire_ec *ec = data; >> + int id; >> + u8 tmp; >> + >> + /* >> + * The original ACPI firmware actually has a small sleep in the handler. >> + * >> + * It seems like in most cases it's not needed but when the device >> + * just exits suspend, our i2c driver has a brief time where data >> + * transfer is not possible yet. So this delay allows us to suppress >> + * quite a bunch of spurious error messages in dmesg. Thus it's kept. >> + */ >> + usleep_range(15000, 30000); >> + >> + id = i2c_smbus_read_byte_data(ec->client, ASPIRE_EC_EVENT); >> + if (id < 0) { >> + dev_err(&ec->client->dev, "Failed to read event id: %pe\n", ERR_PTR(id)); >> + return IRQ_HANDLED; >> + } >> + >> + switch (id) { >> + case 0x0: /* No event */ >> + break; >> + >> + case ASPIRE_EC_EVENT_WATCHDOG: >> + /* >> + * Here acpi responds to the event and clears some bit. >> + * Notify (\_SB.I2C3.BAT1, 0x81) // Information Change >> + * Notify (\_SB.I2C3.ADP1, 0x80) // Status Change >> + */ >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_WATCHDOG, &tmp, sizeof(tmp)); >> + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_WATCHDOG, tmp & 0xbf); >> + break; >> + >> + case ASPIRE_EC_EVENT_LID_CLOSE: >> + /* Notify (\_SB.LID0, 0x80) // Status Change */ >> + input_report_switch(ec->idev, SW_LID, 1); >> + input_sync(ec->idev); >> + break; >> + >> + case ASPIRE_EC_EVENT_LID_OPEN: >> + /* Notify (\_SB.LID0, 0x80) // Status Change */ >> + input_report_switch(ec->idev, SW_LID, 0); >> + input_sync(ec->idev); >> + break; >> + >> + case ASPIRE_EC_EVENT_FG_INF_CHG: >> + /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */ > > fallthrough; > Hm I believe this would not warn since it's just two values for the same code, just with an extra comment inbetween? >> + case ASPIRE_EC_EVENT_FG_STA_CHG: >> + /* Notify (\_SB.I2C3.BAT1, 0x80) // Status Change */ >> + power_supply_changed(ec->bat_psy); >> + power_supply_changed(ec->adp_psy); >> + break; >> + >> + case ASPIRE_EC_EVENT_HPD_DIS: >> + if (ec->bridge_configured) >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); >> + break; >> + >> + case ASPIRE_EC_EVENT_HPD_CON: >> + if (ec->bridge_configured) >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); >> + break; >> + >> + case ASPIRE_EC_EVENT_BKL_BLANKED: >> + case ASPIRE_EC_EVENT_BKL_UNBLANKED: >> + /* Display backlight blanked on FN+F6. No action needed. */ >> + break; >> + >> + case ASPIRE_EC_EVENT_KBD_BKL_ON: >> + case ASPIRE_EC_EVENT_KBD_BKL_OFF: >> + /* >> + * There is a keyboard backlight connector on Aspire 1 that is >> + * controlled by FN+F8. There is no kb backlight on the device though. >> + * Seems like this is used on other devices like Acer Spin 7. >> + * No action needed. >> + */ >> + break; >> + >> + default: >> + dev_warn(&ec->client->dev, "Unknown event id=0x%x\n", id); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +/* >> + * Power supply. >> + */ >> + >> +struct aspire_ec_bat_psy_static_data { >> + u8 unk1; >> + u8 flags; >> + __le16 unk2; >> + __le16 voltage_design; >> + __le16 capacity_full; >> + __le16 unk3; >> + __le16 serial; >> + u8 model_id; >> + u8 vendor_id; >> +} __packed; >> + >> +static const char * const aspire_ec_bat_psy_battery_model[] = { >> + "AP18C4K", >> + "AP18C8K", >> + "AP19B8K", >> + "AP16M4J", >> + "AP16M5J", >> +}; >> + >> +static const char * const aspire_ec_bat_psy_battery_vendor[] = { >> + "SANYO", >> + "SONY", >> + "PANASONIC", >> + "SAMSUNG", >> + "SIMPLO", >> + "MOTOROLA", >> + "CELXPERT", >> + "LGC", >> + "GETAC", >> + "MURATA", >> +}; >> + >> +struct aspire_ec_bat_psy_dynamic_data { >> + u8 unk1; >> + u8 flags; >> + u8 unk2; >> + __le16 capacity_now; >> + __le16 voltage_now; >> + __le16 current_now; >> + __le16 unk3; >> + __le16 unk4; >> +} __packed; >> + >> +static int aspire_ec_bat_psy_get_property(struct power_supply *psy, >> + enum power_supply_property psp, >> + union power_supply_propval *val) >> +{ >> + struct aspire_ec *ec = power_supply_get_drvdata(psy); >> + struct aspire_ec_bat_psy_static_data sdat; >> + struct aspire_ec_bat_psy_dynamic_data ddat; >> + >> + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_STATIC, sizeof(sdat), (u8 *)&sdat); >> + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_DYNAMIC, sizeof(ddat), (u8 *)&ddat); >> + >> + switch (psp) { >> + case POWER_SUPPLY_PROP_STATUS: >> + val->intval = POWER_SUPPLY_STATUS_UNKNOWN; >> + if (ddat.flags & ASPIRE_EC_FG_FLAG_CHARGING) >> + val->intval = POWER_SUPPLY_STATUS_CHARGING; >> + else if (ddat.flags & ASPIRE_EC_FG_FLAG_DISCHARGING) >> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >> + else if (ddat.flags & ASPIRE_EC_FG_FLAG_FULL) >> + val->intval = POWER_SUPPLY_STATUS_FULL; >> + break; >> + >> + case POWER_SUPPLY_PROP_VOLTAGE_NOW: >> + val->intval = le16_to_cpu(ddat.voltage_now) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: >> + val->intval = le16_to_cpu(sdat.voltage_design) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_CHARGE_NOW: >> + val->intval = le16_to_cpu(ddat.capacity_now) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_CHARGE_FULL: >> + val->intval = le16_to_cpu(sdat.capacity_full) * 1000; >> + break; > > You could stick this "* 1000" stuff in a macro > acpi/battery.c also explicitly sets the multiplier so I think it's the "common" way to do this. >> + >> + case POWER_SUPPLY_PROP_CAPACITY: >> + val->intval = le16_to_cpu(ddat.capacity_now) * 100; >> + val->intval /= le16_to_cpu(sdat.capacity_full); >> + break; >> + >> + case POWER_SUPPLY_PROP_CURRENT_NOW: >> + val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_PRESENT: >> + val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT); >> + break; >> + >> + case POWER_SUPPLY_PROP_SCOPE: >> + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; >> + break; >> + >> + case POWER_SUPPLY_PROP_MODEL_NAME: >> + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model)) >> + val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1]; >> + else >> + val->strval = "Unknown"; >> + break; >> + >> + case POWER_SUPPLY_PROP_MANUFACTURER: >> + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor)) >> + val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3]; > > How does this -3 offset not underflow ? > vendor_id here is unsigned so the if check would actually overflow, though explaining that I guess it's better to be explicit there and let the compiler optimize that check away anyway... I will update the if condition with an extra (id >= 3). > Seems a bit dodgy to me - can you add a comment to the code to explain ? Its not immediately obvious the -3 is OK. > > Also could you take an index instead of replicating the -value stepdown each time ? > > int myindex = sdat.model_id - 1; > > if (myindex < someconstraint) > strval = somearry[myindex]; > I decided against adding a dedicated index variable since there is only one actual use for each, so it's easy to see where it goes. >> + else >> + val->strval = "Unknown"; >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static enum power_supply_property aspire_ec_bat_psy_props[] = { >> + POWER_SUPPLY_PROP_STATUS, >> + POWER_SUPPLY_PROP_VOLTAGE_NOW, >> + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, >> + POWER_SUPPLY_PROP_CHARGE_NOW, >> + POWER_SUPPLY_PROP_CHARGE_FULL, >> + POWER_SUPPLY_PROP_CAPACITY, >> + POWER_SUPPLY_PROP_CURRENT_NOW, >> + POWER_SUPPLY_PROP_PRESENT, >> + POWER_SUPPLY_PROP_SCOPE, >> + POWER_SUPPLY_PROP_MODEL_NAME, >> + POWER_SUPPLY_PROP_MANUFACTURER, >> +}; >> + >> +static const struct power_supply_desc aspire_ec_bat_psy_desc = { >> + .name = "aspire-ec-bat", >> + .type = POWER_SUPPLY_TYPE_BATTERY, >> + .get_property = aspire_ec_bat_psy_get_property, >> + .properties = aspire_ec_bat_psy_props, >> + .num_properties = ARRAY_SIZE(aspire_ec_bat_psy_props), >> +}; >> + >> +static int aspire_ec_adp_psy_get_property(struct power_supply *psy, >> + enum power_supply_property psp, >> + union power_supply_propval *val) >> +{ >> + struct aspire_ec *ec = power_supply_get_drvdata(psy); >> + u8 tmp; >> + >> + switch (psp) { >> + case POWER_SUPPLY_PROP_ONLINE: >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_ADP, &tmp, sizeof(tmp)); >> + val->intval = !!(tmp & ASPIRE_EC_AC_STATUS); >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static enum power_supply_property aspire_ec_adp_psy_props[] = { >> + POWER_SUPPLY_PROP_ONLINE, >> +}; >> + >> +static const struct power_supply_desc aspire_ec_adp_psy_desc = { >> + .name = "aspire-ec-adp", >> + .type = POWER_SUPPLY_TYPE_MAINS, >> + .get_property = aspire_ec_adp_psy_get_property, >> + .properties = aspire_ec_adp_psy_props, >> + .num_properties = ARRAY_SIZE(aspire_ec_adp_psy_props), >> +}; >> + >> +/* >> + * USB-C DP Alt mode HPD. >> + */ >> + >> +static int aspire_ec_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) >> +{ >> + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; >> +} >> + >> +static void aspire_ec_bridge_update_hpd_work(struct work_struct *work) >> +{ >> + struct aspire_ec *ec = container_of(work, struct aspire_ec, work); >> + u8 tmp; >> + >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_HPD_STATUS, &tmp, sizeof(tmp)); >> + if (tmp == ASPIRE_EC_HPD_CONNECTED) >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); >> + else >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); >> +} >> + >> +static void aspire_ec_bridge_hpd_enable(struct drm_bridge *bridge) >> +{ >> + struct aspire_ec *ec = container_of(bridge, struct aspire_ec, bridge); >> + >> + schedule_work(&ec->work); >> +} >> + >> +static const struct drm_bridge_funcs aspire_ec_bridge_funcs = { >> + .hpd_enable = aspire_ec_bridge_hpd_enable, >> + .attach = aspire_ec_bridge_attach, >> +}; >> + >> +/* >> + * Sysfs attributes. >> + */ >> + >> +static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf) >> +{ >> + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); >> + u8 tmp; >> + >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); >> + >> + return sysfs_emit(buf, "%d\n", !(tmp & ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP)); >> +} >> + >> +static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); >> + u8 tmp; >> + >> + bool state; >> + int ret; >> + >> + ret = kstrtobool(buf, &state); >> + if (ret) >> + return ret; >> + >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); >> + >> + if (state) >> + tmp &= ~ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; >> + else >> + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; >> + >> + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_KBD_MODE, tmp); >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(fn_lock); >> + >> +static struct attribute *aspire_ec_attributes[] = { >> + &dev_attr_fn_lock.attr, >> + NULL >> +}; >> + >> +static const struct attribute_group aspire_ec_attribute_group = { >> + .attrs = aspire_ec_attributes >> +}; >> + >> +static int aspire_ec_probe(struct i2c_client *client) >> +{ >> + struct power_supply_config psy_cfg = {0}; >> + struct device *dev = &client->dev; >> + struct fwnode_handle *fwnode; >> + struct aspire_ec *ec; >> + int ret; >> + u8 tmp; >> + >> + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL); >> + if (!ec) >> + return -ENOMEM; >> + >> + ec->client = client; >> + i2c_set_clientdata(client, ec); >> + >> + /* Battery status reports */ >> + psy_cfg.drv_data = ec; >> + ec->bat_psy = devm_power_supply_register(dev, &aspire_ec_bat_psy_desc, &psy_cfg); >> + if (IS_ERR(ec->bat_psy)) >> + return dev_err_probe(dev, PTR_ERR(ec->bat_psy), >> + "Failed to register battery power supply\n"); >> + >> + ec->adp_psy = devm_power_supply_register(dev, &aspire_ec_adp_psy_desc, &psy_cfg); >> + if (IS_ERR(ec->adp_psy)) >> + return dev_err_probe(dev, PTR_ERR(ec->adp_psy), >> + "Failed to register ac power supply\n"); >> + >> + /* Lid switch */ >> + ec->idev = devm_input_allocate_device(dev); >> + if (!ec->idev) >> + return -ENOMEM; >> + >> + ec->idev->name = "aspire-ec"; >> + ec->idev->phys = "aspire-ec/input0"; >> + input_set_capability(ec->idev, EV_SW, SW_LID); >> + >> + ret = input_register_device(ec->idev); >> + if (ret) >> + return dev_err_probe(dev, ret, "Input device register failed\n"); >> + >> + /* Enable the keyboard fn keys */ >> + tmp = ASPIRE_EC_RAM_KBD_FN_EN | ASPIRE_EC_RAM_KBD_ALWAYS_SET; >> + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; >> + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE, tmp); >> + >> + aspire_ec_ram_read(client, ASPIRE_EC_RAM_KBD_MODE_2, &tmp, sizeof(tmp)); >> + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY; >> + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE_2, tmp); >> + >> + ret = devm_device_add_group(dev, &aspire_ec_attribute_group); >> + if (ret) > > Don't you need input_unregister_device() in this case ? suggest a goto jump label to clean up down the end of your probe() function. > devm_input_allocate_device guarantees that devm will un-register it, which I myself also forget every time and have to re-check :D * Managed input devices do not need to be explicitly unregistered or * freed as it will be done automatically when owner device unbinds from * its driver (or binding fails). So this driver only uses devm resources and thus doesn't need to clean after itself. Thanks for your review! Nikita >> + return dev_err_probe(dev, ret, "Failed to create attribute group\n"); >> + >> + /* External Type-C display attach reports */ >> + fwnode = device_get_named_child_node(dev, "connector"); >> + if (fwnode) { >> + INIT_WORK(&ec->work, aspire_ec_bridge_update_hpd_work); >> + ec->bridge.funcs = &aspire_ec_bridge_funcs; >> + ec->bridge.of_node = to_of_node(fwnode); >> + ec->bridge.ops = DRM_BRIDGE_OP_HPD; >> + ec->bridge.type = DRM_MODE_CONNECTOR_USB; >> + >> + ret = devm_drm_bridge_add(dev, &ec->bridge); >> + if (ret) { >> + fwnode_handle_put(fwnode); >> + return dev_err_probe(dev, ret, "Failed to register drm bridge\n"); >> + } >> + >> + ec->bridge_configured = true; >> + } >> + >> + ret = devm_request_threaded_irq(dev, client->irq, NULL, >> + aspire_ec_irq_handler, IRQF_ONESHOT, >> + dev_name(dev), ec); >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to request irq\n"); >> + >> + return 0; > > jump label here for stuff that doens't just clean up > >> +} >> + >> +static int aspire_ec_resume(struct device *dev) >> +{ >> + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); >> + u8 tmp; >> + >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_LID_STATUS, &tmp, sizeof(tmp)); >> + input_report_switch(ec->idev, SW_LID, !!(tmp & ASPIRE_EC_LID_OPEN)); >> + input_sync(ec->idev); >> + >> + return 0; >> +} >> + >> +static const struct i2c_device_id aspire_ec_id[] = { >> + { "aspire1-ec", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(i2c, aspire_ec_id); >> + >> +static const struct of_device_id aspire_ec_of_match[] = { >> + { .compatible = "acer,aspire1-ec", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, aspire_ec_of_match); >> + >> +static DEFINE_SIMPLE_DEV_PM_OPS(aspire_ec_pm_ops, NULL, aspire_ec_resume); >> + >> +static struct i2c_driver aspire_ec_driver = { >> + .driver = { >> + .name = "aspire-ec", >> + .of_match_table = aspire_ec_of_match, >> + .pm = pm_sleep_ptr(&aspire_ec_pm_ops), >> + }, >> + .probe = aspire_ec_probe, >> + .id_table = aspire_ec_id, >> +}; >> +module_i2c_driver(aspire_ec_driver); > > Don't you need an exit function to clean up stuff that's not devm_ derived ? > > input_unregister_device() ? > >> + >> +MODULE_DESCRIPTION("Acer Aspire 1 embedded controller"); >> +MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>"); >> +MODULE_LICENSE("GPL"); >>
On Tue, 12 Mar 2024, Nikita Travkin wrote: > Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded > controller to perform a set of various functions, such as: > > - Battery and charger monitoring; > - Keyboard layout control (i.e. fn_lock settings); > - USB Type-C DP alt mode HPD notifications; > - Laptop lid status. > > Unfortunately, while all this functionality is implemented in ACPI, it's > currently not possible to use ACPI to boot Linux on such Qualcomm > devices. To allow Linux to still support the features provided by EC, > this driver reimplments the relevant ACPI parts. This allows us to boot > the laptop with Device Tree and retain all the features. > > Signed-off-by: Nikita Travkin <nikita@trvn.ru> > --- > drivers/platform/arm64/Kconfig | 16 + > drivers/platform/arm64/Makefile | 2 + > drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++ > 3 files changed, 573 insertions(+) > > diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig > index 644b83ede093..07d47879a9e3 100644 > --- a/drivers/platform/arm64/Kconfig > +++ b/drivers/platform/arm64/Kconfig > @@ -16,4 +16,20 @@ menuconfig ARM64_PLATFORM_DEVICES > > if ARM64_PLATFORM_DEVICES > > +config EC_ACER_ASPIRE1 > + tristate "Acer Aspire 1 Emedded Controller driver" > + depends on I2C > + depends on DRM > + depends on POWER_SUPPLY > + depends on INPUT > + help > + Say Y here to enable the EC driver for the (Snapdragon-based) > + Acer Aspire 1 laptop. The EC handles battery and charging > + monitoring as well as some misc functions like the lid sensor > + and USB Type-C DP HPD events. > + > + This driver provides battery and AC status support for the mentioned > + laptop where this information is not properly exposed via the > + standard ACPI devices. > + > endif # ARM64_PLATFORM_DEVICES > diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile > index f91cdc7155e2..4fcc9855579b 100644 > --- a/drivers/platform/arm64/Makefile > +++ b/drivers/platform/arm64/Makefile > @@ -4,3 +4,5 @@ > # > # This dir should only include drivers for EC-like devices. > # > + > +obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o > diff --git a/drivers/platform/arm64/acer-aspire1-ec.c b/drivers/platform/arm64/acer-aspire1-ec.c > new file mode 100644 > index 000000000000..3941e24c5c7c > --- /dev/null > +++ b/drivers/platform/arm64/acer-aspire1-ec.c > @@ -0,0 +1,555 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright (c) 2023, Nikita Travkin <nikita@trvn.ru> */ > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/delay.h> > + > +#include <linux/usb/typec_mux.h> > +#include <drm/drm_bridge.h> > + > +#define ASPIRE_EC_EVENT 0x05 > + > +#define ASPIRE_EC_EVENT_WATCHDOG 0x20 > +#define ASPIRE_EC_EVENT_KBD_BKL_ON 0x57 > +#define ASPIRE_EC_EVENT_KBD_BKL_OFF 0x58 > +#define ASPIRE_EC_EVENT_LID_CLOSE 0x9b > +#define ASPIRE_EC_EVENT_LID_OPEN 0x9c > +#define ASPIRE_EC_EVENT_BKL_UNBLANKED 0x9d > +#define ASPIRE_EC_EVENT_BKL_BLANKED 0x9e > +#define ASPIRE_EC_EVENT_FG_INF_CHG 0x85 > +#define ASPIRE_EC_EVENT_FG_STA_CHG 0xc6 > +#define ASPIRE_EC_EVENT_HPD_DIS 0xa3 > +#define ASPIRE_EC_EVENT_HPD_CON 0xa4 > + > +#define ASPIRE_EC_FG_DYNAMIC 0x07 > +#define ASPIRE_EC_FG_STATIC 0x08 > + > +#define ASPIRE_EC_FG_FLAG_PRESENT BIT(0) Add #include <linux/bits.h> > +#define ASPIRE_EC_FG_FLAG_FULL BIT(1) > +#define ASPIRE_EC_FG_FLAG_DISCHARGING BIT(2) > +#define ASPIRE_EC_FG_FLAG_CHARGING BIT(3) > + > +#define ASPIRE_EC_RAM_READ 0x20 > +#define ASPIRE_EC_RAM_WRITE 0x21 > + > +#define ASPIRE_EC_RAM_WATCHDOG 0x19 > +#define ASPIRE_EC_RAM_KBD_MODE 0x43 > + > +#define ASPIRE_EC_RAM_KBD_FN_EN BIT(0) > +#define ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP BIT(5) > +#define ASPIRE_EC_RAM_KBD_ALWAYS_SET BIT(6) > +#define ASPIRE_EC_RAM_KBD_NUM_LAYER_EN BIT(7) > + > +#define ASPIRE_EC_RAM_KBD_MODE_2 0x60 > + > +#define ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY BIT(3) > + > +#define ASPIRE_EC_RAM_HPD_STATUS 0xf4 > +#define ASPIRE_EC_HPD_CONNECTED 0x03 > + > +#define ASPIRE_EC_RAM_LID_STATUS 0x4c > +#define ASPIRE_EC_LID_OPEN BIT(6) > + > +#define ASPIRE_EC_RAM_ADP 0x40 > +#define ASPIRE_EC_AC_STATUS BIT(0) > + > +struct aspire_ec { > + struct i2c_client *client; > + struct power_supply *bat_psy; > + struct power_supply *adp_psy; > + struct input_dev *idev; > + > + bool bridge_configured; > + struct drm_bridge bridge; > + struct work_struct work; Include missing. > +static int aspire_ec_ram_read(struct i2c_client *client, u8 off, u8 *data, u8 data_len) > +{ > + i2c_smbus_write_byte_data(client, ASPIRE_EC_RAM_READ, off); > + i2c_smbus_read_i2c_block_data(client, ASPIRE_EC_RAM_READ, data_len, data); > + return 0; > +} > + > +static int aspire_ec_ram_write(struct i2c_client *client, u8 off, u8 data) > +{ > + u8 tmp[2] = {off, data}; > + > + i2c_smbus_write_i2c_block_data(client, ASPIRE_EC_RAM_WRITE, sizeof(tmp), tmp); > + return 0; > +} > + > +static irqreturn_t aspire_ec_irq_handler(int irq, void *data) > +{ > + struct aspire_ec *ec = data; > + int id; > + u8 tmp; > + > + /* > + * The original ACPI firmware actually has a small sleep in the handler. > + * > + * It seems like in most cases it's not needed but when the device > + * just exits suspend, our i2c driver has a brief time where data > + * transfer is not possible yet. So this delay allows us to suppress > + * quite a bunch of spurious error messages in dmesg. Thus it's kept. > + */ > + usleep_range(15000, 30000); > + > + id = i2c_smbus_read_byte_data(ec->client, ASPIRE_EC_EVENT); > + if (id < 0) { > + dev_err(&ec->client->dev, "Failed to read event id: %pe\n", ERR_PTR(id)); > + return IRQ_HANDLED; > + } > + > + switch (id) { > + case 0x0: /* No event */ > + break; > + > + case ASPIRE_EC_EVENT_WATCHDOG: > + /* > + * Here acpi responds to the event and clears some bit. > + * Notify (\_SB.I2C3.BAT1, 0x81) // Information Change > + * Notify (\_SB.I2C3.ADP1, 0x80) // Status Change > + */ > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_WATCHDOG, &tmp, sizeof(tmp)); > + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_WATCHDOG, tmp & 0xbf); Magic 0xbf? Is this a clear bit operation which should use tmp & ~DEFINEDNAME ? > + break; > + > + case ASPIRE_EC_EVENT_LID_CLOSE: > + /* Notify (\_SB.LID0, 0x80) // Status Change */ > + input_report_switch(ec->idev, SW_LID, 1); > + input_sync(ec->idev); > + break; > + > + case ASPIRE_EC_EVENT_LID_OPEN: > + /* Notify (\_SB.LID0, 0x80) // Status Change */ > + input_report_switch(ec->idev, SW_LID, 0); > + input_sync(ec->idev); > + break; > + > + case ASPIRE_EC_EVENT_FG_INF_CHG: > + /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */ fallthrough; I'm not 100% sure if it's needed because you only have a comment there but it surely doesn't hurt to have it. > + case ASPIRE_EC_EVENT_FG_STA_CHG: > + /* Notify (\_SB.I2C3.BAT1, 0x80) // Status Change */ > + power_supply_changed(ec->bat_psy); > + power_supply_changed(ec->adp_psy); > + break; > + > + case ASPIRE_EC_EVENT_HPD_DIS: > + if (ec->bridge_configured) > + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); > + break; > + > + case ASPIRE_EC_EVENT_HPD_CON: > + if (ec->bridge_configured) > + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); > + break; > + > + case ASPIRE_EC_EVENT_BKL_BLANKED: > + case ASPIRE_EC_EVENT_BKL_UNBLANKED: > + /* Display backlight blanked on FN+F6. No action needed. */ > + break; > + > + case ASPIRE_EC_EVENT_KBD_BKL_ON: > + case ASPIRE_EC_EVENT_KBD_BKL_OFF: > + /* > + * There is a keyboard backlight connector on Aspire 1 that is > + * controlled by FN+F8. There is no kb backlight on the device though. > + * Seems like this is used on other devices like Acer Spin 7. > + * No action needed. > + */ > + break; > + > + default: > + dev_warn(&ec->client->dev, "Unknown event id=0x%x\n", id); > + } > + > + return IRQ_HANDLED; > +} > + > +/* > + * Power supply. > + */ > + > +struct aspire_ec_bat_psy_static_data { > + u8 unk1; > + u8 flags; > + __le16 unk2; > + __le16 voltage_design; > + __le16 capacity_full; > + __le16 unk3; > + __le16 serial; > + u8 model_id; > + u8 vendor_id; > +} __packed; > + > +static const char * const aspire_ec_bat_psy_battery_model[] = { > + "AP18C4K", > + "AP18C8K", > + "AP19B8K", > + "AP16M4J", > + "AP16M5J", > +}; > + > +static const char * const aspire_ec_bat_psy_battery_vendor[] = { > + "SANYO", > + "SONY", > + "PANASONIC", > + "SAMSUNG", > + "SIMPLO", > + "MOTOROLA", > + "CELXPERT", > + "LGC", > + "GETAC", > + "MURATA", > +}; > + > +struct aspire_ec_bat_psy_dynamic_data { > + u8 unk1; > + u8 flags; > + u8 unk2; > + __le16 capacity_now; > + __le16 voltage_now; > + __le16 current_now; > + __le16 unk3; > + __le16 unk4; > +} __packed; What are the ARM64 rules when it comes to unaligned access? If it's not always supported, you need to use get_unaligned_le16() for accessing those __le16 fields. > +static int aspire_ec_bat_psy_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct aspire_ec *ec = power_supply_get_drvdata(psy); > + struct aspire_ec_bat_psy_static_data sdat; > + struct aspire_ec_bat_psy_dynamic_data ddat; > + > + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_STATIC, sizeof(sdat), (u8 *)&sdat); > + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_DYNAMIC, sizeof(ddat), (u8 *)&ddat); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > + if (ddat.flags & ASPIRE_EC_FG_FLAG_CHARGING) > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + else if (ddat.flags & ASPIRE_EC_FG_FLAG_DISCHARGING) > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + else if (ddat.flags & ASPIRE_EC_FG_FLAG_FULL) > + val->intval = POWER_SUPPLY_STATUS_FULL; > + break; > + > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = le16_to_cpu(ddat.voltage_now) * 1000; linux/units.h provides something that can often be used instead of 10^n literals. > + break; > + > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + val->intval = le16_to_cpu(sdat.voltage_design) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_CHARGE_NOW: > + val->intval = le16_to_cpu(ddat.capacity_now) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_CHARGE_FULL: > + val->intval = le16_to_cpu(sdat.capacity_full) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_CAPACITY: > + val->intval = le16_to_cpu(ddat.capacity_now) * 100; > + val->intval /= le16_to_cpu(sdat.capacity_full); > + break; > + > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000; > + break; > + > + case POWER_SUPPLY_PROP_PRESENT: > + val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT); > + break; > + > + case POWER_SUPPLY_PROP_SCOPE: > + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; > + break; > + > + case POWER_SUPPLY_PROP_MODEL_NAME: > + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model)) > + val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1]; Underflow handling (also here, the other place was mentioned already)? > + else > + val->strval = "Unknown"; > + break; > + > + case POWER_SUPPLY_PROP_MANUFACTURER: > + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor)) > + val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3]; > + else > + val->strval = "Unknown"; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static enum power_supply_property aspire_ec_bat_psy_props[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_NOW, > + POWER_SUPPLY_PROP_CHARGE_FULL, > + POWER_SUPPLY_PROP_CAPACITY, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_PRESENT, > + POWER_SUPPLY_PROP_SCOPE, > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_MANUFACTURER, > +}; > + > +static const struct power_supply_desc aspire_ec_bat_psy_desc = { > + .name = "aspire-ec-bat", > + .type = POWER_SUPPLY_TYPE_BATTERY, > + .get_property = aspire_ec_bat_psy_get_property, > + .properties = aspire_ec_bat_psy_props, > + .num_properties = ARRAY_SIZE(aspire_ec_bat_psy_props), > +}; > + > +static int aspire_ec_adp_psy_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct aspire_ec *ec = power_supply_get_drvdata(psy); > + u8 tmp; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_ADP, &tmp, sizeof(tmp)); > + val->intval = !!(tmp & ASPIRE_EC_AC_STATUS); > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static enum power_supply_property aspire_ec_adp_psy_props[] = { > + POWER_SUPPLY_PROP_ONLINE, > +}; > + > +static const struct power_supply_desc aspire_ec_adp_psy_desc = { > + .name = "aspire-ec-adp", > + .type = POWER_SUPPLY_TYPE_MAINS, > + .get_property = aspire_ec_adp_psy_get_property, > + .properties = aspire_ec_adp_psy_props, > + .num_properties = ARRAY_SIZE(aspire_ec_adp_psy_props), > +}; > + > +/* > + * USB-C DP Alt mode HPD. > + */ > + > +static int aspire_ec_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) > +{ > + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; > +} > + > +static void aspire_ec_bridge_update_hpd_work(struct work_struct *work) > +{ > + struct aspire_ec *ec = container_of(work, struct aspire_ec, work); > + u8 tmp; > + > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_HPD_STATUS, &tmp, sizeof(tmp)); > + if (tmp == ASPIRE_EC_HPD_CONNECTED) > + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); > + else > + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); > +} > + > +static void aspire_ec_bridge_hpd_enable(struct drm_bridge *bridge) > +{ > + struct aspire_ec *ec = container_of(bridge, struct aspire_ec, bridge); > + > + schedule_work(&ec->work); > +} > + > +static const struct drm_bridge_funcs aspire_ec_bridge_funcs = { > + .hpd_enable = aspire_ec_bridge_hpd_enable, > + .attach = aspire_ec_bridge_attach, > +}; > + > +/* > + * Sysfs attributes. > + */ > + > +static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); > + u8 tmp; > + > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); > + > + return sysfs_emit(buf, "%d\n", !(tmp & ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP)); %u > +} > + > +static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); > + u8 tmp; > + > + bool state; > + int ret; > + > + ret = kstrtobool(buf, &state); > + if (ret) > + return ret; > + > + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); > + > + if (state) > + tmp &= ~ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; > + else > + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; > + > + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_KBD_MODE, tmp); > + > + return count; > +} > + > +static DEVICE_ATTR_RW(fn_lock); > + > +static struct attribute *aspire_ec_attributes[] = { > + &dev_attr_fn_lock.attr, > + NULL > +}; > + > +static const struct attribute_group aspire_ec_attribute_group = { > + .attrs = aspire_ec_attributes > +}; > + > +static int aspire_ec_probe(struct i2c_client *client) > +{ > + struct power_supply_config psy_cfg = {0}; > + struct device *dev = &client->dev; > + struct fwnode_handle *fwnode; > + struct aspire_ec *ec; > + int ret; > + u8 tmp; > + > + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL); > + if (!ec) > + return -ENOMEM; > + > + ec->client = client; > + i2c_set_clientdata(client, ec); > + > + /* Battery status reports */ > + psy_cfg.drv_data = ec; > + ec->bat_psy = devm_power_supply_register(dev, &aspire_ec_bat_psy_desc, &psy_cfg); > + if (IS_ERR(ec->bat_psy)) > + return dev_err_probe(dev, PTR_ERR(ec->bat_psy), > + "Failed to register battery power supply\n"); > + > + ec->adp_psy = devm_power_supply_register(dev, &aspire_ec_adp_psy_desc, &psy_cfg); > + if (IS_ERR(ec->adp_psy)) > + return dev_err_probe(dev, PTR_ERR(ec->adp_psy), > + "Failed to register ac power supply\n"); AC > + /* Lid switch */ > + ec->idev = devm_input_allocate_device(dev); > + if (!ec->idev) > + return -ENOMEM; > + > + ec->idev->name = "aspire-ec"; > + ec->idev->phys = "aspire-ec/input0"; > + input_set_capability(ec->idev, EV_SW, SW_LID); > + > + ret = input_register_device(ec->idev); > + if (ret) > + return dev_err_probe(dev, ret, "Input device register failed\n"); > + > + /* Enable the keyboard fn keys */ > + tmp = ASPIRE_EC_RAM_KBD_FN_EN | ASPIRE_EC_RAM_KBD_ALWAYS_SET; > + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; > + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE, tmp); > + > + aspire_ec_ram_read(client, ASPIRE_EC_RAM_KBD_MODE_2, &tmp, sizeof(tmp)); > + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY; > + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE_2, tmp); > + > + ret = devm_device_add_group(dev, &aspire_ec_attribute_group); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to create attribute group\n"); Can't you just .dev_groups ? In general, this felt to be in a quite good shape already, good work!
On 12/03/2024 12:23, Nikita Travkin wrote: > Bryan O'Donoghue писал(а) 12.03.2024 16:58: >> On 12/03/2024 08:42, Nikita Travkin wrote: >>> Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded >>> controller to perform a set of various functions, such as: >>> >>> - Battery and charger monitoring; >>> - Keyboard layout control (i.e. fn_lock settings); >>> - USB Type-C DP alt mode HPD notifications; >>> - Laptop lid status. >>> >>> Unfortunately, while all this functionality is implemented in ACPI, it's >>> currently not possible to use ACPI to boot Linux on such Qualcomm >>> devices. To allow Linux to still support the features provided by EC, >>> this driver reimplments the relevant ACPI parts. This allows us to boot >>> the laptop with Device Tree and retain all the features. >>> >>> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >>> --- >>> drivers/platform/arm64/Kconfig | 16 + >>> drivers/platform/arm64/Makefile | 2 + >>> drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++ >> >> You should be listing yourself as a maintainer for a driver you contribute. > > I always believed that being in the AUTHOR() at the bottom of the driver > would guarantee me being in CC for patches, which so far worked great, > thus I was always hesitent adding extra entries in MAINTAINERS. There's no such rule that I'm aware of there. scripts/get_maintainer.pl won't list a driver author for the CC list This is a substantial body of code, you should own it upstream. >>> + case ASPIRE_EC_EVENT_FG_INF_CHG: >>> + /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */ >> >> fallthrough; >> > > Hm I believe this would not warn since it's just two values for the same > code, just with an extra comment inbetween? True >>> + case POWER_SUPPLY_PROP_VOLTAGE_NOW: >>> + val->intval = le16_to_cpu(ddat.voltage_now) * 1000; >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: >>> + val->intval = le16_to_cpu(sdat.voltage_design) * 1000; >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_CHARGE_NOW: >>> + val->intval = le16_to_cpu(ddat.capacity_now) * 1000; >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_CHARGE_FULL: >>> + val->intval = le16_to_cpu(sdat.capacity_full) * 1000; >>> + break; >> >> You could stick this "* 1000" stuff in a macro >> > > acpi/battery.c also explicitly sets the multiplier so I think it's the > "common" way to do this. common != nice Purely aesthetics but anyway consider decomposing the replication down. >>> + >>> + case POWER_SUPPLY_PROP_CAPACITY: >>> + val->intval = le16_to_cpu(ddat.capacity_now) * 100; >>> + val->intval /= le16_to_cpu(sdat.capacity_full); >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_CURRENT_NOW: >>> + val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000; >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_PRESENT: >>> + val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT); >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_SCOPE: >>> + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_MODEL_NAME: >>> + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model)) >>> + val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1]; >>> + else >>> + val->strval = "Unknown"; >>> + break; >>> + >>> + case POWER_SUPPLY_PROP_MANUFACTURER: >>> + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor)) >>> + val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3]; >> >> How does this -3 offset not underflow ? >> > > vendor_id here is unsigned so the if check would actually overflow, > though explaining that I guess it's better to be explicit there and let > the compiler optimize that check away anyway... I will update the if > condition with an extra (id >= 3). What's the "3" about though, that's what's not jumping out at me here. > >> Seems a bit dodgy to me - can you add a comment to the code to explain ? Its not immediately obvious the -3 is OK. >> >> Also could you take an index instead of replicating the -value stepdown each time ? >> >> int myindex = sdat.model_id - 1; >> >> if (myindex < someconstraint) >> strval = somearry[myindex]; >> > > I decided against adding a dedicated index variable since there is only > one actual use for each, so it's easy to see where it goes. But you do it twice which is why I'm suggesting take an index and do it once. Then add Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Ilpo Järvinen писал(а) 12.03.2024 17:31: > On Tue, 12 Mar 2024, Nikita Travkin wrote: > >> Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded >> controller to perform a set of various functions, such as: >> >> - Battery and charger monitoring; >> - Keyboard layout control (i.e. fn_lock settings); >> - USB Type-C DP alt mode HPD notifications; >> - Laptop lid status. >> >> Unfortunately, while all this functionality is implemented in ACPI, it's >> currently not possible to use ACPI to boot Linux on such Qualcomm >> devices. To allow Linux to still support the features provided by EC, >> this driver reimplments the relevant ACPI parts. This allows us to boot >> the laptop with Device Tree and retain all the features. >> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >> --- >> drivers/platform/arm64/Kconfig | 16 + >> drivers/platform/arm64/Makefile | 2 + >> drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++ >> 3 files changed, 573 insertions(+) >> >> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig >> index 644b83ede093..07d47879a9e3 100644 >> --- a/drivers/platform/arm64/Kconfig >> +++ b/drivers/platform/arm64/Kconfig >> @@ -16,4 +16,20 @@ menuconfig ARM64_PLATFORM_DEVICES >> >> if ARM64_PLATFORM_DEVICES >> >> +config EC_ACER_ASPIRE1 >> + tristate "Acer Aspire 1 Emedded Controller driver" >> + depends on I2C >> + depends on DRM >> + depends on POWER_SUPPLY >> + depends on INPUT >> + help >> + Say Y here to enable the EC driver for the (Snapdragon-based) >> + Acer Aspire 1 laptop. The EC handles battery and charging >> + monitoring as well as some misc functions like the lid sensor >> + and USB Type-C DP HPD events. >> + >> + This driver provides battery and AC status support for the mentioned >> + laptop where this information is not properly exposed via the >> + standard ACPI devices. >> + >> endif # ARM64_PLATFORM_DEVICES >> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile >> index f91cdc7155e2..4fcc9855579b 100644 >> --- a/drivers/platform/arm64/Makefile >> +++ b/drivers/platform/arm64/Makefile >> @@ -4,3 +4,5 @@ >> # >> # This dir should only include drivers for EC-like devices. >> # >> + >> +obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o >> diff --git a/drivers/platform/arm64/acer-aspire1-ec.c b/drivers/platform/arm64/acer-aspire1-ec.c >> new file mode 100644 >> index 000000000000..3941e24c5c7c >> --- /dev/null >> +++ b/drivers/platform/arm64/acer-aspire1-ec.c >> @@ -0,0 +1,555 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* Copyright (c) 2023, Nikita Travkin <nikita@trvn.ru> */ >> + >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/power_supply.h> >> +#include <linux/i2c.h> >> +#include <linux/input.h> >> +#include <linux/delay.h> >> + >> +#include <linux/usb/typec_mux.h> >> +#include <drm/drm_bridge.h> >> + >> +#define ASPIRE_EC_EVENT 0x05 >> + >> +#define ASPIRE_EC_EVENT_WATCHDOG 0x20 >> +#define ASPIRE_EC_EVENT_KBD_BKL_ON 0x57 >> +#define ASPIRE_EC_EVENT_KBD_BKL_OFF 0x58 >> +#define ASPIRE_EC_EVENT_LID_CLOSE 0x9b >> +#define ASPIRE_EC_EVENT_LID_OPEN 0x9c >> +#define ASPIRE_EC_EVENT_BKL_UNBLANKED 0x9d >> +#define ASPIRE_EC_EVENT_BKL_BLANKED 0x9e >> +#define ASPIRE_EC_EVENT_FG_INF_CHG 0x85 >> +#define ASPIRE_EC_EVENT_FG_STA_CHG 0xc6 >> +#define ASPIRE_EC_EVENT_HPD_DIS 0xa3 >> +#define ASPIRE_EC_EVENT_HPD_CON 0xa4 >> + >> +#define ASPIRE_EC_FG_DYNAMIC 0x07 >> +#define ASPIRE_EC_FG_STATIC 0x08 >> + >> +#define ASPIRE_EC_FG_FLAG_PRESENT BIT(0) > > Add #include <linux/bits.h> > Ack >> +#define ASPIRE_EC_FG_FLAG_FULL BIT(1) >> +#define ASPIRE_EC_FG_FLAG_DISCHARGING BIT(2) >> +#define ASPIRE_EC_FG_FLAG_CHARGING BIT(3) >> + >> +#define ASPIRE_EC_RAM_READ 0x20 >> +#define ASPIRE_EC_RAM_WRITE 0x21 >> + >> +#define ASPIRE_EC_RAM_WATCHDOG 0x19 >> +#define ASPIRE_EC_RAM_KBD_MODE 0x43 >> + >> +#define ASPIRE_EC_RAM_KBD_FN_EN BIT(0) >> +#define ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP BIT(5) >> +#define ASPIRE_EC_RAM_KBD_ALWAYS_SET BIT(6) >> +#define ASPIRE_EC_RAM_KBD_NUM_LAYER_EN BIT(7) >> + >> +#define ASPIRE_EC_RAM_KBD_MODE_2 0x60 >> + >> +#define ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY BIT(3) >> + >> +#define ASPIRE_EC_RAM_HPD_STATUS 0xf4 >> +#define ASPIRE_EC_HPD_CONNECTED 0x03 >> + >> +#define ASPIRE_EC_RAM_LID_STATUS 0x4c >> +#define ASPIRE_EC_LID_OPEN BIT(6) >> + >> +#define ASPIRE_EC_RAM_ADP 0x40 >> +#define ASPIRE_EC_AC_STATUS BIT(0) >> + >> +struct aspire_ec { >> + struct i2c_client *client; >> + struct power_supply *bat_psy; >> + struct power_supply *adp_psy; >> + struct input_dev *idev; >> + >> + bool bridge_configured; >> + struct drm_bridge bridge; >> + struct work_struct work; > > Include missing. > Will add too. >> +static int aspire_ec_ram_read(struct i2c_client *client, u8 off, u8 *data, u8 data_len) >> +{ >> + i2c_smbus_write_byte_data(client, ASPIRE_EC_RAM_READ, off); >> + i2c_smbus_read_i2c_block_data(client, ASPIRE_EC_RAM_READ, data_len, data); >> + return 0; >> +} >> + >> +static int aspire_ec_ram_write(struct i2c_client *client, u8 off, u8 data) >> +{ >> + u8 tmp[2] = {off, data}; >> + >> + i2c_smbus_write_i2c_block_data(client, ASPIRE_EC_RAM_WRITE, sizeof(tmp), tmp); >> + return 0; >> +} >> + >> +static irqreturn_t aspire_ec_irq_handler(int irq, void *data) >> +{ >> + struct aspire_ec *ec = data; >> + int id; >> + u8 tmp; >> + >> + /* >> + * The original ACPI firmware actually has a small sleep in the handler. >> + * >> + * It seems like in most cases it's not needed but when the device >> + * just exits suspend, our i2c driver has a brief time where data >> + * transfer is not possible yet. So this delay allows us to suppress >> + * quite a bunch of spurious error messages in dmesg. Thus it's kept. >> + */ >> + usleep_range(15000, 30000); >> + >> + id = i2c_smbus_read_byte_data(ec->client, ASPIRE_EC_EVENT); >> + if (id < 0) { >> + dev_err(&ec->client->dev, "Failed to read event id: %pe\n", ERR_PTR(id)); >> + return IRQ_HANDLED; >> + } >> + >> + switch (id) { >> + case 0x0: /* No event */ >> + break; >> + >> + case ASPIRE_EC_EVENT_WATCHDOG: >> + /* >> + * Here acpi responds to the event and clears some bit. >> + * Notify (\_SB.I2C3.BAT1, 0x81) // Information Change >> + * Notify (\_SB.I2C3.ADP1, 0x80) // Status Change >> + */ >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_WATCHDOG, &tmp, sizeof(tmp)); >> + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_WATCHDOG, tmp & 0xbf); > > Magic 0xbf? Is this a clear bit operation which should use > tmp & ~DEFINEDNAME > ? > Right, unfortunately I couldn't get much more from the decompiled dsdt. But given I've already called this event "watchdog" could name the bit the same too. Will change. >> + break; >> + >> + case ASPIRE_EC_EVENT_LID_CLOSE: >> + /* Notify (\_SB.LID0, 0x80) // Status Change */ >> + input_report_switch(ec->idev, SW_LID, 1); >> + input_sync(ec->idev); >> + break; >> + >> + case ASPIRE_EC_EVENT_LID_OPEN: >> + /* Notify (\_SB.LID0, 0x80) // Status Change */ >> + input_report_switch(ec->idev, SW_LID, 0); >> + input_sync(ec->idev); >> + break; >> + >> + case ASPIRE_EC_EVENT_FG_INF_CHG: >> + /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */ > > fallthrough; > > I'm not 100% sure if it's needed because you only have a comment there but > it surely doesn't hurt to have it. > Ack, given both you and Bryan think it's better to have it here, will add. >> + case ASPIRE_EC_EVENT_FG_STA_CHG: >> + /* Notify (\_SB.I2C3.BAT1, 0x80) // Status Change */ >> + power_supply_changed(ec->bat_psy); >> + power_supply_changed(ec->adp_psy); >> + break; >> + >> + case ASPIRE_EC_EVENT_HPD_DIS: >> + if (ec->bridge_configured) >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); >> + break; >> + >> + case ASPIRE_EC_EVENT_HPD_CON: >> + if (ec->bridge_configured) >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); >> + break; >> + >> + case ASPIRE_EC_EVENT_BKL_BLANKED: >> + case ASPIRE_EC_EVENT_BKL_UNBLANKED: >> + /* Display backlight blanked on FN+F6. No action needed. */ >> + break; >> + >> + case ASPIRE_EC_EVENT_KBD_BKL_ON: >> + case ASPIRE_EC_EVENT_KBD_BKL_OFF: >> + /* >> + * There is a keyboard backlight connector on Aspire 1 that is >> + * controlled by FN+F8. There is no kb backlight on the device though. >> + * Seems like this is used on other devices like Acer Spin 7. >> + * No action needed. >> + */ >> + break; >> + >> + default: >> + dev_warn(&ec->client->dev, "Unknown event id=0x%x\n", id); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +/* >> + * Power supply. >> + */ >> + >> +struct aspire_ec_bat_psy_static_data { >> + u8 unk1; >> + u8 flags; >> + __le16 unk2; >> + __le16 voltage_design; >> + __le16 capacity_full; >> + __le16 unk3; >> + __le16 serial; >> + u8 model_id; >> + u8 vendor_id; >> +} __packed; >> + >> +static const char * const aspire_ec_bat_psy_battery_model[] = { >> + "AP18C4K", >> + "AP18C8K", >> + "AP19B8K", >> + "AP16M4J", >> + "AP16M5J", >> +}; >> + >> +static const char * const aspire_ec_bat_psy_battery_vendor[] = { >> + "SANYO", >> + "SONY", >> + "PANASONIC", >> + "SAMSUNG", >> + "SIMPLO", >> + "MOTOROLA", >> + "CELXPERT", >> + "LGC", >> + "GETAC", >> + "MURATA", >> +}; >> + >> +struct aspire_ec_bat_psy_dynamic_data { >> + u8 unk1; >> + u8 flags; >> + u8 unk2; >> + __le16 capacity_now; >> + __le16 voltage_now; >> + __le16 current_now; >> + __le16 unk3; >> + __le16 unk4; >> +} __packed; > > What are the ARM64 rules when it comes to unaligned access? If it's not > always supported, you need to use get_unaligned_le16() for accessing those > __le16 fields. > Hm, you're right, arm64 spec definitely has something about alignment checks so it's better to use that even though I'm not exactly sure about exact rules. (though I guess in times like that I'd think "why can't compiler take care of this" haha) Will use it for ddat fields. >> +static int aspire_ec_bat_psy_get_property(struct power_supply *psy, >> + enum power_supply_property psp, >> + union power_supply_propval *val) >> +{ >> + struct aspire_ec *ec = power_supply_get_drvdata(psy); >> + struct aspire_ec_bat_psy_static_data sdat; >> + struct aspire_ec_bat_psy_dynamic_data ddat; >> + >> + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_STATIC, sizeof(sdat), (u8 *)&sdat); >> + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_DYNAMIC, sizeof(ddat), (u8 *)&ddat); >> + >> + switch (psp) { >> + case POWER_SUPPLY_PROP_STATUS: >> + val->intval = POWER_SUPPLY_STATUS_UNKNOWN; >> + if (ddat.flags & ASPIRE_EC_FG_FLAG_CHARGING) >> + val->intval = POWER_SUPPLY_STATUS_CHARGING; >> + else if (ddat.flags & ASPIRE_EC_FG_FLAG_DISCHARGING) >> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >> + else if (ddat.flags & ASPIRE_EC_FG_FLAG_FULL) >> + val->intval = POWER_SUPPLY_STATUS_FULL; >> + break; >> + >> + case POWER_SUPPLY_PROP_VOLTAGE_NOW: >> + val->intval = le16_to_cpu(ddat.voltage_now) * 1000; > > linux/units.h provides something that can often be used instead of 10^n > literals. > This is a conversion from Milli-Units to Micro-Units so I guess the best way to define that using existing consts would be (MICRO/MILLI) I will add '#define MILLI_TO_MICRO 1000' and use that here. >> + break; >> + >> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: >> + val->intval = le16_to_cpu(sdat.voltage_design) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_CHARGE_NOW: >> + val->intval = le16_to_cpu(ddat.capacity_now) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_CHARGE_FULL: >> + val->intval = le16_to_cpu(sdat.capacity_full) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_CAPACITY: >> + val->intval = le16_to_cpu(ddat.capacity_now) * 100; >> + val->intval /= le16_to_cpu(sdat.capacity_full); >> + break; >> + >> + case POWER_SUPPLY_PROP_CURRENT_NOW: >> + val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000; >> + break; >> + >> + case POWER_SUPPLY_PROP_PRESENT: >> + val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT); >> + break; >> + >> + case POWER_SUPPLY_PROP_SCOPE: >> + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; >> + break; >> + >> + case POWER_SUPPLY_PROP_MODEL_NAME: >> + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model)) >> + val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1]; > > Underflow handling (also here, the other place was mentioned already)? Ack, I'm going to update if for both. > >> + else >> + val->strval = "Unknown"; >> + break; >> + >> + case POWER_SUPPLY_PROP_MANUFACTURER: >> + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor)) >> + val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3]; >> + else >> + val->strval = "Unknown"; >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static enum power_supply_property aspire_ec_bat_psy_props[] = { >> + POWER_SUPPLY_PROP_STATUS, >> + POWER_SUPPLY_PROP_VOLTAGE_NOW, >> + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, >> + POWER_SUPPLY_PROP_CHARGE_NOW, >> + POWER_SUPPLY_PROP_CHARGE_FULL, >> + POWER_SUPPLY_PROP_CAPACITY, >> + POWER_SUPPLY_PROP_CURRENT_NOW, >> + POWER_SUPPLY_PROP_PRESENT, >> + POWER_SUPPLY_PROP_SCOPE, >> + POWER_SUPPLY_PROP_MODEL_NAME, >> + POWER_SUPPLY_PROP_MANUFACTURER, >> +}; >> + >> +static const struct power_supply_desc aspire_ec_bat_psy_desc = { >> + .name = "aspire-ec-bat", >> + .type = POWER_SUPPLY_TYPE_BATTERY, >> + .get_property = aspire_ec_bat_psy_get_property, >> + .properties = aspire_ec_bat_psy_props, >> + .num_properties = ARRAY_SIZE(aspire_ec_bat_psy_props), >> +}; >> + >> +static int aspire_ec_adp_psy_get_property(struct power_supply *psy, >> + enum power_supply_property psp, >> + union power_supply_propval *val) >> +{ >> + struct aspire_ec *ec = power_supply_get_drvdata(psy); >> + u8 tmp; >> + >> + switch (psp) { >> + case POWER_SUPPLY_PROP_ONLINE: >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_ADP, &tmp, sizeof(tmp)); >> + val->intval = !!(tmp & ASPIRE_EC_AC_STATUS); >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static enum power_supply_property aspire_ec_adp_psy_props[] = { >> + POWER_SUPPLY_PROP_ONLINE, >> +}; >> + >> +static const struct power_supply_desc aspire_ec_adp_psy_desc = { >> + .name = "aspire-ec-adp", >> + .type = POWER_SUPPLY_TYPE_MAINS, >> + .get_property = aspire_ec_adp_psy_get_property, >> + .properties = aspire_ec_adp_psy_props, >> + .num_properties = ARRAY_SIZE(aspire_ec_adp_psy_props), >> +}; >> + >> +/* >> + * USB-C DP Alt mode HPD. >> + */ >> + >> +static int aspire_ec_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) >> +{ >> + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; >> +} >> + >> +static void aspire_ec_bridge_update_hpd_work(struct work_struct *work) >> +{ >> + struct aspire_ec *ec = container_of(work, struct aspire_ec, work); >> + u8 tmp; >> + >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_HPD_STATUS, &tmp, sizeof(tmp)); >> + if (tmp == ASPIRE_EC_HPD_CONNECTED) >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); >> + else >> + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); >> +} >> + >> +static void aspire_ec_bridge_hpd_enable(struct drm_bridge *bridge) >> +{ >> + struct aspire_ec *ec = container_of(bridge, struct aspire_ec, bridge); >> + >> + schedule_work(&ec->work); >> +} >> + >> +static const struct drm_bridge_funcs aspire_ec_bridge_funcs = { >> + .hpd_enable = aspire_ec_bridge_hpd_enable, >> + .attach = aspire_ec_bridge_attach, >> +}; >> + >> +/* >> + * Sysfs attributes. >> + */ >> + >> +static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf) >> +{ >> + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); >> + u8 tmp; >> + >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); >> + >> + return sysfs_emit(buf, "%d\n", !(tmp & ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP)); > > %u Ack. > >> +} >> + >> +static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); >> + u8 tmp; >> + >> + bool state; >> + int ret; >> + >> + ret = kstrtobool(buf, &state); >> + if (ret) >> + return ret; >> + >> + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); >> + >> + if (state) >> + tmp &= ~ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; >> + else >> + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; >> + >> + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_KBD_MODE, tmp); >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(fn_lock); >> + >> +static struct attribute *aspire_ec_attributes[] = { >> + &dev_attr_fn_lock.attr, >> + NULL >> +}; >> + >> +static const struct attribute_group aspire_ec_attribute_group = { >> + .attrs = aspire_ec_attributes >> +}; >> + >> +static int aspire_ec_probe(struct i2c_client *client) >> +{ >> + struct power_supply_config psy_cfg = {0}; >> + struct device *dev = &client->dev; >> + struct fwnode_handle *fwnode; >> + struct aspire_ec *ec; >> + int ret; >> + u8 tmp; >> + >> + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL); >> + if (!ec) >> + return -ENOMEM; >> + >> + ec->client = client; >> + i2c_set_clientdata(client, ec); >> + >> + /* Battery status reports */ >> + psy_cfg.drv_data = ec; >> + ec->bat_psy = devm_power_supply_register(dev, &aspire_ec_bat_psy_desc, &psy_cfg); >> + if (IS_ERR(ec->bat_psy)) >> + return dev_err_probe(dev, PTR_ERR(ec->bat_psy), >> + "Failed to register battery power supply\n"); >> + >> + ec->adp_psy = devm_power_supply_register(dev, &aspire_ec_adp_psy_desc, &psy_cfg); >> + if (IS_ERR(ec->adp_psy)) >> + return dev_err_probe(dev, PTR_ERR(ec->adp_psy), >> + "Failed to register ac power supply\n"); > > AC Ack. > >> + /* Lid switch */ >> + ec->idev = devm_input_allocate_device(dev); >> + if (!ec->idev) >> + return -ENOMEM; >> + >> + ec->idev->name = "aspire-ec"; >> + ec->idev->phys = "aspire-ec/input0"; >> + input_set_capability(ec->idev, EV_SW, SW_LID); >> + >> + ret = input_register_device(ec->idev); >> + if (ret) >> + return dev_err_probe(dev, ret, "Input device register failed\n"); >> + >> + /* Enable the keyboard fn keys */ >> + tmp = ASPIRE_EC_RAM_KBD_FN_EN | ASPIRE_EC_RAM_KBD_ALWAYS_SET; >> + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; >> + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE, tmp); >> + >> + aspire_ec_ram_read(client, ASPIRE_EC_RAM_KBD_MODE_2, &tmp, sizeof(tmp)); >> + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY; >> + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE_2, tmp); >> + >> + ret = devm_device_add_group(dev, &aspire_ec_attribute_group); >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to create attribute group\n"); > > Can't you just .dev_groups ? Oh, I've missed that this exists, will use it instead. > > > In general, this felt to be in a quite good shape already, good work! Thanks for your review! Nikita
Bryan O'Donoghue писал(а) 12.03.2024 17:44: > On 12/03/2024 12:23, Nikita Travkin wrote: >> Bryan O'Donoghue писал(а) 12.03.2024 16:58: >>> On 12/03/2024 08:42, Nikita Travkin wrote: >>>> Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded >>>> controller to perform a set of various functions, such as: >>>> >>>> - Battery and charger monitoring; >>>> - Keyboard layout control (i.e. fn_lock settings); >>>> - USB Type-C DP alt mode HPD notifications; >>>> - Laptop lid status. >>>> >>>> Unfortunately, while all this functionality is implemented in ACPI, it's >>>> currently not possible to use ACPI to boot Linux on such Qualcomm >>>> devices. To allow Linux to still support the features provided by EC, >>>> this driver reimplments the relevant ACPI parts. This allows us to boot >>>> the laptop with Device Tree and retain all the features. >>>> >>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >>>> --- >>>> drivers/platform/arm64/Kconfig | 16 + >>>> drivers/platform/arm64/Makefile | 2 + >>>> drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++ >>> >>> You should be listing yourself as a maintainer for a driver you contribute. >> >> I always believed that being in the AUTHOR() at the bottom of the driver >> would guarantee me being in CC for patches, which so far worked great, >> thus I was always hesitent adding extra entries in MAINTAINERS. > > There's no such rule that I'm aware of there. > > scripts/get_maintainer.pl won't list a driver author for the CC list > > This is a substantial body of code, you should own it upstream. > Hm, ack, will add an entry in MAINTAINERS for this. >>>> + case ASPIRE_EC_EVENT_FG_INF_CHG: >>>> + /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */ >>> >>> fallthrough; >>> >> >> Hm I believe this would not warn since it's just two values for the same >> code, just with an extra comment inbetween? > > True > (Adding anyway given Ilpo also thinks it's better than not) >>>> + case POWER_SUPPLY_PROP_VOLTAGE_NOW: >>>> + val->intval = le16_to_cpu(ddat.voltage_now) * 1000; >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: >>>> + val->intval = le16_to_cpu(sdat.voltage_design) * 1000; >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_CHARGE_NOW: >>>> + val->intval = le16_to_cpu(ddat.capacity_now) * 1000; >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_CHARGE_FULL: >>>> + val->intval = le16_to_cpu(sdat.capacity_full) * 1000; >>>> + break; >>> >>> You could stick this "* 1000" stuff in a macro >>> >> >> acpi/battery.c also explicitly sets the multiplier so I think it's the >> "common" way to do this. > > common != nice > > Purely aesthetics but anyway consider decomposing the replication down. > (Adding a macro for this) >>>> + >>>> + case POWER_SUPPLY_PROP_CAPACITY: >>>> + val->intval = le16_to_cpu(ddat.capacity_now) * 100; >>>> + val->intval /= le16_to_cpu(sdat.capacity_full); >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_CURRENT_NOW: >>>> + val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000; >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_PRESENT: >>>> + val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT); >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_SCOPE: >>>> + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_MODEL_NAME: >>>> + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model)) >>>> + val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1]; >>>> + else >>>> + val->strval = "Unknown"; >>>> + break; >>>> + >>>> + case POWER_SUPPLY_PROP_MANUFACTURER: >>>> + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor)) >>>> + val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3]; >>> >>> How does this -3 offset not underflow ? >>> >> >> vendor_id here is unsigned so the if check would actually overflow, >> though explaining that I guess it's better to be explicit there and let >> the compiler optimize that check away anyway... I will update the if >> condition with an extra (id >= 3). > > What's the "3" about though, that's what's not jumping out at me here. > Ah, well... the 3 comes from a big if/elseif table in the decompiled dsdt which starts at 3... I will add a small comment near it. >> >>> Seems a bit dodgy to me - can you add a comment to the code to explain ? Its not immediately obvious the -3 is OK. >>> >>> Also could you take an index instead of replicating the -value stepdown each time ? >>> >>> int myindex = sdat.model_id - 1; >>> >>> if (myindex < someconstraint) >>> strval = somearry[myindex]; >>> >> >> I decided against adding a dedicated index variable since there is only >> one actual use for each, so it's easy to see where it goes. > > But you do it twice which is why I'm suggesting take an index and do it once. > > Then add > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Ack, given there is also >0, will add str_index variable for the offset index. Thanks! Nikita
diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig index 644b83ede093..07d47879a9e3 100644 --- a/drivers/platform/arm64/Kconfig +++ b/drivers/platform/arm64/Kconfig @@ -16,4 +16,20 @@ menuconfig ARM64_PLATFORM_DEVICES if ARM64_PLATFORM_DEVICES +config EC_ACER_ASPIRE1 + tristate "Acer Aspire 1 Emedded Controller driver" + depends on I2C + depends on DRM + depends on POWER_SUPPLY + depends on INPUT + help + Say Y here to enable the EC driver for the (Snapdragon-based) + Acer Aspire 1 laptop. The EC handles battery and charging + monitoring as well as some misc functions like the lid sensor + and USB Type-C DP HPD events. + + This driver provides battery and AC status support for the mentioned + laptop where this information is not properly exposed via the + standard ACPI devices. + endif # ARM64_PLATFORM_DEVICES diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile index f91cdc7155e2..4fcc9855579b 100644 --- a/drivers/platform/arm64/Makefile +++ b/drivers/platform/arm64/Makefile @@ -4,3 +4,5 @@ # # This dir should only include drivers for EC-like devices. # + +obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o diff --git a/drivers/platform/arm64/acer-aspire1-ec.c b/drivers/platform/arm64/acer-aspire1-ec.c new file mode 100644 index 000000000000..3941e24c5c7c --- /dev/null +++ b/drivers/platform/arm64/acer-aspire1-ec.c @@ -0,0 +1,555 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (c) 2023, Nikita Travkin <nikita@trvn.ru> */ + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/power_supply.h> +#include <linux/i2c.h> +#include <linux/input.h> +#include <linux/delay.h> + +#include <linux/usb/typec_mux.h> +#include <drm/drm_bridge.h> + +#define ASPIRE_EC_EVENT 0x05 + +#define ASPIRE_EC_EVENT_WATCHDOG 0x20 +#define ASPIRE_EC_EVENT_KBD_BKL_ON 0x57 +#define ASPIRE_EC_EVENT_KBD_BKL_OFF 0x58 +#define ASPIRE_EC_EVENT_LID_CLOSE 0x9b +#define ASPIRE_EC_EVENT_LID_OPEN 0x9c +#define ASPIRE_EC_EVENT_BKL_UNBLANKED 0x9d +#define ASPIRE_EC_EVENT_BKL_BLANKED 0x9e +#define ASPIRE_EC_EVENT_FG_INF_CHG 0x85 +#define ASPIRE_EC_EVENT_FG_STA_CHG 0xc6 +#define ASPIRE_EC_EVENT_HPD_DIS 0xa3 +#define ASPIRE_EC_EVENT_HPD_CON 0xa4 + +#define ASPIRE_EC_FG_DYNAMIC 0x07 +#define ASPIRE_EC_FG_STATIC 0x08 + +#define ASPIRE_EC_FG_FLAG_PRESENT BIT(0) +#define ASPIRE_EC_FG_FLAG_FULL BIT(1) +#define ASPIRE_EC_FG_FLAG_DISCHARGING BIT(2) +#define ASPIRE_EC_FG_FLAG_CHARGING BIT(3) + +#define ASPIRE_EC_RAM_READ 0x20 +#define ASPIRE_EC_RAM_WRITE 0x21 + +#define ASPIRE_EC_RAM_WATCHDOG 0x19 +#define ASPIRE_EC_RAM_KBD_MODE 0x43 + +#define ASPIRE_EC_RAM_KBD_FN_EN BIT(0) +#define ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP BIT(5) +#define ASPIRE_EC_RAM_KBD_ALWAYS_SET BIT(6) +#define ASPIRE_EC_RAM_KBD_NUM_LAYER_EN BIT(7) + +#define ASPIRE_EC_RAM_KBD_MODE_2 0x60 + +#define ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY BIT(3) + +#define ASPIRE_EC_RAM_HPD_STATUS 0xf4 +#define ASPIRE_EC_HPD_CONNECTED 0x03 + +#define ASPIRE_EC_RAM_LID_STATUS 0x4c +#define ASPIRE_EC_LID_OPEN BIT(6) + +#define ASPIRE_EC_RAM_ADP 0x40 +#define ASPIRE_EC_AC_STATUS BIT(0) + +struct aspire_ec { + struct i2c_client *client; + struct power_supply *bat_psy; + struct power_supply *adp_psy; + struct input_dev *idev; + + bool bridge_configured; + struct drm_bridge bridge; + struct work_struct work; +}; + +static int aspire_ec_ram_read(struct i2c_client *client, u8 off, u8 *data, u8 data_len) +{ + i2c_smbus_write_byte_data(client, ASPIRE_EC_RAM_READ, off); + i2c_smbus_read_i2c_block_data(client, ASPIRE_EC_RAM_READ, data_len, data); + return 0; +} + +static int aspire_ec_ram_write(struct i2c_client *client, u8 off, u8 data) +{ + u8 tmp[2] = {off, data}; + + i2c_smbus_write_i2c_block_data(client, ASPIRE_EC_RAM_WRITE, sizeof(tmp), tmp); + return 0; +} + +static irqreturn_t aspire_ec_irq_handler(int irq, void *data) +{ + struct aspire_ec *ec = data; + int id; + u8 tmp; + + /* + * The original ACPI firmware actually has a small sleep in the handler. + * + * It seems like in most cases it's not needed but when the device + * just exits suspend, our i2c driver has a brief time where data + * transfer is not possible yet. So this delay allows us to suppress + * quite a bunch of spurious error messages in dmesg. Thus it's kept. + */ + usleep_range(15000, 30000); + + id = i2c_smbus_read_byte_data(ec->client, ASPIRE_EC_EVENT); + if (id < 0) { + dev_err(&ec->client->dev, "Failed to read event id: %pe\n", ERR_PTR(id)); + return IRQ_HANDLED; + } + + switch (id) { + case 0x0: /* No event */ + break; + + case ASPIRE_EC_EVENT_WATCHDOG: + /* + * Here acpi responds to the event and clears some bit. + * Notify (\_SB.I2C3.BAT1, 0x81) // Information Change + * Notify (\_SB.I2C3.ADP1, 0x80) // Status Change + */ + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_WATCHDOG, &tmp, sizeof(tmp)); + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_WATCHDOG, tmp & 0xbf); + break; + + case ASPIRE_EC_EVENT_LID_CLOSE: + /* Notify (\_SB.LID0, 0x80) // Status Change */ + input_report_switch(ec->idev, SW_LID, 1); + input_sync(ec->idev); + break; + + case ASPIRE_EC_EVENT_LID_OPEN: + /* Notify (\_SB.LID0, 0x80) // Status Change */ + input_report_switch(ec->idev, SW_LID, 0); + input_sync(ec->idev); + break; + + case ASPIRE_EC_EVENT_FG_INF_CHG: + /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */ + case ASPIRE_EC_EVENT_FG_STA_CHG: + /* Notify (\_SB.I2C3.BAT1, 0x80) // Status Change */ + power_supply_changed(ec->bat_psy); + power_supply_changed(ec->adp_psy); + break; + + case ASPIRE_EC_EVENT_HPD_DIS: + if (ec->bridge_configured) + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); + break; + + case ASPIRE_EC_EVENT_HPD_CON: + if (ec->bridge_configured) + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); + break; + + case ASPIRE_EC_EVENT_BKL_BLANKED: + case ASPIRE_EC_EVENT_BKL_UNBLANKED: + /* Display backlight blanked on FN+F6. No action needed. */ + break; + + case ASPIRE_EC_EVENT_KBD_BKL_ON: + case ASPIRE_EC_EVENT_KBD_BKL_OFF: + /* + * There is a keyboard backlight connector on Aspire 1 that is + * controlled by FN+F8. There is no kb backlight on the device though. + * Seems like this is used on other devices like Acer Spin 7. + * No action needed. + */ + break; + + default: + dev_warn(&ec->client->dev, "Unknown event id=0x%x\n", id); + } + + return IRQ_HANDLED; +} + +/* + * Power supply. + */ + +struct aspire_ec_bat_psy_static_data { + u8 unk1; + u8 flags; + __le16 unk2; + __le16 voltage_design; + __le16 capacity_full; + __le16 unk3; + __le16 serial; + u8 model_id; + u8 vendor_id; +} __packed; + +static const char * const aspire_ec_bat_psy_battery_model[] = { + "AP18C4K", + "AP18C8K", + "AP19B8K", + "AP16M4J", + "AP16M5J", +}; + +static const char * const aspire_ec_bat_psy_battery_vendor[] = { + "SANYO", + "SONY", + "PANASONIC", + "SAMSUNG", + "SIMPLO", + "MOTOROLA", + "CELXPERT", + "LGC", + "GETAC", + "MURATA", +}; + +struct aspire_ec_bat_psy_dynamic_data { + u8 unk1; + u8 flags; + u8 unk2; + __le16 capacity_now; + __le16 voltage_now; + __le16 current_now; + __le16 unk3; + __le16 unk4; +} __packed; + +static int aspire_ec_bat_psy_get_property(struct power_supply *psy, + enum power_supply_property psp, + union power_supply_propval *val) +{ + struct aspire_ec *ec = power_supply_get_drvdata(psy); + struct aspire_ec_bat_psy_static_data sdat; + struct aspire_ec_bat_psy_dynamic_data ddat; + + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_STATIC, sizeof(sdat), (u8 *)&sdat); + i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_DYNAMIC, sizeof(ddat), (u8 *)&ddat); + + switch (psp) { + case POWER_SUPPLY_PROP_STATUS: + val->intval = POWER_SUPPLY_STATUS_UNKNOWN; + if (ddat.flags & ASPIRE_EC_FG_FLAG_CHARGING) + val->intval = POWER_SUPPLY_STATUS_CHARGING; + else if (ddat.flags & ASPIRE_EC_FG_FLAG_DISCHARGING) + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; + else if (ddat.flags & ASPIRE_EC_FG_FLAG_FULL) + val->intval = POWER_SUPPLY_STATUS_FULL; + break; + + case POWER_SUPPLY_PROP_VOLTAGE_NOW: + val->intval = le16_to_cpu(ddat.voltage_now) * 1000; + break; + + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: + val->intval = le16_to_cpu(sdat.voltage_design) * 1000; + break; + + case POWER_SUPPLY_PROP_CHARGE_NOW: + val->intval = le16_to_cpu(ddat.capacity_now) * 1000; + break; + + case POWER_SUPPLY_PROP_CHARGE_FULL: + val->intval = le16_to_cpu(sdat.capacity_full) * 1000; + break; + + case POWER_SUPPLY_PROP_CAPACITY: + val->intval = le16_to_cpu(ddat.capacity_now) * 100; + val->intval /= le16_to_cpu(sdat.capacity_full); + break; + + case POWER_SUPPLY_PROP_CURRENT_NOW: + val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000; + break; + + case POWER_SUPPLY_PROP_PRESENT: + val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT); + break; + + case POWER_SUPPLY_PROP_SCOPE: + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; + break; + + case POWER_SUPPLY_PROP_MODEL_NAME: + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model)) + val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1]; + else + val->strval = "Unknown"; + break; + + case POWER_SUPPLY_PROP_MANUFACTURER: + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor)) + val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3]; + else + val->strval = "Unknown"; + break; + + default: + return -EINVAL; + } + + return 0; +} + +static enum power_supply_property aspire_ec_bat_psy_props[] = { + POWER_SUPPLY_PROP_STATUS, + POWER_SUPPLY_PROP_VOLTAGE_NOW, + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, + POWER_SUPPLY_PROP_CHARGE_NOW, + POWER_SUPPLY_PROP_CHARGE_FULL, + POWER_SUPPLY_PROP_CAPACITY, + POWER_SUPPLY_PROP_CURRENT_NOW, + POWER_SUPPLY_PROP_PRESENT, + POWER_SUPPLY_PROP_SCOPE, + POWER_SUPPLY_PROP_MODEL_NAME, + POWER_SUPPLY_PROP_MANUFACTURER, +}; + +static const struct power_supply_desc aspire_ec_bat_psy_desc = { + .name = "aspire-ec-bat", + .type = POWER_SUPPLY_TYPE_BATTERY, + .get_property = aspire_ec_bat_psy_get_property, + .properties = aspire_ec_bat_psy_props, + .num_properties = ARRAY_SIZE(aspire_ec_bat_psy_props), +}; + +static int aspire_ec_adp_psy_get_property(struct power_supply *psy, + enum power_supply_property psp, + union power_supply_propval *val) +{ + struct aspire_ec *ec = power_supply_get_drvdata(psy); + u8 tmp; + + switch (psp) { + case POWER_SUPPLY_PROP_ONLINE: + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_ADP, &tmp, sizeof(tmp)); + val->intval = !!(tmp & ASPIRE_EC_AC_STATUS); + break; + + default: + return -EINVAL; + } + + return 0; +} + +static enum power_supply_property aspire_ec_adp_psy_props[] = { + POWER_SUPPLY_PROP_ONLINE, +}; + +static const struct power_supply_desc aspire_ec_adp_psy_desc = { + .name = "aspire-ec-adp", + .type = POWER_SUPPLY_TYPE_MAINS, + .get_property = aspire_ec_adp_psy_get_property, + .properties = aspire_ec_adp_psy_props, + .num_properties = ARRAY_SIZE(aspire_ec_adp_psy_props), +}; + +/* + * USB-C DP Alt mode HPD. + */ + +static int aspire_ec_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) +{ + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; +} + +static void aspire_ec_bridge_update_hpd_work(struct work_struct *work) +{ + struct aspire_ec *ec = container_of(work, struct aspire_ec, work); + u8 tmp; + + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_HPD_STATUS, &tmp, sizeof(tmp)); + if (tmp == ASPIRE_EC_HPD_CONNECTED) + drm_bridge_hpd_notify(&ec->bridge, connector_status_connected); + else + drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected); +} + +static void aspire_ec_bridge_hpd_enable(struct drm_bridge *bridge) +{ + struct aspire_ec *ec = container_of(bridge, struct aspire_ec, bridge); + + schedule_work(&ec->work); +} + +static const struct drm_bridge_funcs aspire_ec_bridge_funcs = { + .hpd_enable = aspire_ec_bridge_hpd_enable, + .attach = aspire_ec_bridge_attach, +}; + +/* + * Sysfs attributes. + */ + +static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); + u8 tmp; + + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); + + return sysfs_emit(buf, "%d\n", !(tmp & ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP)); +} + +static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); + u8 tmp; + + bool state; + int ret; + + ret = kstrtobool(buf, &state); + if (ret) + return ret; + + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_KBD_MODE, &tmp, sizeof(tmp)); + + if (state) + tmp &= ~ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; + else + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; + + aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_KBD_MODE, tmp); + + return count; +} + +static DEVICE_ATTR_RW(fn_lock); + +static struct attribute *aspire_ec_attributes[] = { + &dev_attr_fn_lock.attr, + NULL +}; + +static const struct attribute_group aspire_ec_attribute_group = { + .attrs = aspire_ec_attributes +}; + +static int aspire_ec_probe(struct i2c_client *client) +{ + struct power_supply_config psy_cfg = {0}; + struct device *dev = &client->dev; + struct fwnode_handle *fwnode; + struct aspire_ec *ec; + int ret; + u8 tmp; + + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL); + if (!ec) + return -ENOMEM; + + ec->client = client; + i2c_set_clientdata(client, ec); + + /* Battery status reports */ + psy_cfg.drv_data = ec; + ec->bat_psy = devm_power_supply_register(dev, &aspire_ec_bat_psy_desc, &psy_cfg); + if (IS_ERR(ec->bat_psy)) + return dev_err_probe(dev, PTR_ERR(ec->bat_psy), + "Failed to register battery power supply\n"); + + ec->adp_psy = devm_power_supply_register(dev, &aspire_ec_adp_psy_desc, &psy_cfg); + if (IS_ERR(ec->adp_psy)) + return dev_err_probe(dev, PTR_ERR(ec->adp_psy), + "Failed to register ac power supply\n"); + + /* Lid switch */ + ec->idev = devm_input_allocate_device(dev); + if (!ec->idev) + return -ENOMEM; + + ec->idev->name = "aspire-ec"; + ec->idev->phys = "aspire-ec/input0"; + input_set_capability(ec->idev, EV_SW, SW_LID); + + ret = input_register_device(ec->idev); + if (ret) + return dev_err_probe(dev, ret, "Input device register failed\n"); + + /* Enable the keyboard fn keys */ + tmp = ASPIRE_EC_RAM_KBD_FN_EN | ASPIRE_EC_RAM_KBD_ALWAYS_SET; + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP; + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE, tmp); + + aspire_ec_ram_read(client, ASPIRE_EC_RAM_KBD_MODE_2, &tmp, sizeof(tmp)); + tmp |= ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY; + aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE_2, tmp); + + ret = devm_device_add_group(dev, &aspire_ec_attribute_group); + if (ret) + return dev_err_probe(dev, ret, "Failed to create attribute group\n"); + + /* External Type-C display attach reports */ + fwnode = device_get_named_child_node(dev, "connector"); + if (fwnode) { + INIT_WORK(&ec->work, aspire_ec_bridge_update_hpd_work); + ec->bridge.funcs = &aspire_ec_bridge_funcs; + ec->bridge.of_node = to_of_node(fwnode); + ec->bridge.ops = DRM_BRIDGE_OP_HPD; + ec->bridge.type = DRM_MODE_CONNECTOR_USB; + + ret = devm_drm_bridge_add(dev, &ec->bridge); + if (ret) { + fwnode_handle_put(fwnode); + return dev_err_probe(dev, ret, "Failed to register drm bridge\n"); + } + + ec->bridge_configured = true; + } + + ret = devm_request_threaded_irq(dev, client->irq, NULL, + aspire_ec_irq_handler, IRQF_ONESHOT, + dev_name(dev), ec); + if (ret) + return dev_err_probe(dev, ret, "Failed to request irq\n"); + + return 0; +} + +static int aspire_ec_resume(struct device *dev) +{ + struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev)); + u8 tmp; + + aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_LID_STATUS, &tmp, sizeof(tmp)); + input_report_switch(ec->idev, SW_LID, !!(tmp & ASPIRE_EC_LID_OPEN)); + input_sync(ec->idev); + + return 0; +} + +static const struct i2c_device_id aspire_ec_id[] = { + { "aspire1-ec", }, + { } +}; +MODULE_DEVICE_TABLE(i2c, aspire_ec_id); + +static const struct of_device_id aspire_ec_of_match[] = { + { .compatible = "acer,aspire1-ec", }, + { } +}; +MODULE_DEVICE_TABLE(of, aspire_ec_of_match); + +static DEFINE_SIMPLE_DEV_PM_OPS(aspire_ec_pm_ops, NULL, aspire_ec_resume); + +static struct i2c_driver aspire_ec_driver = { + .driver = { + .name = "aspire-ec", + .of_match_table = aspire_ec_of_match, + .pm = pm_sleep_ptr(&aspire_ec_pm_ops), + }, + .probe = aspire_ec_probe, + .id_table = aspire_ec_id, +}; +module_i2c_driver(aspire_ec_driver); + +MODULE_DESCRIPTION("Acer Aspire 1 embedded controller"); +MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>"); +MODULE_LICENSE("GPL");
Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded controller to perform a set of various functions, such as: - Battery and charger monitoring; - Keyboard layout control (i.e. fn_lock settings); - USB Type-C DP alt mode HPD notifications; - Laptop lid status. Unfortunately, while all this functionality is implemented in ACPI, it's currently not possible to use ACPI to boot Linux on such Qualcomm devices. To allow Linux to still support the features provided by EC, this driver reimplments the relevant ACPI parts. This allows us to boot the laptop with Device Tree and retain all the features. Signed-off-by: Nikita Travkin <nikita@trvn.ru> --- drivers/platform/arm64/Kconfig | 16 + drivers/platform/arm64/Makefile | 2 + drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++ 3 files changed, 573 insertions(+)