linux-kernel - Re: [jenny.tc@...el.com: [RFC] power_supply: Introduce generic psy charging driver]

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20140507084655.GA25243@jenny-desktop>
Date:	Wed, 7 May 2014 14:16:55 +0530
From:	Jenny Tc <jenny.tc@...el.com>
To:	Pavel Machek <pavel@....cz>
Cc:	linux-kernel@...r.kernel.org, dbaryshkov@...il.com,
	cbouatmailru@...il.com, anton.vorontsov@...aro.org,
	tony@...mide.com, linux-omap@...r.kernel.org
Subject: Re: [jenny.tc@...el.com: [RFC] power_supply: Introduce generic psy
 charging driver]

> > +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?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/