Hello Sebastian,

On Thu, 2019-05-02 at 21:50 +0200, Sebastian Reichel wrote:

Hi,

On Thu, May 02, 2019 at 12:17:12PM +0300, Matti Vaittinen wrote:

quoted

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 <redacted>
Acked-by: Sebastian Reichel <redacted>

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

Sorry. This is my bad. I translated your: "Otherwise looks ok to me."
- to an ack. And I was pretty sure you didn't want to staticize the
local array. I should've known better. I should have waited for your Ok
to leaving the static out before adding an ack.

quoted

+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).

Ok. I see your point and it sounds reasonable.

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.

It would be great to get this in, it has been respinned quite a few
times already... I hope Lee takes the series in when he's back from his
leave - but it is not a problem for me to change this with later patch
(or change this in the series if it still needs to be reworked).


Br,
	Matti Vaittinen