-----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 thefunctionality 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.