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

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

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:
> > 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 :)

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.

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