[PATCH v2 1/2] platform/x86/intel: bytcrc_pwrsrc: Optionally register a power_supply dev

[PATCH v2 1/2] platform/x86/intel: bytcrc_pwrsrc: Optionally register a power_supply dev

[Hans de Goede]

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>
---
Changes in v2:
- Adress a few small review remarks
---
 drivers/platform/x86/intel/bytcrc_pwrsrc.c | 79 +++++++++++++++++++++-
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/bytcrc_pwrsrc.c b/drivers/platform/x86/intel/bytcrc_pwrsrc.c
index 418b71af27ff..73121f77c017 100644
--- a/drivers/platform/x86/intel/bytcrc_pwrsrc.c
+++ b/drivers/platform/x86/intel/bytcrc_pwrsrc.c
@@ -8,13 +8,22 @@
  * Copyright (C) 2013 Intel Corporation
  */
 
+#include <linux/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>
 
+#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 +31,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,13 +128,60 @@ 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 device *dev = &pdev->dev;
 	struct crc_pwrsrc_data *data;
-	int ret;
+	int irq, ret;
 
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -149,6 +206,24 @@ static int crc_pwrsrc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (device_property_read_bool(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 irq;
+
+		data->psy = devm_power_supply_register(dev, &crc_pwrsrc_psy_desc, &psy_cfg);
+		if (IS_ERR(data->psy))
+			return dev_err_probe(dev, PTR_ERR(data->psy), "registering power-supply\n");
+
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						crc_pwrsrc_irq_handler,
+						IRQF_ONESHOT, KBUILD_MODNAME, data);
+		if (ret)
+			return dev_err_probe(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);
-- 
2.47.0

[PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Hans de Goede]

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
except that it expects the I2C controller to be enumerated as an ACPI
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>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Makefile tweaks
- postfix variable/define names with units (e.g. _mV / _mAh)
- Replace i2c_smbus_read_i2c_block_data() with i2c_smbus_read_block_data()
  which takes care of the length byte prefixing the buffer for us
- Adress other small review remarks
---
 .../platform/x86/x86-android-tablets/Makefile |   2 +-
 .../x86/x86-android-tablets/vexia_atla10_ec.c | 259 ++++++++++++++++++
 2 files changed, 260 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c

diff --git a/drivers/platform/x86/x86-android-tablets/Makefile b/drivers/platform/x86/x86-android-tablets/Makefile
index 41ece5a37137..313be30548bc 100644
--- a/drivers/platform/x86/x86-android-tablets/Makefile
+++ b/drivers/platform/x86/x86-android-tablets/Makefile
@@ -3,7 +3,7 @@
 # X86 Android tablet support Makefile
 #
 
+obj-$(CONFIG_X86_ANDROID_TABLETS) += vexia_atla10_ec.o
 obj-$(CONFIG_X86_ANDROID_TABLETS) += x86-android-tablets.o
-
 x86-android-tablets-y := core.o dmi.o shared-psy-info.o \
 			 asus.o lenovo.o other.o
diff --git a/drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c b/drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c
new file mode 100644
index 000000000000..07df69f6db00
--- /dev/null
+++ b/drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * power_supply class (battery) driver for the I2C attached embedded controller
+ * found on Vexia EDU ATLA 10 (9V version) tablets.
+ *
+ * This is based on the ACPI Battery device in the DSDT which should work
+ * expect that it expects the I2C controller to be enumerated as an ACPI
+ * 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).
+ *
+ * Copyright (c) 2024 Hans de Goede <hansg@kernel.org>
+ */
+
+#include <linux/bits.h>
+#include <linux/devm-helpers.h>
+#include <linux/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>
+
+/* State field uses ACPI Battery spec status bits */
+#define ACPI_BATTERY_STATE_DISCHARGING		BIT(0)
+#define ACPI_BATTERY_STATE_CHARGING		BIT(1)
+
+#define ATLA10_EC_BATTERY_STATE_COMMAND		0x87
+#define ATLA10_EC_BATTERY_INFO_COMMAND		0x88
+
+/* From broken ACPI battery device in DSDT */
+#define ATLA10_EC_VOLTAGE_MIN_DESIGN_uV		3750000
+
+struct atla10_ec_battery_state {
+	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 Celsius */
+} __packed;
+
+struct atla10_ec_battery_info {
+	__le16 charge_full_design_mAh;
+	__le16 voltage_now_mV;		/* Should be design voltage, but is not ? */
+	__le16 charge_full_design2_mAh;
+} __packed;
+
+struct atla10_ec_data {
+	struct i2c_client *client;
+	struct power_supply *psy;
+	struct delayed_work work;
+	struct mutex update_lock;
+	struct atla10_ec_battery_info info;
+	struct atla10_ec_battery_state state;
+	bool valid;			/* true if state is valid */
+	unsigned long last_update;	/* In jiffies */
+};
+
+static int atla10_ec_cmd(struct atla10_ec_data *data, u8 cmd, u8 len, u8 *values)
+{
+	struct device *dev = &data->client->dev;
+	u8 buf[32]; /* i2c_smbus_read_block_data() transfers max 32 bytes */
+	int ret;
+
+	ret = i2c_smbus_read_block_data(data->client, cmd, buf);
+	if (ret != len) {
+		dev_err(dev, "I2C command 0x%02x error: %d\n", cmd, ret);
+		return -EIO;
+	}
+
+	memcpy(values, buf, len);
+	return 0;
+}
+
+static int atla10_ec_update(struct atla10_ec_data *data)
+{
+	int ret;
+
+	/* Cache data for 5 seconds */
+	if (data->valid && time_before(jiffies, data->last_update + 5 * HZ))
+		return 0;
+
+	ret = atla10_ec_cmd(data, ATLA10_EC_BATTERY_STATE_COMMAND,
+			    sizeof(data->state), (u8 *)&data->state);
+	if (ret)
+		return ret;
+
+	data->last_update = jiffies;
+	data->valid = true;
+	return 0;
+}
+
+static int atla10_ec_psy_get_property(struct power_supply *psy,
+				      enum power_supply_property psp,
+				      union power_supply_propval *val)
+{
+	struct atla10_ec_data *data = power_supply_get_drvdata(psy);
+	int charge_now_mAh, charge_full_mAh, ret;
+
+	guard(mutex)(&data->update_lock);
+
+	ret = atla10_ec_update(data);
+	if (ret)
+		return ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (data->state.status & ACPI_BATTERY_STATE_DISCHARGING)
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		else if (data->state.status & ACPI_BATTERY_STATE_CHARGING)
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else if (data->state.capacity == 100)
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = data->state.capacity;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		/*
+		 * The EC has a bug where it reports charge-full-design as
+		 * charge-now when the battery is full. Clamp charge-now to
+		 * charge-full to workaround this.
+		 */
+		charge_now_mAh = le16_to_cpu(data->state.charge_now_mAh);
+		charge_full_mAh = le16_to_cpu(data->state.charge_full_mAh);
+		val->intval = min(charge_now_mAh, charge_full_mAh) * 1000;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = le16_to_cpu(data->state.voltage_now_mV) * 1000;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = le16_to_cpu(data->state.current_now_mA) * 1000;
+		/*
+		 * Documentation/ABI/testing/sysfs-class-power specifies
+		 * negative current for discharging.
+		 */
+		if (data->state.status & ACPI_BATTERY_STATE_DISCHARGING)
+			val->intval = -val->intval;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		val->intval = le16_to_cpu(data->state.charge_full_mAh) * 1000;
+		break;
+	case POWER_SUPPLY_PROP_TEMP:
+		val->intval = le16_to_cpu(data->state.temp) / 10;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		val->intval = le16_to_cpu(data->info.charge_full_design_mAh) * 1000;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		val->intval = ATLA10_EC_VOLTAGE_MIN_DESIGN_uV;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LIPO;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void atla10_ec_external_power_changed_work(struct work_struct *work)
+{
+	struct atla10_ec_data *data = container_of(work, struct atla10_ec_data, work.work);
+
+	dev_dbg(&data->client->dev, "External power changed\n");
+	data->valid = false;
+	power_supply_changed(data->psy);
+}
+
+static void atla10_ec_external_power_changed(struct power_supply *psy)
+{
+	struct atla10_ec_data *data = power_supply_get_drvdata(psy);
+
+	/* After charger plug in/out wait 0.5s for things to stabilize */
+	mod_delayed_work(system_wq, &data->work, HZ / 2);
+}
+
+static const enum power_supply_property atla10_ec_psy_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+};
+
+static const struct power_supply_desc atla10_ec_psy_desc = {
+	.name = "atla10_ec_battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = atla10_ec_psy_props,
+	.num_properties = ARRAY_SIZE(atla10_ec_psy_props),
+	.get_property = atla10_ec_psy_get_property,
+	.external_power_changed = atla10_ec_external_power_changed,
+};
+
+static int atla10_ec_probe(struct i2c_client *client)
+{
+	struct power_supply_config psy_cfg = { };
+	struct device *dev = &client->dev;
+	struct atla10_ec_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	psy_cfg.drv_data = data;
+	data->client = client;
+
+	ret = devm_mutex_init(dev, &data->update_lock);
+	if (ret)
+		return ret;
+
+	ret = devm_delayed_work_autocancel(dev, &data->work,
+					   atla10_ec_external_power_changed_work);
+	if (ret)
+		return ret;
+
+	ret = atla10_ec_cmd(data, ATLA10_EC_BATTERY_INFO_COMMAND,
+			    sizeof(data->info), (u8 *)&data->info);
+	if (ret)
+		return ret;
+
+	data->psy = devm_power_supply_register(dev, &atla10_ec_psy_desc, &psy_cfg);
+	return PTR_ERR_OR_ZERO(data->psy);
+}
+
+static const struct i2c_device_id atla10_ec_id_table[] = {
+	{ "vexia_atla10_ec" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, atla10_ec_id_table);
+
+static struct i2c_driver atla10_ec_driver = {
+	.driver = {
+		.name = "vexia_atla10_ec",
+	},
+	.probe = atla10_ec_probe,
+	.id_table = atla10_ec_id_table,
+};
+module_i2c_driver(atla10_ec_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("Battery driver for Vexia EDU ATLA 10 tablet EC");
+MODULE_LICENSE("GPL");
-- 
2.47.0

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Ilpo Järvinen]

On Sat, 16 Nov 2024, Hans de Goede 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
> except that it expects the I2C controller to be enumerated as an ACPI
> 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>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Makefile tweaks
> - postfix variable/define names with units (e.g. _mV / _mAh)
> - Replace i2c_smbus_read_i2c_block_data() with i2c_smbus_read_block_data()
>   which takes care of the length byte prefixing the buffer for us
> - Adress other small review remarks
> ---
>  .../platform/x86/x86-android-tablets/Makefile |   2 +-
>  .../x86/x86-android-tablets/vexia_atla10_ec.c | 259 ++++++++++++++++++
>  2 files changed, 260 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c
> 
> diff --git a/drivers/platform/x86/x86-android-tablets/Makefile b/drivers/platform/x86/x86-android-tablets/Makefile
> index 41ece5a37137..313be30548bc 100644
> --- a/drivers/platform/x86/x86-android-tablets/Makefile
> +++ b/drivers/platform/x86/x86-android-tablets/Makefile
> @@ -3,7 +3,7 @@
>  # X86 Android tablet support Makefile
>  #
>  
> +obj-$(CONFIG_X86_ANDROID_TABLETS) += vexia_atla10_ec.o
>  obj-$(CONFIG_X86_ANDROID_TABLETS) += x86-android-tablets.o
> -
>  x86-android-tablets-y := core.o dmi.o shared-psy-info.o \
>  			 asus.o lenovo.o other.o
> diff --git a/drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c b/drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c
> new file mode 100644
> index 000000000000..07df69f6db00
> --- /dev/null
> +++ b/drivers/platform/x86/x86-android-tablets/vexia_atla10_ec.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * power_supply class (battery) driver for the I2C attached embedded controller
> + * found on Vexia EDU ATLA 10 (9V version) tablets.
> + *
> + * This is based on the ACPI Battery device in the DSDT which should work
> + * expect that it expects the I2C controller to be enumerated as an ACPI
> + * 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).
> + *
> + * Copyright (c) 2024 Hans de Goede <hansg@kernel.org>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/devm-helpers.h>
> +#include <linux/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>
> +
> +/* State field uses ACPI Battery spec status bits */
> +#define ACPI_BATTERY_STATE_DISCHARGING		BIT(0)
> +#define ACPI_BATTERY_STATE_CHARGING		BIT(1)
> +
> +#define ATLA10_EC_BATTERY_STATE_COMMAND		0x87
> +#define ATLA10_EC_BATTERY_INFO_COMMAND		0x88
> +
> +/* From broken ACPI battery device in DSDT */
> +#define ATLA10_EC_VOLTAGE_MIN_DESIGN_uV		3750000



> +
> +struct atla10_ec_battery_state {
> +	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 Celsius */
> +} __packed;
> +
> +struct atla10_ec_battery_info {
> +	__le16 charge_full_design_mAh;
> +	__le16 voltage_now_mV;		/* Should be design voltage, but is not ? */
> +	__le16 charge_full_design2_mAh;
> +} __packed;

Both struct have only naturally aligned members. Why is __packed needed?

> +
> +struct atla10_ec_data {
> +	struct i2c_client *client;
> +	struct power_supply *psy;
> +	struct delayed_work work;
> +	struct mutex update_lock;
> +	struct atla10_ec_battery_info info;
> +	struct atla10_ec_battery_state state;
> +	bool valid;			/* true if state is valid */
> +	unsigned long last_update;	/* In jiffies */
> +};
> +
> +static int atla10_ec_cmd(struct atla10_ec_data *data, u8 cmd, u8 len, u8 *values)
> +{
> +	struct device *dev = &data->client->dev;
> +	u8 buf[32]; /* i2c_smbus_read_block_data() transfers max 32 bytes */

I2C_SMBUS_BLOCK_MAX ?

> +	int ret;
> +
> +	ret = i2c_smbus_read_block_data(data->client, cmd, buf);
> +	if (ret != len) {
> +		dev_err(dev, "I2C command 0x%02x error: %d\n", cmd, ret);
> +		return -EIO;
> +	}
> +
> +	memcpy(values, buf, len);
> +	return 0;
> +}
> +
> +static int atla10_ec_update(struct atla10_ec_data *data)
> +{
> +	int ret;
> +
> +	/* Cache data for 5 seconds */
> +	if (data->valid && time_before(jiffies, data->last_update + 5 * HZ))

Make a named define out of 5s and put the comment at the define.

> +		return 0;
> +
> +	ret = atla10_ec_cmd(data, ATLA10_EC_BATTERY_STATE_COMMAND,
> +			    sizeof(data->state), (u8 *)&data->state);
> +	if (ret)
> +		return ret;
> +
> +	data->last_update = jiffies;
> +	data->valid = true;
> +	return 0;
> +}
> +
> +static int atla10_ec_psy_get_property(struct power_supply *psy,
> +				      enum power_supply_property psp,
> +				      union power_supply_propval *val)
> +{
> +	struct atla10_ec_data *data = power_supply_get_drvdata(psy);
> +	int charge_now_mAh, charge_full_mAh, ret;
> +
> +	guard(mutex)(&data->update_lock);
> +
> +	ret = atla10_ec_update(data);
> +	if (ret)
> +		return ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (data->state.status & ACPI_BATTERY_STATE_DISCHARGING)
> +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +		else if (data->state.status & ACPI_BATTERY_STATE_CHARGING)
> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +		else if (data->state.capacity == 100)
> +			val->intval = POWER_SUPPLY_STATUS_FULL;
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	case POWER_SUPPLY_PROP_CAPACITY:
> +		val->intval = data->state.capacity;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +		/*
> +		 * The EC has a bug where it reports charge-full-design as
> +		 * charge-now when the battery is full. Clamp charge-now to
> +		 * charge-full to workaround this.
> +		 */
> +		charge_now_mAh = le16_to_cpu(data->state.charge_now_mAh);
> +		charge_full_mAh = le16_to_cpu(data->state.charge_full_mAh);
> +		val->intval = min(charge_now_mAh, charge_full_mAh) * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = le16_to_cpu(data->state.voltage_now_mV) * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		val->intval = le16_to_cpu(data->state.current_now_mA) * 1000;
> +		/*
> +		 * Documentation/ABI/testing/sysfs-class-power specifies
> +		 * negative current for discharging.
> +		 */
> +		if (data->state.status & ACPI_BATTERY_STATE_DISCHARGING)
> +			val->intval = -val->intval;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL:
> +		val->intval = le16_to_cpu(data->state.charge_full_mAh) * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_TEMP:
> +		val->intval = le16_to_cpu(data->state.temp) / 10;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +		val->intval = le16_to_cpu(data->info.charge_full_design_mAh) * 1000;

I find it somewhat odd we seem to have Watt and Degree prefix conversion 
defines in linux/units.h but nothing for Amps nor Volts (I was going to 
suggest use the constants there instead of literal but it seems there 
were no defines).

-- 
 i.

> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +		val->intval = ATLA10_EC_VOLTAGE_MIN_DESIGN_uV;
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = 1;
> +		break;
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LIPO;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void atla10_ec_external_power_changed_work(struct work_struct *work)
> +{
> +	struct atla10_ec_data *data = container_of(work, struct atla10_ec_data, work.work);
> +
> +	dev_dbg(&data->client->dev, "External power changed\n");
> +	data->valid = false;
> +	power_supply_changed(data->psy);
> +}
> +
> +static void atla10_ec_external_power_changed(struct power_supply *psy)
> +{
> +	struct atla10_ec_data *data = power_supply_get_drvdata(psy);
> +
> +	/* After charger plug in/out wait 0.5s for things to stabilize */
> +	mod_delayed_work(system_wq, &data->work, HZ / 2);
> +}
> +
> +static const enum power_supply_property atla10_ec_psy_props[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CHARGE_FULL,
> +	POWER_SUPPLY_PROP_TEMP,
> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +};
> +
> +static const struct power_supply_desc atla10_ec_psy_desc = {
> +	.name = "atla10_ec_battery",
> +	.type = POWER_SUPPLY_TYPE_BATTERY,
> +	.properties = atla10_ec_psy_props,
> +	.num_properties = ARRAY_SIZE(atla10_ec_psy_props),
> +	.get_property = atla10_ec_psy_get_property,
> +	.external_power_changed = atla10_ec_external_power_changed,
> +};
> +
> +static int atla10_ec_probe(struct i2c_client *client)
> +{
> +	struct power_supply_config psy_cfg = { };
> +	struct device *dev = &client->dev;
> +	struct atla10_ec_data *data;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	psy_cfg.drv_data = data;
> +	data->client = client;
> +
> +	ret = devm_mutex_init(dev, &data->update_lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_delayed_work_autocancel(dev, &data->work,
> +					   atla10_ec_external_power_changed_work);
> +	if (ret)
> +		return ret;
> +
> +	ret = atla10_ec_cmd(data, ATLA10_EC_BATTERY_INFO_COMMAND,
> +			    sizeof(data->info), (u8 *)&data->info);
> +	if (ret)
> +		return ret;
> +
> +	data->psy = devm_power_supply_register(dev, &atla10_ec_psy_desc, &psy_cfg);
> +	return PTR_ERR_OR_ZERO(data->psy);
> +}
> +
> +static const struct i2c_device_id atla10_ec_id_table[] = {
> +	{ "vexia_atla10_ec" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, atla10_ec_id_table);
> +
> +static struct i2c_driver atla10_ec_driver = {
> +	.driver = {
> +		.name = "vexia_atla10_ec",
> +	},
> +	.probe = atla10_ec_probe,
> +	.id_table = atla10_ec_id_table,
> +};
> +module_i2c_driver(atla10_ec_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_DESCRIPTION("Battery driver for Vexia EDU ATLA 10 tablet EC");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Andy Shevchenko]

On Mon, Dec 02, 2024 at 08:34:01PM +0200, Ilpo Järvinen wrote:
> On Sat, 16 Nov 2024, Hans de Goede wrote:

...

> > +struct atla10_ec_battery_state {
> > +	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 Celsius */
> > +} __packed;
> > +
> > +struct atla10_ec_battery_info {
> > +	__le16 charge_full_design_mAh;
> > +	__le16 voltage_now_mV;		/* Should be design voltage, but is not ? */
> > +	__le16 charge_full_design2_mAh;
> > +} __packed;
> 
> Both struct have only naturally aligned members. Why is __packed needed?

Wouldn't the second one give sizeof() == 8 rather than 6? Sorry, my memory
about this in C is always flaky.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Hans de Goede]

Hi,

On 2-Dec-24 7:45 PM, Andy Shevchenko wrote:
> On Mon, Dec 02, 2024 at 08:34:01PM +0200, Ilpo Järvinen wrote:
>> On Sat, 16 Nov 2024, Hans de Goede wrote:
> 
> ...
> 
>>> +struct atla10_ec_battery_state {
>>> +	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 Celsius */
>>> +} __packed;
>>> +
>>> +struct atla10_ec_battery_info {
>>> +	__le16 charge_full_design_mAh;
>>> +	__le16 voltage_now_mV;		/* Should be design voltage, but is not ? */
>>> +	__le16 charge_full_design2_mAh;
>>> +} __packed;
>>
>> Both struct have only naturally aligned members. Why is __packed needed?
> 
> Wouldn't the second one give sizeof() == 8 rather than 6? Sorry, my memory
> about this in C is always flaky.

That might be one way how things could go wrong, yes.

To answer Ilpo's original question: these structures represent
the on wire format, hence also the __le16 use and the __packed
is there to disable any possible compiler shenanigans.

I basically always add __packed to structures representing
hw memory / wire formats just to be sure.

Regards,

Hans

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Andy Shevchenko]

On Mon, Dec 2, 2024 at 11:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 2-Dec-24 7:45 PM, Andy Shevchenko wrote:
> > On Mon, Dec 02, 2024 at 08:34:01PM +0200, Ilpo Järvinen wrote:
> >> On Sat, 16 Nov 2024, Hans de Goede wrote:

...

> >>> +struct atla10_ec_battery_state {
> >>> +   u8 status;                      /* Using ACPI Battery spec status bits */
> >>> +   u8 capacity;                    /* Percent */

Then an obvious remark based on Hans' reply, why are these internal
kernel types and not external ones, i.e. __u8?

> >>> +   __le16 charge_now_mAh;
> >>> +   __le16 voltage_now_mV;
> >>> +   __le16 current_now_mA;
> >>> +   __le16 charge_full_mAh;
> >>> +   __le16 temp;                    /* centi degrees Celsius */
> >>> +} __packed;
> >>> +
> >>> +struct atla10_ec_battery_info {
> >>> +   __le16 charge_full_design_mAh;
> >>> +   __le16 voltage_now_mV;          /* Should be design voltage, but is not ? */
> >>> +   __le16 charge_full_design2_mAh;
> >>> +} __packed;
> >>
> >> Both struct have only naturally aligned members. Why is __packed needed?
> >
> > Wouldn't the second one give sizeof() == 8 rather than 6? Sorry, my memory
> > about this in C is always flaky.
>
> That might be one way how things could go wrong, yes.
>
> To answer Ilpo's original question: these structures represent
> the on wire format, hence also the __le16 use and the __packed
> is there to disable any possible compiler shenanigans.
>
> I basically always add __packed to structures representing
> hw memory / wire formats just to be sure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

On Tue, 3 Dec 2024, Andy Shevchenko wrote:

> On Mon, Dec 2, 2024 at 11:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 2-Dec-24 7:45 PM, Andy Shevchenko wrote:
> > > On Mon, Dec 02, 2024 at 08:34:01PM +0200, Ilpo Järvinen wrote:
> > >> On Sat, 16 Nov 2024, Hans de Goede wrote:
> 
> ...
> 
> > >>> +struct atla10_ec_battery_state {
> > >>> +   u8 status;                      /* Using ACPI Battery spec status bits */
> > >>> +   u8 capacity;                    /* Percent */
> 
> Then an obvious remark based on Hans' reply, why are these internal
> kernel types and not external ones, i.e. __u8?
> 
> > >>> +   __le16 charge_now_mAh;
> > >>> +   __le16 voltage_now_mV;
> > >>> +   __le16 current_now_mA;
> > >>> +   __le16 charge_full_mAh;
> > >>> +   __le16 temp;                    /* centi degrees Celsius */
> > >>> +} __packed;
> > >>> +
> > >>> +struct atla10_ec_battery_info {
> > >>> +   __le16 charge_full_design_mAh;
> > >>> +   __le16 voltage_now_mV;          /* Should be design voltage, but is not ? */
> > >>> +   __le16 charge_full_design2_mAh;
> > >>> +} __packed;
> > >>
> > >> Both struct have only naturally aligned members. Why is __packed needed?
> > >
> > > Wouldn't the second one give sizeof() == 8 rather than 6? Sorry, my memory
> > > about this in C is always flaky.
> >
> > That might be one way how things could go wrong, yes.
> >
> > To answer Ilpo's original question: these structures represent
> > the on wire format, hence also the __le16 use and the __packed
> > is there to disable any possible compiler shenanigans.
> >
> > I basically always add __packed to structures representing
> > hw memory / wire formats just to be sure.

Things will break spectacularly if such shenanigans are more than a myth.
It's not like they can change ABIs just like that without impacting real 
code. Essential networking headers such as tcp.h and ip.h do not use 
packed despite being literally wire formats. The day compiler people would 
break natural alignment means no padding, world will stop so chances of 
that happening are practically nil.

I admit though networking structs tend to be dword sized so Andy's 
point about sizeof() has more validity in it. My tests seemed to indicate 
it would be based on the alignment requirements of members within the 
struct so all __le16 would not need extra tail padding. But unlike the
natural alignment case, I've not seen the ABI/spec language about that so 
it seems acceptable in this case to use __packed, just in case.

-- 
 i.

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Hans de Goede]

Hi,

On 3-Dec-24 1:58 PM, Andy Shevchenko wrote:
> On Mon, Dec 2, 2024 at 11:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 2-Dec-24 7:45 PM, Andy Shevchenko wrote:
>>> On Mon, Dec 02, 2024 at 08:34:01PM +0200, Ilpo Järvinen wrote:
>>>> On Sat, 16 Nov 2024, Hans de Goede wrote:
> 
> ...
> 
>>>>> +struct atla10_ec_battery_state {
>>>>> +   u8 status;                      /* Using ACPI Battery spec status bits */
>>>>> +   u8 capacity;                    /* Percent */
> 
> Then an obvious remark based on Hans' reply, why are these internal
> kernel types and not external ones, i.e. __u8?

Because this is not a uapi header? 

Regards,

Hans

Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

[Andy Shevchenko]

On Tue, Dec 03, 2024 at 05:09:57PM +0100, Hans de Goede wrote:
> On 3-Dec-24 1:58 PM, Andy Shevchenko wrote:
> > On Mon, Dec 2, 2024 at 11:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 2-Dec-24 7:45 PM, Andy Shevchenko wrote:
> >>> On Mon, Dec 02, 2024 at 08:34:01PM +0200, Ilpo Järvinen wrote:
> >>>> On Sat, 16 Nov 2024, Hans de Goede wrote:

...

> >>>>> +struct atla10_ec_battery_state {
> >>>>> +   u8 status;                      /* Using ACPI Battery spec status bits */
> >>>>> +   u8 capacity;                    /* Percent */
> > 
> > Then an obvious remark based on Hans' reply, why are these internal
> > kernel types and not external ones, i.e. __u8?
> 
> Because this is not a uapi header? 

Hmm... Yes and no. The protocols (on a wire) are kinda external to the kernel
even if they are part of the kernel project as we usually define endianess
there and other things (like packing the data). The same applicable to the SW
protocols to commumicate with FW. But I haven't seen a clear documentation
on these cases and I see examples of all possible combinations of the types.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86/intel: bytcrc_pwrsrc: Optionally register a power_supply dev

[Hans de Goede]

Hi,

On 16-Nov-24 1:16 PM, Hans de Goede 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Adress a few small review remarks

I forgot to add Andy's Reviewed-by, since the changes were very
minimal and all address remarks from Andy I presume that the review
still stands:

Reviewed-by: Andy Shevchenko <andy@kernel.org>

Regards,

Hans


> ---
>  drivers/platform/x86/intel/bytcrc_pwrsrc.c | 79 +++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/bytcrc_pwrsrc.c b/drivers/platform/x86/intel/bytcrc_pwrsrc.c
> index 418b71af27ff..73121f77c017 100644
> --- a/drivers/platform/x86/intel/bytcrc_pwrsrc.c
> +++ b/drivers/platform/x86/intel/bytcrc_pwrsrc.c
> @@ -8,13 +8,22 @@
>   * Copyright (C) 2013 Intel Corporation
>   */
>  
> +#include <linux/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>
>  
> +#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 +31,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,13 +128,60 @@ 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 device *dev = &pdev->dev;
>  	struct crc_pwrsrc_data *data;
> -	int ret;
> +	int irq, ret;
>  
> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -149,6 +206,24 @@ static int crc_pwrsrc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	if (device_property_read_bool(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 irq;
> +
> +		data->psy = devm_power_supply_register(dev, &crc_pwrsrc_psy_desc, &psy_cfg);
> +		if (IS_ERR(data->psy))
> +			return dev_err_probe(dev, PTR_ERR(data->psy), "registering power-supply\n");
> +
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						crc_pwrsrc_irq_handler,
> +						IRQF_ONESHOT, KBUILD_MODNAME, data);
> +		if (ret)
> +			return dev_err_probe(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);

Re: [PATCH v2 1/2] platform/x86/intel: bytcrc_pwrsrc: Optionally register a power_supply dev

[Andy Shevchenko]

On Sat, Nov 16, 2024 at 5:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 16-Nov-24 1:16 PM, Hans de Goede 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.

...

> > Changes in v2:
> > - Adress a few small review remarks
>
> I forgot to add Andy's Reviewed-by, since the changes were very
> minimal and all address remarks from Andy I presume that the review
> still stands:
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>

Correct.

-- 
With Best Regards,
Andy Shevchenko