| Message ID | 20241104203555.61104-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] platform/x86/intel: bytcrc_pwrsrc: Optionally register a power_supply dev | expand |
On Mon, Nov 4, 2024 at 10:36 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On some Android tablets with Crystal Cove PMIC the DSDT lacks an ACPI AC > device to indicate whether a charger is plugged in or not. > > Add support for registering a "crystal_cove_pwrsrc" power_supply class > device to indicate charger online status. This is made conditional on > a "linux,register-pwrsrc-power_supply" boolean device-property to avoid > registering a duplicate power_supply class device on devices where this > is already handled by an ACPI AC device. > > Note the "linux,register-pwrsrc-power_supply" property is only used on > x86/ACPI (non devicetree) devs and the devicetree-bindings maintainers > have requested properties like these to not be added to the devicetree > bindings, so the new property is deliberately not added to any bindings. Reviewed-by: Andy Shevchenko <andy@kernel.org> ... + array_size.h > +#include <linux/bits.h> > #include <linux/debugfs.h> > +#include <linux/interrupt.h> > #include <linux/mfd/intel_soc_pmic.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <linux/property.h> > #include <linux/regmap.h> ... > + if (device_property_read_bool(pdev->dev.parent, "linux,register-pwrsrc-power_supply")) { Btw, is that property type of boolean? If not, device_property_present() has to be used. ... > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return dev_err_probe(&pdev->dev, irq, "getting IRQ\n"); This dups the embedded error message. ... > + data->psy = devm_power_supply_register(&pdev->dev, &crc_pwrsrc_psy_desc, &psy_cfg); > + if (IS_ERR(data->psy)) > + return dev_err_probe(&pdev->dev, PTR_ERR(data->psy), > + "registering power-supply\n"); > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, > + crc_pwrsrc_irq_handler, > + IRQF_ONESHOT, KBUILD_MODNAME, data); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "requesting IRQ\n"); With struct device *dev = &pdev->dev; at the top of the function you may make lines shorten and neater. > + }
On Mon, Nov 4, 2024 at 10:36 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The Vexia EDU ATLA 10 tablet has an embedded controller instead of > giving the os direct access to the charger + fuel-gauge ICs as is normal > on tablets designed for Android. > > There is ACPI Battery device in the DSDT using the EC which should work > expect that it expects the I2C controller to be enumerated as an ACPI expect --> except > device and the tablet's BIOS enumerates all LPSS devices as PCI devices > (and changing the LPSS BIOS settings from PCI -> ACPI does not work). > > Add a power_supply class driver for the Atla 10 EC to expert battery info > to userspace. This is made part of the x86-android-tablets directory and > Kconfig option because the i2c_client it binds to is instantiated by > the x86-android-tablets kmod. Reviewed-by: Andy Shevchenko <andy@kernel.org> ... > obj-$(CONFIG_X86_ANDROID_TABLETS) += x86-android-tablets.o > +obj-$(CONFIG_X86_ANDROID_TABLETS) += vexia_atla10_ec.o This splits the original (compound) object lines, please move it either before (and this seems even better with ordering by name in mind) or after this block. > Actually this blank line gives the false impression that the originally two lines are not related. I would drop this blank line as well. > x86-android-tablets-y := core.o dmi.o shared-psy-info.o \ > asus.o lenovo.o other.o ... > +#include <linux/bits.h> > +#include <linux/devm-helpers.h> + err.h > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/power_supply.h> > +#include <linux/types.h> > +#include <linux/workqueue.h> > + > +#include <asm/byteorder.h> ... > +/* From broken ACPI battery device in DSDT */ > +#define ATLA10_EC_VOLTAGE_MIN_DESIGN 3750000 _uV ? ... > +struct atla10_ec_battery_state { > + u8 len; /* Struct length excluding the len field, always 12 */ > + u8 status; /* Using ACPI Battery spec status bits */ > + u8 capacity; /* Percent */ > + __le16 charge_now; /* mAh */ > + __le16 voltage_now; /* mV */ > + __le16 current_now; /* mA */ > + __le16 charge_full; /* mAh */ > + __le16 temp; /* centi degrees celcius */ Celsius / celsius > +} __packed; > + > +struct atla10_ec_battery_info { > + u8 len; /* Struct length excluding the len field, always 6 */ > + __le16 charge_full_design; /* mAh */ > + __le16 voltage_now; /* mV, should be design voltage, but is not ? */ > + __le16 charge_full_design2; /* mAh */ > +} __packed; Instead I would add the respective units to the variable names: _mAh _mV ...etc. (* yes, with the capital letters to follow the proper spelling) ... > +static int atla10_ec_cmd(struct atla10_ec_data *data, u8 cmd, u8 len, u8 *values) > +{ > + struct device *dev = &data->client->dev; > + int ret; > + > + ret = i2c_smbus_read_i2c_block_data(data->client, cmd, len, values); > + if (ret != len) { > + dev_err(dev, "I2C command 0x%02x error: %d\n", cmd, ret); > + return -EIO; > + } > + if (values[0] != (len - 1)) { Hmm... AFAIU this is part of SMBus protocol. Why do we need to care about this? Or is this an additional header on top of that? > + dev_err(dev, "I2C command 0x%02x header length mismatch expected %u got %u\n", > + cmd, len - 1, values[0]); > + return -EIO; > + } > + > + return 0; > +} ... > + val->intval = min(charge_now, charge_full) * 1000; MILLI (here and below)? > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = le16_to_cpu(data->state.voltage_now) * 1000; > + break; > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + val->intval = le16_to_cpu(data->state.current_now) * 1000; > + /* > + * Documentation/ABI/testing/sysfs-class-power specifies > + * negative current for discharing. discharging > + */ -- With Best Regards, Andy Shevchenko
diff --git a/drivers/platform/x86/intel/bytcrc_pwrsrc.c b/drivers/platform/x86/intel/bytcrc_pwrsrc.c index 418b71af27ff..98b5057d4c68 100644 --- a/drivers/platform/x86/intel/bytcrc_pwrsrc.c +++ b/drivers/platform/x86/intel/bytcrc_pwrsrc.c @@ -8,13 +8,21 @@ * Copyright (C) 2013 Intel Corporation */ +#include <linux/bits.h> #include <linux/debugfs.h> +#include <linux/interrupt.h> #include <linux/mfd/intel_soc_pmic.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/power_supply.h> +#include <linux/property.h> #include <linux/regmap.h> +#define CRYSTALCOVE_PWRSRC_IRQ 0x03 #define CRYSTALCOVE_SPWRSRC_REG 0x1E +#define CRYSTALCOVE_SPWRSRC_USB BIT(0) +#define CRYSTALCOVE_SPWRSRC_DC BIT(1) +#define CRYSTALCOVE_SPWRSRC_BATTERY BIT(2) #define CRYSTALCOVE_RESETSRC0_REG 0x20 #define CRYSTALCOVE_RESETSRC1_REG 0x21 #define CRYSTALCOVE_WAKESRC_REG 0x22 @@ -22,6 +30,7 @@ struct crc_pwrsrc_data { struct regmap *regmap; struct dentry *debug_dentry; + struct power_supply *psy; unsigned int resetsrc0; unsigned int resetsrc1; unsigned int wakesrc; @@ -118,11 +127,57 @@ static int crc_pwrsrc_read_and_clear(struct crc_pwrsrc_data *data, return regmap_write(data->regmap, reg, *val); } +static irqreturn_t crc_pwrsrc_irq_handler(int irq, void *_data) +{ + struct crc_pwrsrc_data *data = _data; + unsigned int irq_mask; + + if (regmap_read(data->regmap, CRYSTALCOVE_PWRSRC_IRQ, &irq_mask)) + return IRQ_NONE; + + regmap_write(data->regmap, CRYSTALCOVE_PWRSRC_IRQ, irq_mask); + + power_supply_changed(data->psy); + return IRQ_HANDLED; +} + +static int crc_pwrsrc_psy_get_property(struct power_supply *psy, + enum power_supply_property psp, + union power_supply_propval *val) +{ + struct crc_pwrsrc_data *data = power_supply_get_drvdata(psy); + unsigned int pwrsrc; + int ret; + + if (psp != POWER_SUPPLY_PROP_ONLINE) + return -EINVAL; + + ret = regmap_read(data->regmap, CRYSTALCOVE_SPWRSRC_REG, &pwrsrc); + if (ret) + return ret; + + val->intval = !!(pwrsrc & (CRYSTALCOVE_SPWRSRC_USB | + CRYSTALCOVE_SPWRSRC_DC)); + return 0; +} + +static const enum power_supply_property crc_pwrsrc_psy_props[] = { + POWER_SUPPLY_PROP_ONLINE, +}; + +static const struct power_supply_desc crc_pwrsrc_psy_desc = { + .name = "crystal_cove_pwrsrc", + .type = POWER_SUPPLY_TYPE_MAINS, + .properties = crc_pwrsrc_psy_props, + .num_properties = ARRAY_SIZE(crc_pwrsrc_psy_props), + .get_property = crc_pwrsrc_psy_get_property, +}; + static int crc_pwrsrc_probe(struct platform_device *pdev) { struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent); struct crc_pwrsrc_data *data; - int ret; + int irq, ret; data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); if (!data) @@ -149,6 +204,25 @@ static int crc_pwrsrc_probe(struct platform_device *pdev) if (ret) return ret; + if (device_property_read_bool(pdev->dev.parent, "linux,register-pwrsrc-power_supply")) { + struct power_supply_config psy_cfg = { .drv_data = data }; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return dev_err_probe(&pdev->dev, irq, "getting IRQ\n"); + + data->psy = devm_power_supply_register(&pdev->dev, &crc_pwrsrc_psy_desc, &psy_cfg); + if (IS_ERR(data->psy)) + return dev_err_probe(&pdev->dev, PTR_ERR(data->psy), + "registering power-supply\n"); + + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + crc_pwrsrc_irq_handler, + IRQF_ONESHOT, KBUILD_MODNAME, data); + if (ret) + return dev_err_probe(&pdev->dev, ret, "requesting IRQ\n"); + } + data->debug_dentry = debugfs_create_dir(KBUILD_MODNAME, NULL); debugfs_create_file("pwrsrc", 0444, data->debug_dentry, data, &pwrsrc_fops); debugfs_create_file("resetsrc", 0444, data->debug_dentry, data, &resetsrc_fops);
On some Android tablets with Crystal Cove PMIC the DSDT lacks an ACPI AC device to indicate whether a charger is plugged in or not. Add support for registering a "crystal_cove_pwrsrc" power_supply class device to indicate charger online status. This is made conditional on a "linux,register-pwrsrc-power_supply" boolean device-property to avoid registering a duplicate power_supply class device on devices where this is already handled by an ACPI AC device. Note the "linux,register-pwrsrc-power_supply" property is only used on x86/ACPI (non devicetree) devs and the devicetree-bindings maintainers have requested properties like these to not be added to the devicetree bindings, so the new property is deliberately not added to any bindings. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/intel/bytcrc_pwrsrc.c | 76 +++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-)