Re: [PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2023-02-23 at 14:56 +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 23, 2023 at 02:24:48PM +0100, Bastien Nocera wrote:
> > This is used in a number of headsets to report the voltage of the
> > battery.
> > 
> > The voltage to capacity conversion is based on the C implementation
> > in HeadsetControl.
> 
> What is "HeadsetControl"?

It the software referenced in the comment after which I don't need a
blank line, just below this comment ;)

> 
> > 
> > Signed-off-by: Bastien Nocera <hadess@xxxxxxxxxx>
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 174
> > ++++++++++++++++++++++++++++++-
> >  1 file changed, 172 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index ff1fcebf2ec7..f6365cdf2e21 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -94,6 +94,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> >  #define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL  BIT(7)
> >  #define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL BIT(8)
> >  #define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL   BIT(9)
> > +#define HIDPP_CAPABILITY_ADC_MEASUREMENT       BIT(10)
> >  
> >  #define lg_map_key_clear(c)  hid_map_usage_clear(hi, usage, bit,
> > max, EV_KEY, (c))
> >  
> > @@ -145,6 +146,7 @@ struct hidpp_battery {
> >         u8 feature_index;
> >         u8 solar_feature_index;
> >         u8 voltage_feature_index;
> > +       u8 adc_measurement_feature_index;
> >         struct power_supply_desc desc;
> >         struct power_supply *ps;
> >         char name[64];
> > @@ -1744,6 +1746,164 @@ static int
> > hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
> >         return ret;
> >  }
> >  
> > +/* ---------------------------------------------------------------
> > ----------- */
> > +/* 0x1f20: ADC
> > measurement                                                    */
> > +/* ---------------------------------------------------------------
> > ----------- */
> > +
> > +#define HIDPP_PAGE_ADC_MEASUREMENT 0x1f20
> > +
> > +#define CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT 0x00
> > +
> > +#define EVENT_ADC_MEASUREMENT_STATUS_BROADCAST 0x00
> > +
> > +static int hidpp20_map_adc_measurement_1f20_capacity(struct
> > hid_device *hid_dev, int voltage)
> > +{
> > +       /* NB: This voltage curve doesn't necessarily map perfectly
> > to all
> > +        * devices that implement the ADC_MEASUREMENT feature. This
> > is because
> > +        * there are a few devices that use different battery
> > technology.
> > +        *
> > +        * Adapted from:
> > +        *
> > https://github.com/Sapd/HeadsetControl/blob/acd972be0468e039b93aae81221f20a54d2d60f7/src/devices/logitech_g633_g933_935.c#L44-L52
> > +        */
> > +

This is the software ^^^^^^^^^^

> No need for a blank line.

Sure.

> > +       static const int voltages[] = {
> > +               4030, 4024, 4018, 4011, 4003, 3994, 3985, 3975,
> > 3963, 3951,
> > +               3937, 3922, 3907, 3893, 3880, 3868, 3857, 3846,
> > 3837, 3828,
> > +               3820, 3812, 3805, 3798, 3791, 3785, 3779, 3773,
> > 3768, 3762,
> > +               3757, 3752, 3747, 3742, 3738, 3733, 3729, 3724,
> > 3720, 3716,
> > +               3712, 3708, 3704, 3700, 3696, 3692, 3688, 3685,
> > 3681, 3677,
> > +               3674, 3670, 3667, 3663, 3660, 3657, 3653, 3650,
> > 3646, 3643,
> > +               3640, 3637, 3633, 3630, 3627, 3624, 3620, 3617,
> > 3614, 3611,
> > +               3608, 3604, 3601, 3598, 3595, 3592, 3589, 3585,
> > 3582, 3579,
> > +               3576, 3573, 3569, 3566, 3563, 3560, 3556, 3553,
> > 3550, 3546,
> > +               3543, 3539, 3536, 3532, 3529, 3525, 3499, 3466,
> > 3433, 3399,
> > +       };
> > +
> > +       int i;
> > +
> > +       BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
> 
> Why is 100 magic?  If you want it to be 100, say so up in the
> declaraion
> so that the code will enforce that.

It's the same pattern used in hidpp20_map_battery_capacity() in the
same file. I'll send a patch to fix that too.

> 
> > +
> > +       if (voltage == 0)
> > +               return 0;
> > +
> > +       if (unlikely(voltage < 3400 || voltage >= 5000))
> 
> Why unlikely?  That should only ever be used if you can measure the
> performance impact, otherwise please remove it.
> 
> > +               hid_warn_once(hid_dev,
> > +                             "%s: possibly using the wrong voltage
> > curve\n",
> > +                             __func__);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(voltages); i++) {
> > +               if (voltage >= voltages[i])
> > +                       return ARRAY_SIZE(voltages) - i;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int hidpp20_map_adc_measurement_1f20(u8 data[3], int
> > *voltage)
> > +{
> > +       int status, flags;
> > +
> > +       flags = (int) data[2];
> 
> Why is this now an int?

Sure.

> > +
> > +       switch (flags) {
> > +       case 0x01:
> > +               status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +               break;
> > +       case 0x03:
> > +               status = POWER_SUPPLY_STATUS_CHARGING;
> > +               break;
> > +       case 0x07:
> > +               status = POWER_SUPPLY_STATUS_FULL;
> > +               break;
> > +       case 0x0F:
> > +       default:
> 
> You are only checking it for a range 1-f, u8 is just fine, right?

Sure.

> 
> > +               status = POWER_SUPPLY_STATUS_UNKNOWN;
> > +               break;
> > +       }
> > +
> > +       *voltage = get_unaligned_be16(data);
> > +
> > +       dbg_hid("%s: Parsed 1f20 data as flag 0x%02x voltage
> > %dmV\n",
> > +               __func__, flags, *voltage);
> 
> I doubt you need the __func__ line, right?  dynamic debug provides
> that
> for you automatically if you ask for it.

Removed.




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux