Hi Andy,
On 5-Nov-24 11:39 AM, Andy Shevchenko wrote:
> 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>
Thank you for the reviews for both patches. I'll try to prepare a v2
series addressing the small remarks you had tomorrow.
Regards,
Hans
>
> ...
>
>> 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
>
