Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Thu, Mar 13, 2014 at 10:12 AM, Pavel Machek pa...@ucw.cz wrote: Hi! 30*HZ means 30 seconds in the kernel... what is hard to understand about it? Well I might be picky, but since it is a charging algorithm dealing with ampères, volts, constant-current/constant-voltage, watchdogs and timeouts, all stated in SI units, it would be nice if all such constants were specified in simple units instead of kernel-specific terms. I agree HZ is badly named, but hopefully anyone working on kernel is already familiar with it. But... what would actually help: I believe we should introduce milivolt_t, miliamp_t, milisec_t etc... types. Storing milivolts in int, then having comment saying milivolts is just wrong. Hm! I bet the regulator subsystem maintainers have opinions on that. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Fri, Mar 14, 2014 at 11:36:40AM +0100, Linus Walleij wrote: On Thu, Mar 13, 2014 at 10:12 AM, Pavel Machek pa...@ucw.cz wrote: But... what would actually help: I believe we should introduce milivolt_t, miliamp_t, milisec_t etc... types. Storing milivolts in int, then having comment saying milivolts is just wrong. Hm! I bet the regulator subsystem maintainers have opinions on that. The regulator subsystem is quite happily using unsigned ints here, usually we're using hungarian style names to clarify the type where that's useful (it tends to be closer to the usual naming conventions when representing these things mathematically). I'm not sure that having a typedef is going to buy us terribly much unless we convert the kernel to C++ where we can do more exciting stuff. I would say that we are using microvolts rather than milivolts since it does occasionally matter. signature.asc Description: Digital signature
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
Hi! 30*HZ means 30 seconds in the kernel... what is hard to understand about it? Well I might be picky, but since it is a charging algorithm dealing with ampères, volts, constant-current/constant-voltage, watchdogs and timeouts, all stated in SI units, it would be nice if all such constants were specified in simple units instead of kernel-specific terms. I agree HZ is badly named, but hopefully anyone working on kernel is already familiar with it. But... what would actually help: I believe we should introduce milivolt_t, miliamp_t, milisec_t etc... types. Storing milivolts in int, then having comment saying milivolts is just wrong. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Fri, Mar 7, 2014 at 9:10 PM, Pavel Machek pa...@ucw.cz wrote: On Fri 2014-03-07 11:04:59, Linus Walleij wrote: On Fri, Feb 28, 2014 at 6:01 PM, Pavel Machek pa...@ucw.cz wrote: On Thu 2014-02-27 21:08:01, Linus Walleij wrote: On Thu, Feb 20, 2014 at 6:53 AM, Jenny TC jenny...@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? It is actually ok. The define is relative to jiffies, and that's what interface expects. So consider the option that the interface is wrong. Stating something like a sample period in system-specific jiffies instead of period time T is just weird. What control systems guy would understand this? 30*HZ means 30 seconds in the kernel... what is hard to understand about it? Well I might be picky, but since it is a charging algorithm dealing with ampères, volts, constant-current/constant-voltage, watchdogs and timeouts, all stated in SI units, it would be nice if all such constants were specified in simple units instead of kernel-specific terms. One reason is that this kind of code actually needs review from non-programmers, people like chemists and physicists. I know it may be far fetched so no strong preference for sure. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
Hi! +++ 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(3)? Keep the #define DEF_CUR_VOLT_SAMPLE_PERIOD 3 Then use msecs_to_jiffies(DEF_CUR_VOLT_SAMPLE_PERIOD) in the call site. Actually, I do not think this is good suggestion. 30*HZ -- is pretty clearly 30 seconds. 3 may be 30sec or 30msec ... hard to tell. If the constant is used in just one place, maybe you don't need the define? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Fri 2014-03-07 11:04:59, Linus Walleij wrote: On Fri, Feb 28, 2014 at 6:01 PM, Pavel Machek pa...@ucw.cz wrote: On Thu 2014-02-27 21:08:01, Linus Walleij wrote: On Thu, Feb 20, 2014 at 6:53 AM, Jenny TC jenny...@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? It is actually ok. The define is relative to jiffies, and that's what interface expects. So consider the option that the interface is wrong. Stating something like a sample period in system-specific jiffies instead of period time T is just weird. What control systems guy would understand this? 30*HZ means 30 seconds in the kernel... what is hard to understand about it? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Fri, Feb 28, 2014 at 12:27 PM, Jenny Tc jenny...@intel.com wrote:
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...@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(3)?
Keep the
#define DEF_CUR_VOLT_SAMPLE_PERIOD 3
Then use msecs_to_jiffies(DEF_CUR_VOLT_SAMPLE_PERIOD)
in the call site.
+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.
Type checks with static code check tools? But misrepresenting
a bitfield as an enum just to satisfy a static code checker is not
OK IMO.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Fri, Feb 28, 2014 at 6:01 PM, Pavel Machek pa...@ucw.cz wrote: On Thu 2014-02-27 21:08:01, Linus Walleij wrote: On Thu, Feb 20, 2014 at 6:53 AM, Jenny TC jenny...@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? It is actually ok. The define is relative to jiffies, and that's what interface expects. So consider the option that the interface is wrong. Stating something like a sample period in system-specific jiffies instead of period time T is just weird. What control systems guy would understand this? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Fri, Mar 07, 2014 at 11:03:02AM +0800, Linus Walleij wrote:
On Fri, Feb 28, 2014 at 12:27 PM, Jenny Tc jenny...@intel.com wrote:
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...@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(3)?
Keep the
#define DEF_CUR_VOLT_SAMPLE_PERIOD 3
Then use msecs_to_jiffies(DEF_CUR_VOLT_SAMPLE_PERIOD)
in the call site.
Ok..fine will fix it in next patch set
+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.
Type checks with static code check tools? But misrepresenting
a bitfield as an enum just to satisfy a static code checker is not
OK IMO.
not just for tools, compile time type checks also.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Thu 2014-02-27 21:08:01, Linus Walleij wrote: On Thu, Feb 20, 2014 at 6:53 AM, Jenny TC jenny...@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? It is actually ok. The define is relative to jiffies, and that's what interface expects. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Thu, Feb 20, 2014 at 6:53 AM, Jenny TC jenny...@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?
+/*
+* 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_eventchrg_evt;
+ enum psy_charger_cable_type chrg_type;
+ unsigned intmA; /* 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
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
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...@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(3)?
+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_eventchrg_evt;
+ enum psy_charger_cable_type chrg_type;
+ unsigned intmA; /* 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
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Wed 2014-02-05 13:44:58, Jenny Tc wrote:
On Tue, Feb 04, 2014 at 12:36:30PM +0100, Pavel Machek wrote:
+struct psy_charger_context {
+ bool is_usb_cable_evt_reg;
+ int psyc_cnt;
+ int batt_status;
+ /*cache battery and charger properties */
Comment coding style. Please run you patches through checkpatch.
checkpatch doesn't throw any error/warning. /* ... */ not allowed for single
line
comments? (will fix missing space after /*)
I meant the space after /*. Maybe checkpatch does not report anything
here, but I guess there are other places where it will comment :-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Wednesday, February 12, 2014 8:00 PM, Pavel Machek wrote:
On Wed 2014-02-05 13:44:58, Jenny Tc wrote:
On Tue, Feb 04, 2014 at 12:36:30PM +0100, Pavel Machek wrote:
+struct psy_charger_context {
+ bool is_usb_cable_evt_reg;
+ int psyc_cnt;
+ int batt_status;
+ /*cache battery and charger properties */
Comment coding style. Please run you patches through checkpatch.
checkpatch doesn't throw any error/warning. /* ... */ not allowed for
single line
comments? (will fix missing space after /*)
I meant the space after /*. Maybe checkpatch does not report anything
here, but I guess there are other places where it will comment :-).
Yes, right.
+ /*cache battery and charger properties */
^
'one space' is necessary between '/*' and 'cache'.
This can be fixed as below.
+ /* cache battery and charger properties */
Best regards,
Jingoo Han
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
On Tue, Feb 04, 2014 at 12:36:30PM +0100, Pavel Machek wrote:
+struct psy_charger_context {
+ bool is_usb_cable_evt_reg;
+ int psyc_cnt;
+ int batt_status;
+ /*cache battery and charger properties */
Comment coding style. Please run you patches through checkpatch.
checkpatch doesn't throw any error/warning. /* ... */ not allowed for single
line
comments? (will fix missing space after /*)
-Jenny
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver
Hi!
+Throttling configuration example:
+
+struct psy_throttle_state my_throttle_states[] = {
+
+ /* Level 0: Limit charge current to 1500mA. Normal Level */
+ {
+ .throttle_action = PSY_THROTTLE_CC_LIMIT,
+ .throttle_val = 1500,
+ },
+
+ /* Level 1: Limit charge current to 500mA */
+ {
+ .throttle_action = PSY_THROTTLE_CC_LIMIT,
+ .throttle_val = 500,
+ },
+
+ /*
+ * Level 2: Disable charging: Stop charging, charger supply power to
+ * platform.
+ */
+ {
+ .throttle_action = PSY_THROTTLE_DISABLE_CHARGING,
+ },
+
+ /* Level 3: Disable charger: Battery start discharging */
+ {
+ .throttle_action = PSY_THROTTLE_DISABLE_CHARGER,
+ },
+
+};
Does it make sense to have throttling description as a data, as
opposed to normal C code?
+struct psy_charger_context {
+ bool is_usb_cable_evt_reg;
+ int psyc_cnt;
+ int batt_status;
+ /*cache battery and charger properties */
Comment coding style. Please run you patches through checkpatch.
+ struct list_head evt_queue;
+ struct mutex evt_lock;
+ struct list_head event_queue;
+ struct psy_batt_chrg_prof batt_property;
Again, please use full words in variable names. How am I supposed to
know what evt_queue is? Especially when you have event_queue, also?!
And please do it globally.
+static void __power_supply_trigger_charging_handler(struct power_supply *psy)
+{
+ int i;
+ struct power_supply *psb = NULL;
+
+
+ if (is_trigger_charging_algo(psy)) {
+ if (psy_is_battery(psy)) {
+ if (trigger_algo(psy))
+ enable_supplied_by_charging(psy, false);
+ else
+ enable_supplied_by_charging(psy, true);
+ } else if (psy_is_charger(psy)) {
+ for (i = 0; i psy-num_supplicants; i++) {
+ psb =
+ power_supply_get_by_name(psy-
+ supplied_to[i]);
+
+ if (psb psy_is_battery(psb)
+ psy_is_present(psb)) {
+ if (trigger_algo(psb)) {
+ psy_disable_charging(psy);
+ break;
+ } else if
+ (psy_is_charging_can_be_enabled
+ (psy)) {
+ psy_enable_charging(psy);
+ wait_for_charging_enabled(psy);
+ }
+ }
+ }
+ }
+ update_sysfs(psy);
+ power_supply_changed(psy);
+ }
+}
See coding style about excessive nesting. Please fix it globally.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
