Re: [PATCH v5] hid-logitech-hidpp: read battery voltage from newer devices

[Pedro Vanzella]

On 9/9/19 8:00 AM, Filipe LaÃns wrote:
> On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote:
> > +static int hidpp20_battery_map_status_voltage(u8 data[3], int
> > *voltage)
> > +{
> > +	int status;
> > +
> > +	switch (data[2]) {
> > +	case 0x00: /* discharging */
> > +		status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +		break;
> > +	case 0x10: /* wireless charging */
> > +	case 0x80: /* charging */
> > +		status = POWER_SUPPLY_STATUS_CHARGING;
> > +		break;
> > +	case 0x81: /* fully charged */
> > +		status = POWER_SUPPLY_STATUS_FULL;
> > +		break;
> > +	default:
> > +		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > +	}
> > +
> > +	*voltage = get_unaligned_be16(data);
> > +
> > +	return status;
> > +}
>
> Here's the missing specification:
>
> +--------+-------------------------------------------------------------------------+
> |  byte  |                                    2                                    |
> +--------+--------------+------------+------------+----------+----------+----------+
> |   bit  |     0..2     |      3     |      4     |     5    |     6    |     7    |
> +--------+--------------+------------+------------+----------+----------+----------+
> | buffer | chargeStatus | fastCharge | slowCharge | critical | (unused) | extPower |
> +--------+--------------+------------+------------+----------+----------+----------+
> Table 1 - battery voltage (0x1001), getBatteryInfo() (ASE 0), 3rd byte
>
> +-------+--------------------------------------+
> | value |                meaning               |
> +-------+--------------------------------------+
> |   0   | Charging                             |
> +-------+--------------------------------------+
> |   1   | End of charge (100% charged)         |
> +-------+--------------------------------------+
> |   2   | Charge stopped (any "normal" reason) |
> +-------+--------------------------------------+
> |   7   | Hardware error                       |
> +-------+--------------------------------------+
> Table 2 - chargeStatus value
>
> I know this is already on the 5th revision but could you please change
> hidpp20_battery_map_status_voltage() to properly handle the 3rd byte?
> Also, if you could make sure those values are properly exported via
> sysfs it would be great! We will need them for proper upower support.

Sure thing. I'll have a new patch in a few days.

Thanks for figuring that stuff out, by the way.

> Sorry for not saying this in my first review. Since then I have done
> some testing and I discovered that if we want to get accurate upower
> support we will need the values exposed by the 3rd byte to be
> exported properly.
>
> I am not sure about the endianness of chargeStatus but you can find
> that easily.
>
> Thank you!
> Filipe LaÃns