lkml.org 
[lkml]   [2014]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/4] power_supply: Introduce generic psy charging driver
    Hi!

    > +Throttling configuration example:
    > +
    > +struct psy_throttle_state my_throttle_states[] = {
    > +
    > + /* Level 0: Limit charge current to 1500mA. Normal Level */
    > + {
    > + .throttle_action = PSY_THROTTLE_CC_LIMIT,
    > + .throttle_val = 1500,
    > + },
    > +
    > + /* Level 1: Limit charge current to 500mA */
    > + {
    > + .throttle_action = PSY_THROTTLE_CC_LIMIT,
    > + .throttle_val = 500,
    > + },
    > +
    > + /*
    > + * Level 2: Disable charging: Stop charging, charger supply power to
    > + * platform.
    > + */
    > + {
    > + .throttle_action = PSY_THROTTLE_DISABLE_CHARGING,
    > + },
    > +
    > + /* Level 3: Disable charger: Battery start discharging */
    > + {
    > + .throttle_action = PSY_THROTTLE_DISABLE_CHARGER,
    > + },
    > +
    > +};


    Does it make sense to have throttling description as a data, as
    opposed to normal C code?

    > +struct psy_charger_context {
    > + bool is_usb_cable_evt_reg;
    > + int psyc_cnt;
    > + int batt_status;
    > + /*cache battery and charger properties */

    Comment coding style. Please run you patches through checkpatch.

    > + struct list_head evt_queue;
    > + struct mutex evt_lock;
    > + struct list_head event_queue;
    > + struct psy_batt_chrg_prof batt_property;

    Again, please use full words in variable names. How am I supposed to
    know what evt_queue is? Especially when you have event_queue, also?!

    And please do it globally.

    > +static void __power_supply_trigger_charging_handler(struct power_supply *psy)
    > +{
    > + int i;
    > + struct power_supply *psb = NULL;
    > +
    > +
    > + if (is_trigger_charging_algo(psy)) {
    > + if (psy_is_battery(psy)) {
    > + if (trigger_algo(psy))
    > + enable_supplied_by_charging(psy, false);
    > + else
    > + enable_supplied_by_charging(psy, true);
    > + } else if (psy_is_charger(psy)) {
    > + for (i = 0; i < psy->num_supplicants; i++) {
    > + psb =
    > + power_supply_get_by_name(psy->
    > + supplied_to[i]);
    > +
    > + if (psb && psy_is_battery(psb) &&
    > + psy_is_present(psb)) {
    > + if (trigger_algo(psb)) {
    > + psy_disable_charging(psy);
    > + break;
    > + } else if
    > + (psy_is_charging_can_be_enabled
    > + (psy)) {
    > + psy_enable_charging(psy);
    > + wait_for_charging_enabled(psy);
    > + }
    > + }
    > + }
    > + }
    > + update_sysfs(psy);
    > + power_supply_changed(psy);
    > + }
    > +}

    See coding style about excessive nesting. Please fix it globally.

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


    \
     
     \ /
      Last update: 2014-02-04 13:41    [W:4.099 / U:0.164 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site