Messages in this thread | | | Date | Thu, 27 Feb 2014 21:18:57 +0100 | Subject | Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm | From | Linus Walleij <> |
| |
On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC <jenny.tc@intel.com> wrote:
> +static inline bool __is_battery_full > + (long volt, long cur, long iterm, unsigned long cv)
Overall I wonder if you've run checkpatch on these patches, but why are you naming this one function with a double __underscore? Just is_battery_full_check() or something would work fine I guess?
(...) > +/* Parameters defining the charging range */ > +struct psy_ps_temp_chg_table { > + /* upper temperature limit for each zone */ > + short int temp_up_lim; /* Degree Celsius */ > + > + /* charge current and voltage */ > + short int full_chrg_vol; /* mV */ > + short int full_chrg_cur; /* mA */ > + > + /* > + * Maintenance charging thresholds. > + * Maintenance charging voltage lower limit - Once battery hits full, > + * charging will be resumed when battery voltage <= this voltage > + */ > + short int maint_chrg_vol_ll; /* mV */ > + > + /* Charge current and voltage in maintenance charging mode */ > + short int maint_chrg_vol_ul; /* mV */ > + short int maint_chrg_cur; /* mA */ > +} __packed;
Why are you packing these structs? If no real reason, remove it. The compiler will pack what it thinks is appropriate anyway.
Convert all comments to kerneldoc.
> +#define BATTID_STR_LEN 8 > +#define BATT_TEMP_NR_RNG 6 > + > +struct psy_pse_chrg_prof { > + /* battery id */ > + char batt_id[BATTID_STR_LEN]; > + u16 battery_type; /* Defined as POWER_SUPPLY_TECHNOLOGY_* */
Use a named enum by patching that in <linux/power_supply.h>?
> + u16 capacity; /* mAh */ > + u16 voltage_max; /* mV */ > + /* charge termination current */ > + u16 chrg_term_mA; > + /* Low battery level voltage */ > + u16 low_batt_mV; > + /* upper and lower temperature limits on discharging */ > + s8 disch_temp_ul; /* Degree Celsius */ > + s8 disch_temp_ll; /* Degree Celsius */ > + /* number of temperature monitoring ranges */ > + u16 temp_mon_ranges; > + struct psy_ps_temp_chg_table temp_mon_range[BATT_TEMP_NR_RNG]; > + /* lowest temperature supported */ > + s8 temp_low_lim; > +} __packed;
Why packed, and convert to kerneldoc for this struct.
Yours, Linus Walleij
| |