Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Jul 11, 2014 at 02:46:35AM +, Tc, Jenny wrote:
> > From: Sebastian Reichel [mailto:s...@kernel.org]
> > Sent: Tuesday, July 08, 2014 9:26 PM
> > To: Tc, Jenny
> > Cc: linux-kernel@vger.kernel.org; Dmitry Eremin-Solenikov; Pavel Machek;
> > Stephen
> > Rothwell; Anton Vorontsov; David Woodhouse; David Cohen; Pallala,
> > Ramakrishna
> > Subject: Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
> >
> > On Tue, Jul 08, 2014 at 06:07:29AM +, Tc, Jenny wrote:
> > > > > +static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
> > > > > + int temp)
> > > > > +{
> > > > > + int i = 0;
> > > > > + int temp_range_cnt;
> > > > > +
> > > > > + temp_range_cnt = min_t(u16, pse_mod_bprof->temp_mon_ranges,
> > > > > + BATT_TEMP_NR_RNG);
> > > > > + if ((temp < pse_mod_bprof->temp_low_lim) ||
> > > > > + (temp > pse_mod_bprof->temp_mon_range[0].temp_up_lim))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + for (i = 0; i < temp_range_cnt; ++i)
> > > > > + if (temp > pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> > > > > + break;
> > > > > + return i-1;
> > > > > +}
> > > >
> > > > pse_mod_bprof->temp_mon_ranges > BATT_TEMP_NR_RNG is not allowed, so
> > > > I suggest to print an error and return some error code.
> > > >
> > > min_t takes care of the upper bound. The algorithm process
> > > BATT_TEMP_NR_RNG even if the actual number of zones are greater than this.
> >
> > Right, the function will not fail, but the zone information table is
> > truncated. I would
> > expect at least warning about that. I think it doesn't hurt to have a small
> > function,
> > which validates the zone data as good as possible. Using incorrect
> > temperature
> > zones is a safety thread and we should try our best to avoid exploding
> > batteries ;)
> >
> > Maybe something like that:
> >
> > static bool check_tempzones(struct psy_pse_chrg_prof *pse_mod_bprof) {
> > int i = 0;
> > int last_temp = ;
> >
> > /* check size */
> > if (BATT_TEMP_NR_RNG > pse_mod_bprof->temp_mon_ranges)
> > return false;
>
> This is in a way good to have, OK to implement the same.
>
> But KO with below suggestion. This doesn't guarantee safety. IMHO
> Safety is 1/0 - SAFE or NOT SAFE. No half safety.
We won't be able to handle every possible case, but we can try our
best to avoid problems. One (possibly useless) safety check too much
is better than one missing check.
> To ensure complete safety, measures should be taken at the entry
> point- where data is read from external source. Since the
> algorithm gets the data from internal kernel component
> (power_supply_charger.c), it trust the data. Since the data is
> originated from battery identification driver, the safety should
> be ensured at that level.
I think some basic checks of kernel's platform data help to avoid
potential problems during development phase.
> > /* check zone order */
> > for (i = 0; i < pse_mod_bprof->temp_mon_ranges; i++) {
> > if (last_temp < pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> > return false;
> > last_temp = pse_mod_bprof->temp_mon_range[i].temp_up_lim;
> > }
> >
> > return true;
> > }
-- Sebastian
signature.asc
Description: Digital signature
Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Jul 11, 2014 at 02:46:35AM +, Tc, Jenny wrote:
From: Sebastian Reichel [mailto:s...@kernel.org]
Sent: Tuesday, July 08, 2014 9:26 PM
To: Tc, Jenny
Cc: linux-kernel@vger.kernel.org; Dmitry Eremin-Solenikov; Pavel Machek;
Stephen
Rothwell; Anton Vorontsov; David Woodhouse; David Cohen; Pallala,
Ramakrishna
Subject: Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Jul 08, 2014 at 06:07:29AM +, Tc, Jenny wrote:
+static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
+ int temp)
+{
+ int i = 0;
+ int temp_range_cnt;
+
+ temp_range_cnt = min_t(u16, pse_mod_bprof-temp_mon_ranges,
+ BATT_TEMP_NR_RNG);
+ if ((temp pse_mod_bprof-temp_low_lim) ||
+ (temp pse_mod_bprof-temp_mon_range[0].temp_up_lim))
+ return -EINVAL;
+
+ for (i = 0; i temp_range_cnt; ++i)
+ if (temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
+ break;
+ return i-1;
+}
pse_mod_bprof-temp_mon_ranges BATT_TEMP_NR_RNG is not allowed, so
I suggest to print an error and return some error code.
min_t takes care of the upper bound. The algorithm process
BATT_TEMP_NR_RNG even if the actual number of zones are greater than this.
Right, the function will not fail, but the zone information table is
truncated. I would
expect at least warning about that. I think it doesn't hurt to have a small
function,
which validates the zone data as good as possible. Using incorrect
temperature
zones is a safety thread and we should try our best to avoid exploding
batteries ;)
Maybe something like that:
static bool check_tempzones(struct psy_pse_chrg_prof *pse_mod_bprof) {
int i = 0;
int last_temp = ;
/* check size */
if (BATT_TEMP_NR_RNG pse_mod_bprof-temp_mon_ranges)
return false;
This is in a way good to have, OK to implement the same.
But KO with below suggestion. This doesn't guarantee safety. IMHO
Safety is 1/0 - SAFE or NOT SAFE. No half safety.
We won't be able to handle every possible case, but we can try our
best to avoid problems. One (possibly useless) safety check too much
is better than one missing check.
To ensure complete safety, measures should be taken at the entry
point- where data is read from external source. Since the
algorithm gets the data from internal kernel component
(power_supply_charger.c), it trust the data. Since the data is
originated from battery identification driver, the safety should
be ensured at that level.
I think some basic checks of kernel's platform data help to avoid
potential problems during development phase.
/* check zone order */
for (i = 0; i pse_mod_bprof-temp_mon_ranges; i++) {
if (last_temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
return false;
last_temp = pse_mod_bprof-temp_mon_range[i].temp_up_lim;
}
return true;
}
-- Sebastian
signature.asc
Description: Digital signature
RE: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
> From: Sebastian Reichel [mailto:s...@kernel.org]
> Sent: Tuesday, July 08, 2014 9:26 PM
> To: Tc, Jenny
> Cc: linux-kernel@vger.kernel.org; Dmitry Eremin-Solenikov; Pavel Machek;
> Stephen
> Rothwell; Anton Vorontsov; David Woodhouse; David Cohen; Pallala, Ramakrishna
> Subject: Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
>
> On Tue, Jul 08, 2014 at 06:07:29AM +, Tc, Jenny wrote:
> > > > +static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
> > > > + int temp)
> > > > +{
> > > > + int i = 0;
> > > > + int temp_range_cnt;
> > > > +
> > > > + temp_range_cnt = min_t(u16, pse_mod_bprof->temp_mon_ranges,
> > > > + BATT_TEMP_NR_RNG);
> > > > + if ((temp < pse_mod_bprof->temp_low_lim) ||
> > > > + (temp > pse_mod_bprof->temp_mon_range[0].temp_up_lim))
> > > > + return -EINVAL;
> > > > +
> > > > + for (i = 0; i < temp_range_cnt; ++i)
> > > > + if (temp > pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> > > > + break;
> > > > + return i-1;
> > > > +}
> > >
> > > pse_mod_bprof->temp_mon_ranges > BATT_TEMP_NR_RNG is not allowed, so
> > > I suggest to print an error and return some error code.
> > >
> > min_t takes care of the upper bound. The algorithm process
> > BATT_TEMP_NR_RNG even if the actual number of zones are greater than this.
>
> Right, the function will not fail, but the zone information table is
> truncated. I would
> expect at least warning about that. I think it doesn't hurt to have a small
> function,
> which validates the zone data as good as possible. Using incorrect temperature
> zones is a safety thread and we should try our best to avoid exploding
> batteries ;)
>
> Maybe something like that:
>
> static bool check_tempzones(struct psy_pse_chrg_prof *pse_mod_bprof) {
> int i = 0;
> int last_temp = ;
>
> /* check size */
> if (BATT_TEMP_NR_RNG > pse_mod_bprof->temp_mon_ranges)
> return false;
This is in a way good to have, OK to implement the same.
But KO with below suggestion. This doesn't guarantee safety. IMHO
Safety is 1/0 - SAFE or NOT SAFE. No half safety.
To ensure complete safety, measures should be taken at the entry point- where
data
is read from external source. Since the algorithm gets the data from internal
kernel component (power_supply_charger.c), it trust the data. Since the data
is originated from battery identification driver, the safety should be ensured
at
that level.
>
> /* check zone order */
> for (i = 0; i < pse_mod_bprof->temp_mon_ranges; i++) {
> if (last_temp < pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> return false;
> last_temp = pse_mod_bprof->temp_mon_range[i].temp_up_lim;
> }
>
> return true;
> }
>
> -- Sebastian
--
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
From: Sebastian Reichel [mailto:s...@kernel.org]
Sent: Tuesday, July 08, 2014 9:26 PM
To: Tc, Jenny
Cc: linux-kernel@vger.kernel.org; Dmitry Eremin-Solenikov; Pavel Machek;
Stephen
Rothwell; Anton Vorontsov; David Woodhouse; David Cohen; Pallala, Ramakrishna
Subject: Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Jul 08, 2014 at 06:07:29AM +, Tc, Jenny wrote:
+static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
+ int temp)
+{
+ int i = 0;
+ int temp_range_cnt;
+
+ temp_range_cnt = min_t(u16, pse_mod_bprof-temp_mon_ranges,
+ BATT_TEMP_NR_RNG);
+ if ((temp pse_mod_bprof-temp_low_lim) ||
+ (temp pse_mod_bprof-temp_mon_range[0].temp_up_lim))
+ return -EINVAL;
+
+ for (i = 0; i temp_range_cnt; ++i)
+ if (temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
+ break;
+ return i-1;
+}
pse_mod_bprof-temp_mon_ranges BATT_TEMP_NR_RNG is not allowed, so
I suggest to print an error and return some error code.
min_t takes care of the upper bound. The algorithm process
BATT_TEMP_NR_RNG even if the actual number of zones are greater than this.
Right, the function will not fail, but the zone information table is
truncated. I would
expect at least warning about that. I think it doesn't hurt to have a small
function,
which validates the zone data as good as possible. Using incorrect temperature
zones is a safety thread and we should try our best to avoid exploding
batteries ;)
Maybe something like that:
static bool check_tempzones(struct psy_pse_chrg_prof *pse_mod_bprof) {
int i = 0;
int last_temp = ;
/* check size */
if (BATT_TEMP_NR_RNG pse_mod_bprof-temp_mon_ranges)
return false;
This is in a way good to have, OK to implement the same.
But KO with below suggestion. This doesn't guarantee safety. IMHO
Safety is 1/0 - SAFE or NOT SAFE. No half safety.
To ensure complete safety, measures should be taken at the entry point- where
data
is read from external source. Since the algorithm gets the data from internal
kernel component (power_supply_charger.c), it trust the data. Since the data
is originated from battery identification driver, the safety should be ensured
at
that level.
/* check zone order */
for (i = 0; i pse_mod_bprof-temp_mon_ranges; i++) {
if (last_temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
return false;
last_temp = pse_mod_bprof-temp_mon_range[i].temp_up_lim;
}
return true;
}
-- Sebastian
--
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Jul 08, 2014 at 06:07:29AM +, Tc, Jenny wrote:
> > > +static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
> > > + int temp)
> > > +{
> > > + int i = 0;
> > > + int temp_range_cnt;
> > > +
> > > + temp_range_cnt = min_t(u16, pse_mod_bprof->temp_mon_ranges,
> > > + BATT_TEMP_NR_RNG);
> > > + if ((temp < pse_mod_bprof->temp_low_lim) ||
> > > + (temp > pse_mod_bprof->temp_mon_range[0].temp_up_lim))
> > > + return -EINVAL;
> > > +
> > > + for (i = 0; i < temp_range_cnt; ++i)
> > > + if (temp > pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> > > + break;
> > > + return i-1;
> > > +}
> >
> > pse_mod_bprof->temp_mon_ranges > BATT_TEMP_NR_RNG is not allowed, so I
> > suggest to print an error and return some error code.
> >
> min_t takes care of the upper bound. The algorithm process BATT_TEMP_NR_RNG
> even if the actual number of zones are greater than this.
Right, the function will not fail, but the zone information table is
truncated. I would expect at least warning about that. I think it
doesn't hurt to have a small function, which validates the zone
data as good as possible. Using incorrect temperature zones is a
safety thread and we should try our best to avoid exploding
batteries ;)
Maybe something like that:
static bool check_tempzones(struct psy_pse_chrg_prof *pse_mod_bprof)
{
int i = 0;
int last_temp = ;
/* check size */
if (BATT_TEMP_NR_RNG > pse_mod_bprof->temp_mon_ranges)
return false;
/* check zone order */
for (i = 0; i < pse_mod_bprof->temp_mon_ranges; i++) {
if (last_temp < pse_mod_bprof->temp_mon_range[i].temp_up_lim)
return false;
last_temp = pse_mod_bprof->temp_mon_range[i].temp_up_lim;
}
return true;
}
-- Sebastian
signature.asc
Description: Digital signature
RE: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
> > +static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
> > + int temp)
> > +{
> > + int i = 0;
> > + int temp_range_cnt;
> > +
> > + temp_range_cnt = min_t(u16, pse_mod_bprof->temp_mon_ranges,
> > + BATT_TEMP_NR_RNG);
> > + if ((temp < pse_mod_bprof->temp_low_lim) ||
> > + (temp > pse_mod_bprof->temp_mon_range[0].temp_up_lim))
> > + return -EINVAL;
> > +
> > + for (i = 0; i < temp_range_cnt; ++i)
> > + if (temp > pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> > + break;
> > + return i-1;
> > +}
>
> pse_mod_bprof->temp_mon_ranges > BATT_TEMP_NR_RNG is not allowed, so I
> suggest to print an error and return some error code.
>
min_t takes care of the upper bound. The algorithm process BATT_TEMP_NR_RNG
even if the actual number of zones are greater than this.
Agree to other comments
-Jenny
--
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
+static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
+ int temp)
+{
+ int i = 0;
+ int temp_range_cnt;
+
+ temp_range_cnt = min_t(u16, pse_mod_bprof-temp_mon_ranges,
+ BATT_TEMP_NR_RNG);
+ if ((temp pse_mod_bprof-temp_low_lim) ||
+ (temp pse_mod_bprof-temp_mon_range[0].temp_up_lim))
+ return -EINVAL;
+
+ for (i = 0; i temp_range_cnt; ++i)
+ if (temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
+ break;
+ return i-1;
+}
pse_mod_bprof-temp_mon_ranges BATT_TEMP_NR_RNG is not allowed, so I
suggest to print an error and return some error code.
min_t takes care of the upper bound. The algorithm process BATT_TEMP_NR_RNG
even if the actual number of zones are greater than this.
Agree to other comments
-Jenny
--
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Jul 08, 2014 at 06:07:29AM +, Tc, Jenny wrote:
+static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
+ int temp)
+{
+ int i = 0;
+ int temp_range_cnt;
+
+ temp_range_cnt = min_t(u16, pse_mod_bprof-temp_mon_ranges,
+ BATT_TEMP_NR_RNG);
+ if ((temp pse_mod_bprof-temp_low_lim) ||
+ (temp pse_mod_bprof-temp_mon_range[0].temp_up_lim))
+ return -EINVAL;
+
+ for (i = 0; i temp_range_cnt; ++i)
+ if (temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
+ break;
+ return i-1;
+}
pse_mod_bprof-temp_mon_ranges BATT_TEMP_NR_RNG is not allowed, so I
suggest to print an error and return some error code.
min_t takes care of the upper bound. The algorithm process BATT_TEMP_NR_RNG
even if the actual number of zones are greater than this.
Right, the function will not fail, but the zone information table is
truncated. I would expect at least warning about that. I think it
doesn't hurt to have a small function, which validates the zone
data as good as possible. Using incorrect temperature zones is a
safety thread and we should try our best to avoid exploding
batteries ;)
Maybe something like that:
static bool check_tempzones(struct psy_pse_chrg_prof *pse_mod_bprof)
{
int i = 0;
int last_temp = ;
/* check size */
if (BATT_TEMP_NR_RNG pse_mod_bprof-temp_mon_ranges)
return false;
/* check zone order */
for (i = 0; i pse_mod_bprof-temp_mon_ranges; i++) {
if (last_temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
return false;
last_temp = pse_mod_bprof-temp_mon_range[i].temp_up_lim;
}
return true;
}
-- Sebastian
signature.asc
Description: Digital signature
Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi Jenny,
On Mon, Jun 30, 2014 at 03:25:54PM +0530, Jenny TC wrote:
> As per Product Safety Engineering (PSE) specification for battery charging,
> the
> battery characteristics and thereby the charging rates can vary on different
> temperature zones. This patch introduces a PSE compliant charging algorithm
> with
> maintenance charging support. The algorithm can be selected by the power
> supply
> charging driver based on the type of the battery charging profile.
>
> Signed-off-by: Jenny TC
Code looks quite good. I have a couple of minor nits:
> ---
> drivers/power/Kconfig | 15 +++
> drivers/power/Makefile |1 +
> drivers/power/charging_algo_pse.c | 202
>
> include/linux/power/power_supply_charger.h | 63 +
> 4 files changed, 281 insertions(+)
> create mode 100644 drivers/power/charging_algo_pse.c
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index f679f82..54a0321 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -22,6 +22,21 @@ config POWER_SUPPLY_CHARGER
> drivers to keep the charging logic outside and the charger driver
> just need to abstract the charger hardware.
>
> +config POWER_SUPPLY_CHARGING_ALGO_PSE
> + bool "PSE compliant charging algorithm"
> + depends on POWER_SUPPLY_CHARGER
> + help
> + Say Y here to select Product Safety Engineering (PSE) compliant
> + charging algorithm. As per PSE standard the battery characteristics
> + and thereby the charging rates can vary on different temperature
> + zones. Select this if your charging algorithm need to change the
> + charging parameters based on the battery temperature and the battery
> + charging profile follows the struct psy_pse_chrg_prof definition.
> + This config will enable PSE compliant charging algorithm with
> + maintenance charging support. At runtime the algorithm will be
> + selected by the psy charger driver based on the type of the battery
> + charging profile.
> +
> config PDA_POWER
> tristate "Generic PDA/phone power driver"
> depends on !S390
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 405f0f4..77535fd 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_POWER_SUPPLY)+= power_supply.o
> obj-$(CONFIG_GENERIC_ADC_BATTERY)+= generic-adc-battery.o
>
> obj-$(CONFIG_POWER_SUPPLY_CHARGER) += power_supply_charger.o
> +obj-$(CONFIG_POWER_SUPPLY_CHARGING_ALGO_PSE) += charging_algo_pse.o
> obj-$(CONFIG_PDA_POWER) += pda_power.o
> obj-$(CONFIG_APM_POWER) += apm_power.o
> obj-$(CONFIG_MAX8925_POWER) += max8925_power.o
> diff --git a/drivers/power/charging_algo_pse.c
> b/drivers/power/charging_algo_pse.c
> new file mode 100644
> index 000..6ec4873
> --- /dev/null
> +++ b/drivers/power/charging_algo_pse.c
> @@ -0,0 +1,202 @@
> +/*
> + * Copyright (C) 2012 Intel Corporation
> + *
> + * ~~
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * ~~
> + * Author: Jenny TC
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "power_supply.h"
> +#include "power_supply_charger.h"
> +
> +/* 98% of CV is considered as voltage to detect Full */
> +#define FULL_CV_MIN 98
> +
> +/*
> + * Offset to exit from maintenance charging. In maintenance charging
> + * if the volatge is less than the (maintenance_lower_threshold -
> + * MAINT_EXIT_OFFSET) then system can switch to normal charging
> + */
> +
> +#define MAINT_EXIT_OFFSET 50 /* mV */
> +
> +static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
> + int temp)
> +{
> + int i = 0;
> + int temp_range_cnt;
> +
> + temp_range_cnt = min_t(u16, pse_mod_bprof->temp_mon_ranges,
> + BATT_TEMP_NR_RNG);
> + if ((temp < pse_mod_bprof->temp_low_lim) ||
> + (temp > pse_mod_bprof->temp_mon_range[0].temp_up_lim))
> + return -EINVAL;
> +
> + for (i = 0; i < temp_range_cnt; ++i)
> + if (temp > pse_mod_bprof->temp_mon_range[i].temp_up_lim)
> + break;
> + return i-1;
> +}
pse_mod_bprof->temp_mon_ranges > BATT_TEMP_NR_RNG is not allowed,
so I suggest to print an error
Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi Jenny,
On Mon, Jun 30, 2014 at 03:25:54PM +0530, Jenny TC wrote:
As per Product Safety Engineering (PSE) specification for battery charging,
the
battery characteristics and thereby the charging rates can vary on different
temperature zones. This patch introduces a PSE compliant charging algorithm
with
maintenance charging support. The algorithm can be selected by the power
supply
charging driver based on the type of the battery charging profile.
Signed-off-by: Jenny TC jenny...@intel.com
Code looks quite good. I have a couple of minor nits:
---
drivers/power/Kconfig | 15 +++
drivers/power/Makefile |1 +
drivers/power/charging_algo_pse.c | 202
include/linux/power/power_supply_charger.h | 63 +
4 files changed, 281 insertions(+)
create mode 100644 drivers/power/charging_algo_pse.c
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index f679f82..54a0321 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -22,6 +22,21 @@ config POWER_SUPPLY_CHARGER
drivers to keep the charging logic outside and the charger driver
just need to abstract the charger hardware.
+config POWER_SUPPLY_CHARGING_ALGO_PSE
+ bool PSE compliant charging algorithm
+ depends on POWER_SUPPLY_CHARGER
+ help
+ Say Y here to select Product Safety Engineering (PSE) compliant
+ charging algorithm. As per PSE standard the battery characteristics
+ and thereby the charging rates can vary on different temperature
+ zones. Select this if your charging algorithm need to change the
+ charging parameters based on the battery temperature and the battery
+ charging profile follows the struct psy_pse_chrg_prof definition.
+ This config will enable PSE compliant charging algorithm with
+ maintenance charging support. At runtime the algorithm will be
+ selected by the psy charger driver based on the type of the battery
+ charging profile.
+
config PDA_POWER
tristate Generic PDA/phone power driver
depends on !S390
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 405f0f4..77535fd 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_POWER_SUPPLY)+= power_supply.o
obj-$(CONFIG_GENERIC_ADC_BATTERY)+= generic-adc-battery.o
obj-$(CONFIG_POWER_SUPPLY_CHARGER) += power_supply_charger.o
+obj-$(CONFIG_POWER_SUPPLY_CHARGING_ALGO_PSE) += charging_algo_pse.o
obj-$(CONFIG_PDA_POWER) += pda_power.o
obj-$(CONFIG_APM_POWER) += apm_power.o
obj-$(CONFIG_MAX8925_POWER) += max8925_power.o
diff --git a/drivers/power/charging_algo_pse.c
b/drivers/power/charging_algo_pse.c
new file mode 100644
index 000..6ec4873
--- /dev/null
+++ b/drivers/power/charging_algo_pse.c
@@ -0,0 +1,202 @@
+/*
+ * Copyright (C) 2012 Intel Corporation
+ *
+ * ~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * ~~
+ * Author: Jenny TC jenny...@intel.com
+ */
+
+#include linux/module.h
+#include linux/types.h
+#include linux/init.h
+#include linux/slab.h
+#include linux/device.h
+#include linux/err.h
+#include linux/power_supply.h
+#include linux/thermal.h
+#include power_supply.h
+#include power_supply_charger.h
+
+/* 98% of CV is considered as voltage to detect Full */
+#define FULL_CV_MIN 98
+
+/*
+ * Offset to exit from maintenance charging. In maintenance charging
+ * if the volatge is less than the (maintenance_lower_threshold -
+ * MAINT_EXIT_OFFSET) then system can switch to normal charging
+ */
+
+#define MAINT_EXIT_OFFSET 50 /* mV */
+
+static int get_tempzone(struct psy_pse_chrg_prof *pse_mod_bprof,
+ int temp)
+{
+ int i = 0;
+ int temp_range_cnt;
+
+ temp_range_cnt = min_t(u16, pse_mod_bprof-temp_mon_ranges,
+ BATT_TEMP_NR_RNG);
+ if ((temp pse_mod_bprof-temp_low_lim) ||
+ (temp pse_mod_bprof-temp_mon_range[0].temp_up_lim))
+ return -EINVAL;
+
+ for (i = 0; i temp_range_cnt; ++i)
+ if (temp pse_mod_bprof-temp_mon_range[i].temp_up_lim)
+ break;
+ return i-1;
+}
pse_mod_bprof-temp_mon_ranges BATT_TEMP_NR_RNG is not allowed,
so I suggest to print an
Re: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Mar 07, 2014 at 11:34:14AM +0800, Linus Walleij wrote: > On Fri, Feb 28, 2014 at 11:07 AM, Jenny Tc wrote: > > On Thu, Feb 27, 2014 at 09:18:57PM +0100, Linus Walleij wrote: > >> On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC 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? > > The preference is different, usually __ is for compiler things, but > while I dislike it (disturbs my perception) I can sure live with it. Fixed in patch set v7 -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Feb 28, 2014 at 11:07 AM, Jenny Tc wrote: > On Thu, Feb 27, 2014 at 09:18:57PM +0100, Linus Walleij wrote: >> On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC 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? The preference is different, usually __ is for compiler things, but while I dislike it (disturbs my perception) I can sure live with it. >> 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. OK I buy that. Make sure this is noted somewhere (or maybe I missed it). 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Feb 28, 2014 at 11:07 AM, Jenny Tc jenny...@intel.com wrote: 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...@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? The preference is different, usually __ is for compiler things, but while I dislike it (disturbs my perception) I can sure live with it. 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. OK I buy that. Make sure this is noted somewhere (or maybe I missed it). 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Mar 07, 2014 at 11:34:14AM +0800, Linus Walleij wrote: On Fri, Feb 28, 2014 at 11:07 AM, Jenny Tc jenny...@intel.com wrote: 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...@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? The preference is different, usually __ is for compiler things, but while I dislike it (disturbs my perception) I can sure live with it. Fixed in patch set v7 -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi! > > > 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? > > > > It is static; everybody sees it is local. __ prefix usually means > > something else. > > Agreed, I will remove the __ prefix in next patchset. Meanwhile I would > appreciate if anyone could help me to understand what __ prefix > really means. If you have __foo() and foo(), the __foo() is typically worker function, where foo() provides locking around it etc. 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi! 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? It is static; everybody sees it is local. __ prefix usually means something else. Agreed, I will remove the __ prefix in next patchset. Meanwhile I would appreciate if anyone could help me to understand what __ prefix really means. If you have __foo() and foo(), the __foo() is typically worker function, where foo() provides locking around it etc. 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Feb 28, 2014 at 11:08:16AM +0100, Pavel Machek wrote: > On Fri 2014-02-28 08:37:27, Jenny Tc wrote: > > On Thu, Feb 27, 2014 at 09:18:57PM +0100, Linus Walleij wrote: > > > On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC 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? > > It is static; everybody sees it is local. __ prefix usually means > something else. Agreed, I will remove the __ prefix in next patchset. Meanwhile I would appreciate if anyone could help me to understand what __ prefix really means. > > > > + 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. > > Do you need to do endianness conversion, too? May/may not depending on the stored format. If needed, the endianess conversion should be done at driver level where the EEPROM/emmc reading happens. -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Feb 28, 2014 at 11:08:16AM +0100, Pavel Machek wrote: On Fri 2014-02-28 08:37:27, Jenny Tc wrote: 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...@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? It is static; everybody sees it is local. __ prefix usually means something else. Agreed, I will remove the __ prefix in next patchset. Meanwhile I would appreciate if anyone could help me to understand what __ prefix really means. + 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. Do you need to do endianness conversion, too? May/may not depending on the stored format. If needed, the endianess conversion should be done at driver level where the EEPROM/emmc reading happens. -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri 2014-02-28 08:37:27, Jenny Tc wrote: > On Thu, Feb 27, 2014 at 09:18:57PM +0100, Linus Walleij wrote: > > On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC 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? It is static; everybody sees it is local. __ prefix usually means something else. > > > + 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. Do you need to do endianness conversion, too? 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri 2014-02-28 08:37:27, Jenny Tc wrote: 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...@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? It is static; everybody sees it is local. __ prefix usually means something else. + 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. Do you need to do endianness conversion, too? 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Thu, Feb 27, 2014 at 09:18:57PM +0100, Linus Walleij wrote:
> On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC 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 ?
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.
--
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Thu, Feb 27, 2014 at 08:47:07PM +0100, Linus Walleij wrote: > On Wed, Feb 26, 2014 at 3:54 AM, Jenny Tc wrote: > > > The idea is to allow pluggable charging algorithms. Currently we have only > > one > > charging algorithm proposed, but can have other charging algorithms (like > > pulse > > charging, rule based charging etc.). Based on the platform need, the > > algorithms > > can be selected. So this should be a user configurable option. I can add > > more > > explanation on when to select this option. > > Do you see a generic framework for pluggable algorithms on an abstracted > level, so that it could be used for the CC/CV charging, measurement and > temperature check algorithm that is found in e.g. > drivers/power/abx500_chargalg.c > drivers/power/ab8500_charger.c etc, or do you envision a set of pluggable > algorithms for this one hardware? > > I'm asking because these drivers are a massive set of code and we may > need to start to abstract out and define library functions and frameworks > already now before it becomes impossible to contain. The idea of power_supply_charger driver is to move the charging logic outside of the charger chip driver. This makes the charger chip driver as a h/w abstraction layer without having any charging logic in it. power supply charger driver invokes charging algorithm to decide the CC, CV and to stop/start charging on different conditions (based on voltage/temperature ...). Detailed note on using power supply charger driver can be found in Documentation/power/power_supply_charger.txt which is part of patch power_supply-Introduce-generic-psy-charging-driver.patch -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Wed 2014-02-26 08:24:44, Jenny Tc wrote: > On Fri, Feb 21, 2014 at 03:45:29PM +0100, Pavel Machek wrote: > > On Thu 2014-02-20 10:46:55, Jenny Tc wrote: > > > On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: > > > > > --- a/drivers/power/Kconfig > > > > > +++ b/drivers/power/Kconfig > > > > > @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER > > > > > drivers to keep the charging logic outside and the charger > > > > > driver > > > > > just need to abstract the charger hardware. > > > > > > > > > > +config POWER_SUPPLY_CHARGING_ALGO_PSE > > > > > + bool "PSE compliant charging algorithm" > > > > > + help > > > > > + Say Y here to select Product Safety Engineering (PSE) > > > > > compliant > > > > > + charging algorithm. As per PSE standard the battery > > > > > characteristics > > > > > + and thereby the charging rates can vary on different > > > > > temperature > > > > > + zones. This config will enable PSE compliant charging > > > > > algorithm with > > > > > + maintenance charging support. At runtime the algorithm will be > > > > > + selected by the psy charger driver based on the type of the > > > > > battery > > > > > + charging profile. > > > > > > > > Information where to expect PSE compliant chargers would be nice. > > > > > > This algorithm can be used with non PSE compliant chargers also. This is > > > a SW > > > based charging algorithm. > > > > Ok, but you need to explain for the users when it might be good idea > > to select this option... > > > > Or maybe this should not be user configurable and drivers should just > > select it? > > The idea is to allow pluggable charging algorithms. Currently we have only one > charging algorithm proposed, but can have other charging algorithms (like > pulse > charging, rule based charging etc.). Based on the platform need, the > algorithms > can be selected. So this should be a user configurable option. I can add more > explanation on when to select this option. Yes please. Because with current description, user has no chance. 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC 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?
(...)
> +/* 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.
Convert all comments to kerneldoc.
> +#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 ?
> + 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.
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Wed, Feb 26, 2014 at 3:54 AM, Jenny Tc wrote: > The idea is to allow pluggable charging algorithms. Currently we have only one > charging algorithm proposed, but can have other charging algorithms (like > pulse > charging, rule based charging etc.). Based on the platform need, the > algorithms > can be selected. So this should be a user configurable option. I can add more > explanation on when to select this option. Do you see a generic framework for pluggable algorithms on an abstracted level, so that it could be used for the CC/CV charging, measurement and temperature check algorithm that is found in e.g. drivers/power/abx500_chargalg.c drivers/power/ab8500_charger.c etc, or do you envision a set of pluggable algorithms for this one hardware? I'm asking because these drivers are a massive set of code and we may need to start to abstract out and define library functions and frameworks already now before it becomes impossible to contain. 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Wed, Feb 26, 2014 at 3:54 AM, Jenny Tc jenny...@intel.com wrote: The idea is to allow pluggable charging algorithms. Currently we have only one charging algorithm proposed, but can have other charging algorithms (like pulse charging, rule based charging etc.). Based on the platform need, the algorithms can be selected. So this should be a user configurable option. I can add more explanation on when to select this option. Do you see a generic framework for pluggable algorithms on an abstracted level, so that it could be used for the CC/CV charging, measurement and temperature check algorithm that is found in e.g. drivers/power/abx500_chargalg.c drivers/power/ab8500_charger.c etc, or do you envision a set of pluggable algorithms for this one hardware? I'm asking because these drivers are a massive set of code and we may need to start to abstract out and define library functions and frameworks already now before it becomes impossible to contain. 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Feb 4, 2014 at 6:12 AM, Jenny TC jenny...@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?
(...)
+/* 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.
Convert all comments to kerneldoc.
+#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?
+ 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.
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Wed 2014-02-26 08:24:44, Jenny Tc wrote: On Fri, Feb 21, 2014 at 03:45:29PM +0100, Pavel Machek wrote: On Thu 2014-02-20 10:46:55, Jenny Tc wrote: On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER drivers to keep the charging logic outside and the charger driver just need to abstract the charger hardware. +config POWER_SUPPLY_CHARGING_ALGO_PSE + bool PSE compliant charging algorithm + help + Say Y here to select Product Safety Engineering (PSE) compliant + charging algorithm. As per PSE standard the battery characteristics + and thereby the charging rates can vary on different temperature + zones. This config will enable PSE compliant charging algorithm with + maintenance charging support. At runtime the algorithm will be + selected by the psy charger driver based on the type of the battery + charging profile. Information where to expect PSE compliant chargers would be nice. This algorithm can be used with non PSE compliant chargers also. This is a SW based charging algorithm. Ok, but you need to explain for the users when it might be good idea to select this option... Or maybe this should not be user configurable and drivers should just select it? The idea is to allow pluggable charging algorithms. Currently we have only one charging algorithm proposed, but can have other charging algorithms (like pulse charging, rule based charging etc.). Based on the platform need, the algorithms can be selected. So this should be a user configurable option. I can add more explanation on when to select this option. Yes please. Because with current description, user has no chance. 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Thu, Feb 27, 2014 at 08:47:07PM +0100, Linus Walleij wrote: On Wed, Feb 26, 2014 at 3:54 AM, Jenny Tc jenny...@intel.com wrote: The idea is to allow pluggable charging algorithms. Currently we have only one charging algorithm proposed, but can have other charging algorithms (like pulse charging, rule based charging etc.). Based on the platform need, the algorithms can be selected. So this should be a user configurable option. I can add more explanation on when to select this option. Do you see a generic framework for pluggable algorithms on an abstracted level, so that it could be used for the CC/CV charging, measurement and temperature check algorithm that is found in e.g. drivers/power/abx500_chargalg.c drivers/power/ab8500_charger.c etc, or do you envision a set of pluggable algorithms for this one hardware? I'm asking because these drivers are a massive set of code and we may need to start to abstract out and define library functions and frameworks already now before it becomes impossible to contain. The idea of power_supply_charger driver is to move the charging logic outside of the charger chip driver. This makes the charger chip driver as a h/w abstraction layer without having any charging logic in it. power supply charger driver invokes charging algorithm to decide the CC, CV and to stop/start charging on different conditions (based on voltage/temperature ...). Detailed note on using power supply charger driver can be found in Documentation/power/power_supply_charger.txt which is part of patch power_supply-Introduce-generic-psy-charging-driver.patch -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
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...@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.
--
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Feb 21, 2014 at 03:45:29PM +0100, Pavel Machek wrote: > On Thu 2014-02-20 10:46:55, Jenny Tc wrote: > > On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: > > > > --- a/drivers/power/Kconfig > > > > +++ b/drivers/power/Kconfig > > > > @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER > > > > drivers to keep the charging logic outside and the charger > > > > driver > > > > just need to abstract the charger hardware. > > > > > > > > +config POWER_SUPPLY_CHARGING_ALGO_PSE > > > > + bool "PSE compliant charging algorithm" > > > > + help > > > > + Say Y here to select Product Safety Engineering (PSE) > > > > compliant > > > > + charging algorithm. As per PSE standard the battery > > > > characteristics > > > > + and thereby the charging rates can vary on different > > > > temperature > > > > + zones. This config will enable PSE compliant charging > > > > algorithm with > > > > + maintenance charging support. At runtime the algorithm will be > > > > + selected by the psy charger driver based on the type of the > > > > battery > > > > + charging profile. > > > > > > Information where to expect PSE compliant chargers would be nice. > > > > This algorithm can be used with non PSE compliant chargers also. This is a > > SW > > based charging algorithm. > > Ok, but you need to explain for the users when it might be good idea > to select this option... > > Or maybe this should not be user configurable and drivers should just > select it? The idea is to allow pluggable charging algorithms. Currently we have only one charging algorithm proposed, but can have other charging algorithms (like pulse charging, rule based charging etc.). Based on the platform need, the algorithms can be selected. So this should be a user configurable option. I can add more explanation on when to select this option. -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Fri, Feb 21, 2014 at 03:45:29PM +0100, Pavel Machek wrote: On Thu 2014-02-20 10:46:55, Jenny Tc wrote: On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER drivers to keep the charging logic outside and the charger driver just need to abstract the charger hardware. +config POWER_SUPPLY_CHARGING_ALGO_PSE + bool PSE compliant charging algorithm + help + Say Y here to select Product Safety Engineering (PSE) compliant + charging algorithm. As per PSE standard the battery characteristics + and thereby the charging rates can vary on different temperature + zones. This config will enable PSE compliant charging algorithm with + maintenance charging support. At runtime the algorithm will be + selected by the psy charger driver based on the type of the battery + charging profile. Information where to expect PSE compliant chargers would be nice. This algorithm can be used with non PSE compliant chargers also. This is a SW based charging algorithm. Ok, but you need to explain for the users when it might be good idea to select this option... Or maybe this should not be user configurable and drivers should just select it? The idea is to allow pluggable charging algorithms. Currently we have only one charging algorithm proposed, but can have other charging algorithms (like pulse charging, rule based charging etc.). Based on the platform need, the algorithms can be selected. So this should be a user configurable option. I can add more explanation on when to select this option. -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Thu 2014-02-20 10:46:55, Jenny Tc wrote: > On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: > > > --- a/drivers/power/Kconfig > > > +++ b/drivers/power/Kconfig > > > @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER > > > drivers to keep the charging logic outside and the charger driver > > > just need to abstract the charger hardware. > > > > > > +config POWER_SUPPLY_CHARGING_ALGO_PSE > > > + bool "PSE compliant charging algorithm" > > > + help > > > + Say Y here to select Product Safety Engineering (PSE) compliant > > > + charging algorithm. As per PSE standard the battery characteristics > > > + and thereby the charging rates can vary on different temperature > > > + zones. This config will enable PSE compliant charging algorithm with > > > + maintenance charging support. At runtime the algorithm will be > > > + selected by the psy charger driver based on the type of the battery > > > + charging profile. > > > > Information where to expect PSE compliant chargers would be nice. > > This algorithm can be used with non PSE compliant chargers also. This is a SW > based charging algorithm. Ok, but you need to explain for the users when it might be good idea to select this option... Or maybe this should not be user configurable and drivers should just select 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-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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Thu 2014-02-20 10:46:55, Jenny Tc wrote: On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER drivers to keep the charging logic outside and the charger driver just need to abstract the charger hardware. +config POWER_SUPPLY_CHARGING_ALGO_PSE + bool PSE compliant charging algorithm + help + Say Y here to select Product Safety Engineering (PSE) compliant + charging algorithm. As per PSE standard the battery characteristics + and thereby the charging rates can vary on different temperature + zones. This config will enable PSE compliant charging algorithm with + maintenance charging support. At runtime the algorithm will be + selected by the psy charger driver based on the type of the battery + charging profile. Information where to expect PSE compliant chargers would be nice. This algorithm can be used with non PSE compliant chargers also. This is a SW based charging algorithm. Ok, but you need to explain for the users when it might be good idea to select this option... Or maybe this should not be user configurable and drivers should just select 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-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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: > > --- a/drivers/power/Kconfig > > +++ b/drivers/power/Kconfig > > @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER > > drivers to keep the charging logic outside and the charger driver > > just need to abstract the charger hardware. > > > > +config POWER_SUPPLY_CHARGING_ALGO_PSE > > + bool "PSE compliant charging algorithm" > > + help > > + Say Y here to select Product Safety Engineering (PSE) compliant > > + charging algorithm. As per PSE standard the battery characteristics > > + and thereby the charging rates can vary on different temperature > > + zones. This config will enable PSE compliant charging algorithm with > > + maintenance charging support. At runtime the algorithm will be > > + selected by the psy charger driver based on the type of the battery > > + charging profile. > > Information where to expect PSE compliant chargers would be nice. This algorithm can be used with non PSE compliant chargers also. This is a SW based charging algorithm. -Jenny -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
On Tue, Feb 04, 2014 at 12:36:40PM +0100, Pavel Machek wrote: --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER drivers to keep the charging logic outside and the charger driver just need to abstract the charger hardware. +config POWER_SUPPLY_CHARGING_ALGO_PSE + bool PSE compliant charging algorithm + help + Say Y here to select Product Safety Engineering (PSE) compliant + charging algorithm. As per PSE standard the battery characteristics + and thereby the charging rates can vary on different temperature + zones. This config will enable PSE compliant charging algorithm with + maintenance charging support. At runtime the algorithm will be + selected by the psy charger driver based on the type of the battery + charging profile. Information where to expect PSE compliant chargers would be nice. This algorithm can be used with non PSE compliant chargers also. This is a SW based charging algorithm. -Jenny -- 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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi!
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER
> drivers to keep the charging logic outside and the charger driver
> just need to abstract the charger hardware.
>
> +config POWER_SUPPLY_CHARGING_ALGO_PSE
> + bool "PSE compliant charging algorithm"
> + help
> + Say Y here to select Product Safety Engineering (PSE) compliant
> + charging algorithm. As per PSE standard the battery characteristics
> + and thereby the charging rates can vary on different temperature
> + zones. This config will enable PSE compliant charging algorithm with
> + maintenance charging support. At runtime the algorithm will be
> + selected by the psy charger driver based on the type of the battery
> + charging profile.
Information where to expect PSE compliant chargers would be nice.
> +static inline bool __is_battery_full
> + (long volt, long cur, long iterm, unsigned long cv)
> +{
codingstyle.
> + pr_devel("%s:current=%ld pse_mod_bprof->chrg_term_mA =%ld
> voltage_now=%ld full_cond=%ld",
> + __func__, cur, iterm, volt * 100, (FULL_CV_MIN * cv));
> +
> + return (cur > 0) && (cur <= iterm) &&
> + ((volt * 100) >= (FULL_CV_MIN * cv));
Codingstyle. Just run checkpatch.
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi!
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -22,6 +22,19 @@ config POWER_SUPPLY_CHARGER
drivers to keep the charging logic outside and the charger driver
just need to abstract the charger hardware.
+config POWER_SUPPLY_CHARGING_ALGO_PSE
+ bool PSE compliant charging algorithm
+ help
+ Say Y here to select Product Safety Engineering (PSE) compliant
+ charging algorithm. As per PSE standard the battery characteristics
+ and thereby the charging rates can vary on different temperature
+ zones. This config will enable PSE compliant charging algorithm with
+ maintenance charging support. At runtime the algorithm will be
+ selected by the psy charger driver based on the type of the battery
+ charging profile.
Information where to expect PSE compliant chargers would be nice.
+static inline bool __is_battery_full
+ (long volt, long cur, long iterm, unsigned long cv)
+{
codingstyle.
+ pr_devel(%s:current=%ld pse_mod_bprof-chrg_term_mA =%ld
voltage_now=%ld full_cond=%ld,
+ __func__, cur, iterm, volt * 100, (FULL_CV_MIN * cv));
+
+ return (cur 0) (cur = iterm)
+ ((volt * 100) = (FULL_CV_MIN * cv));
Codingstyle. Just run checkpatch.
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi!
> +config POWER_SUPPLY_CHARGING_ALGO_PSE
> + bool "PSE compliant charging algorithm"
> + help
> + Say Y here to select PSE compliant charging algorithm. As per PSE
> + standard the battery characteristics and thereby the charging rates
It would be good to explain PSE here.
> +#define MAINT_EXIT_OFFSET 50 /* mv */
mV
> +/* PSE Modified Algo Structure */
> +/* Parameters defining the charging range */
> +struct psy_ps_temp_chg_table {
> + /* upper temperature limit for each zone */
> + short int temp_up_lim;
> + /* charge current and voltage */
> + short int full_chrg_vol;
> + short int full_chrg_cur;
It would be good to add units here. And vowels.
> + /* type of battery */
> + u16 battery_type;
> + u16 capacity;
> + u16 voltage_max;
> + /* charge termination current */
> + u16 chrg_term_ma;
> + /* Low battery level voltage */
> + u16 low_batt_mv;
> + /* upper and lower temperature limits on discharging */
> + u8 disch_tmp_ul;
> + u8 disch_tmp_ll;
tmp sounds like temporary, not temperature. just use full words.
Add units. And u8 is probably not right type for temperature.
> + /* 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 */
> + short int temp_low_lim;
Add units.
Also check multiline comment codingstyle.
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: [PATCH 3/4] power_supply: Introduce PSE compliant algorithm
Hi!
+config POWER_SUPPLY_CHARGING_ALGO_PSE
+ bool PSE compliant charging algorithm
+ help
+ Say Y here to select PSE compliant charging algorithm. As per PSE
+ standard the battery characteristics and thereby the charging rates
It would be good to explain PSE here.
+#define MAINT_EXIT_OFFSET 50 /* mv */
mV
+/* PSE Modified Algo Structure */
+/* Parameters defining the charging range */
+struct psy_ps_temp_chg_table {
+ /* upper temperature limit for each zone */
+ short int temp_up_lim;
+ /* charge current and voltage */
+ short int full_chrg_vol;
+ short int full_chrg_cur;
It would be good to add units here. And vowels.
+ /* type of battery */
+ u16 battery_type;
+ u16 capacity;
+ u16 voltage_max;
+ /* charge termination current */
+ u16 chrg_term_ma;
+ /* Low battery level voltage */
+ u16 low_batt_mv;
+ /* upper and lower temperature limits on discharging */
+ u8 disch_tmp_ul;
+ u8 disch_tmp_ll;
tmp sounds like temporary, not temperature. just use full words.
Add units. And u8 is probably not right type for temperature.
+ /* 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 */
+ short int temp_low_lim;
Add units.
Also check multiline comment codingstyle.
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/
