Hi!
> 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...@intel.com>
No, sorry, this still does not look like kernel code.
> +3. Use Charging Driver to setup charging
> +===========================================
I think intention here is to match the lengths?
> +static inline bool psy_is_battery_prop_changed(struct psy_batt_props
> *bat_prop,
> + struct psy_batt_props *bat_cache)
> +{
> + if (!bat_cache)
> + return true;
Are all these !null checks neccessary? I find them excessive
... especially for debug functions below.
> +static inline bool psy_is_charger_prop_changed(struct psy_charger_props
> *prop,
> + struct psy_charger_props *cache_prop)
> +{
> + /* if online/prsent/health/is_charging is changed, then return true */
typo "present".
> +static int __update_supplied_psy(struct device *dev, void *data)
Too many spaces before __.
> + for (i = 0; i < psy->num_supplicants; i++) {
> + psb = power_supply_get_by_name(psy->supplied_to[i]);
> + if (!psb)
> + continue;
> +
> + if (!psb->data)
> + psb->data = devm_kzalloc(psb->dev,
> + sizeof(struct psy_batt_context), GFP_KERNEL);
> +
> + batt_context = psb->data;
> + batt_context->supplied_by_psy
> + [batt_context->num_supplied_by_psy++] = psy;
> + charger_context->supplied_to_psy
> + [charger_context->num_supplied_to_psy++] = psb;
Really strange indent.
What ensures you don't write beyond end of array.
And why have arrays at all? Could you simply link the structures
together?
> +static void cache_successive_samples(long *sample_array, long new_sample)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_CUR_VOLT_SAMPLES - 1; ++i)
> + *(sample_array + i) = *(sample_array + i + 1);
> +
> + *(sample_array + i) = new_sample;
> +}
But this is why I write. Would a ring buffer be faster and more
elegant?
Also we do sample_array[i], not *(sample_array + i).
Oh and we also do i++, not ++i.
> +static int process_cable_props(struct psy_cable_props *cap)
> +{
> + struct charger_cable *cable = NULL;
> + unsigned off;
unsigned int.
> + memcpy((void *)&cable->cable_props, (void *)cap,
> + sizeof(cable->cable_props));
Are the casts actually neccessary?
Would cable->cable_props = caps work as well?
> + spin_lock(&psy_chrgr.event_queue_lock);
> + list_for_each_entry_safe(evt, tmp, &psy_chrgr.event_queue, node) {
> + list_del(&evt->node);
> + spin_unlock(&psy_chrgr.event_queue_lock);
> +
> + mutex_lock(&psy_chrgr.event_lock);
> +
> + switch (evt->event) {
...
> + }
> +
> + mutex_unlock(&psy_chrgr.event_lock);
> +
> + spin_lock(&psy_chrgr.event_queue_lock);
> + kfree(evt);
> + }
> +
> + spin_unlock(&psy_chrgr.event_queue_lock);
> +}
Are you sure about locking here? Care to drop some comments to help us
understand it?
> +static int register_usb_notifier(void)
> +{
> + int retval = 0;
> +
> + otg_xceiver = usb_get_phy(USB_PHY_TYPE_USB2);
> + if (!otg_xceiver) {
> + pr_err("failure to get otg transceiver\n");
> + retval = -EIO;
> + goto notifier_reg_failed;
> + }
just do return -EIO here.
> + retval = usb_register_notifier(otg_xceiver, &nb);
> + if (retval) {
> + pr_err("failure to register otg notifier\n");
> + goto notifier_reg_failed;
> + }
> +
> +notifier_reg_failed:
> + return retval;
> +}
Delete goto and label.
> +static inline bool is_trigger_charging_algo(struct power_supply *psy)
> +{
> + /*
> + * trigger charging algorithm if battery or
> + * charger properties are changed. Also no need to
> + * invoke algorithm for power_supply_changed from
> + * charger, if all supplied_to has the ext_port_changed defined.
> + * On invoking the ext_port_changed the supplied to can send
> + * power_supplied_changed event.
> + */
> +
> + if ((psy_is_charger(psy) && !is_supplied_to_has_ext_pwr_changed(psy)) &&
> + is_chrgr_prop_changed(psy))
> + return true;
> +
> + if ((psy_is_battery(psy)) && (is_batt_prop_changed(psy) ||
> + is_supplied_by_changed(psy)))
> + return true;
> +
Begin function with if (!psy_is_charger...) to simplify the rest?
> +static int get_battery_status(struct power_supply *psy)
> +{
> + int status;
> + struct power_supply *psc;
> + struct psy_batt_context *batt_context;
> + int cnt;
> +
> + if (!psy_is_battery(psy) || (!psy->data))
> + return POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + batt_context = psy->data;
> + status = POWER_SUPPLY_STATUS_DISCHARGING;
> + cnt = batt_context->num_supplied_by_psy;
> +
> + while (cnt--) {
> + psc = batt_context->supplied_by_psy[cnt];
> +
> + if (psy_is_present(psc))
> + status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +
> + if (!(psy_is_charging_can_be_enabled(psc)) ||
> + (!psy_is_health_good(psy)) ||
> + (!psy_is_health_good(psc)))
> + continue;
> + if ((batt_context->algo_stat == PSY_ALGO_STAT_FULL) ||
> + (batt_context->algo_stat == PSY_ALGO_STAT_MAINT))
> + status = POWER_SUPPLY_STATUS_FULL;
> + else if (psy_is_charging_enabled(psc))
> + status = POWER_SUPPLY_STATUS_CHARGING;
> + }
> +
> + pr_debug("%s: Set status=%d for %s\n", __func__, status, psy->name);
> +
> + return status;
So if you have two batteries, return value is basically random?
> +static void update_charger_online(struct power_supply *psy)
> +{
> + int online;
> + struct psy_charger_context *charger_context;
> +
> + online = psy_is_charger_enabled(psy) ? 1 : 0;
!!(..)
> + psy_set_charger_online(psy, online);
> + charger_context = psy->data;
> + charger_context->charger_props.online = online;
> +}
Or perhaps online should work with bools, as you use them elsewhere?
> +static inline void wait_for_charging_enabled(struct power_supply *psy)
> +{
> + wait_event_timeout(psy_chrgr.wait_chrg_enable,
> + (psy_is_charging_enabled(psy)), HZ);
> +}
Is one second timeout enough?
> +static inline void enable_supplied_by_charging(struct power_supply *psy,
> + bool is_enable)
> +{
> + struct power_supply *psc;
> + struct psy_batt_context *batt_context;
> + int cnt;
> +
> + if (!psy_is_battery(psy))
> + return;
> + /*
> + * Get list of chargers supplying power to this battery and
> + * disable charging for all chargers
> + */
> + batt_context = psy->data;
two spaces.
> + cnt = batt_context->num_supplied_by_psy;
> +
> + while (cnt--) {
> + psc = batt_context->supplied_by_psy[cnt];
> + if (!psy_is_present(psc))
> + continue;
> + if (is_enable && psy_is_charging_can_be_enabled(psc)) {
> + psy_enable_charging(psc);
> + wait_for_charging_enabled(psc);
> +
extra empty line
> + } else {
> + psy_disable_charging(psc);
> + }
Do continue and get rid of else?
> + if (!is_trigger_charging_algo(psy))
> + return;
> +
> + if (psy_is_battery(psy)) {
> +
> + enable_supplied_by_charging(psy, !trigger_algo(psy));
> + goto update_sysfs;
> +
> + }
More extra lines.
Best regards,
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/
