Re: [PATCH v14 7/8] power: supply: Initial support for ROHM BD70528 PMIC charger block

From: Sebastian Reichel <sebastian.reichel@collabora.com>
To: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Cc: mazziesaccount@gmail.com, Lee Jones <lee.jones@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-rtc@vger.kernel.org,
	linux-watchdog@vger.kernel.org, heikki.haikola@fi.rohmeurope.com,
	mikko.mutanen@fi.rohmeurope.com
Subject: Re: [PATCH v14 7/8] power: supply: Initial support for ROHM BD70528 PMIC charger block
Date: Thu, 2 May 2019 21:50:49 +0200	[thread overview]
Message-ID: <20190502195049.brysexbyyq7khtr4@earth.universe> (raw)
In-Reply-To: <eece016c86483d55befab1a06fb299c9d6d17134.1556787930.git.matti.vaittinen@fi.rohmeurope.com>

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

Hi,

On Thu, May 02, 2019 at 12:17:12PM +0300, Matti Vaittinen wrote:
> ROHM BD70528 PMIC includes battery charger block. Support charger
> staus queries and doing few basic settings like input current limit
> and charging current.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Please only add Acked-by when you receive one, especially when you
do not implement all requested changes :)

[...]

> +static int bd70528_get_irqs(struct platform_device *pdev,
> +			    struct bd70528_psy *bdpsy)
> +{
> +	int irq, i, ret;
> +	unsigned int mask;
> +	const struct irq_name_pair bd70528_chg_irqs[] = {
> +		{ .n = "bd70528-bat-ov-res", .h = BD_IRQ_HND(BAT_OV_RES) },
> +		{ .n = "bd70528-bat-ov-det", .h = BD_IRQ_HND(BAT_OV_DET) },
> +		{ .n = "bd70528-bat-dead", .h = BD_IRQ_HND(DBAT_DET) },
> +		{ .n = "bd70528-bat-warmed", .h = BD_IRQ_HND(COLD_RES) },
> +		{ .n = "bd70528-bat-cold", .h = BD_IRQ_HND(COLD_DET) },
> +		{ .n = "bd70528-bat-cooled", .h = BD_IRQ_HND(HOT_RES) },
> +		{ .n = "bd70528-bat-hot", .h = BD_IRQ_HND(HOT_DET) },
> +		{ .n = "bd70528-chg-tshd", .h = BD_IRQ_HND(CHG_TSD) },
> +		{ .n = "bd70528-bat-removed", .h = BD_IRQ_HND(BAT_RMV) },
> +		{ .n = "bd70528-bat-detected", .h = BD_IRQ_HND(BAT_DET) },
> +		{ .n = "bd70528-dcin2-ov-res", .h = BD_IRQ_HND(DCIN2_OV_RES) },
> +		{ .n = "bd70528-dcin2-ov-det", .h = BD_IRQ_HND(DCIN2_OV_DET) },
> +		{ .n = "bd70528-dcin2-removed", .h = BD_IRQ_HND(DCIN2_RMV) },
> +		{ .n = "bd70528-dcin2-detected", .h = BD_IRQ_HND(DCIN2_DET) },
> +		{ .n = "bd70528-dcin1-removed", .h = BD_IRQ_HND(DCIN1_RMV) },
> +		{ .n = "bd70528-dcin1-detected", .h = BD_IRQ_HND(DCIN1_DET) },
> +	};

Please also make it static. That will move the whole thing to
read-only (because of const) data section. This improves the
security and the required cpu time at the same time (no need
to copy values to the stack).

But this can be changed later, so no need to block the whole
patchset just because of this. If Lee wants to merge this for
5.2, that would be fine with me. But please add it directly in
a new patch revision if the patch does not make it into 5.2.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]