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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 22 Sep 2015 15:44:32 +0200
From:	Sebastian Reichel <sre@...nel.org>
To:	Enric Balletbo i Serra <eballetbo@...il.com>
Cc:	devicetree@...r.kernel.org, linux-omap@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	dbaryshkov@...il.com, dwmw2@...radead.org, tony@...mide.com,
	sameo@...ux.intel.com, lee.jones@...aro.org,
	sjoerd.simons@...labora.co.uk, javier@...hile0.org
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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)