linux-kernel - Re: [PATCH v2 1/5] mfd: max14577: Add max14577 MFD driver core

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]

Message-id: <1385029909.26695.5.camel@AMDC1943>
Date:	Thu, 21 Nov 2013 11:31:49 +0100
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	MyungJoo Ham <myungjoo.ham@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>,
	Anton Vorontsov <anton@...msg.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	Pawel Moll <pawel.moll@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Rob Landley <rob@...dley.net>, linux-doc@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH v2 1/5] mfd: max14577: Add max14577 MFD driver core

On Wed, 2013-11-20 at 15:31 +0000, Mark Brown wrote:
> On Wed, Nov 20, 2013 at 03:12:08PM +0100, Krzysztof Kozlowski wrote:
> 
> > +static irqreturn_t max14577_irq_thread(int irq, void *data)
> > +{
> > +	struct max14577 *max14577 = data;
> > +	u8 irq_reg[MAX14577_IRQ_REGS_NUM] = {0};
> > +	u8 tmp_irq_reg[MAX14577_IRQ_REGS_NUM] = {};
> > +	int i, cur_irq;
> > +
> > +	max14577_bulk_read(max14577->regmap, MAX14577_REG_INT1,
> > +			&tmp_irq_reg[MAX14577_IRQ_INT1],
> > +			MAX14577_IRQ_REGS_NUM);
> 
> All this interrupt handling looks like a straight copy of the regmap
> code?

You are right, I will convert it to regmap_irq_chip.

> > +int max14577_irq_resume(struct max14577 *max14577)
> > +{
> > +	int ret = 0;
> > +
> > +	if (max14577->irq && max14577->irq_domain)
> > +		ret = max14577_irq_thread(0, max14577);
> 
> Are you sure that this is not going to race nastily if the interrupt
> goes off during resume?

I believe this code came from max77693. At least it looks very similar.
Also it seems it was a fast workaround for interrupt occurring during
device resume (before I2C bus resume). Handling this interrupt failed
and interrupt registers were not cleared.

This code is not needed now, especially after using regmap IRQ handling.

> 
> > +
> > +	return ret >= 0 ? 0 : ret;
> 
> This check isn't a triumph of legibility.  

OK.

> 
> > +	if (IS_ERR_OR_NULL(pdata)) {
> > +		dev_err(&i2c->dev, "No platform data found: %ld\n",
> > +				PTR_ERR(pdata));
> > +		return -EINVAL;
> > +	}
> 
> Why IS_ERR_OR_NULL().

Right, it should be just check for null.

> 
> > +#else
> > +#define max14577_suspend	NULL
> > +#define max14577_resume		NULL
> > +#endif /* CONFIG_PM */
> 
> You don't need these since you use SIMPLE_DEV_PM_OPS().

OK.

> 
> > +#else
> > +#define max14577_dt_match	NULL
> > +#endif
> 
> Similarly here.

OK.

Thanks for comments.

Best regards,
Krzysztof




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/