Re: [PATCH 2/3] power_supply: Add support for tps65217-charger.

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]