RE: [RFC 1/4] power_supply: Introduce charging object table

RE: [RFC 1/4] power_supply: Introduce charging object table

[Tc, Jenny]

Hi,

> > +#define PSY_MAX_BAT_NAME_LEN 8
> > +#define PSY_MAX_TEMP_ZONE 6
> > +
> > +struct psy_charging_obj {
> 
> This is not just about charging data, but also about the batteries
> thermal limits, technology and full capacity, so how about
> 
> struct psy_battery_information {

Ok, Agree.
> 
> > +	char name[PSY_MAX_BAT_NAME_LEN];
> 
> char *name;
> 
> No need for arbitrary length limitation.

The length limitation is introduced to form a packed structure so that
the data can be read directly from memory without parsing.

> 
> > +	int battery_type;
> > +	int temp_max;
> > +	int temp_min;
> > +	int full_condition_soc;
> 
> Please be more verbose about the information being stored here.

Ok..Agree.
> 
> > +	int full_condition_capacity;
> > +	int full_condition_voltage;
> > +	int iterm; /* charge termination current */
> > +	/* CC/CV table for different temperature range */
> > +	int temp_mon_count; /* number of entries in temp_mon_table */
> > +	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
> 
> No need to embed this into the struct. Just point to the array and
> remove the size limitation.

This is to make sure that the array is packed so that the data can be
read directly from memory without parsing.

-Jenny

Re: [RFC 1/4] power_supply: Introduce charging object table

['Sebastian Reichel']

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

Hi,

On Mon, Mar 09, 2015 at 12:26:21PM +0000, Tc, Jenny wrote:
> > > +	char name[PSY_MAX_BAT_NAME_LEN];
> > 
> > char *name;
> > 
> > No need for arbitrary length limitation.
> 
> The length limitation is introduced to form a packed structure so that
> the data can be read directly from memory without parsing.

This reason is ok for a device driver, but not for a kernel
subsystem.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [RFC 1/4] power_supply: Introduce charging object table

[Tc, Jenny]

> 
> On Mon, Mar 09, 2015 at 12:26:21PM +0000, Tc, Jenny wrote:
> > > > +	char name[PSY_MAX_BAT_NAME_LEN];
> > >
> > > char *name;
> > >
> > > No need for arbitrary length limitation.
> >
> > The length limitation is introduced to form a packed structure so that
> > the data can be read directly from memory without parsing.
> 
> This reason is ok for a device driver, but not for a kernel
> subsystem.

Agree, will fix this. Same for psy_temp_mon_table temp_mon_table

-Jenny

Re: [RFC 1/4] power_supply: Introduce charging object table

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

Hi,

On Fri, Mar 06, 2015 at 12:12:50PM +0100, Oliver Neukum wrote:
> On Fri, 2015-03-06 at 16:03 +0530, Jenny TC wrote:
> > +struct psy_temp_mon_table {
> > +	int temp_max;
> > +	int temp_min;
> > +	int charging_current; /* CC */
> > +	int charging_voltage; /* CV */
> 
> In which units?

First few lines from power_supply.h:

/*
 * All voltages, currents, charges, energies, time and temperatures in uV,
 * µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise
 * stated. It's driver's job to convert its raw values to units in which
 * this class operates.
 */

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/4] power_supply: Introduce charging object table

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]

Hi,

On Fri, Mar 06, 2015 at 04:03:24PM +0530, Jenny TC wrote:
> Charging current (CC) and charging voltage (CV) may vary based on
> battery temperature. To support CC and CV for different temperature
> zones, defined a charging object which holds the properties related
> to battery charging.
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  include/linux/power_supply.h |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 096dbce..7aada44 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -252,6 +252,33 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +
> +struct psy_temp_mon_table {
> +	int temp_max;
> +	int temp_min;
> +	int charging_current; /* CC */
> +	int charging_voltage; /* CV */
> +	/* delta voltage at which charging should restart */
> +	int maint_voltage_delta;
> +};
> +
> +#define PSY_MAX_BAT_NAME_LEN 8
> +#define PSY_MAX_TEMP_ZONE 6
> +
> +struct psy_charging_obj {

This is not just about charging data, but also about the batteries
thermal limits, technology and full capacity, so how about

struct psy_battery_information {

> +	char name[PSY_MAX_BAT_NAME_LEN];

char *name;

No need for arbitrary length limitation.

> +	int battery_type;
> +	int temp_max;
> +	int temp_min;
> +	int full_condition_soc;

Please be more verbose about the information being stored here.

> +	int full_condition_capacity;
> +	int full_condition_voltage;
> +	int iterm; /* charge termination current */
> +	/* CC/CV table for different temperature range */
> +	int temp_mon_count; /* number of entries in temp_mon_table */
> +	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];

No need to embed this into the struct. Just point to the array and
remove the size limitation.

> +};
> +
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/4] power_supply: Introduce charging object table

[Oliver Neukum]

On Fri, 2015-03-06 at 16:03 +0530, Jenny TC wrote:
> Charging current (CC) and charging voltage (CV) may vary based on
> battery temperature. To support CC and CV for different temperature
> zones, defined a charging object which holds the properties related
> to battery charging.
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  include/linux/power_supply.h |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 096dbce..7aada44 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -252,6 +252,33 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +
> +struct psy_temp_mon_table {
> +	int temp_max;
> +	int temp_min;
> +	int charging_current; /* CC */
> +	int charging_voltage; /* CV */

In which units?

> +	/* delta voltage at which charging should restart */
> +	int maint_voltage_delta;
> +};
> +
> +#define PSY_MAX_BAT_NAME_LEN 8
> +#define PSY_MAX_TEMP_ZONE 6
> +
> +struct psy_charging_obj {
> +	char name[PSY_MAX_BAT_NAME_LEN];
> +	int battery_type;
> +	int temp_max;
> +	int temp_min;
> +	int full_condition_soc;
> +	int full_condition_capacity;
> +	int full_condition_voltage;
> +	int iterm; /* charge termination current */
> +	/* CC/CV table for different temperature range */
> +	int temp_mon_count; /* number of entries in temp_mon_table */
> +	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
> +};
> +
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);


-- 
Oliver Neukum <oneukum@suse.de>

[RFC 1/4] power_supply: Introduce charging object table

[Jenny TC]

Charging current (CC) and charging voltage (CV) may vary based on
battery temperature. To support CC and CV for different temperature
zones, defined a charging object which holds the properties related
to battery charging.

Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
 include/linux/power_supply.h |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 096dbce..7aada44 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -252,6 +252,33 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+
+struct psy_temp_mon_table {
+	int temp_max;
+	int temp_min;
+	int charging_current; /* CC */
+	int charging_voltage; /* CV */
+	/* delta voltage at which charging should restart */
+	int maint_voltage_delta;
+};
+
+#define PSY_MAX_BAT_NAME_LEN 8
+#define PSY_MAX_TEMP_ZONE 6
+
+struct psy_charging_obj {
+	char name[PSY_MAX_BAT_NAME_LEN];
+	int battery_type;
+	int temp_max;
+	int temp_min;
+	int full_condition_soc;
+	int full_condition_capacity;
+	int full_condition_voltage;
+	int iterm; /* charge termination current */
+	/* CC/CV table for different temperature range */
+	int temp_mon_count; /* number of entries in temp_mon_table */
+	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
+};
+
 extern struct atomic_notifier_head power_supply_notifier;
 extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
-- 
1.7.9.5