linux-kernel - Re: RE: [PATCH v2] charger_manager: update charge profile upon temperature zone change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:	Wed, 09 May 2012 08:09:30 +0000 (GMT)
From:	함명주 <myungjoo.ham@...sung.com>
To:	"Pallala, Ramakrishna" <ramakrishna.pallala@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:	Anton Vorontsov <cbouatmailru@...il.com>,
	Anton Vorontsov <anton.vorontsov@...aro.org>,
	최찬우 <cw00.choi@...sung.com>,
	jenny.tc@...el.com
Subject: Re: RE: [PATCH v2] charger_manager: update charge profile upon
 temperature zone change

> > > This patch allows the Charger-Manager to adjust the charging
> > > parameters upon events like VBUS rise or drop and allows batteries to
> > > have multiple charge profiles for different temperature zones.
> > >
> > > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@...el.com>
> > 
> > I don't see how the parameters are changed when update_charger is true.
> My initial thought was to keep these details hide from CM.

We are integrating charger max current configuration with charger status:
e.g., whether "TA" is connected, "USB" is connected, or "Solar" is connected
should determine the current configuration. In our testbed system, if "TA" is
connected, it becomes (regulator_set_current_limit) < 1A and it becomes 500mA
if "USB 2.0" is connected.

Such information is given to the charger manager instance via charger_desc along
with current limit. We will release the patchset after applying and testing in
our testbed.

Thus, the details may/should be in CM; we will be controlling them in CM anyway.

The data structure will look like this (this is an abstract and psuedo):
struct charger_cable {
	const char *extcon_dev_name;
	const char *extcon_cable_name;
	unsigned long current_uA;
};
struct charger {
	const char *regulator_name;
	ARRAY of struct charger_cable;
};
struct charger_desc {
	...
	ARRAY of struct charger;
};

I'm not sure whether the final value will be max(enabled_cable_uA) or
sum(enabled_cable_uA). And we might need another "current_limit_uA"
in struct charger.

> 
> > Are you intending to do it at userspace after getting uevent_notify()? (I don't think
> > it's good)
> No, we will do it from driver only.

Fine.

> 
> > If the intension is to update some of the charger-manager internal parameters (struct
> > charger_manager's struct charger_desc) according to the temperature, we'd need a more
> > general method that can also update values in the charger-manager context.
> > 
> > For example, instead of simply putting a callback to determine whether an update is
> > required or not, a table of (including hysterisis) temperatures and values to be updated
> > (or callbacks to update charger_desc based on the temperature) might be a starting
> > point. You may also need to consider using notifier chain w/ temperatures.
> > 
> Yes I agree, I will submit another patch with these changes.
> 
> As part of charge enablement we generally program charge current, charge voltage
> into the charger chip.
> 
> We can pass the charging parameters CC and CV in two ways.
> 1. Add these params in charger_desc struct and the  charger regulator can get these
> params using container_of() call? but becomes complex.
> 
> 2. use regulator_set_voltage()/regulator_set_current_limit() functions to set the CV and CC params.
> but not suitable as is, we have add support in regulator framework
> 
> 3. use regulator_get_drvdata()/regulator_set_drvdata() to set CC & CV params. These functions
> allow us to add more params in future if required.
> 
> I am thinking of using option 3.
> 
> Let me know your feedback.

I'd suggest you to do option 2 with the following interface.

Anyway, (reading another mails from this thread) it appears that it may look
like this, assuming that the charger_current control regarding the charger-type
based on Extcon cannot be applied to this:

struct charger_temp_notify {
	bool high; /* true: notifies if it goes higher from lower */
	int notify_mC;
	int recovery_mC;
	struct notifier_block nb;
	struct charger_desc *desc; /* CM will automatically set this */
};
struct charger_desc {
	...
	ARRAY of struct charger_temp_notify;
};

If you want to alarm at 80C and turn it off at 75C;
	{ high = true, notify_mC = 80000, recovery_mc = 75000, ... };

Then you can handle temperature-based events at your own charger device driver.


Cheers!
MyungJoo.