Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver

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

On Thu, Feb 27, 2014 at 09:08:01PM +0100, Linus Walleij wrote:
> On Thu, Feb 20, 2014 at 6:53 AM, Jenny TC <jenny.tc@intel.com> wrote:
> 
> > +++ b/include/linux/power/power_supply_charger.h
> 
> > +#define MAX_CUR_VOLT_SAMPLES 3
> > +#define DEF_CUR_VOLT_SAMPLE_JIFF (30*HZ)
> 
> Why are things defined in Jiffies like this insead of seconds, milliseconds
> etc? This will vary with the current operating frequency of the system,
> why should physical measurements do that?

Is it fine if I use msecs_to_jiffies(30000)?

> > +enum psy_charger_cable_event {
> > +       PSY_CHARGER_CABLE_EVENT_DISCONNECT = 0,
> > +       PSY_CHARGER_CABLE_EVENT_CONNECT,
> > +       PSY_CHARGER_CABLE_EVENT_UPDATE,
> > +       PSY_CHARGER_CABLE_EVENT_RESUME,
> > +       PSY_CHARGER_CABLE_EVENT_SUSPEND,
> > +};
> > +
> > +enum psy_charger_cable_type {
> > +       PSY_CHARGER_CABLE_TYPE_NONE = 0,
> > +       PSY_CHARGER_CABLE_TYPE_USB_SDP = 1 << 0,
> > +       PSY_CHARGER_CABLE_TYPE_USB_DCP = 1 << 1,
> > +       PSY_CHARGER_CABLE_TYPE_USB_CDP = 1 << 2,
> > +       PSY_CHARGER_CABLE_TYPE_USB_ACA = 1 << 3,
> > +       PSY_CHARGER_CABLE_TYPE_AC = 1 << 4,
> > +       PSY_CHARGER_CABLE_TYPE_ACA_DOCK = 1 << 5,
> > +       PSY_CHARGER_CABLE_TYPE_ACA_A = 1 << 6,
> > +       PSY_CHARGER_CABLE_TYPE_ACA_B = 1 << 7,
> > +       PSY_CHARGER_CABLE_TYPE_ACA_C = 1 << 8,
> > +       PSY_CHARGER_CABLE_TYPE_SE1 = 1 << 9,
> > +       PSY_CHARGER_CABLE_TYPE_MHL = 1 << 10,
> > +       PSY_CHARGER_CABLE_TYPE_B_DEVICE = 1 << 11,
> > +};
> 
> Why is this even an enum? It is clearly bitfields. I would just:
> 
> #include <linux/bitops.h>
> 
> #define PSY_CHARGER_CABLE_TYPE_NONE 0x0
> #define PSY_CHARGER_CABLE_TYPE_USB_SDP BIT(0)
> #define PSY_CHARGER_CABLE_TYPE_USB_DCP BIT(1)
> (etc)

This is to ensure type checks when the cable types are handled, #defines will
not help in type checks. 

> 
> > +enum {
> > + POWER_SUPPLY_BATTERY_REMOVED = 0,
> > + POWER_SUPPLY_BATTERY_INSERTED,
> > +};
> 
> Why is this enum anonymous? Does that mean the code just
> casts the enum to an int?

OK.I'll name the enum.
> 
> > +
> > +struct psy_cable_props {
> > +       enum psy_charger_cable_event    chrg_evt;
> > +       enum psy_charger_cable_type     chrg_type;
> > +       unsigned int                    mA;     /* input current limit */
> 
> You are naming a struct member after a unit, can it not
> be given a better name like "current_limit" and write in the
> kerneldoc (not a comment) that it is stated in mA?

I'll change the variable name in next patch set.
> > +struct psy_batt_props {
> > +       struct list_head node;
> > +       const char *name;
> > +       long voltage_now; /* mV */
> > +       long voltage_now_cache[MAX_CUR_VOLT_SAMPLES]; /* mV */
> > +       long current_now; /* mA */
> > +       long current_now_cache[MAX_CUR_VOLT_SAMPLES]; /* mV */
> > +       int temperature; /* Degree Celsius */
> > +       long status; /* POWER_SUPPLY_STATUS_* */
> 
> I don't understand these comments... Do you mean you are
> using the enums from <linux/power_supply.h>?
>
> Would it not be better to give those enums a real name
> (as a separate patch) and then use:
> 
> enum power_supply_status status;
> 
> here? That would be helpful methinks.

My intention is to convey that status variable takes values
POWER_SUPPLY_STATUS_*. I'll submit a separate patch to name the enums in
power_supply.h. Also I'll make the appropriate changes in power_supply_charger.h
> > +struct power_supply_charger {
> > +       struct power_supply *psy;
> > +       struct psy_throttle_state *throttle_states;
> > +       size_t num_throttle_states;
> > +       unsigned long supported_cables;
> > +       int (*get_property)(struct power_supply_charger *psyc,
> > +                           enum power_supply_charger_property psp,
> > +                           union power_supply_propval *val);
> > +       int (*set_property)(struct power_supply_charger *psyc,
> > +                           enum power_supply_charger_property psp,
> > +                           const union power_supply_propval *val);
> > +       int (*property_is_writeable)(struct power_supply_charger *psyc,
> > +                                    enum power_supply_charger_property psp);
> > +};
> 
> Kerneldoc this vtable struct.

I'll make the necessary kerneldoc changes as you suggested for this structure
and other structures.
> 
> > +/* power_supply_charger functions */
> > +
> > +#ifdef CONFIG_POWER_SUPPLY_CHARGER
> > +
> > +extern int power_supply_register_charger(struct power_supply_charger *psyc);
> > +extern int power_supply_unregister_charger(struct power_supply_charger *psyc);
> > +extern int power_supply_register_charging_algo(struct psy_charging_algo *);
> > +extern int power_supply_unregister_charging_algo(struct psy_charging_algo *);
> > +extern int psy_get_battery_prop(struct psy_batt_chrg_prof *batt_prop);
> > +extern void psy_battery_prop_changed(int battery_conn_stat,
> > +                               struct psy_batt_chrg_prof *batt_prop);
> > +
> > +#else
> > +
> > +static int power_supply_register_charger(struct power_supply_charger *psyc)
> > +{ return 0; }
> > +static  int power_supply_unregister_charger(struct power_supply_charger *psyc)
> > +{ return 0; }
> > +static int power_supply_register_charging_algo(struct psy_charging_algo *algo)
> > +{ return 0; }
> > +static int power_supply_unregister_charging_algo(struct psy_charging_algo *algo)
> > +{ return 0; }
> 
> Why do these return 0? Should they not just fail if the power supply
> charger support is not compiled in, like return -EINVAL etc?
> 
> Sorry for just making some random review of the header files, but
> this caught my attention and I couldn't resist.

No issues, it make sense to return error instead of 0. Thanks for the comments.

-Jenny