linux-kernel - Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC

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]

Date:	Tue, 10 Jun 2014 01:40:30 +0200
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Mark Brown <broonie@...nel.org>
CC:	Lee Jones <lee.jones@...aro.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Mike Turquette <mturquette@...aro.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Doug Anderson <dianders@...omium.org>,
	Olof Johansson <olof@...om.net>,
	Sjoerd Simons <sjoerd.simons@...labora.co.uk>,
	Daniel Stone <daniels@...labora.com>,
	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management
 IC

Hello Mark,

On 06/09/2014 09:47 PM, Mark Brown wrote:
> On Mon, Jun 09, 2014 at 11:37:46AM +0200, Javier Martinez Canillas wrote:
> 
>> +Optional node:
>> +- voltage-regulators : The regulators of max77802 have to be instantiated
>> +  under subnode named "voltage-regulators" using the following format.
> 
> Every other PMIC calls this node regulators...
> 

Ok, I'll change for consistency.

>> +	regulator_name {
>> +		regulator-compatible = LDOn/BUCKn
> 
> regulator-compatible is deprecated, use the node name instead.
> 

Ok.

>> +config MFD_MAX77802
>> +	bool "Maxim Integrated MAX77802 PMIC Support"
> 
> Why is this bool and not tristate?
> 

I noticed that the majority of the mfd PMIC drivers were bool and not tristate
so I thought it was a convention. But nothing prevents this driver to be built
as a module so I'll change it to tristate.

>> +int max77802_irq_resume(struct max77802_dev *max77802)
>> +{
>> +	/*
>> +	 * The IRQ that woke us up may still need to be ACK'ed on resume.
>> +	 * If it isn't ever ACK'ed, future IRQs may not be delivered.
>> +	 */
>> +	if (max77802->irq)
>> +		max77802_irq_thread(0, max77802);
>> +
>> +	return 0;
>> +}
> 
> As covered in another subthread all this code looks like it should be
> regmap-irq.
> 

It seems so, I'll take that into account for v2.

>> +	if (regmap_read(max77802->regmap,
>> +			 MAX77802_REG_DEVICE_ID, &data) < 0) {
>> +		dev_err(max77802->dev,
>> +			"device not found on this channel (this is not an error)\n");
>> +		return -ENODEV;
> 
> If this is not an error why is it printed as dev_err()?  It does look
> like an error to me, though.
> 

Yeah, it is an error so I'll clean that message.

>> +	} else {
>> +		dev_info(max77802->dev, "device found\n");
>> +	}
> 
> These sort of prints are just noise, remove this unless there is some
> revision information you can display.  It's also better practice to
> check that the device ID is actually what was expected in case there was
> an error in the DT.
> 

Ok, will do.

>> +static const struct i2c_device_id max77802_i2c_id[] = {
>> +	{ "max77802", TYPE_MAX77802 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, max77802_i2c_id);
> 
> We have type information here but not in the OF ID table (not that we
> ever look at it).
> 

Yeah, I'll remove the type information here. It is a left over when trying to
combine both max77802 and max77686 drivers since in a combined driver we need
the type information.

Thanks a lot for your feedback.

Best regards,
Javier
--
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/