On Mon, Nov 4, 2024 at 10:36 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx>
...
> 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
