Messages in this thread | | | Date | Tue, 22 Sep 2015 15:44:32 +0200 | From | Sebastian Reichel <> | Subject | Re: [PATCH 2/3] power_supply: Add support for tps65217-charger. |
| |
Hi,
On Tue, Sep 08, 2015 at 10:09:38AM +0200, Enric Balletbo i Serra wrote: > This patch adds support for the tps65217 charger driver. This driver is > responsible for controlling the charger aspect of the tps65217 mfd. > Currently, this mainly consists of turning on and off the charger, but > some other features of the charger can be supported through this driver.
Driver looks mostly fine. I have two comments though:
> [...] > > +static int tps65217_ac_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct tps65217_charger *charger = power_supply_get_drvdata(psy); > + > + if (psp == POWER_SUPPLY_PROP_ONLINE) { > + val->intval = charger->ac_online; > + charger->prev_ac_online = charger->ac_online;
I think prev_ac_online should be set at the beginning of tps65217_charger_irq().
> [...] > > +static int tps65217_charger_probe(struct platform_device *pdev) > +{ > > [...] > > + charger->ac = power_supply_register(&pdev->dev, > + &tps65217_charger_desc, NULL); > + if (IS_ERR(charger->ac)) { > + dev_err(&pdev->dev, "failed: power supply register\n"); > + return PTR_ERR(charger->ac); > + }
You can use devm_power_supply_register(...) to simplify the driver a bit more.
-- Sebastian [unhandled content-type:application/pgp-signature] | |