Re: [PATCH v7 3/6] mfd: Add support for the MediaTek MT6358 PMIC

From: Lee Jones <lee.jones@linaro.org>
To: Hsin-hsiung Wang <hsin-hsiung.wang@mediatek.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Alexandre Belloni <alexandre.belloni@free-electrons.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Eddie Huang <eddie.huang@mediatek.com>,
	Sean Wang <sean.wang@mediatek.com>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Tianping Fang <tianping.fang@mediatek.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Josef Friedl <josef.friedl@speed.at>,
	Kate Stewart <kstewart@linuxfoundation.org>,
	Ran Bi <ran.bi@mediatek.com>,
	Frank Wunderlich <frank-w@public-files.de>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-rtc@vger.kernel.org, srv_heupstream@mediatek.com
Subject: Re: [PATCH v7 3/6] mfd: Add support for the MediaTek MT6358 PMIC
Date: Mon, 3 Feb 2020 07:46:21 +0000	[thread overview]
Message-ID: <20200203074621.GB13919@dell> (raw)
In-Reply-To: <1579664886.6399.24.camel@mtksdaap41>

On Wed, 22 Jan 2020, Hsin-hsiung Wang wrote:

> Hi,
> 
> On Mon, 2019-12-16 at 15:17 +0000, Lee Jones wrote:
> > On Wed, 11 Dec 2019, Hsin-Hsiung Wang wrote:
> > 
> > > This adds support for the MediaTek MT6358 PMIC. This is a
> > > multifunction device with the following sub modules:
> > > 
> > > - Regulator
> > > - RTC
> > > - Codec
> > > - Interrupt
> > > 
> > > It is interfaced to the host controller using SPI interface
> > > by a proprietary hardware called PMIC wrapper or pwrap.
> > > MT6358 MFD is a child device of the pwrap.
> > > 
> > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> > > ---
> > >  drivers/mfd/Makefile                 |   2 +-
> > >  drivers/mfd/mt6358-irq.c             | 224 ++++++++++++++++++++++++++++
> > >  drivers/mfd/mt6397-core.c            |  45 +++++-
> > >  include/linux/mfd/mt6358/core.h      | 158 ++++++++++++++++++++
> > >  include/linux/mfd/mt6358/registers.h | 282 +++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/mt6397/core.h      |   3 +
> > >  6 files changed, 712 insertions(+), 2 deletions(-)
> > >  create mode 100644 drivers/mfd/mt6358-irq.c
> > >  create mode 100644 include/linux/mfd/mt6358/core.h
> > >  create mode 100644 include/linux/mfd/mt6358/registers.h

[...]

> > > +int mt6358_irq_init(struct mt6397_chip *chip)
> > > +{
> > > +	int i, j, ret;
> > > +	struct pmic_irq_data *irqd;
> > > +
> > > +	irqd = devm_kzalloc(chip->dev, sizeof(struct pmic_irq_data *),
> > > +			    GFP_KERNEL);
> > > +	if (!irqd)
> > > +		return -ENOMEM;
> > > +
> > > +	chip->irq_data = irqd;
> > > +
> > > +	mutex_init(&chip->irqlock);
> > > +	irqd->top_int_status_reg = MT6358_TOP_INT_STATUS0;
> > > +	irqd->num_pmic_irqs = MT6358_IRQ_NR;
> > > +	irqd->num_top = ARRAY_SIZE(mt6358_ints);
> > > +
> > > +	irqd->enable_hwirq = devm_kcalloc(chip->dev,
> > > +					  irqd->num_pmic_irqs,
> > > +					  sizeof(bool),
> > 
> > This is fragile.  What if the type changes elsewhere?
> > 
> 
> Thanks for your comment.
> Do you mean using 'sizeof(*irqd->enable_hwirq)' instead of
> 'sizeof(bool)'?

Yes please.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog