Re: [jenny...@intel.com: [RFC] power_supply: Introduce generic psy charging driver]
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 -- 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: [jenny...@intel.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 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: [jenny...@intel.com: [RFC] power_supply: Introduce generic psy charging driver]
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
> ---
> 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 000..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;
Re: [RFC] power_supply: Introduce generic psy charging driver
On Mon, Apr 28, 2014 at 07:56:06PM +0200, Pavel Machek wrote: > On Mon 2014-04-28 22:24:36, Jenny Tc wrote: > > Dmitry/Pavel, > > > > Request your feedback on this. Fixed the comments from Pavel and waiting > > for > > your feedback on the changes > > IIRC, my latest comments were "this is completely misdesigned, it is > using strings and table searches where it should use pointers". Did > that change? Yes, that's changed. > > -- > (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: [RFC] power_supply: Introduce generic psy charging driver
On Mon 2014-04-28 22:24:36, Jenny Tc wrote: > Dmitry/Pavel, > > Request your feedback on this. Fixed the comments from Pavel and waiting for > your feedback on the changes IIRC, my latest comments were "this is completely misdesigned, it is using strings and table searches where it should use pointers". Did that change? 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/
