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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
