Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm

[Jenny Tc <jenny.tc@intel.com>]

On Thu, Feb 27, 2014 at 09:18:57PM +0100, Linus Walleij wrote:
> 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?

Just to convey that is_battery_full is a local function and not generic. You
can find similar usage in power_supply_core.c (__power_supply_changed_work)
and in other drivers. Isn't it advised to have __ prefixes?
> (...)
> > +/* 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.

The structure is part of the  battery charging profile which can be read directly
from an EEPROM or from secondary storage (emmc). So it should be packed to keep
it align with the stored format.

> > +#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>?

This is to convey that battery_type takes value as defined by
POWER_SUPPLY_TECHNOLOGY_*
> 
> > +       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.

Battery charging profile, may come from EEPROM/emmc which would be packed.