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
signature.asc
Description: Digital signature
