On Friday 10 June 2011, Ying-Chun Liu (PaulLiu) wrote:
+config PMIC_DIALOG
- bool "Support Dialog Semiconductor PMIC"
- depends on I2C=y
- depends on SPI=y
- select MFD_CORE
- help
- Support for Dialog semiconductor PMIC chips.
- Use the options provided to support the desired PMIC's.
+choice
- prompt "Chip Type"
- depends on PMIC_DIALOG
+config PMIC_DA9052
- bool "Support Dialog Semiconductor DA9052 PMIC"
- help
- Support for Dialog semiconductor DA9052 PMIC with inbuilt
- SPI & I2C connectivities.
- 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_DA9053AA
- bool "Support Dialog Semiconductor DA9053 AA PMIC"
- help
- Support for Dialog semiconductor DA9053 AA PMIC with inbuilt
- SPI & I2C connectivities.
- 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
- bool "Support Dialog Semiconductor DA9053 BA/BB PMIC"
- help
- Support for Dialog semiconductor DA9053 BA/BB PMIC with inbuilt
- SPI & I2C connectivities.
- This driver provides common support for accessing the device,
- additional drivers must be enabled in order to use the
- functionality of the device.
+endchoice
This can use a few lines of whitespace.
Why do you need a choice statement here? I would think that you need to be able to build a kernel with all three chips builtin at the same time, in order to run on all boards that have one of them.
+#define SUCCESS 0 +#define FAILURE 1
Remove these. We use negative error codes from linux/errno.h to indicate failure.
+struct da9052_eh_nb eve_nb_array[EVE_CNT];
Why do you need this array? Better keep the information local to your devices.
+static struct da9052_ssc_ops ssc_ops;
Make this const and statically initialize it with the functions instead of assigning the function pointers at runtime.
+struct mutex manconv_lock;
use DEFINE_MUTEX
+static struct semaphore eve_nb_array_lock;
No new semaphores. Use a mutex.
+void da9052_lock(struct da9052 *da9052) +{
- mutex_lock(&da9052->ssc_lock);
+} +EXPORT_SYMBOL(da9052_lock);
+void da9052_unlock(struct da9052 *da9052) +{
- mutex_unlock(&da9052->ssc_lock);
+} +EXPORT_SYMBOL(da9052_unlock);
use EXPORT_SYMBOL_GPL()
+int da9052_ssc_write(struct da9052 *da9052, struct da9052_ssc_msg *sscmsg) +{
- int ret = 0;
- /* Reg address should be a valid address on PAGE0 or PAGE1 */
- if ((sscmsg->addr < DA9052_PAGE0_REG_START) ||
- (sscmsg->addr > DA9052_PAGE1_REG_END) ||
- ((sscmsg->addr > DA9052_PAGE0_REG_END) &&
- (sscmsg->addr < DA9052_PAGE1_REG_START)))
return INVALID_REGISTER;
return a proper error code, e.g. "-EINVAL"
- ret = ssc_ops.write(da9052, sscmsg);
Take the ops from the device, e.g.
ret = da9052->scc_ops.write(da9052, sscmsg);
+static irqreturn_t da9052_eh_isr(int irq, void *dev_id) +{
- struct da9052 *da9052 = dev_id;
- /* Schedule work to be done */
- schedule_work(&da9052->eh_isr_work);
- /* Disable IRQ */
- disable_irq_nosync(da9052->irq);
- return IRQ_HANDLED;
+}
You shouldn't need to disable the interrupt if you don't clear the event bits at the source.
- ret = da9052_add_subdevice(da9052, "da9052-rtc");
- if (ret)
return ret;- ret = da9052_add_subdevice(da9052, "da9052-onkey");
- if (ret)
return ret;- ret = da9052_add_subdevice(da9052, "WLED-1");
- if (ret)
return ret;- ret = da9052_add_subdevice(da9052, "WLED-2");
- if (ret)
return ret;- ret = da9052_add_subdevice(da9052, "WLED-3");
- if (ret)
return ret;
Why not add them all at once?
- /* Initialize mutex required for ADC Manual read */
- mutex_init(&manconv_lock);
- /* Initialize NB array lock */
- sema_init(&eve_nb_array_lock, 1);
these are not needed if you use DEFINE_MUTEX
- /* Assign the read-write function pointers */
- da9052->read = da9052_ssc_read;
- da9052->write = da9052_ssc_write;
- da9052->read_many = da9052_ssc_read_many;
- da9052->write_many = da9052_ssc_write_many;
- if (SPI == da9052->connecting_device && ssc_ops.write == NULL) {
/* Assign the read/write pointers to SPI/read/write */ssc_ops.write = da9052_spi_write;ssc_ops.read = da9052_spi_read;ssc_ops.write_many = da9052_spi_write_many;ssc_ops.read_many = da9052_spi_read_many;- } else if (I2C == da9052->connecting_device
&& ssc_ops.write == NULL) {/* Assign the read/write pointers to SPI/read/write */ssc_ops.write = da9052_i2c_write;ssc_ops.read = da9052_i2c_read;ssc_ops.write_many = da9052_i2c_write_many;ssc_ops.read_many = da9052_i2c_read_many;- } else
return -1;
Just define two different operations structures for the difference.
diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c new file mode 100644 index 0000000..5fd966a --- /dev/null +++ b/drivers/mfd/da9052-i2c.c
+static struct da9052 *da9052_i2c;
A static pointer should not be necessary, just pass the device around to all the functions. This is especially necessary if you have more than one instance.
- /* Test spi connectivity by performing read of the GPIO_0-1 register */
- if (0 != da9052_i2c_read(da9052_i2c, &msg)) {
Do comparisons like
if (da9052_i2c_read(da9052_i2c, &msg) != 0)
or better
if (da9052_i2c_read(da9052_i2c, &msg))
printk(KERN_DEBUG "da9052_i2c_is_connected - "\"i2c read failed.............\n");return -1;- } else {
printk(KERN_DEBUG "da9052_i2c_is_connected - "\"i2c read success..............\n");return 0;- }
Use proper error codes, don't return -1.
- /* Validate I2C connectivity */
- if (I2C_CONNECTED == da9052_i2c_is_connected()) {
/* I2C is connected */da9052_i2c->connecting_device = I2C;if (0 != da9052_ssc_init(da9052_i2c))return -ENODEV;- } else {
return -ENODEV;- }
See above for the comparisons.
Make sure you free the memory in the error path. The best way to do that is to have a "goto error" statement to a place where you free the memory and return.
- /* printk("Exiting da9052_i2c_probe.....\n"); */
Remove stale comments like this.
+struct da9052 *da9052_spi;
+#define SPI_CONNECTED 0
+static int da9052_spi_is_connected(void) +{
- struct da9052_ssc_msg msg;
- /* printk("Entered da9052_spi_is_connected.............\n"); */
- msg.addr = DA9052_INTERFACE_REG;
- /* Test spi connectivity by performing read of the GPIO_0-1 register
and then verify the read value*/- if (0 != da9052_spi_read(da9052_spi, &msg)) {
printk(KERN_DEBUG "da9052_spi_is_connected - "\"spi read failed.............\n");return -1;- } else if (0x88 != msg.data) {
printk(KERN_DEBUG "da9052_spi_is_connected - " \"spi read failed. Msg data =%x ..............\n",msg.data);return -1;
Same comments as above apply.
+int da9052_spi_write(struct da9052 *da9052, struct da9052_ssc_msg *msg) +{
The functions should probably all be static. Just pass the ssc_ops to da9052_ssc_init.
Arnd