Messages in this thread | | | Subject | Re: [PATCH v2 2/2] power: supply: add CellWise cw2015 fuel gauge driver | From | Tobias Schramm <> | Date | Tue, 10 Mar 2020 19:55:42 +0100 |
| |
Hi Andy,
thanks for your review. Please find my comments inline.
>> +#define CW2015_DEFAULT_MONITOR_MS 8000 > > 8 seconds? Any comments about this? > Default suggested by CellWise Android example code. I'll add a comment explaining that.
>> +static int cw_read(struct cw_battery *cw_bat, u8 reg, u8 *val) >> +{ >> + u32 tmp; >> + int ret; >> + >> + ret = regmap_read(cw_bat->regmap, reg, &tmp); >> + *val = tmp; >> + return ret; >> +} >> + [ ... ] > > This two has no value. Why? > This helper translates the memory write size. An u8 * is passed in while regmap_read takes an unsigned int *. I'll drop it and just use an unsigned int in the first place though.
>> +static int cw_read_word(struct cw_battery *cw_bat, u8 reg, u16 *val) >> +{ >> + u8 reg_val[2]; >> + int ret; >> + >> + ret = regmap_raw_read(cw_bat->regmap, reg, reg_val, 2); >> + *val = (reg_val[0] << 8) + reg_val[1]; >> + return ret; >> +} > > NIH BE type of readings? Can REGMAP_ENDIAN_BIG help?
Not really, or can it? Registers on the cw2015 are a wild mix of single bytes and words. There does not seem to be a per register override for reg_/val_bits.
> > I didn't review some parts because of style issues. Please, fix all of them and > send v2. > I'll fix all style issues I find.
>> + cancel_delayed_work(&cw_bat->battery_delay_work); > > Are you sure you have no scheduled work after that? >
Will replace that with cancel_delayed_work_sync.
Best Regards,
Tobias
| |