linux-kernel - Re: [PATCH 08/22] power: supply: add AC power supply driver for AXP20X and AXP22X PMICs

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Jan 2017 04:00:13 +0100
From:   Sebastian Reichel <sre@...nel.org>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>
Cc:     jic23@...nel.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, robh+dt@...nel.org, mark.rutland@....com,
        wens@...e.org, linux@...linux.org.uk,
        maxime.ripard@...e-electrons.com, lee.jones@...aro.org,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        thomas.petazzoni@...e-electrons.com, icenowy@...c.xyz,
        bonbons@...ux-vserver.org
Subject: Re: [PATCH 08/22] power: supply: add AC power supply driver for
 AXP20X and AXP22X PMICs

Hi Quentin,

The driver looks mostly fine. I do have a two comments, though.

On Mon, Jan 02, 2017 at 05:37:08PM +0100, Quentin Schulz wrote:
> [...]
>
> +static int axp20x_ac_power_probe(struct platform_device *pdev)
> +{
> +	static const char * const axp20x_irq_names[] = { "ACIN_PLUGIN",
> +		"ACIN_REMOVAL", NULL };
>
> +	static const char * const *irq_names;
> +	const struct power_supply_desc *ac_power_desc;
> +	int i, irq, ret;
> +
> +	if (!of_device_is_available(pdev->dev.of_node))
> +		return -ENODEV;
> +
> +	if (!axp20x) {
> +		dev_err(&pdev->dev, "Parent drvdata not set\n");
> +		return -EINVAL;
> +	}

axp20x will no longer be needed after implementing below
comments.

> [...]
> +	irq_names = axp20x_irq_names;

Just rename axp20x_irq_names into irq_names, since its only used
here.

> [...]
>
> +	power->np = pdev->dev.of_node;

This can be dropped, it's not used at all.

> +	power->regmap = axp20x->regmap;

power->regmap = dev_get_regmap(pdev->dev.parent, NULL);

> [...]
> +	/* Request irqs after registering, as irqs may trigger immediately */
> +	for (i = 0; irq_names[i]; i++) {
> +		irq = platform_get_irq_byname(pdev, irq_names[i]);
> +		if (irq < 0) {
> +			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
> +				 irq_names[i], irq);
> +			continue;
> +		}
> +		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);

The mapping should actually happen in the mfd driver, so that
the platform resource contains a valid irq.

> +		ret = devm_request_any_context_irq(&pdev->dev, irq,
> +						   axp20x_ac_power_irq, 0,
> +						   DRVNAME, power);
> +		if (ret < 0)
> +			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
> +				 irq_names[i], ret);
> +	}

-- Sebastian

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