Re: [jenny.tc@intel.com: [RFC] power_supply: Introduce generic psy charging driver]

Re: [jenny.tc@intel.com: [RFC] power_supply: Introduce generic psy charging driver]

[Pavel Machek]

Hi!

(Only part of original cc-list preserved.)

> RFC: Fixed comments for patch v8, removed sorting and string comparisons

Ok, its better now.

> The Power Supply charging driver connects multiple subsystems
> to do charging in a generic way. The subsystems involves power_supply,
> thermal and battery communication subsystems (1wire).With this the charging is
> handled in a generic way.
> 
> The driver makes use of different new features - Battery Identification
> interfaces, pluggable charging algorithms, charger cable arbitrations etc.
> The patch also introduces generic interface for charger cable notifications.
> Charger cable events and capabilities can be notified using the generic
> power_supply_notifier chain.
> 
> Overall this driver removes the charging logic out of the charger chip driver
> and the charger chip driver can just listen to the request from the power
> supply charging driver to set the charger properties. This can be implemented
> by exposing get_property and set property callbacks.
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  Documentation/power/power_supply_charger.txt |  350 +++++++++
>  drivers/power/Kconfig                        |    8 +
>  drivers/power/Makefile                       |    1 +
>  drivers/power/power_supply_charger.c         | 1066 ++++++++++++++++++++++++++
>  drivers/power/power_supply_charger.h         |  226 ++++++
>  drivers/power/power_supply_core.c            |    3 +
>  include/linux/power/power_supply_charger.h   |  304 ++++++++
>  include/linux/power_supply.h                 |  161 ++++
>  8 files changed, 2119 insertions(+)
>  create mode 100644 Documentation/power/power_supply_charger.txt
>  create mode 100644 drivers/power/power_supply_charger.c
>  create mode 100644 drivers/power/power_supply_charger.h
>  create mode 100644 include/linux/power/power_supply_charger.h
> 
> diff --git a/Documentation/power/power_supply_charger.txt b/Documentation/power/power_supply_charger.txt
> new file mode 100644
> index 0000000..1bb8cb4
> --- /dev/null
> +++ b/Documentation/power/power_supply_charger.txt
> @@ -0,0 +1,350 @@
> +1. Introduction
> +===============
> +
> +The Power Supply charging driver connects multiple subsystems
> +to do charging in a generic way. The subsystems involves power_supply,
> +thermal and battery communication subsystems (1wire).With this the charging is

Space after '.'.

> +static struct charger_cable cable_list[] = {
> +	{
> +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_SDP,
> +	 },
> +	{
> +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_CDP,
> +	 },
> +	{
> +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_DCP,
> +	 },
> +	{
> +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_ACA,
> +	 },
> +	{
> +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_ACA_DOCK,
> +	 },
> +	{
> +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_SE1,
> +	 },
> +	{
> +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_AC,
> +	 },
> +};

Can we get rid of this?

> +struct usb_phy *otg_xceiver;
> +static int handle_event_notification(struct notifier_block *nb,
> +				   unsigned long event, void *data);
> +struct notifier_block nb = {
> +		   .notifier_call = handle_event_notification,
> +		};

You need to add way more statics.



> +static void update_supplied_to_psy(struct power_supply *psy)
> +{
> +	WARN_ON(charger_context == NULL);

Useless.

> +	for (i = 0; i < psy->num_supplicants; i++) {
> +		charger_context->supplied_to_psy[cnt++] =
> +			power_supply_get_by_name(psy->supplied_to[i]);
> +		charger_context->supplied_to_psy[cnt] = NULL;
> +	}
> +}

Still some name lookups to be killed.

> +		for (i = 0; i < pst->num_supplicants; i++) {
> +			psb = power_supply_get_by_name(pst->supplied_to[i]);
> +			if (psb == psy) {
> +				batt_context->supplied_by_psy[cnt++] = pst;
> +				batt_context->supplied_by_psy[cnt] = NULL;
> +				break;
> +			}
> +		}
> +	}

And here.

> +	WARN_ON(psy->data == NULL);

Useless.

> +	charger_context = (struct psy_charger_context *)psy->data;

> +static inline bool is_supplied_to_has_ext_pwr_changed(struct power_supply *psy)
> +{
> +	int i;
> +	struct power_supply *psb;
> +	bool is_pwr_changed_defined = true;
> +
> +	for (i = 0; i < psy->num_supplicants; i++) {
> +		psb =
> +		    power_supply_get_by_name(psy->
> +					     supplied_to[i]);
> +		if (psb && !psb->external_power_changed)
> +			is_pwr_changed_defined &= false;
> +	}

You did not really get rid of the name lookups..

&= false. Bad idea.


> +static int trigger_algo(struct power_supply *psy)
> +{
> +	unsigned long cc = 0, cv = 0, cc_min;
> +	struct psy_batt_context *batt_context;
> +	struct psy_charging_algo *algo;
> +	struct psy_batt_chrg_prof chrg_profile;
> +	struct power_supply *psc;
> +	int cnt = 0;
> +
> +	if (psy->type != POWER_SUPPLY_TYPE_BATTERY)
> +		return 0;
> +
> +	if (psy_get_battery_prop(&chrg_profile)) {
> +		pr_err("Error in getting charge profile:%s:%d\n", __FILE__,
> +		       __LINE__);
> +		return -EINVAL;
> +	}
> +
> +	batt_context = (struct psy_batt_context *)psy->data;

Create inline function doing the cast. You have too many opencoded
casts.

> +	batt_context->algo_stat = algo->get_next_cc_cv
> +		(&batt_context->batt_props, chrg_profile, &cc, &cv);
> +static inline void enable_supplied_by_charging
> +		(struct power_supply *psy, bool is_enable)

This is not how you indent that, is it.

> +static void __power_supply_trigger_charging_handler(struct power_supply *psy)
> +{
> +	int i;
> +	struct power_supply *psb = NULL;
> +
> +	if (!is_trigger_charging_algo(psy))
> +		return;
> +
> +	if (psy_is_battery(psy)) {
> +		if (trigger_algo(psy))
> +			enable_supplied_by_charging(psy, false);
> +		else
> +			enable_supplied_by_charging(psy, true);

enable...(psy, trigger_algo())



> +static inline int psy_throttle_cc_value
> +		(struct power_supply *psy, unsigned int state)

Indent.


> +#define PSY_MAX_CV(psy) \
> +		psy_get_ps_int_property(psy,\
> +			POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX)
> +#define PSY_VOLTAGE_NOW(psy) \
> +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW)
> +#define PSY_VOLTAGE_OCV(psy) \
> +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_OCV)
> +#define PSY_CURRENT_NOW(psy) \
> +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW)
> +#define PSY_STATUS(psy) \
> +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_STATUS)
> +#define PSY_TEMPERATURE(psy) \
> +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TEMP)
> +#define PSY_BATTERY_TYPE(psy) \
> +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TECHNOLOGY)
> +#define PSY_ONLINE(psy) \
> +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_ONLINE)

This looks like bad idea. Just opencode it.

s/return  /return /g

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [jenny.tc@intel.com: [RFC] power_supply: Introduce generic psy charging driver]

[Jenny Tc]

> > +static struct charger_cable cable_list[] = {
> > +	{
> > +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_SDP,
> > +	 },
> > +	{
> > +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_CDP,
> > +	 },
> > +	{
> > +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_DCP,
> > +	 },
> > +	{
> > +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_ACA,
> > +	 },
> > +	{
> > +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_ACA_DOCK,
> > +	 },
> > +	{
> > +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_SE1,
> > +	 },
> > +	{
> > +	 .psy_cable_type = PSY_CHARGER_CABLE_TYPE_AC,
> > +	 },
> > +};
> 
> Can we get rid of this?

Agreed, can initialize it runtime.

> > +	for (i = 0; i < psy->num_supplicants; i++) {
> > +		charger_context->supplied_to_psy[cnt++] =
> > +			power_supply_get_by_name(psy->supplied_to[i]);
> > +		charger_context->supplied_to_psy[cnt] = NULL;
> > +	}
> > +}
> 
> Still some name lookups to be killed.

The look up is only once, when the charger/battery driver is registered.
The supplied_to is defined as character pointer in struct power_supply.

char **supplied_to;
size_t num_supplicants;

This allows the drivers to define the supplied_to argument even if the
supplied_to driver is loaded at later stage. So the name lookup cannot be
avoided. But it is optimized to do lookup only once
> 
> > +		for (i = 0; i < pst->num_supplicants; i++) {
> > +			psb = power_supply_get_by_name(pst->supplied_to[i]);
> > +			if (psb == psy) {
> > +				batt_context->supplied_by_psy[cnt++] = pst;
> > +				batt_context->supplied_by_psy[cnt] = NULL;
> > +				break;
> > +			}
> > +		}
> > +	}
> 
> And here.

Same here
> > +	charger_context = (struct psy_charger_context *)psy->data;
> 
> > +static inline bool is_supplied_to_has_ext_pwr_changed(struct power_supply *psy)
> > +{
> > +	int i;
> > +	struct power_supply *psb;
> > +	bool is_pwr_changed_defined = true;
> > +
> > +	for (i = 0; i < psy->num_supplicants; i++) {
> > +		psb =
> > +		    power_supply_get_by_name(psy->
> > +					     supplied_to[i]);
> > +		if (psb && !psb->external_power_changed)
> > +			is_pwr_changed_defined &= false;
> > +	}
> 
> You did not really get rid of the name lookups..

This lookup can be removed
> > +#define PSY_MAX_CV(psy) \
> > +		psy_get_ps_int_property(psy,\
> > +			POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX)
> > +#define PSY_VOLTAGE_NOW(psy) \
> > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW)
> > +#define PSY_VOLTAGE_OCV(psy) \
> > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_OCV)
> > +#define PSY_CURRENT_NOW(psy) \
> > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW)
> > +#define PSY_STATUS(psy) \
> > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_STATUS)
> > +#define PSY_TEMPERATURE(psy) \
> > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TEMP)
> > +#define PSY_BATTERY_TYPE(psy) \
> > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TECHNOLOGY)
> > +#define PSY_ONLINE(psy) \
> > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_ONLINE)
> 
> This looks like bad idea. Just opencode it.

This was to make it more readable, and avoids open codes in multiple places.
Initially Anton had positive thoughts about this. Isn't it more readable with
the macros?

Re: [jenny.tc@intel.com: [RFC] power_supply: Introduce generic psy charging driver]

[Pavel Machek]

Hi!

> > > +#define PSY_MAX_CV(psy) \
> > > +		psy_get_ps_int_property(psy,\
> > > +			POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX)
> > > +#define PSY_VOLTAGE_NOW(psy) \
> > > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW)
> > > +#define PSY_VOLTAGE_OCV(psy) \
> > > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_OCV)
> > > +#define PSY_CURRENT_NOW(psy) \
> > > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW)
> > > +#define PSY_STATUS(psy) \
> > > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_STATUS)
> > > +#define PSY_TEMPERATURE(psy) \
> > > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TEMP)
> > > +#define PSY_BATTERY_TYPE(psy) \
> > > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TECHNOLOGY)
> > > +#define PSY_ONLINE(psy) \
> > > +		psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_ONLINE)
> > 
> > This looks like bad idea. Just opencode it.
> 
> This was to make it more readable, and avoids open codes in multiple places.
> Initially Anton had positive thoughts about this. Isn't it more readable with
> the macros?

Well... I'd expect PSY_ONLINE() macro to do something like (psy &
0x01), not call function. Yes, it is shorter, but IMO it is not
clearer.

But if Anton likes it...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html