| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 27 Feb 2014 21:08:01 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Jenny TC <jenny.tc@...el.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
Anton Vorontsov <cbouatmailru@...il.com>,
Anton Vorontsov <anton.vorontsov@...aro.org>,
Kim Milo <Milo.Kim@...com>, Lee Jones <lee.jones@...aro.org>,
Jingoo Han <jg1.han@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Sachin Kamat <sachin.kamat@...aro.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Pali Rohár <pali.rohar@...il.com>,
Rhyland Klein <rklein@...dia.com>, Pavel Machek <pavel@....cz>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
David Woodhouse <dwmw2@...radead.org>,
Tony Lindgren <tony@...mide.com>,
Russell King <linux@....linux.org.uk>,
Sebastian Reichel <sre@...g0.de>,
Aaro Koskinen <aaro.koskinen@....fi>,
Pallala Ramakrishna <ramakrishna.pallala@...el.com>,
Ивайло Димитров <freemangordon@....bg>,
Linux-OMAP <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Thu, Feb 20, 2014 at 6:53 AM, Jenny TC <jenny.tc@...el.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?
> +/*
> +* Define a TTL for some properties to optimize the frequency of
> +* algorithm calls. This can be used by properties which will be changed
> +* very frequently (e.g. Current, Voltage..)
> +*/
> +#define PROP_TTL (HZ*10)
Same comment.
> +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)
> +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?
> +
> +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?
(...)
> +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.
> + unsigned long long tstamp;
> + enum psy_algo_stat algo_stat;
> + int health; /* POWER_SUPPLY_HEALTH_* */
Same thing.
> +struct psy_charger_props {
> + struct list_head node;
> + struct power_supply_charger *psyc;
> + const char *name;
> + bool present;
> + bool is_charging;
> + int health; /* POWER_SUPPLY_HEALTH_* */
Same thing.
> + bool online;
> + unsigned long cable;
> + unsigned long tstamp;
> +};
Kerneldoc this struct...
> +
> +struct psy_batt_thresholds {
> + int temp_min; /* Degree Celsius */
> + int temp_max; /* Degree Celsius */
> + unsigned int iterm; /* mA */
> +};
Kerneldoc this struct.
> +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.
> +struct psy_charging_algo {
> + struct list_head node;
> + unsigned int chrg_prof_type;
> + char *name;
> + enum psy_algo_stat (*get_next_cc_cv)(struct psy_batt_props,
> + struct psy_batt_chrg_prof, unsigned long *cc,
> + unsigned long *cv);
> + int (*get_batt_thresholds)(struct psy_batt_chrg_prof,
> + struct psy_batt_thresholds *bat_thr);
> +};
And this.
> +/* 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.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists