RE: [PATCH v3 01/11] MFD: DA9052 MFD core module

[Ashish Jangam <Ashish.Jangam@kpitcummins.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3230 bytes --]


> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Saturday, August 06, 2011 7:09 PM
> To: Ashish Jangam
> Cc: sameo@openedhand.com; linux-kernel@vger.kernel.org; Dajun; linaro-
> dev@lists.linaro.org
> Subject: Re: [PATCH v3 01/11] MFD: DA9052 MFD core module
> 
> On Fri, Aug 05, 2011 at 07:23:44PM +0530, ashishj3 wrote:
> 
> > +choice
> > +	prompt "Chip Type"
> > +	depends on MFD_DA9053_SPI || MFD_DA9053_I2C
> > +config PMIC_DA9053AA
> > +	bool "Support Dialog Semiconductor DA9053 AA PMIC"
> > +	help
> > +	  Support for Dialog semiconductor DA9053 AA PMIC.
> > +	  This driver provides common support for accessing  the device,
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the device.
> > +config PMIC_DA9053Bx
> 
> Could do with blank lines between blocks.  Though looking at the code
> here I don't understand why these are compile options at all, or if they
> need to be compile options for some reason why they're not independantly
> selectable?
DA9052 PMIC chip id may get OTP therefore chip id cannot be used as 
a distinguishing factor. Hence these compile time options were introduced.
DA9053 is a higher version of DA9052 therefore not independently selectable.
This means that there can be either DA9052 or DA9053 on system. I need to correct
this Kconfig to take care of this. 
> 
> > +int da9052_reg_read(struct da9052 *da9052, unsigned char reg)
> > +{
> > +	int val, ret;
> > +
> > +	if (reg > DA9052_MAX_REG_CNT) {
> > +		dev_err(da9052->dev, "invalid reg %x\n", reg);
> > +		return -EINVAL;
> > +	}
> > +
> > +	#ifdef CONFIG_MFD_DA9052_SPI
> > +		reg = (reg << 1) | 1;
> > +	#endif
> 
> There's several problems here:
> 
> - You shouldn't be indenting preprocessor directives.
> - You shouldn't be adding extra indentation before.
> - This will break I2C devices if SPI support is built into the driver.
> 
> Please, when writing code try to understand the abstractions you're
> using.  For example here think about the purpose of being able to build
> I2C and SPI separately and simultaneously and the goal of the regmap
> API.
> 
> Looks like we need to add per device mangling for the SPI register
> read/write flag.
For now we will handle this as below:-
During SPI and I2C registration we will add bus type(SPI/I2C) info in the
struct da9052. And before initiating any device I/O this struct member will
be read and reg address will be manipulated if needed.
> 
> > +	da9052_group_write(da9052, DA9052_EVENT_A_REG, 4, v);
> > +
> > +	#ifndef CONFIG_PMIC_DA9053Bx
> > +	DA9052_FIXME();
> > +	#endif
> 
> This should be runtime detected based on the device name, either from
> the device registration or by reading back chip identification.
As said above getting chip info will not work in DA9053/53 case. Also DA9052 code 
works for DA9053 except for few minor changes in MFD and regulator module.
In this case registering different device will also require a preprocessor directive
Or separate DA9053 file therefore this option was not opt. 
>



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥