Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Wed 2014-03-12 15:32:59, Linus Walleij wrote: > On Mon, Mar 10, 2014 at 5:33 AM, Jenny Tc wrote: > > On Fri, Mar 07, 2014 at 09:25:20PM +0100, Pavel Machek wrote: > > >> > + /* sort based on priority. 0 has the highest priority */ > >> > + for (i = 0; i < cnt; ++i) > >> > + for (j = 0; j < cnt; ++j) > >> > + if (psy_prioirty(psy_lst[j]) > > >> > psy_prioirty(psy_lst[i])) > >> > + swap(psy_lst[j], psy_lst[i]); > >> > + > >> > >> WTF? Bubble sort in kernel? > > > > Yes, it's bubble sort. Since the number of power supply objects in real > > systems > > (max 4) are limited, I feel the complexity would be as same as any other > > sorting algorithms. Any suggestions? > > You already have a kernel quicksort implementation in lib/sort.c. > > Please restructure the code to make use of this as it is already > compiled into every kernel. Actually.. I believe the code needs more surgery than that. It should not need those string lookups -- just attach data directly to struct psy. And yes, if sort remains, it needs to use library function, but I strongly believe sorting should not be neccessary here. 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Wed 2014-03-12 15:32:59, Linus Walleij wrote: On Mon, Mar 10, 2014 at 5:33 AM, Jenny Tc jenny...@intel.com wrote: On Fri, Mar 07, 2014 at 09:25:20PM +0100, Pavel Machek wrote: + /* sort based on priority. 0 has the highest priority */ + for (i = 0; i cnt; ++i) + for (j = 0; j cnt; ++j) + if (psy_prioirty(psy_lst[j]) psy_prioirty(psy_lst[i])) + swap(psy_lst[j], psy_lst[i]); + WTF? Bubble sort in kernel? Yes, it's bubble sort. Since the number of power supply objects in real systems (max 4) are limited, I feel the complexity would be as same as any other sorting algorithms. Any suggestions? You already have a kernel quicksort implementation in lib/sort.c. Please restructure the code to make use of this as it is already compiled into every kernel. Actually.. I believe the code needs more surgery than that. It should not need those string lookups -- just attach data directly to struct psy. And yes, if sort remains, it needs to use library function, but I strongly believe sorting should not be neccessary here. 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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Fri, Mar 7, 2014 at 6:29 AM, Jenny TC wrote:
> +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,
> +};
I still disagree with using an enum as bitfield.
Atleast
#include
and use BIT(0), BIT(1) etc to define the bits.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Mon, Mar 10, 2014 at 5:33 AM, Jenny Tc wrote: > On Fri, Mar 07, 2014 at 09:25:20PM +0100, Pavel Machek wrote: >> > + /* sort based on priority. 0 has the highest priority */ >> > + for (i = 0; i < cnt; ++i) >> > + for (j = 0; j < cnt; ++j) >> > + if (psy_prioirty(psy_lst[j]) > >> > psy_prioirty(psy_lst[i])) >> > + swap(psy_lst[j], psy_lst[i]); >> > + >> >> WTF? Bubble sort in kernel? > > Yes, it's bubble sort. Since the number of power supply objects in real > systems > (max 4) are limited, I feel the complexity would be as same as any other > sorting algorithms. Any suggestions? You already have a kernel quicksort implementation in lib/sort.c. Please restructure the code to make use of this as it is already compiled into every kernel. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Mon, Mar 10, 2014 at 5:33 AM, Jenny Tc jenny...@intel.com wrote: On Fri, Mar 07, 2014 at 09:25:20PM +0100, Pavel Machek wrote: + /* sort based on priority. 0 has the highest priority */ + for (i = 0; i cnt; ++i) + for (j = 0; j cnt; ++j) + if (psy_prioirty(psy_lst[j]) psy_prioirty(psy_lst[i])) + swap(psy_lst[j], psy_lst[i]); + WTF? Bubble sort in kernel? Yes, it's bubble sort. Since the number of power supply objects in real systems (max 4) are limited, I feel the complexity would be as same as any other sorting algorithms. Any suggestions? You already have a kernel quicksort implementation in lib/sort.c. Please restructure the code to make use of this as it is already compiled into every kernel. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Fri, Mar 7, 2014 at 6:29 AM, Jenny TC jenny...@intel.com wrote:
+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,
+};
I still disagree with using an enum as bitfield.
Atleast
#include linux/bitops.h
and use BIT(0), BIT(1) etc to define the bits.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
Hi!
> > You still miss some wovels here. Sometimes it imakes it unlear:
> > chrg is charge? charger?
>
> chrgr means charger, chrg means charge. Isn't it used consistently?. Can fix
> it if
> it's really annoying. Please suggest.
Well... with all the missing letters, it is not clear if letter is
missing because you just made it short, or it is real difference.
Please just use full words. ... but read below.
> > > + list_for_each_entry(bat_cache, _chrgr.batt_cache_lst, node) {
> > > + if (!strcmp(bat_cache->name, bat_prop_new->name))
> > > + goto update_props;
> > > + }
> > > +
> > > + bat_cache = kzalloc(sizeof(*bat_cache), GFP_KERNEL);
> >
> > What is it with all the caching? Is it good idea to have caches
> > indexed by strings? Can't you go without caching, or attach caches to
> > some structure? Interesting goto again.
>
> Cache is to store previous battery properties. On receiving a new event
> compare the properties with cached value to decide charging state
> (charging/not charging/full etc.) and to control charging. There is added
> saving with caching. If the previous and current battery properties doesn't
> differ much, ignore the event instead of continuing (as implemented in
> is_trigger_charging_algo) with invoking charging algorithms. Also caching
> helps
> to take few critical charging decisions - like charge termination which need
> charge current and voltage over a period of time.
>
> Since a power_supply object (driver) is identified by name, it's the only
> index
> available.
Why can't you just use address of struct power_supply? There should be
no need to work with names.
Feel free to extend struct power_supply, wrap it in another struct,
anything.
> > > + if (psb && !psb->external_power_changed)
> > > + is_pwr_changed_defined &= false;
> >
> > WTF?
>
> The is_supplied_to_has_ext_pwr_changed() return true if any of the
> power_supply
> objects in supplied_to list has the external_power_changed() call back
> defined.
> This to optimize the notifications and to reduce charging algo invocations.
> This is used in is_trigger_charging_algo() to decide charging algorithms
> should
> be invoked or not.
No, I mean... using "&=" operator in case where plain assignment
should work is evil.
> > > + /* Identify chargers which are supplying power to the battery */
> > > + class_dev_iter_init(, power_supply_class, NULL, NULL);
> > > + while ((dev = class_dev_iter_next())) {
> > > + pst = (struct power_supply *)dev_get_drvdata(dev);
> > > + if (!psy_is_charger(pst))
> > > + continue;
> > > + for (i = 0; i < pst->num_supplicants; i++) {
> > > + if (!strcmp(pst->supplied_to[i], psy->name))
> > > + psy_lst[cnt++] = pst;
> > > + }
> >
> > Awful lot of string compares around.
>
> In reality this would be one/two, just because the number of chargers
> supplying
> power to a battery would be limited.
This whole file is nothing but string compares, bubble sorts, and
weird caches.
Please find a way to simplify a design. Rafael works for same company,
can he help?
> > Does it really be so complex? Dynamically allocated caches, name
> > compares everywhere?
>
> It's really not so complex. The caches are allocated dynamically only when
> a new charger/battery power supply object gets registered - basically when a
> charger/battery driver gets loaded. At runtime no dynamic allocation is
> needed.
> There is not too many string comparisons just because the number of power
> supply objects are limited. Power supply subsystem has only name as unique
> identifier (psy->name). I couldn't find a better way without string
> comparisons
> to identify a power supply object.
(void *) psy is also an unique identifier. Plus, you can add new
fields to psy.
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
Hi!
You still miss some wovels here. Sometimes it imakes it unlear:
chrg is charge? charger?
chrgr means charger, chrg means charge. Isn't it used consistently?. Can fix
it if
it's really annoying. Please suggest.
Well... with all the missing letters, it is not clear if letter is
missing because you just made it short, or it is real difference.
Please just use full words. ... but read below.
+ list_for_each_entry(bat_cache, psy_chrgr.batt_cache_lst, node) {
+ if (!strcmp(bat_cache-name, bat_prop_new-name))
+ goto update_props;
+ }
+
+ bat_cache = kzalloc(sizeof(*bat_cache), GFP_KERNEL);
What is it with all the caching? Is it good idea to have caches
indexed by strings? Can't you go without caching, or attach caches to
some structure? Interesting goto again.
Cache is to store previous battery properties. On receiving a new event
compare the properties with cached value to decide charging state
(charging/not charging/full etc.) and to control charging. There is added
saving with caching. If the previous and current battery properties doesn't
differ much, ignore the event instead of continuing (as implemented in
is_trigger_charging_algo) with invoking charging algorithms. Also caching
helps
to take few critical charging decisions - like charge termination which need
charge current and voltage over a period of time.
Since a power_supply object (driver) is identified by name, it's the only
index
available.
Why can't you just use address of struct power_supply? There should be
no need to work with names.
Feel free to extend struct power_supply, wrap it in another struct,
anything.
+ if (psb !psb-external_power_changed)
+ is_pwr_changed_defined = false;
WTF?
The is_supplied_to_has_ext_pwr_changed() return true if any of the
power_supply
objects in supplied_to list has the external_power_changed() call back
defined.
This to optimize the notifications and to reduce charging algo invocations.
This is used in is_trigger_charging_algo() to decide charging algorithms
should
be invoked or not.
No, I mean... using = operator in case where plain assignment
should work is evil.
+ /* Identify chargers which are supplying power to the battery */
+ class_dev_iter_init(iter, power_supply_class, NULL, NULL);
+ while ((dev = class_dev_iter_next(iter))) {
+ pst = (struct power_supply *)dev_get_drvdata(dev);
+ if (!psy_is_charger(pst))
+ continue;
+ for (i = 0; i pst-num_supplicants; i++) {
+ if (!strcmp(pst-supplied_to[i], psy-name))
+ psy_lst[cnt++] = pst;
+ }
Awful lot of string compares around.
In reality this would be one/two, just because the number of chargers
supplying
power to a battery would be limited.
This whole file is nothing but string compares, bubble sorts, and
weird caches.
Please find a way to simplify a design. Rafael works for same company,
can he help?
Does it really be so complex? Dynamically allocated caches, name
compares everywhere?
It's really not so complex. The caches are allocated dynamically only when
a new charger/battery power supply object gets registered - basically when a
charger/battery driver gets loaded. At runtime no dynamic allocation is
needed.
There is not too many string comparisons just because the number of power
supply objects are limited. Power supply subsystem has only name as unique
identifier (psy-name). I couldn't find a better way without string
comparisons
to identify a power supply object.
(void *) psy is also an unique identifier. Plus, you can add new
fields to psy.
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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Fri, Mar 07, 2014 at 09:25:20PM +0100, Pavel Machek wrote:
Hi,
> > The Power Supply charging driver connects multiple subsystems
> > to do charging in a generic way. The subsystems involves power_supply,
> > thermal and battery communication subsystems (1wire).With this the charging
> > is
> > handled in a generic way.
>
> " " after ".", please.
Will fix in next patch set
> > +
> > +The Power Supply charging driver connects multiple subsystems
> > +to do charging in a generic way. The subsystems involves power_supply,
> > +thermal and battery communication subsystems (1wire).With this the
> > charging is
>
> Here too.
Same here
>
>
>
> > +
> > +The driver introduces different new features - Battery Identification
> > +interfaces, pluggable charging algorithms, charger cable arbitrations etc.
> > +
> > +In existing driver implementations the charging is done based on the static
> > +battery characteristics. This is done at the boot time by passing the
> > battery
> > +properties (max_voltage, capacity) etc. as a platform data to the
>
> -> (max_voltage, capacity, etc.)
Same here
>
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -14,6 +14,14 @@ config POWER_SUPPLY_DEBUG
> > Say Y here to enable debugging messages for power supply class
> > and drivers.
> >
> > +config POWER_SUPPLY_CHARGER
> > + bool "Power Supply Charger"
> > + help
> > + Say Y here to enable the power supply charging control driver.
> > Charging
> > + control supports charging in a generic way. This allows the charger
> > + drivers to keep the charging logic outside and the charger driver
> > + just need to abstract the charger hardware.
> > +
>
> Umm. This is not too helpful for our users.
Will add more text to help users.
>
> > +struct psy_charger_context {
> > + bool is_usb_cable_evt_reg;
> > + int psyc_cnt;
> > + int batt_status;
> > + /* cache battery and charger properties */
> > + struct list_head chrgr_cache_lst;
> > + struct list_head batt_cache_lst;
> > + struct mutex event_lock;
> > + struct list_head event_queue;
> > + struct psy_batt_chrg_prof batt_property;
> > + wait_queue_head_t wait_chrg_enable;
> > + spinlock_t battid_spinlock;
> > + spinlock_t event_queue_lock;
> > + struct work_struct event_work;
> > +};
> > +
> > +struct charger_cable {
> > + struct psy_cable_props cable_props;
> > + enum psy_charger_cable_type psy_cable_type;
> > +};
> > +
> > +static struct psy_charger_context psy_chrgr;
>
> You still miss some wovels here. Sometimes it imakes it unlear:
> chrg is charge? charger?
chrgr means charger, chrg means charge. Isn't it used consistently?. Can fix it
if
it's really annoying. Please suggest.
>
>
> > +static inline bool psy_is_charger_prop_changed(struct psy_charger_props
> > prop,
> > + struct psy_charger_props cache_prop)
> > +{
> > + /* if online/prsent/health/is_charging is changed, then return true */
>
> Typo - present.
Will fix in next patch set
>
> > +static inline void cache_chrgr_prop(struct psy_charger_props
> > *chrgr_prop_new)
> > +{
> > + struct psy_charger_props *chrgr_cache;
> > +
> > + list_for_each_entry(chrgr_cache, _chrgr.chrgr_cache_lst, node) {
> > + if (!strcmp(chrgr_cache->name, chrgr_prop_new->name))
> > + goto update_props;
> > + }
>
> Interesting use of goto. Maybe update_properties should be separate function?
I feel, having a function just for few assignments may not be a good idea.
What about having memcpy?
>
> > +update_props:
> > + chrgr_cache->is_charging = chrgr_prop_new->is_charging;
> > + chrgr_cache->online = chrgr_prop_new->online;
> > + chrgr_cache->health = chrgr_prop_new->health;
> > + chrgr_cache->present = chrgr_prop_new->present;
> > + chrgr_cache->cable = chrgr_prop_new->cable;
> > + chrgr_cache->tstamp = chrgr_prop_new->tstamp;
> > + chrgr_cache->psyc = chrgr_prop_new->psyc;
> > +}
> > +
> > + chrgr_prop.psyc = chrgr_prop_cache.psyc;
> > + cache_chrgr_prop(_prop);
> > + return true;
> > +}
> > +static void cache_successive_samples(long *sample_array, long new_sample)
> > +{
>
> Add empty line between the functions.
Ok..Will fix in next patch set
>
> > +static inline void cache_bat_prop(struct psy_batt_props *bat_prop_new)
> > +{
> > + struct psy_batt_props *bat_cache;
> > +
> > + /*
> > + * Find entry in cache list. If an entry is located update
> > + * the existing entry else create new entry in the list
> > + */
> > + list_for_each_entry(bat_cache, _chrgr.batt_cache_lst, node) {
> > + if (!strcmp(bat_cache->name, bat_prop_new->name))
> > + goto update_props;
> > + }
> > +
> > + bat_cache = kzalloc(sizeof(*bat_cache), GFP_KERNEL);
>
> What is it with all the caching? Is it good idea to have caches
> indexed by strings? Can't you go without caching, or attach caches to
> some structure? Interesting goto again.
Cache is to store
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
On Fri, Mar 07, 2014 at 09:25:20PM +0100, Pavel Machek wrote:
Hi,
The Power Supply charging driver connects multiple subsystems
to do charging in a generic way. The subsystems involves power_supply,
thermal and battery communication subsystems (1wire).With this the charging
is
handled in a generic way.
after ., please.
Will fix in next patch set
+
+The Power Supply charging driver connects multiple subsystems
+to do charging in a generic way. The subsystems involves power_supply,
+thermal and battery communication subsystems (1wire).With this the
charging is
Here too.
Same here
+
+The driver introduces different new features - Battery Identification
+interfaces, pluggable charging algorithms, charger cable arbitrations etc.
+
+In existing driver implementations the charging is done based on the static
+battery characteristics. This is done at the boot time by passing the
battery
+properties (max_voltage, capacity) etc. as a platform data to the
- (max_voltage, capacity, etc.)
Same here
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -14,6 +14,14 @@ config POWER_SUPPLY_DEBUG
Say Y here to enable debugging messages for power supply class
and drivers.
+config POWER_SUPPLY_CHARGER
+ bool Power Supply Charger
+ help
+ Say Y here to enable the power supply charging control driver.
Charging
+ control supports charging in a generic way. This allows the charger
+ drivers to keep the charging logic outside and the charger driver
+ just need to abstract the charger hardware.
+
Umm. This is not too helpful for our users.
Will add more text to help users.
+struct psy_charger_context {
+ bool is_usb_cable_evt_reg;
+ int psyc_cnt;
+ int batt_status;
+ /* cache battery and charger properties */
+ struct list_head chrgr_cache_lst;
+ struct list_head batt_cache_lst;
+ struct mutex event_lock;
+ struct list_head event_queue;
+ struct psy_batt_chrg_prof batt_property;
+ wait_queue_head_t wait_chrg_enable;
+ spinlock_t battid_spinlock;
+ spinlock_t event_queue_lock;
+ struct work_struct event_work;
+};
+
+struct charger_cable {
+ struct psy_cable_props cable_props;
+ enum psy_charger_cable_type psy_cable_type;
+};
+
+static struct psy_charger_context psy_chrgr;
You still miss some wovels here. Sometimes it imakes it unlear:
chrg is charge? charger?
chrgr means charger, chrg means charge. Isn't it used consistently?. Can fix it
if
it's really annoying. Please suggest.
+static inline bool psy_is_charger_prop_changed(struct psy_charger_props
prop,
+ struct psy_charger_props cache_prop)
+{
+ /* if online/prsent/health/is_charging is changed, then return true */
Typo - present.
Will fix in next patch set
+static inline void cache_chrgr_prop(struct psy_charger_props
*chrgr_prop_new)
+{
+ struct psy_charger_props *chrgr_cache;
+
+ list_for_each_entry(chrgr_cache, psy_chrgr.chrgr_cache_lst, node) {
+ if (!strcmp(chrgr_cache-name, chrgr_prop_new-name))
+ goto update_props;
+ }
Interesting use of goto. Maybe update_properties should be separate function?
I feel, having a function just for few assignments may not be a good idea.
What about having memcpy?
+update_props:
+ chrgr_cache-is_charging = chrgr_prop_new-is_charging;
+ chrgr_cache-online = chrgr_prop_new-online;
+ chrgr_cache-health = chrgr_prop_new-health;
+ chrgr_cache-present = chrgr_prop_new-present;
+ chrgr_cache-cable = chrgr_prop_new-cable;
+ chrgr_cache-tstamp = chrgr_prop_new-tstamp;
+ chrgr_cache-psyc = chrgr_prop_new-psyc;
+}
+
+ chrgr_prop.psyc = chrgr_prop_cache.psyc;
+ cache_chrgr_prop(chrgr_prop);
+ return true;
+}
+static void cache_successive_samples(long *sample_array, long new_sample)
+{
Add empty line between the functions.
Ok..Will fix in next patch set
+static inline void cache_bat_prop(struct psy_batt_props *bat_prop_new)
+{
+ struct psy_batt_props *bat_cache;
+
+ /*
+ * Find entry in cache list. If an entry is located update
+ * the existing entry else create new entry in the list
+ */
+ list_for_each_entry(bat_cache, psy_chrgr.batt_cache_lst, node) {
+ if (!strcmp(bat_cache-name, bat_prop_new-name))
+ goto update_props;
+ }
+
+ bat_cache = kzalloc(sizeof(*bat_cache), GFP_KERNEL);
What is it with all the caching? Is it good idea to have caches
indexed by strings? Can't you go without caching, or attach caches to
some structure? Interesting goto again.
Cache is to store previous battery properties. On receiving a new event
compare the properties with cached value to decide charging state
(charging/not charging/full etc.) and to control charging. There is added
saving with caching. If the previous and current
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
Hi!
> The Power Supply charging driver connects multiple subsystems
> to do charging in a generic way. The subsystems involves power_supply,
> thermal and battery communication subsystems (1wire).With this the charging is
> handled in a generic way.
" " after ".", please.
> +
> +The Power Supply charging driver connects multiple subsystems
> +to do charging in a generic way. The subsystems involves power_supply,
> +thermal and battery communication subsystems (1wire).With this the charging
> is
Here too.
> +
> +The driver introduces different new features - Battery Identification
> +interfaces, pluggable charging algorithms, charger cable arbitrations etc.
> +
> +In existing driver implementations the charging is done based on the static
> +battery characteristics. This is done at the boot time by passing the battery
> +properties (max_voltage, capacity) etc. as a platform data to the
-> (max_voltage, capacity, etc.)
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -14,6 +14,14 @@ config POWER_SUPPLY_DEBUG
> Say Y here to enable debugging messages for power supply class
> and drivers.
>
> +config POWER_SUPPLY_CHARGER
> + bool "Power Supply Charger"
> + help
> + Say Y here to enable the power supply charging control driver.
> Charging
> + control supports charging in a generic way. This allows the charger
> + drivers to keep the charging logic outside and the charger driver
> + just need to abstract the charger hardware.
> +
Umm. This is not too helpful for our users.
> +struct psy_charger_context {
> + bool is_usb_cable_evt_reg;
> + int psyc_cnt;
> + int batt_status;
> + /* cache battery and charger properties */
> + struct list_head chrgr_cache_lst;
> + struct list_head batt_cache_lst;
> + struct mutex event_lock;
> + struct list_head event_queue;
> + struct psy_batt_chrg_prof batt_property;
> + wait_queue_head_t wait_chrg_enable;
> + spinlock_t battid_spinlock;
> + spinlock_t event_queue_lock;
> + struct work_struct event_work;
> +};
> +
> +struct charger_cable {
> + struct psy_cable_props cable_props;
> + enum psy_charger_cable_type psy_cable_type;
> +};
> +
> +static struct psy_charger_context psy_chrgr;
You still miss some wovels here. Sometimes it imakes it unlear:
chrg is charge? charger?
> +static inline bool psy_is_charger_prop_changed(struct psy_charger_props prop,
> + struct psy_charger_props cache_prop)
> +{
> + /* if online/prsent/health/is_charging is changed, then return true */
Typo - present.
> +static inline void cache_chrgr_prop(struct psy_charger_props *chrgr_prop_new)
> +{
> + struct psy_charger_props *chrgr_cache;
> +
> + list_for_each_entry(chrgr_cache, _chrgr.chrgr_cache_lst, node) {
> + if (!strcmp(chrgr_cache->name, chrgr_prop_new->name))
> + goto update_props;
> + }
Interesting use of goto. Maybe update_properties should be separate function?
> +update_props:
> + chrgr_cache->is_charging = chrgr_prop_new->is_charging;
> + chrgr_cache->online = chrgr_prop_new->online;
> + chrgr_cache->health = chrgr_prop_new->health;
> + chrgr_cache->present = chrgr_prop_new->present;
> + chrgr_cache->cable = chrgr_prop_new->cable;
> + chrgr_cache->tstamp = chrgr_prop_new->tstamp;
> + chrgr_cache->psyc = chrgr_prop_new->psyc;
> +}
> +
> + chrgr_prop.psyc = chrgr_prop_cache.psyc;
> + cache_chrgr_prop(_prop);
> + return true;
> +}
> +static void cache_successive_samples(long *sample_array, long new_sample)
> +{
Add empty line between the functions.
> +static inline void cache_bat_prop(struct psy_batt_props *bat_prop_new)
> +{
> + struct psy_batt_props *bat_cache;
> +
> + /*
> + * Find entry in cache list. If an entry is located update
> + * the existing entry else create new entry in the list
> + */
> + list_for_each_entry(bat_cache, _chrgr.batt_cache_lst, node) {
> + if (!strcmp(bat_cache->name, bat_prop_new->name))
> + goto update_props;
> + }
> +
> + bat_cache = kzalloc(sizeof(*bat_cache), GFP_KERNEL);
What is it with all the caching? Is it good idea to have caches
indexed by strings? Can't you go without caching, or attach caches to
some structure? Interesting goto again.
> +static inline void get_cur_bat_prop(struct power_supply *psy,
> + struct psy_batt_props *bat_prop)
> +{
> + struct psy_batt_props bat_prop_cache;
> + int ret;
> +
> + bat_prop->name = psy->name;
> + bat_prop->voltage_now = PSY_VOLTAGE_OCV(psy) / 1000;
Not sure what OCV means...
> +static inline bool is_supplied_to_has_ext_pwr_changed(struct power_supply
> *psy)
> +{
> + int i;
> + struct power_supply *psb;
> + bool is_pwr_changed_defined = true;
> +
> + for (i = 0; i < psy->num_supplicants; i++) {
> + psb =
> +
Re: [PATCHv8 2/4] power_supply: Introduce generic psy charging driver
Hi!
The Power Supply charging driver connects multiple subsystems
to do charging in a generic way. The subsystems involves power_supply,
thermal and battery communication subsystems (1wire).With this the charging is
handled in a generic way.
after ., please.
+
+The Power Supply charging driver connects multiple subsystems
+to do charging in a generic way. The subsystems involves power_supply,
+thermal and battery communication subsystems (1wire).With this the charging
is
Here too.
+
+The driver introduces different new features - Battery Identification
+interfaces, pluggable charging algorithms, charger cable arbitrations etc.
+
+In existing driver implementations the charging is done based on the static
+battery characteristics. This is done at the boot time by passing the battery
+properties (max_voltage, capacity) etc. as a platform data to the
- (max_voltage, capacity, etc.)
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -14,6 +14,14 @@ config POWER_SUPPLY_DEBUG
Say Y here to enable debugging messages for power supply class
and drivers.
+config POWER_SUPPLY_CHARGER
+ bool Power Supply Charger
+ help
+ Say Y here to enable the power supply charging control driver.
Charging
+ control supports charging in a generic way. This allows the charger
+ drivers to keep the charging logic outside and the charger driver
+ just need to abstract the charger hardware.
+
Umm. This is not too helpful for our users.
+struct psy_charger_context {
+ bool is_usb_cable_evt_reg;
+ int psyc_cnt;
+ int batt_status;
+ /* cache battery and charger properties */
+ struct list_head chrgr_cache_lst;
+ struct list_head batt_cache_lst;
+ struct mutex event_lock;
+ struct list_head event_queue;
+ struct psy_batt_chrg_prof batt_property;
+ wait_queue_head_t wait_chrg_enable;
+ spinlock_t battid_spinlock;
+ spinlock_t event_queue_lock;
+ struct work_struct event_work;
+};
+
+struct charger_cable {
+ struct psy_cable_props cable_props;
+ enum psy_charger_cable_type psy_cable_type;
+};
+
+static struct psy_charger_context psy_chrgr;
You still miss some wovels here. Sometimes it imakes it unlear:
chrg is charge? charger?
+static inline bool psy_is_charger_prop_changed(struct psy_charger_props prop,
+ struct psy_charger_props cache_prop)
+{
+ /* if online/prsent/health/is_charging is changed, then return true */
Typo - present.
+static inline void cache_chrgr_prop(struct psy_charger_props *chrgr_prop_new)
+{
+ struct psy_charger_props *chrgr_cache;
+
+ list_for_each_entry(chrgr_cache, psy_chrgr.chrgr_cache_lst, node) {
+ if (!strcmp(chrgr_cache-name, chrgr_prop_new-name))
+ goto update_props;
+ }
Interesting use of goto. Maybe update_properties should be separate function?
+update_props:
+ chrgr_cache-is_charging = chrgr_prop_new-is_charging;
+ chrgr_cache-online = chrgr_prop_new-online;
+ chrgr_cache-health = chrgr_prop_new-health;
+ chrgr_cache-present = chrgr_prop_new-present;
+ chrgr_cache-cable = chrgr_prop_new-cable;
+ chrgr_cache-tstamp = chrgr_prop_new-tstamp;
+ chrgr_cache-psyc = chrgr_prop_new-psyc;
+}
+
+ chrgr_prop.psyc = chrgr_prop_cache.psyc;
+ cache_chrgr_prop(chrgr_prop);
+ return true;
+}
+static void cache_successive_samples(long *sample_array, long new_sample)
+{
Add empty line between the functions.
+static inline void cache_bat_prop(struct psy_batt_props *bat_prop_new)
+{
+ struct psy_batt_props *bat_cache;
+
+ /*
+ * Find entry in cache list. If an entry is located update
+ * the existing entry else create new entry in the list
+ */
+ list_for_each_entry(bat_cache, psy_chrgr.batt_cache_lst, node) {
+ if (!strcmp(bat_cache-name, bat_prop_new-name))
+ goto update_props;
+ }
+
+ bat_cache = kzalloc(sizeof(*bat_cache), GFP_KERNEL);
What is it with all the caching? Is it good idea to have caches
indexed by strings? Can't you go without caching, or attach caches to
some structure? Interesting goto again.
+static inline void get_cur_bat_prop(struct power_supply *psy,
+ struct psy_batt_props *bat_prop)
+{
+ struct psy_batt_props bat_prop_cache;
+ int ret;
+
+ bat_prop-name = psy-name;
+ bat_prop-voltage_now = PSY_VOLTAGE_OCV(psy) / 1000;
Not sure what OCV means...
+static inline bool is_supplied_to_has_ext_pwr_changed(struct power_supply
*psy)
+{
+ int i;
+ struct power_supply *psb;
+ bool is_pwr_changed_defined = true;
+
+ for (i = 0; i psy-num_supplicants; i++) {
+ psb =
+ power_supply_get_by_name(psy-
+ supplied_to[i]);
You can do it on one line, right?
+
