> -----Original Message----- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: Saturday, August 06, 2011 7:09 PM > To: Ashish Jangam > Cc: sa...@openedhand.com; linux-ker...@vger.kernel.org; Dajun; linaro- > d...@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. >
_______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
