MFD of DA9052 Linux device drivers (1/9)

Prev: [PATCH] POWER (BAT) of DA9052 Linux device drivers (4/9)
Next: [GIT PULL] Lockup detector updates

From: Jonathan Cameron on 19 May 2010 06:20

---

On 05/19/10 10:16, David Dajun Chen wrote:
> Dear sir/madam,
>
> The attached is the MFD part of the device drivers newly developed for DA9052 Power Management IC from Dialog Semiconductor.
>
> Regards

Dear David,

Whilst it is good to see you have broken the da9052 support up into a number
of different patches there are still a number of issues with how this patch
set is presented that will make it difficult for anyone to review and even
harder for anyone to test (even if it is just build testing).

Firstly, the naming of the patches. Convention says that a series of 9 like
this should probably have an explanatory cover letter. If you are using
git (if you are not, please move to it as it makes putting together large
sets like this in the right format trivial). Typically that email subject
would be

[PATCH 0/9] DA9052 driver

Then the others would be
[PATCH 1/9] DA9052 mfd
[PATCH 2/9] ....
Ideally all these patches in the series should be replies to the first email.
(If you can use git send-email it will do this nicely for you).

The next thing is that the structure at the top of the patch is very formal
and here git format-patch will do the hard work for you.

A typical git format-patch call for this series (assuming they are the last nine
patches in your tree) would look like

git format-patch --patch-with-stat --cover-letter -o for-lkml HEAD^^^^^^^^^
Then you edit the resulting files in the for-lkml directory.

Please be careful not to change the overall layout. Any comments should go
below the --- line just above the stats. Also please try to work out how
to get rid of the block disclaimer at the bottom of this email or prefix
with a line containing -- as iirc that will make git am (which applies the
patches to someone elses tree) ignore the signature.

Please review the documentation on submitting patches (in the Documentation
directory of the kernel tree) for more guidance on getting this right.

It may seem over restrictive but it saves reviewers a lot of time an hassel
and generally makes them more likely to take a look at your patches.

Jonathan

(p.s. if you are still unsure on patch formatting, feel free to send me any
patches first and I'll take a quick look for any of the things likely to annoy
other reviewers!)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Alan Cox on 19 May 2010 07:10

---

A first general pass over part 1 - from the look of it there are some
general themes here worth looking it across them all before resubmitting.

If bits of this commentary don't make sense let me know and I'll try and
explain more.

Alan


> +
> +#ifndef CONFIG_ARCH_S3C64XX
> +#include <linux/syscalls.h>
> +#endif

You have the odd ifdef like this which seems odd. If you need syscalls.h
include it, if not don't.


> +/*--------------------------------------------------------------------------*/
> +/* Local Macro Definitions */
> +/*--------------------------------------------------------------------------*/

Style wise it would be helpful I think to lose blank sections


> +typedef struct {
> + u32 adc_vddout :1;
> + u32 adc_ich :1;
> + u32 adc_tbat :1;
> + u32 adc_vbat :1;
> + u32 adc_in4 :1;
> + u32 adc_in5 :1;
> + u32 adc_in6 :1;
> + u32 adc_tsi :1;
> + u32 adc_tsense :1;
> + u32 adc_vbbat :1;
> + u32 dummy :22;
> +} adc_mode_status_t;

Be careful here. Firstly the typedefs generally make it less obvious what
is going on, but secondly the compiler can choose to put typedef bits in
either endian order so you may well find that dummy ends up at the top or
bottom on different systems. This is one reason a lot of code in the
kernel tends not to use typdef enums for bit operations


> +/* EH structures for all possible events */
> +da9052_eh_nb adc4_eh_data;
> +da9052_eh_nb adc5_eh_data;
> +da9052_eh_nb adc6_eh_data;
> +da9052_eh_nb comp_1v2_eh_data;

static ?


> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.

This is visible across the code. The kernel has a set of error codes that
are generally used, and 0 is used for success.

So a quick search/replace of SUCCESS for 0 and the others for negative
error codes (-EINVAL etc) would help make the code easier for everyone to
follow.

> + /* ADC channel 4 and 5 are by default enabled */
> +#if (ADC_CONF_ADC4)

Things like this should be runtime switchable unless massive amounts of
code. Otherwise people end up recompiling stuff which makes it harder
work for those trying to ship generic kernels for boards.

> + return (ADC_SSC_BUS_ERROR);

Most return (x) don't need the brackets. Again a quick search/replace
operation would clean that up


> + * da9052_adc_signal_to_user;
> + * This function signal to the library to call the call back function.
> + *
> + * @param event
> + * @return s32
> + */
> +static s32 da9052_adc_signal_to_user(u8 event)
> +{
> + DA9052_DEBUG("%s: event status: %d \n", __FUNCTION__, event);
> + kill_fasync(&adc_fasync_queue, SIGIO, POLL_IN);
> +
> + return(SUCCESS);
> +}

Why make it s32 if it always returns 0 ?

> + switch (channel) {
> + case ADC_VDDOUT:
> + adc_mode_status.adc_vddout = mode;

Is there a reason the adc_mode_status isn't just an array ? Then this
would all collapse into

if (< 0 || > range)
error
else
adc_mode_status[channel] = mode
>
> + switch(channel){
> + /* VDD_OUT channel */
> + case ADC_VDDOUT:{

and much of this lot ..


> +void da9052_adc_adcin6_handler(u32 data)
> +{
> + /* Acquire lock before accessing the status variable */
> + mutex_lock(&adc_event_occ_lock);

Similarly with all these you seem to end up with lots of functions
because you use typdefs for it all so can't just do

adc_event_occ_status.event_bits |= 1 << mask[x];
signal_to_user(event[x])

and have a single handler that is generic

> + /* Store the status of the event in the variable */
> + adc_event_occ_status.event_bits_t.adc_adcin6_e_occ = TRUE;
> + /* Release the lock */
> + mutex_unlock(&adc_event_occ_lock);
> + /* Signal to the user space */
> + da9052_adc_signal_to_user(GPI2_EVE);
> +}
> +

> +s32 da9052_adc_register_event(u8 event_type)
> +{
> + /* Register the callback function with event handler */
> + switch(event_type) {
> + case GPI0_EVE:
> +#if ADC_CONF_ADC4
> + /* Check if event is already registered */
> + if (!is_eh_registered.adc4_event) {
> + /* Set the EH structure */
> + adc4_eh_data.call_back = da9052_adc_adcin4_handler;
> + adc4_eh_data.eve_type = event_type;
> + /* Register the event with EH */
> + if (da9052_eh_register_nb(&adc4_eh_data))
> + return (ADC_EVENT_REGISTRATION_FAILED);
> + /* Set the registration flag to 1 */
> + is_eh_registered.adc4_event = TRUE;

And again here.. you'd be looking at

if (!(events_registered & (1 << event_type))) {
event[event_type].callback = foo;
register ..
events_registered |= (1 << event_type);
}

plus any locking needed in both cases

And all the other bits would vanish

> +#else
> + return (ADC_CH4_NOT_CONF);
> +#endif

and an
if (! (events_enabled & (1 << event_type))
return -Esomething;

would fix the rest



> +s32 da9052_adc_unregister_event(u8 event_type)
> +{

Similarly in this lot


> +/*--------------------------------------------------------------------------*/
> +/* Infrastructure Functions */
> +/*--------------------------------------------------------------------------*/
> +/**
> + * da9052_adc_open:
> + *
> + * @param *inode pointer to device inode
> + * @param *file file pointer
> + * @return s32 Error Code, zero: no error
> + */
> +s32 da9052_adc_open(struct inode *inode, struct file *file)
> +{
> + /* Check if device is already open */
> + if(adc_device_open) {
> + printk("DA9052: ADC device already open.\n");
> + return (-EBUSY);

Locking... open/close could race. Use something like test_and_set_bit()
to do this.


> +s32 da9052_adc_ioctl(struct inode *inode, struct file *file,
> + u32 cmd, unsigned long arg)
> +{
> +/**
> + * da9052_adc_remove: Called when ditaching device from driver
> + * @param void
> + * @return void
> + */
> +static s32 __devexit da9052_adc_remove(struct platform_device *dev)
> +{
> + DA9052_DEBUG(KERN_DEBUG "Removing %s \n", DA9052_ADC_DEVICE_NAME);

How about freeing things like the character device allocation ?

> +EXPORT_SYMBOL(da9052_set_adc_mode);
> +EXPORT_SYMBOL(da9052_configure_thresholds);

Stylewise should be under the function itself


> +/* Increment with wrap operation for TSI FIFO */
> +#define incr_with_wrap(x) \
> + if(++x >= TSI_FIFO_SIZE) \
> + x = 0

Hiding this sort of stuff needs care - its not locking safe but it hides
that fact



> + /* Acqurie nb array lock */
> + if(down_interruptible(&eh_info.eve_nb_array_lock))
> + return (FAILURE);

Please use mutexes whenever possible - that makes realtime the realtime
Linux people much happier.


> + /* Acquire TSI FIFO lock */
> + if(down_interruptible(&tsi_info.tsi_fifo_lock))
> + return (0);
> +
> + if(tsi_info.tsi_fifo_start < tsi_info.tsi_fifo_end){
> + /* FIFO partly or completely full but NOT overflown */

See the kfifo functionality in the kernel - it should save you lots of
work and some locks.


>
> + /*
> + * This delay is necessary to avoid hardware fake interrupts from DA9052.
> + * Because of the 33 us internal delay in sync operation of DA9052, nIRQ line
> + * changes state from LOW to HIGH after max. 33 us of clearing event register.
> + * If we don't wait for this time (by adding udelay()) before re-enabling the
> + * IRQ, we will immediately get another interrupt, as it is configured as
> + * LOW LEVEL triggered. Experiments showed that delay of 50 us seems to be
> + * safe enough to get rid of fake interrupts.
> + */
> + udelay(50);
> +
> + //printk(KERN_CRIT "T=%d ", (jiffies - start));
> +
> + /* Enable HOST interrupt */
> + enable_irq(DA9052_IRQ);
> +}

If you need the IRQ masked during processing you really want to be doing
this in the IRQ handler - otherwise your task may get delayed and the
delay may mean important other hardware has its IRQ blocked.

> +
> +/**
> + * process_events: Function to process all events occured
> + * @param
> + * @return int
> + */
> +static s32 process_events(s32 events_sts){
> +

> + /* Event bit is set, execute all registered call backs */
> + if(down_interruptible(&eh_info.eve_nb_array_lock)){
> + printk(KERN_CRIT "Can't acquire eve_nb_array_lock \n");
> + return (FAILURE);
> + }

Randomly losing events seems a bit of bad way to handle this ? Would it
not be better to scan them all in the IRQ handler into a queue then kick
off work that can sleep to empty the queue ?



> + * da9052_eh_open: Function that is called when some one opens EH device node
> + *
> + * @param *inode pointer to device inode
> + * @param *file file pointer
> + * @return int Error Code, zero: no error
> + */
> +s32 da9052_eh_open(struct inode *inode, struct file *file)
> +{
> + /* Check if device is already open */
> + if(eh_info.device_open) {
> + printk( KERN_INFO "DA9052: EH device already open.\n");
> + return (-EBUSY);

Again locking


> +static int __devexit da9052_eh_remove(struct platform_device *dev)
> +{
> + DA9052_DEBUG(KERN_DEBUG "Removing %s \n", DA9052_EH_DEVICE_NAME);

Again not actually coded for some reason so leaks resources on unload

> + /* Now selectively set type for all Volatile registers */
> + ssc_info.ssc_cache[DA9052_STATUSA_REG].type = VOLATILE; // Reg - 1

See test_bit/set_bit/clear_bit - they work on arrays of bits so you can
simply set up a 128bit array pair, initialise it at compile time and turn
the tests into

if (test_bit(n, &ssc_reg_volatile))

and
if (test_bit(n, &ssc_info.ssc_valid))


However you do it you can initialise it at compile time...

> + * It is better to do un-interruptible sleep here. Otherwise we will get
> + * interttupted in case of high frequency events e.g. ADC THRESHOLD.
> + * As a result this SSC write operation will fail!
> + */
> + down(&ssc_info.ssc_sem);

Use mutexes


> + * Check if this is a Non-volatile register, if yes then return value -
> + * from cache instead of actual reading from hardware. Before reading -
> + * cache entry, make sure that the entry is valid
> + */
> + if(ssc_info.ssc_cache[sscmsg->addr].type != VOLATILE){
> + /* The read request is for Non-volatile register */
> + /* Check if we have valid cached value for this */
> + if(ssc_info.ssc_cache[sscmsg->addr].status == VALID){

If you never set the valid bit for a volatile register cache why test
both ?


> + /* We have valid cached value, copy this value */
> + sscmsg->data = ssc_info.ssc_cache[sscmsg->addr].val;
> +
> + /* Release ssc lock */
> + up(&ssc_info.ssc_sem);
> + return (SUCCESS);
> + }
> + }
> +
> +#if defined (CONFIG_MFD_DA9052_SPI)
> + /* Call SPI specific function */
> + ret = da9052_ssc_spi_read(sscmsg);
> +
> +#elif defined (CONFIG_MFD_DA9052_I2C)
> + /* Put I2C specific function here */
> + ret = da9052_ssc_i2c_read(sscmsg);
> +#endif

This should be handled at runtime (probably you want an ssc_read function
pointer or function pointer array). You'll see several other SPI/I�C
drivers have

foo.c - the device driver
foo_spi.c - SPI read/write functions for driver +
struct something_ops
foo_i2c.c - Ditto for I2C




> +/* Macros to configure the ADC port pins */
> +#if (DA9052_GPIO_PIN_0 == DA9052_GPIO_CONFIG_ADC)
> +#define ADC_CONF_ADC4 ENABLE

This sort of magic wants to be runtime, and certainly not hidden away


> +typedef struct {
> + u8 auto4_high;
> + u8 auto4_low;
> + u8 auto5_high;
> + u8 auto5_low;
> + u8 auto6_high;
> + u8 auto6_low;
> +}threshold_res;

Avoid typedefs for this kind fo stuff (see Documentation/CodingStyle)



> +/* To enable debug output for your module, set this to 1 */
> +#define DA9052_EH_DEBUG 0
> +
> +#undef DA9052_DEBUG
> +#if DA9052_EH_DEBUG
> +#define DA9052_DEBUG( fmt, args... ) printk( KERN_CRIT "" fmt, ##args )
> +#else
> +#define DA9052_DEBUG( fmt, args... )
> +#endif

See dev_dbg and friends


> +/* Failed Copy from User */
> +#define COPY_FROM_USER_FAILED (3)
> +
> +/* Failed Copy to User */
> +#define COPY_TO_USER_FAILED (4)

And use standard Linux error codes


> +#define TRUE 1
> +#define FALSE 0

The kernel has its own set of these

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Jonathan Cameron on 19 May 2010 07:20

---

Hi David,

Below is an initial review. Please take a look at more code for similar drivers
in the various subsystems as it will give you an idea how how much of this should
be done.

I've run out of time, so I'm afraid I didn't get quite to the bottom of this patch.
Still plenty of stuff still to do in the bit I did look at!

Jonathan
>
> diff -Naur linux-2.6.33.2/drivers/mfd/da9052_adc.c linux-2.6.33.2_patch/drivers/mfd/da9052_adc.c
> --- linux-2.6.33.2/drivers/mfd/da9052_adc.c 1970-01-01 05:00:00.000000000 +0500
> +++ linux-2.6.33.2_patch/drivers/mfd/da9052_adc.c 2010-05-18 15:29:36.000000000 +0500
> @@ -0,0 +1,1810 @@
> +/*
> + * Copyright(c) 2009 Dialog Semiconductor Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * da9052_adc.c: ADC driver for DA9052
> + *
> + * History:
> + *
> + * (05/08/2009): Initial version
> + * (05/22/2009): Changes as per code review done
> + *
> + * (27/04/2010): Created initial draft for Linux community release
> + *
> + * Best Viewed with TabSize=8 and ColumnWidth=80
Please clean up the comments, people reading it certainly don't need advice
on tabsizes :) I guess the history doesn't do any harm if it is useful for
your internal records.
> + */
> +
> +/*--------------------------------------------------------------------------*/
> +/* System wide include files */
> +/*--------------------------------------------------------------------------*/
These blocks of comments are very nice, but completely pointless. It's self evident
what is in each of the sections so I'd get rid of them.
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <asm/uaccess.h>
> +#include <linux/platform_device.h>
> +


Why does this not apply if the arch is s3c64xx? If you really want to avoid including
a generic header like this then there is almost certainly a bug in the underlying
architecture code.
> +#ifndef CONFIG_ARCH_S3C64XX
> +#include <linux/syscalls.h>
> +#endif
> +
> +/*--------------------------------------------------------------------------*/
> +/* Module specific include files */
> +/*--------------------------------------------------------------------------*/
> +#include <linux/mfd/da9052/da9052_reg.h>
> +#include <linux/mfd/da9052/da9052_lib.h>
> +#include <linux/mfd/da9052/da9052_adc.h>
> +#include <linux/mfd/da9052/da9052_eh.h>
> +
> +/*--------------------------------------------------------------------------*/
> +/* Local Type Definitions */
> +/*--------------------------------------------------------------------------*/
Definitely kill comments that refer to nothing.
> +
> +/*--------------------------------------------------------------------------*/
> +/* Local Constant Definitions */
> +/*--------------------------------------------------------------------------*/
> +static u8 banner[] __initdata = "DA9052 ADC Driver, v1.0\n";
I think this is only used in one place. Put it directly in the relevant printk.
> +
> +
> +/*--------------------------------------------------------------------------*/
> +/* Local Macro Definitions */
> +/*--------------------------------------------------------------------------*/
> +
> +/*--------------------------------------------------------------------------*/
> +/* Local Structure Definitions */
> +/*--------------------------------------------------------------------------*/
> +
> +/**
> + * struct adc_mode_status_t - Stores the status of mode selected for the ADC
> + * channel.
> + * @adc_vddout : Mode status for channel 0 VDDOUT.
> + * @adc_ich : Mode status for channel 1 ICH.
> + * @adc_tbat : Mode status for channel 2 TBAT.
> + * @adc_vbat : Mode status for channel 3 VBAT.
> + * @adc_in4 : Mode status for channel 4 ADC_IN4.
> + * @adc_in5 : Mode status for channel 5 ADC_IN5.
> + * @adc_in6 : Mode status for channel 6 ADC_IN6.
> + * @adc_tsi : Mode status for channel 7 XY� TSI
> + * @adc_tsense : Mode status for channel 8 Internal Tjunc.
> + * @adc_vbbat : Mode status for channel 9 VBBAT.
> + */
> +
I'm inclined to say the typedef usage here just hides the fact this is a structure
for no real gain in reduced code etc. Just have a struct adc_mode_status and use
that.
> +typedef struct {
> + u32 adc_vddout :1;
> + u32 adc_ich :1;
> + u32 adc_tbat :1;
> + u32 adc_vbat :1;
> + u32 adc_in4 :1;
> + u32 adc_in5 :1;
> + u32 adc_in6 :1;
> + u32 adc_tsi :1;
> + u32 adc_tsense :1;
> + u32 adc_vbbat :1;
> + u32 dummy :22;
> +} adc_mode_status_t;
> +
> +/**
> + * struct eh_registration_status_t - Interrupt registration status with EH
> + * @adc4_event : States whether ADC4 interrupt is registered with EH
> + * @adc5_event : States whether ADC5 interrupt is registered with EH
> + * @adc6_event : States whether ADC6 interrupt is registered with EH
> + * @comp_1v2_event : States whether COMP1V2 interrupt is registered with EH
> + *
> + */
> +typedef struct {
> + u8 adc4_event :1;
> + u8 adc5_event :1;
> + u8 adc6_event :1;
> + u8 comp_1v2_event :1;
> + u8 dummy :4;
> +} eh_registration_status_t;
> +

Here's the first big no no. You really don't want to have global variables in
an individual driver. It might be a strange thing to do, but it's always possible
someone will wire up two of these devices to one board. Also it is generally
better to hold all state in one convenient structure as then people know where
to find it! To see how to do this, see the other similar drivers.

Basically you need a structure containing these variables, that is kzalloc'd in
the probe function for the driver.
> +/*--------------------------------------------------------------------------*/
> +/* Global Variables */
> +/*--------------------------------------------------------------------------*/
> +/* Stores the state of ADC driver */
> +static u8 adc_device_open = 0;
> +/* Major number for ADC driver */
> +static s32 adc_major_number = 0;
> +/* Mutex for ADC manual conversion */
> +struct mutex da9052_adc_manconv_lock;
> +/* Structure to store the mode status */
> +static adc_mode_status_t adc_mode_status;
> +static struct platform_device *da9052_adc_platform_device;
> +static struct fasync_struct *adc_fasync_queue;
> +
> +/* States whether a particular event is registered with EH */
> +static eh_registration_status_t is_eh_registered;
> +
> +/* EH structures for all possible events */
> +da9052_eh_nb adc4_eh_data;
> +da9052_eh_nb adc5_eh_data;
> +da9052_eh_nb adc6_eh_data;
> +da9052_eh_nb comp_1v2_eh_data;
> +
> +/* Storing the event status for multiple event handling */
> +static adc_event_occ_status_t adc_event_occ_status;
> +
> +/* Mutex erquired for protecting the access to the event occurance status */
> +struct mutex adc_event_occ_lock;
> +
> +/*--------------------------------------------------------------------------*/
> +/* Local Functions */
> +/*--------------------------------------------------------------------------*/
> +
> +/**
> + * da9052_manual_read :
This look like kernel doc, but isn't. Please fix and ensure running kernel-doc
on it (in scripts directory) doesn't give any errors (run with -v)
> + * This function is local to ADC module and it reads the
> + * data from ADC manual conversion registers. Manual
> + * conversion register is also accessed by TSI module, so
> + * mutex is used to make sure that no unathorised access
> + * to this register is made.
> + *
> + * @param channel - This parameter gives the channel number to be read
> + * @param *data - This is pointer to location where data is to written
> + *
> + * @return status:
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +static s32 da9052_manual_read(u8 channel, u16 *data)
> +{
> + u8 man_timeout_cnt = MAX_MANCONV_RETRY_COUNT;

Here why not:

da9052_ssc_msg msg = {
.addr = DA9052_ADCMAN_REG,
.data = channel | DA9052_ADCMAN_MANCONV,
};

> + da9052_ssc_msg msg;
> + u16 calc_data;
> + /* lock mutex */
> + mutex_lock(&da9052_adc_manconv_lock);
> +
> + msg.addr = DA9052_ADCMAN_REG;
> + msg.data = channel;

This set bits function is a silly waste of space. Just code
it long hand.
> + msg.data = set_bits(msg.data, DA9052_ADCMAN_MANCONV);
> + /* Write data to start manual conversion */

Take the lock just before here; there is no point in doing it earlier.
> + if (SUCCESS != da9052_ssc_write(&msg)){
> + mutex_unlock(&da9052_adc_manconv_lock);
> + return (ADC_SSC_BUS_ERROR);
> + }
> +
> + /* Wait for the event */
> + do {
> + /* Read Sampling interval timer */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg)){
> + mutex_unlock(&da9052_adc_manconv_lock);
> + return (ADC_SSC_BUS_ERROR);
> + }
> +
Check patch would have given you this one. Code style says:

if (...)
msleep(1);
else
msleep(10);
> + if (DA9052_ADCCONT_ADCMODE & msg.data) {
> + /* ADC sampling interval time is 1msec */
> + msleep (1);
> + } else {
> + /* ADC sampling interval time is 10msec */
> + msleep (10);
> + }
> +
> + msg.addr = DA9052_ADCMAN_REG;
> + /* read data to start manual conversion */
> + if (SUCCESS != da9052_ssc_read(&msg)){
> + mutex_unlock(&da9052_adc_manconv_lock);
> + return (ADC_SSC_BUS_ERROR);
> + }
> +
> + /* Counter to avoid endless while loop */
> + man_timeout_cnt --;
> + if(man_timeout_cnt == 1){
> + if (!(msg.data & DA9052_ADCMAN_MANCONV)) {
> + break;
> + } else {
> + mutex_unlock(&da9052_adc_manconv_lock);
> + return (ADC_MANCONV_TIMEOUT_ERROR);
> + }
> + }
> + /* Wait until the MAN_CONV bit is cleared to zero */
> + } while(msg.data & DA9052_ADCMAN_MANCONV);
> +
> + msg.addr = DA9052_ADCRESH_REG; /* Read MSB data */
> + if (SUCCESS != da9052_ssc_read(&msg)){
> + mutex_unlock(&da9052_adc_manconv_lock);
> + return (ADC_SSC_BUS_ERROR);
> + }
> + calc_data = (u16)msg.data;
> + *data = (calc_data << 2);
> +
As a general rule, get rid of debug lines like this. Now you have
a working driver they aren't terribly useful to anyone.

> + DA9052_DEBUG("ADCRESH_REG data = %x\n", *data);
> + DA9052_DEBUG("ADCRESH_REG msg.data = %x\n", msg.data);
> +
> + msg.addr = DA9052_ADCRESL_REG; /* Read LSB data */
> + if (SUCCESS != da9052_ssc_read(&msg)){
> + mutex_unlock(&da9052_adc_manconv_lock);
Please use general error codes rather than making up new ones.
I'm guessing -EIO is most appropriate here.
> + return (ADC_SSC_BUS_ERROR);
> + }
> + /* Clear first 14 bits before ORing */
> + calc_data = (u16)msg.data & 0x0003;
> + *data |= calc_data;
> +
> + DA9052_DEBUG("ADCRESL_REG data = %x\n", *data);
> + DA9052_DEBUG("ADCRESL_REG msg.data = %x\n", msg.data);
> + /* unlock mutex */
> + mutex_unlock(&da9052_adc_manconv_lock);
Don't use you're own magic numbers. Generally 0 is 'success' and
any errors are reported at appropriate negative.
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_hw_init: This function initializes the hardware for ADC module/
> + *
> + * @param void
> + * @return
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +static s32 da9052_adc_hw_init(void)
> +{
> + da9052_ssc_msg msg;
> +
> + /* Initialize the ADC pins */

This looks like a classic case for some platform data passing in whether
these should be enabled on a particular board.

> + /* ADC channel 4 and 5 are by default enabled */
> +#if (ADC_CONF_ADC4)
> + /* For channel 4 */
> + msg.addr = DA9052_GPIO0001_REG;
> + /* Read previous contents of the GPIO_0-1 register */
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Set the Pin functionality */
> + msg.data = clear_bits(msg.data, DA9052_GPIO0001_GPIO0PIN);
> + /* Write back the GPIO_0-1 register data */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +#endif /* ADC_CONF_ADC4 */
> +#if (ADC_CONF_ADC5)
> + /* For channel 5 */
> + msg.addr = DA9052_GPIO0001_REG;
> + /* Read previous contents of the GPIO_0-1 register */
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Set the Pin functionality */
> + msg.data = clear_bits(msg.data, DA9052_GPIO0001_GPIO1PIN);
> + /* Write back the GPIO_0-1 register data */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +#endif /* ADC_CONF_ADC5 */
> +#if (ADC_CONF_ADC6)
> + /* For channel 6 */
> + msg.addr = DA9052_GPIO0203_REG;
> + /* Read previous contents of the GPIO_0-1 register */
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Set the Pin functionality */
> + msg.data = clear_bits(msg.data, DA9052_GPIO0203_GPIO2PIN);
> + /* Write back the GPIO_0-1 register data */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +#endif /* ADC_CONF_ADC6 */
> +
> + /* By default configure the Measurement sequence interval to 1ms */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Set the ADC MODE bit for 1msec sampling timer */
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_ADCMODE);
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + /* By default configure ADC channel to auto mode */
> + adc_mode_status.adc_vddout = AUTO_MODE;
> + adc_mode_status.adc_ich = AUTO_MODE;
> + adc_mode_status.adc_tbat = AUTO_MODE;
> + adc_mode_status.adc_vbat = MANUAL_MODE;
> + adc_mode_status.adc_in4 = AUTO_MODE;
> + adc_mode_status.adc_in5 = AUTO_MODE;
> + adc_mode_status.adc_in6 = AUTO_MODE;
> + adc_mode_status.adc_tsense = AUTO_MODE;
> + adc_mode_status.adc_vbbat = MANUAL_MODE;
> +
> + /* Initialize mutex required for ADC Manual read */
> + mutex_init(&da9052_adc_manconv_lock);
> +
> + /* Initialize mutex for ADC event occurance status */
> + mutex_init(&adc_event_occ_lock);
> +
> + /* Init the status variable for event occurance */
> + adc_event_occ_status.adc_event_status = FALSE;
> +
> + /* Set EH registration flag to 0 */
> + is_eh_registered.adc4_event = FALSE;
> + is_eh_registered.adc5_event = FALSE;
> + is_eh_registered.adc6_event = FALSE;
> + is_eh_registered.comp_1v2_event = FALSE;
> +
> + DA9052_DEBUG("Finished hardware initialization\n");
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_hw_deinit: This function deinitializes the hardware.
> + *
> + * @param void
> + * @return s32
> + * SUCCESS - On successful completion of the call.
> + */
> +static s32 da9052_adc_hw_deinit(void)
> +{
> + /* Before exiting - unregister events with EH */
> + if (is_eh_registered.adc4_event)
> + da9052_eh_unregister_nb(&adc4_eh_data);
> + if (is_eh_registered.adc5_event)
> + da9052_eh_unregister_nb(&adc5_eh_data);
> + if (is_eh_registered.adc6_event)
> + da9052_eh_unregister_nb(&adc6_eh_data);
> + if (is_eh_registered.comp_1v2_event)
> + da9052_eh_unregister_nb(&comp_1v2_eh_data);
> +
> + /* Delete Mutex */
> + mutex_destroy(&da9052_adc_manconv_lock);
> +
> + /* Delete mutex for ADC event occurance status */
> + mutex_destroy(&adc_event_occ_lock);
> +
> + DA9052_DEBUG("Finished hardware de-initialization\n");
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_signal_to_user;
> + * This function signal to the library to call the call back function.
> + *
> + * @param event
> + * @return s32
> + */
> +static s32 da9052_adc_signal_to_user(u8 event)
> +{
> + DA9052_DEBUG("%s: event status: %d \n", __FUNCTION__, event);
> + kill_fasync(&adc_fasync_queue, SIGIO, POLL_IN);
> +
> + return(SUCCESS);
> +}
> +
> +/*--------------------------------------------------------------------------*/
> +/* Global Functions */
> +/*--------------------------------------------------------------------------*/
> +/**
> + * da9052_set_adc_mode: This function sets the mode for which the ADC channel
> + * is to be read. all channels can be configured in both
> + * modes. Except for the following conditions
> + * 1. TSI channel cannot be configured in any mode.
> + * 2. VBAT and VBBAT cannot be configured in AUTO mode.
> + *
> + * @param channel - This parameter selects the channel to be read.
> + * @param mode - This parameter selects the mode for the ADC channel
> + * selected.
> + * @return status:
> + * ADC_INVALID_CHANNEL_ERROR - If the selected channel is out of range
> + * A0-A10
> + * MODE - If the selected mode is other then AUTO or MANUAL
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_set_adc_mode(u8 channel, u8 mode)
> +{
> + /* TSI channel cannot be read by ADC */
> + if (channel == ADC_TSI)
> + return (ADC_INVALID_CHANNEL_ERROR);
> +
> + /* Check for Illegal mode set */
> + if ((mode != AUTO_MODE) && (mode != MANUAL_MODE))
> + return (ADC_INVALID_MODE_ERROR);
> + /* Check for Illegal channel set */
> + if((mode == AUTO_MODE) && ((channel == ADC_VBAT) ||
> + (channel == ADC_VBBAT)))
> + return (ADC_INVALID_CHANNEL_ERROR);
> +
This is looking like a classic case for an array and appropriate
enum, rather than a structure containing named elements.
> + switch (channel) {
> + case ADC_VDDOUT:
> + adc_mode_status.adc_vddout = mode;
> + break;
> + case ADC_ICH:
> + adc_mode_status.adc_ich = mode;
> + break;
> + case ADC_TBAT:
> + adc_mode_status.adc_tbat = mode;
> + break;
> + case ADC_VBAT:
> + adc_mode_status.adc_vbat = mode;
> + break;
> + case ADC_ADCIN4:
> + adc_mode_status.adc_in4 = mode;
> + break;
> + case ADC_ADCIN5:
> + adc_mode_status.adc_in5 = mode;
> + break;
> + case ADC_ADCIN6:
> + adc_mode_status.adc_in6 = mode;
> + break;
> + case ADC_TSI:
> + adc_mode_status.adc_tsi = mode;
> + break;
> + case ADC_TJUNC:
> + adc_mode_status.adc_tsense = mode;
> + break;
> + case ADC_VBBAT:
> + adc_mode_status.adc_vbbat = mode;
> + break;
> + default :
> + /* Illegal Channel set */
> + return (ADC_INVALID_CHANNEL_ERROR);
> + }
> +
> + DA9052_DEBUG("vddout = %d\n",adc_mode_status.adc_vddout);
> + DA9052_DEBUG("adc_ich = %d\n",adc_mode_status.adc_ich);
> + DA9052_DEBUG("adc_tbat = %d\n",adc_mode_status.adc_tbat);
> + DA9052_DEBUG("adc_vbat = %d\n",adc_mode_status.adc_vbat);
> + DA9052_DEBUG("adc_in4 = %d\n",adc_mode_status.adc_in4);
> + DA9052_DEBUG("adc_in5 = %d\n",adc_mode_status.adc_in5);
> + DA9052_DEBUG("adc_in6 = %d\n",adc_mode_status.adc_in6);
> + DA9052_DEBUG("adc_tsi = %d\n",adc_mode_status.adc_tsi);
> + DA9052_DEBUG("adc_tsense= %d\n",adc_mode_status.adc_tsense);
> + DA9052_DEBUG("adc_vbbat = %d\n",adc_mode_status.adc_vbbat);
> +
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_set_sampling_interval :
> + * This function sets the measurement sequence interval.
> + * Only two measurement sequence interval setting is
> + * possible 10 ms and 1 ms.
> + *
> + * @param interval - This parameter selects the sampling speed.
> + *
> + * @return status:
> + * ADC_INVALID_TIME_ERROR - If time is set to some other value other than
> + * 1ms or 10ms.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_adc_set_sampling_interval(u8 interval)
> +{
> + da9052_ssc_msg msg;
> +
> + /* Read existing data in the register */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Modify the data to be written */
Incorrect brackets for coding style
> + if (interval == ADC_INTERVAL_10MS) {
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_ADCMODE);
> + } else if (interval == ADC_INTERVAL_1MS) {
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_ADCMODE);
> + } else {
> + return (ADC_INVALID_TIME_ERROR);
> + }
> + /* Write the data */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_read_vddout :
> + * This function reads the value from ADC
> + * channel 0. If the vddout channel is disabled, this
> + * function enables it and read the value and again
> + * disables the channel. If vddout channel is already
> + * enabled this function directly reads the value from
> + * the ADC register.
> + *
> + * @param *data - This parameter is pointer to u16 where data can be
> + * written.
> + *
> + * @return status:
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_adc_read_vddout(u16 *data)
> +{
> + da9052_ssc_msg msg;
> +
> + /* Check ADC (VDDOUT) status enabled/disabled */
> + msg.addr = DA9052_ADCCONT_REG;
> + if(SUCCESS != da9052_ssc_read(&msg))
> + return(ADC_SSC_BUS_ERROR);
> +
> + if (!(msg.data & DA9052_ADCCONT_AUTOVDDEN)) {
> + /* If disabled, enable it and then read the value */
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_AUTOVDDEN);
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + /* Wait for the conversion time period */
> + /* Read sampling interval set in the ADC_CONT register */
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + if (msg.data & DA9052_ADCCONT_ADCMODE) {
> + /* If sampling interval is 1ms */
> + msleep (1);
> + } else {
> + /* If sampling interval is 10ms */
> + msleep (10);
> + }
> +
> + /* Read the ADC converted value */
> + msg.addr = DA9052_VDDRES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + *data = (u16)msg.data;
> + DA9052_DEBUG(
> + "In function: %s, VDDCONT_RES value read (1)= 0x%X \n",
> + __FUNCTION__, msg.data);
> + /* Disable the VDDOUT ADC again */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_AUTOVDDEN);
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + } else {
> + /* Read the ADC converted value */
> + msg.addr = DA9052_VDDRES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + *data = (u16)msg.data;
> + DA9052_DEBUG(
> + "In function: %s, VDDCONT_RES value read (1)= 0x%X \n",
> + __FUNCTION__, msg.data);
> + }
> + return (SUCCESS);
> +}

If you really need to have independant functions for these, please write
a core inline function and have the others call that. Otherwise there
is a lot unneeded repeating of code.
> +
> +/**
> + * da9052_adc_read_ich :
> + * This function reads the value from ADC
> + * channel 1. There is no need to call the channel mode,
> + * start and read functions.
> + *
> + * @param data - This parameter is pointer to u16 where data can be
> + * written.
> + *
> + * @return status:
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_adc_read_ich(u16 *data)
> +{
> + da9052_ssc_msg msg;
> + /* Read charging conversion register */
> + msg.addr = DA9052_ICHGAV_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + *data = (u16)msg.data;
> + DA9052_DEBUG(
> + "In function: %s, ICHGAV_REG value read (1)= 0x%X \n",
> + __FUNCTION__, msg.data);
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_read_tbat :
> + * This function reads the value from ADC
> + * channel 2. There is no need to call the channel mode,
> + * start and read functions.
> + *
> + * @param data - This parameter is pointer to u9 where data can be
> + * written.
> + *
> + * @return status:
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_adc_read_tbat(u16 *data)
> +{
> + da9052_ssc_msg msg;
> + /* Read TBAT conversion result */
> + msg.addr = DA9052_TBATRES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + *data = (u16)msg.data;
> + DA9052_DEBUG(
> + "In function: %s, TBATRES_REG value read (1)= 0x%X \n",
> + __FUNCTION__, msg.data);
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_read_vbat :
> + * This function reads the value from ADC
> + * channel 3. There is no need to call the channel mode,
> + * start and read functions. This function reads the
> + * value by manual conversion (hardware limitation).
> + *
> + * @param *data - This is pointer to location where data is to written
> + *
> + * @return status:
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_adc_read_vbat(u16 *data)
> +{
> + s32 error;
> + u16 temp;
> +
> + error = da9052_manual_read(ADC_VBAT, &temp);
> + DA9052_DEBUG(
> + "In function: %s, VBAT value read (1)= 0x%X \n",
> + __FUNCTION__, temp);
> + if(SUCCESS == error){
> + *data = temp;
> + return (SUCCESS);
> + } else {
> + return(error);
> + }
> +}
> +
> +/**
> + * da9052_adc_read_tjunc :
> + * This function reads the value from ADC
> + * channel 8. There is no need to call the channel mode,
> + * start and read functions.
> + *
> + * @param *data - This is pointer to location where data is to written
> + *
> + * @return status:
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_adc_read_tjunc(u16 *data)
> +{
> + da9052_ssc_msg msg;
> + u16 temp;
> + /* Read TJunction conversion result */
> + msg.addr = DA9052_TJUNCRES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + temp = msg.data;
> + /* Read calibration for junction temperature */
> + msg.addr = DA9052_TOFFSET_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Calculate Junction temperature */
> + *data = temp - (u16)msg.data;
> + DA9052_DEBUG(
> + "In function: %s, Calc JUNC TEMP value (1)= 0x%X \n",
> + __FUNCTION__, *data);
> + return(SUCCESS);
> +}
> +
> +
> +/**
> + * da9052_adc_read_vbbat :
> + * This function reads the value from ADC
> + * channel 9. There is no need to call the channel mode,
> + * start and read functions. This function reads the
> + * value by manual conversion (hardware limitation).
> + *
> + * @param *data - This is pointer to location where data is to written
> + *
> + * @return status:
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_adc_read_vbbat(u16 *data)
> +{
> + s32 error;
> + u16 temp;
> +
> + error = da9052_manual_read(ADC_VBBAT, &temp);
> + DA9052_DEBUG(
> + "In function: %s, VBBAT value read (1)= 0x%X \n",
> + __FUNCTION__, temp);
> + if(SUCCESS == error){
> + *data = temp;
> + return (SUCCESS);
> + } else {
> + return(error);
> + }
> +}
> +
> +/**
> + * da9052_configure_thresholds :
> + * This function sets the threshold value of the ADCIN4,
> + * ADCIN5 and ADCIN6 channels. If value for any of the
> + * channel is wrong this function will return error
> + * without writing any value in the registers.
> + *
> + * @param threshold_res thresholds (structure containing threshold settings)
> + *
> + *
> + * @return status:
> + * ADC_INVALID_VALUE - Error in the high and low value.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_configure_thresholds(threshold_res thresholds)
> +{
> + da9052_ssc_msg msg;
> +
> + DA9052_DEBUG("In function: %s \n", __FUNCTION__);
> + DA9052_DEBUG("auto4_high : %d \n", thresholds.auto4_high);
> + DA9052_DEBUG("auto4_low : %d \n", thresholds.auto4_low);
> + DA9052_DEBUG("auto5_high : %d \n", thresholds.auto5_high);
> + DA9052_DEBUG("auto5_low : %d \n", thresholds.auto5_low);
> + DA9052_DEBUG("auto6_high : %d \n", thresholds.auto6_high);
> + DA9052_DEBUG("auto6_low : %d \n", thresholds.auto6_low);
> +
> + /* Check if the High value is higher then Low value */
> + if (thresholds.auto4_high <= thresholds.auto4_low)
> + return (ADC_INVALID_VALUE);
> + if (thresholds.auto5_high <= thresholds.auto5_low)
> + return (ADC_INVALID_VALUE);
> + if (thresholds.auto6_high <= thresholds.auto6_low)
> + return (ADC_INVALID_VALUE);
> +
> + /* Write threshold value to the register */
> + /* ADCIN4 */
> + msg.addr = DA9052_AUTO4HIGH_REG;
> + msg.data = thresholds.auto4_high;
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + msg.addr = DA9052_AUTO4LOW_REG;
> + msg.data = thresholds.auto4_low;
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + /* ADCIN5 */
> + msg.addr = DA9052_AUTO5HIGH_REG;
> + msg.data = thresholds.auto5_high;
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + msg.addr = DA9052_AUTO5LOW_REG;
> + msg.data = thresholds.auto5_low;
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + /* ADCIN6 */
> + msg.addr = DA9052_AUTO6HIGH_REG;
> + msg.data = thresholds.auto6_high;
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + msg.addr = DA9052_AUTO6LOW_REG;
> + msg.data = thresholds.auto6_low;
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_set_adcin5_comparator :
> + * This function enables/disables the comparator for
> + * ADCIN5.
> + *
> + * @param action -Enable or Disable action to be performed.
> + *
> + * @return status:
> + * ADC_INVALID_VALUE - If the parameter passed is wrong.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_set_adcin5_comparator(u8 action)
> +{
> + da9052_ssc_msg msg;
> +
> + msg.addr = DA9052_ADCCONT_REG;
> + msg.data = 0;
> + /* Read previous contents of the register */
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Modify the bit */
> + if (action == ENABLE) {
> + msg.data = set_bits(msg.data,DA9052_ADCCONT_COMP1V2EN);
> + } else if (action == DISABLE) {
> + msg.data = clear_bits(msg.data,DA9052_ADCCONT_COMP1V2EN);
> + } else {
> + return (ADC_INVALID_VALUE);
> + }
> + /* Write back the value to the register */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return(ADC_SSC_BUS_ERROR);
> +
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_set_tbat_source_current :
> + * This function enables the 50uA current source for
> + * TBAT permanently or only for two slots of auto mode
> + * ADC.
> + *
> + * @param action - Two slots OR premanently.
> + *
> + * @return status:
> + * ADC_INVALID_VALUE - If the parameter passed is wrong.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_set_tbat_source_current(u8 action)
> +{
> + da9052_ssc_msg msg;
> +
> + msg.addr = DA9052_ADCCONT_REG;
> + msg.data = 0;
> + /* Read previous contents of the register */
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Modify the bit value */
> + if (action == TBAT_50UA_PREMANENTLY) {
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_TBATISRCEN);
> + } else if (action == TBAT_50UA_ONE_SLOT) {
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_TBATISRCEN);
> + } else {
> + return (ADC_INVALID_VALUE);
> + }
> + /* Write back to the register */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return(ADC_SSC_BUS_ERROR);
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_set_ad4_isrc_source_current :
> + * This function enables the 15uA current source for
> + * ADCIN4.
> + *
> + * @param action - Enable or Disable.
> + *
> + * @return status:
> + * ADC_INVALID_VALUE - If the parameter passed is wrong.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_set_ad4_isrc_source_current(u8 action)
> +{
> + da9052_ssc_msg msg;
> +
> + msg.addr = DA9052_ADCCONT_REG;
> + msg.data = 0;
> + /* Read previous contents of the register */
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + /* Modify the bit value */
> + if (action == ENABLE) {
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_AD4ISRCEN);
> + } else if (action == DISABLE) {
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_AD4ISRCEN);
> + } else {
> + return (ADC_INVALID_VALUE);
> + }
> + /* Write back to the register */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + return (SUCCESS);
> +}
> + /**
> + * da9052_start_adc :
> + * This function starts the ADC channel in AUTO mode.
> + *
> + * @param channel - Channel No. to be started.
> + *
> + * @return status:
> + * CHANNEL - If the channel value passed to this functions is
> + * wrong.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_start_adc (u8 channel)
> +{
> + da9052_ssc_msg msg;
> +
> + msg.addr = DA9052_ADCCONT_REG;
> + msg.data = 0;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + /* Set the bit for corresponding channel in ADC_CONT register */
> + if (channel == ADC_VDDOUT) {
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_AUTOVDDEN);
> + } else if (channel == ADC_ADCIN4) {
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_AUTOAD4EN);
> + } else if (channel == ADC_ADCIN5) {
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_AUTOAD5EN);
> + } else if (channel == ADC_ADCIN6) {
> + msg.data = set_bits(msg.data, DA9052_ADCCONT_AUTOAD6EN);
> + } else {
> + return (ADC_INVALID_CHANNEL_ERROR);
> + }
> + /* Write the value in the register */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + return (SUCCESS);
> +}
> +
> + /**
> + * da9052_stop_adc :
> + * This function stops the ADC channel in AUTO mode.
> + *
> + * @param channel - Channel No. to be stopped.
> + *
> + * @return status:
> + * CHANNEL - If the channel value passed to this functions is
> + * wrong.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_stop_adc (u8 channel)
> +{
> + da9052_ssc_msg msg;
> +
> + msg.addr = DA9052_ADCCONT_REG;
> + msg.data = 0;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + /* Clear the data in the ADC_CONT register */
> + if (channel == ADC_VDDOUT) {
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_AUTOVDDEN);
> + } else if (channel == ADC_ADCIN4) {
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_AUTOAD4EN);
> + } else if (channel == ADC_ADCIN5) {
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_AUTOAD5EN);
> + } else if (channel == ADC_ADCIN6) {
> + msg.data = clear_bits(msg.data, DA9052_ADCCONT_AUTOAD6EN);
> + } else {
> + return (ADC_INVALID_CHANNEL_ERROR);
> + }
> + /* Write the data */
> + if (SUCCESS != da9052_ssc_write(&msg))
> + return (ADC_SSC_BUS_ERROR);
> +
> + return (SUCCESS);
> +}
> +
> + /**
> + * da9052_read_adc :
> + * This function reads the ADC value for the channel
> + * specified in the parameter.
> + *
> + * @param channel - Channel No. to be read.
> + * @param data - Pointer to the location where data is to be copied.
> + *
> + * @return status:
> + * CHANNEL - If the channel value passed to this functions is
> + * wrong.
> + * ADC_CH4_NOT_CONF - If ADCIN4 pins not configured for ADC.
> + * ADC_CH5_NOT_CONF - If ADCIN5 pins not configured for ADC.
> + * ADC_CH6_NOT_CONF - If ADCIN6 pins not configured for ADC.
> + * ADC_CHANNEL_NOT_STARTED_ERROR - If the channel is not started in Auto
> + * mode.
> + * ADC_SSC_BUS_ERROR - If there is an error on the SSC bus.
> + * SUCCESS - On successful completion of the call.
> + */
> +s32 da9052_read_adc (u8 channel, u16 *data)
> +{
> + da9052_ssc_msg msg;
> + s32 error;
> + u16 calc_data;
> +
> + switch(channel){
> + /* VDD_OUT channel */
> + case ADC_VDDOUT:{
> + /* If manual mode do manual read */
> + if(adc_mode_status.adc_vddout == MANUAL_MODE){
> + error = da9052_manual_read(channel, &calc_data);
> + if (error != SUCCESS) {
> + return (error);
> + } else {
> + /* 8 bit data in manual mode */
> + *data = calc_data >> 2;
> + }
> + /* Check Auto mode */
> + } else {
> + /* Check if channel is enabled */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + if(msg.data & DA9052_ADCCONT_AUTOVDDEN) {
> + msg.addr = DA9052_VDDRES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + *data = (u16)msg.data;
> + } else {
> + return (ADC_CHANNEL_NOT_STARTED_ERROR);
> + }
> + }
> + return (SUCCESS);
> + }
> + case ADC_ICH:{
> + /* If manual mode do manual read */
> + if(adc_mode_status.adc_ich == MANUAL_MODE) {
> + error = da9052_manual_read(channel, &calc_data);
> + if (error != SUCCESS) {
> + return (error);
> + } else {
> + /* 8 bit data in manual mode */
> + *data = calc_data >> 2;
> + }
> + } else {
> + /* Read data in AUTO mode */
> + msg.addr = DA9052_ICHGAV_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + *data = (u16)msg.data;
> + }
> + return (SUCCESS);
> + }
> + case ADC_TBAT:{
> + /* If manual mode do manual read */
> + if(adc_mode_status.adc_tbat == MANUAL_MODE){
> + error = da9052_manual_read(channel, &calc_data);
> + if (error != SUCCESS) {
> + return (error);
> + } else {
> + /* 8 bit data in manual mode */
> + *data = calc_data >> 2;
> + }
> + } else {
> + /* Read data in AUTO mode */
> + msg.addr = DA9052_TBATRES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + *data = (u16)msg.data;
> + }
> + return (SUCCESS);
> + }
> + case ADC_VBAT:{
> + if(adc_mode_status.adc_vbat == MANUAL_MODE) {
> + return (da9052_manual_read(channel, data));
> + } else {
> + return (ADC_INVALID_CHANNEL_ERROR);
> + }
> + return (SUCCESS);
> + }
> + case ADC_ADCIN4:{
> +#if (ADC_CONF_ADC4)
> + /* If manual mode do manual read */
> + if(adc_mode_status.adc_in4 == MANUAL_MODE){
> + error = da9052_manual_read(channel, &calc_data);
> + if (error != SUCCESS) {
> + return (error);
> + } else {
> + /* 8 bit data in manual mode */
> + *data = calc_data >> 2;
> + }
> + /* Check Auto mode */
> + } else {
> + /* Check if channel is enabled */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + if(msg.data & DA9052_ADCCONT_AUTOAD4EN) {
> + msg.addr = DA9052_ADCIN4RES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + *data = (u16)msg.data;
> + } else {
> + return (ADC_CHANNEL_NOT_STARTED_ERROR);
> + }
> + }
> + return (SUCCESS);
> +#else
> + /* If the ADCIN4 pin not configured for ADC */
> + return (ADC_CH4_NOT_CONF);
> +#endif
> + }
> + case ADC_ADCIN5:{/* ADCIN5 */
> +#if (ADC_CONF_ADC5)
> + /* If manual mode do manual read */
> + if(adc_mode_status.adc_in5 == MANUAL_MODE){
> + error = da9052_manual_read(channel, &calc_data);
> + if (error != SUCCESS) {
> + return (error);
> + } else {
> + /* 8 bit data in manual mode */
> + *data = calc_data >> 2;
> + }
> + /* Check Auto mode */
> + } else {
> + /* Check if channel is enabled */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + if(msg.data & DA9052_ADCCONT_AUTOAD5EN) {
> + msg.addr = DA9052_ADCIN5RES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + *data = (u16)msg.data;
> + } else {
> + return (ADC_CHANNEL_NOT_STARTED_ERROR);
> + }
> + }
> + return (SUCCESS);
> +#else
> + /* If the ADCIN5 pin not configured for ADC */
> + return (ADC_CH5_NOT_CONF);
> +#endif
> + }
> + case ADC_ADCIN6:{/* ADCIN6 */
> +#if (ADC_CONF_ADC6)
> + /* If manual mode do manual read */
> + if(adc_mode_status.adc_in6 == MANUAL_MODE){
> + error = da9052_manual_read(channel, &calc_data);
> + if (error != SUCCESS) {
> + return (error);
> + } else {
> + /* 8 bit data in manual mode */
> + *data = calc_data >> 2;
> + }
> + /* Check Auto mode */
> + } else {
> + /* Check if channel is enabled */
> + msg.addr = DA9052_ADCCONT_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + if(msg.data & DA9052_ADCCONT_AUTOAD6EN) {
> + msg.addr = DA9052_ADCIN6RES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + *data = (u16)msg.data;
> + } else {
> + return (ADC_CHANNEL_NOT_STARTED_ERROR);
> + }
> + }
> + return (SUCCESS);
> +#else
> + /* If the ADCIN6 pin not configured for ADC */
> + return (ADC_CH6_NOT_CONF);
> +#endif
> + }
> + case ADC_TSI:{
> + return (ADC_INVALID_CHANNEL_ERROR);
> + }
> + case ADC_TJUNC:{
> + if (adc_mode_status.adc_tsense == MANUAL_MODE) {
> + error = da9052_manual_read(channel, &calc_data);
> + if (error != SUCCESS) {
> + return (error);
> + } else {
> + /* 8 bit data in manual mode */
> + *data = calc_data >> 2;
> + }
> + } else {
> + /* Read data in AUTO mode */
> + msg.addr = DA9052_TJUNCRES_REG;
> + if (SUCCESS != da9052_ssc_read(&msg))
> + return (ADC_SSC_BUS_ERROR);
> + *data = (u16)msg.data;
> + }
> + return (SUCCESS);
> + }
> + case ADC_VBBAT:{
> + if (adc_mode_status.adc_vbbat == MANUAL_MODE) {
> + return (da9052_manual_read(channel, data));
> + } else {
> + return (ADC_INVALID_CHANNEL_ERROR);
> + }
> + return (SUCCESS);
> + }
> + /* Check Illegal Channel */
> + default :
> + return (ADC_INVALID_CHANNEL_ERROR);
> + }
> +
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_adcin4_handler : register the event
> + *
> + * @param void
> + *
> + * @return s32 Error Code, zero: no error
> + */
> +void da9052_adc_adcin4_handler(u32 data)
> +{
> + /* Acquire lock before accessing the status variable */
> + mutex_lock(&adc_event_occ_lock);
> + /* Store the status of the event in the variable */
> + adc_event_occ_status.event_bits_t.adc_adcin4_e_occ = TRUE;
> + /* Release the lock */
> + mutex_unlock(&adc_event_occ_lock);
> + /* Signal to the user space */
> + da9052_adc_signal_to_user(GPI0_EVE);
> +}
> +
> +/**
> + * da9052_adc_adcin5_handler : register the event
> + *
> + * @param void
> + *
> + * @return s32 Error Code, zero: no error
> + */
> +void da9052_adc_adcin5_handler(u32 data)
> +{
> + /* Acquire lock before accessing the status variable */
> + mutex_lock(&adc_event_occ_lock);
> + /* Store the status of the event in the variable */
> + adc_event_occ_status.event_bits_t.adc_adcin5_e_occ = TRUE;
> + /* Release the lock */
> + mutex_unlock(&adc_event_occ_lock);
> + /* Signal to the user space */
> + da9052_adc_signal_to_user(GPI1_EVE);
> +}
> +
> +/**
> + * da9052_adc_adcin6_handler : register the event
> + *
> + * @param void
> + *
> + * @return s32 Error Code, zero: no error
> + */
> +void da9052_adc_adcin6_handler(u32 data)
> +{
> + /* Acquire lock before accessing the status variable */
> + mutex_lock(&adc_event_occ_lock);
> + /* Store the status of the event in the variable */
> + adc_event_occ_status.event_bits_t.adc_adcin6_e_occ = TRUE;
> + /* Release the lock */
> + mutex_unlock(&adc_event_occ_lock);
> + /* Signal to the user space */
> + da9052_adc_signal_to_user(GPI2_EVE);
> +}
> +
> +
> +/**
> + * da9052_1v2_comp_handler : register the event
> + *
> + * @param void
> + *
> + * @return s32 Error Code, zero: no error
> + */
> +void da9052_1v2_comp_handler(u32 data)
> +{
> + /* Acquire lock before accessing the status variable */
> + mutex_lock(&adc_event_occ_lock);
> + /* Store the status of the event in the variable */
> + adc_event_occ_status.event_bits_t.adc_comp1v2_e_occ = TRUE;
> + /* Release the lock */
> + mutex_unlock(&adc_event_occ_lock);
> + /* Signal to the user space */
> + da9052_adc_signal_to_user(COMP_1V2);
> +}
> +
> +/**
> + * da9052_adc_register_event : register the event
> + *
> + * @param u8 event_type event type
> + *
> + * @return s32 Error Code, zero: no error
> + */
> +s32 da9052_adc_register_event(u8 event_type)
> +{
> + /* Register the callback function with event handler */
> + switch(event_type) {
> + case GPI0_EVE:
> +#if ADC_CONF_ADC4
> + /* Check if event is already registered */
> + if (!is_eh_registered.adc4_event) {
> + /* Set the EH structure */
> + adc4_eh_data.call_back = da9052_adc_adcin4_handler;
> + adc4_eh_data.eve_type = event_type;
> + /* Register the event with EH */
> + if (da9052_eh_register_nb(&adc4_eh_data))
> + return (ADC_EVENT_REGISTRATION_FAILED);
> + /* Set the registration flag to 1 */
> + is_eh_registered.adc4_event = TRUE;
> + }
> +#else
> + return (ADC_CH4_NOT_CONF);
> +#endif
> + break;
> + case GPI1_EVE:
> +#if ADC_CONF_ADC5
> + /* Check if event is already registered */
> + if (!is_eh_registered.adc5_event) {
> + /* Set the EH structure */
> + adc5_eh_data.call_back = da9052_adc_adcin5_handler;
> + adc5_eh_data.eve_type = event_type;
> + /* Register the event with EH */
> + if (da9052_eh_register_nb(&adc5_eh_data))
> + return (ADC_EVENT_REGISTRATION_FAILED);
> + /* Set the registration flag to 1 */
> + is_eh_registered.adc5_event = TRUE;
> + }
> +#else
> + return (ADC_CH5_NOT_CONF);
> +#endif
> + break;
> + case GPI2_EVE:
> +#if ADC_CONF_ADC6
> + /* Check if event is already registered */
> + if (!is_eh_registered.adc6_event) {
> + /* Set the EH structure */
> + adc6_eh_data.call_back = da9052_adc_adcin6_handler;
> + adc6_eh_data.eve_type = event_type;
> + /* Register the event with EH */
> + if (da9052_eh_register_nb(&adc6_eh_data))
> + return (ADC_EVENT_REGISTRATION_FAILED);
> + /* Set the registration flag to 1 */
> + is_eh_registered.adc6_event = TRUE;
> + }
> +#else
> + return (ADC_CH6_NOT_CONF);
> +#endif
> + break;
> + case COMP_1V2:
> + /* Check if event is already registered */
> + if (!is_eh_registered.comp_1v2_event) {
> + /* Set the EH structure */
> + comp_1v2_eh_data.call_back = da9052_1v2_comp_handler;
> + comp_1v2_eh_data.eve_type = event_type;
> + /* Register the event with EH */
> + if (da9052_eh_register_nb(&comp_1v2_eh_data))
> + return (ADC_EVENT_REGISTRATION_FAILED);
> + /* Set the registration flag to 1 */
> + is_eh_registered.comp_1v2_event = TRUE;
> + }
> + break;
> + default:
> + return (ADC_INVALID_EVENT);
> + }
> +
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_get_event : Get the status of the events occured. Required by the
> + * user space application
> + *
> + * @param void
> + *
> + * @return status
> + */
> +s32 da9052_adc_get_event(unsigned long *argument)
> +{
> + u8 events;
> + /* Acquire lock */
> + mutex_lock(&adc_event_occ_lock);
> + /* Read the value to the local variable */
> + events = adc_event_occ_status.adc_event_status;
> + /* Clear the Global variable */
> + adc_event_occ_status.adc_event_status = FALSE;
> + /* Release the lock */
> + mutex_unlock(&adc_event_occ_lock);
> + /* Copy the event to the user space */
> + return(copy_to_user((u8 *)(*argument), (void *)&events, sizeof(u8)));
> +}
> +
> +/**
> + * da9052_adc_unregister_event : register the event
> + *
> + * @param u8 event_type event type
> + *
> + * @return s32 Error Code, zero: no error
> + */
> +s32 da9052_adc_unregister_event(u8 event_type)
> +{
> + /* Check event type */
> + switch (event_type) {
> + case GPI0_EVE:
> +#if ADC_CONF_ADC4
> + /* Unregister event */
> + if (is_eh_registered.adc4_event) {
> + if (da9052_eh_unregister_nb(&adc4_eh_data)) {
> + return (ADC_EVENT_UNREGISTRATION_FAILED);
> + }
> + /* Set flag to 0 */
> + is_eh_registered.adc4_event = FALSE;
> + }
> +#else
> + return (ADC_CH4_NOT_CONF);
> +#endif
> + break;
> + case GPI1_EVE:
> +#if ADC_CONF_ADC5
> + /* Unregister event */
> + if (is_eh_registered.adc5_event) {
> + if (da9052_eh_unregister_nb(&adc5_eh_data)) {
> + return (ADC_EVENT_UNREGISTRATION_FAILED);
> + }
> + /* Set flag to 0 */
> + is_eh_registered.adc5_event = FALSE;
> + }
> +#else
> + return (ADC_CH5_NOT_CONF);
> +#endif
> + break;
> + case GPI2_EVE:
> +#if ADC_CONF_ADC6
> + /* Unregister event */
> + if (is_eh_registered.adc6_event) {
> + if (da9052_eh_unregister_nb(&adc6_eh_data)) {
> + return (ADC_EVENT_UNREGISTRATION_FAILED);
> + }
> + /* Set flag to 0 */
> + is_eh_registered.adc6_event = FALSE;
> + }
> +#else
> + return (ADC_CH6_NOT_CONF);
> +#endif
> + break;
> + case COMP_1V2:
> + /* Unregister event */
> + if (is_eh_registered.comp_1v2_event) {
> + if (da9052_eh_unregister_nb(&comp_1v2_eh_data)) {
> + return (ADC_EVENT_UNREGISTRATION_FAILED);
> + }
> + /* Set flag to 0 */
> + is_eh_registered.comp_1v2_event = FALSE;
> + }
> + break;
> + default:
> + return (ADC_INVALID_EVENT);
> + break;
> + }
> + return (SUCCESS);
> +}
> +/*--------------------------------------------------------------------------*/
> +/* Infrastructure Functions */
> +/*--------------------------------------------------------------------------*/
> +/**
> + * da9052_adc_open:
> + *
> + * @param *inode pointer to device inode
> + * @param *file file pointer
> + * @return s32 Error Code, zero: no error
> + */
> +s32 da9052_adc_open(struct inode *inode, struct file *file)
> +{
> + /* Check if device is already open */
> + if(adc_device_open) {
> + printk("DA9052: ADC device already open.\n");
> + return (-EBUSY);
> + } else {
> + adc_device_open++;
> + return (SUCCESS);
> + }
> +}
> +
> +/**
> + * da9052_adc_release:
> + *
> + * @param *inode pointer to device inode
> + * @param *file file pointer
> + * @return s32 Error Code, zero: no error
> + */
> +s32 da9052_adc_release(struct inode *inode, struct file *file)
> +{
> + adc_device_open--;
> + printk("DA9052: ADC device closed.\n");
> + return (SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_suspend: Power Management support function
> + *
> + * @param *dev pointer to platform device
> + * @param state pm state
> + * @return s32 status of suspend operation
> + */
> +static s32 da9052_adc_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + /* Put your suspend related operations here */
> + printk(KERN_INFO "%s: called\n", __FUNCTION__);
> + return (0);
> +}
> +
> +/**
> + * da9052_adc_resume: Power Management support function
> + *
> + * @param *dev pointer to platform device
> + * @return s32 status of resume operation
> + */
> +static s32 da9052_adc_resume(struct platform_device *dev)
> +{
> + /* Put your resume related operations here */
> + printk(KERN_INFO "%s: called\n", __FUNCTION__);
> + return (0);
> +}
> +
> +/**
> + * da9052_adc_ioctl:
> + *
> + * @param *inode pointer to device inode
> + * @param *file file pointer
> + * @param cmd command to be executed
> + * @param arg argument to command
> + * @return s32 Error Code, zero: no error
> + */
> +s32 da9052_adc_ioctl(struct inode *inode, struct file *file,
> + u32 cmd, unsigned long arg)
> +{
> + s32 error = SUCCESS;
> + u8 event_data, *event_ptr;
> + da9052_adc_data_t usr_param, *ptr;
> +#if DA9052_ADC_DEBUG
> + u16 data;
> +#endif /* DA9052_UNIT_TESTING_ADC */
> +
> +
> + ptr = (da9052_adc_data_t *) arg;
> +
> + /* Switch as per the command requested */
> + switch(cmd) {
> + case DA9052_ADC_IOCTL_SET_ADC_MODE:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_set_adc_mode(usr_param.channel, usr_param.mode);
> + break;
> + case DA9052_ADC_IOCTL_SET_SAMPLING_INTERVAL:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_adc_set_sampling_interval(usr_param.interval);
> + break;
> + case DA9052_ADC_IOCTL_CONFIGURE_THRESHOLDS:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_configure_thresholds (usr_param.threshold);
> + break;
> + case DA9052_ADC_IOCTL_SET_ADCIN5_COMPARATOR:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_set_adcin5_comparator(usr_param.action);
> + break;
> + case DA9052_ADC_IOCTL_SET_TBAT_SOURCE_CURRENT:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_set_tbat_source_current(usr_param.action);
> + break;
> + case DA9052_ADC_IOCTL_SET_ADCIN4_SOURCE_CURRENT:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_set_ad4_isrc_source_current(usr_param.action);
> + break;
> + case DA9052_ADC_IOCTL_START_ADC:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_start_adc(usr_param.channel);
> + break;
> + case DA9052_ADC_IOCTL_STOP_ADC:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_stop_adc(usr_param.channel);
> + break;
> + case DA9052_ADC_IOCTL_READ_ADC:
> + if (copy_from_user(&usr_param, ptr, sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + error = da9052_read_adc(usr_param.channel, usr_param.data);
> + if (copy_to_user((u16 *)arg, (void *)&usr_param,
> + sizeof(da9052_adc_data_t)))
> + return (FAILURE);
> + break;
> + case DA9052_ADC_IOCTL_REGISTER_EVENT:
> + event_ptr = (u8 *) arg;
> + if (copy_from_user(&event_data, event_ptr, sizeof(u8)))
> + return (FAILURE);
> + error = da9052_adc_register_event(event_data);
> + break;
> + case DA9052_ADC_IOCTL_UNREGISTER_EVENT:
> + event_ptr = (u8 *) arg;
> + if (copy_from_user(&event_data, event_ptr, sizeof(u8)))
> + return (FAILURE);
> + error = da9052_adc_unregister_event(event_data);
> + break;
> + case DA9052_ADC_IOCTL_GET_EVENT:
> + if (da9052_adc_get_event(&arg))
> + return (FAILURE);
> + break;
> +#if DA9052_ADC_DEBUG
> + case DA9052_ADC_IOCTL_UT_VDDOUT:
> + error = da9052_adc_read_vddout(&data);
> + if (copy_to_user((u16 *)arg, (void *)&data, sizeof(u16)))
> + return (FAILURE);
> + break;
> + case DA9052_ADC_IOCTL_UT_ICH:
> + error = da9052_adc_read_ich(&data);
> + if (copy_to_user((u16 *)arg, (void *)&data, sizeof(u16)))
> + return (FAILURE);
> + break;
> + case DA9052_ADC_IOCTL_UT_TBAT:
> + error = da9052_adc_read_tbat(&data);
> + if (copy_to_user((u16 *)arg, (void *)&data, sizeof(u16)))
> + return (FAILURE);
> + break;
> + case DA9052_ADC_IOCTL_UT_VBAT:
> + error = da9052_adc_read_vbat(&data);
> + if (copy_to_user((u16 *)arg, (void *)&data, sizeof(u16)))
> + return (FAILURE);
> + break;
> + case DA9050_ADC_IOCTL_UT_TJUNC:
> + error = da9052_adc_read_tjunc(&data);
> + if (copy_to_user((u16 *)arg, (void *)&data, sizeof(u16)))
> + return (FAILURE);
> + break;
> + case DA9052_ADC_IOCTL_UT_VBBAT:
> + error = da9052_adc_read_vbbat(&data);
> + if (copy_to_user((u16 *)arg, (void *)&data, sizeof(u16)))
> + return (FAILURE);
> + break;
> +#endif /* DA9052_ADC_DEBUG */
> + default:
> + DA9052_DEBUG("Invalid ioctl command\n");
> + return (ADC_NO_IOCTL_CMD);
> + }
> + return (error);
> +}
> +
> +/**
> + * da9052_bat_fasync:
> + *
> + * @param void <Direction> <Description>
> + * @return s32 <Description>
> + */
> +s32 da9052_adc_fasync (s32 fd, struct file *filp, s32 on)
> +{
> + DA9052_DEBUG ("In %s: %s\n",__FILE__, __FUNCTION__);
> + return(fasync_helper(fd, filp, on, &adc_fasync_queue));
> +}
> +
> +/**
> + * static struct file_operations da9052_adc_fops -
> + * This structure definition has to be defined here as an exception.
> + * @owner: member description
> + * @open : member description
> + * @release: member description
> + * @read: member description
> + * @write: member description
> + * @ioctl: member description
> + *
> + */

I don't see any reason to break with kernel conventions for interfacing
to this. Please make it a hwmon driver, or explain whyn this needs
a chrdev.
> +static const struct file_operations da9052_adc_fops = {
> + .owner = THIS_MODULE,
> + .open = da9052_adc_open,
> + .release = da9052_adc_release,
> + .ioctl = da9052_adc_ioctl,
> + .fasync = da9052_adc_fasync
> +};
> +
> +/**
> + * da9052_adc_probe: Called when a device gets attached to driver
> + * @param *dev pointer to platform device
> + * @return s32 Error Code, zero: no error
> + */
> +static s32 __devinit da9052_adc_probe(struct platform_device *dev)
> +{
> + s32 ret;
> +
> + /* Register the device */
> + ret = register_chrdev(adc_major_number,
> + DA9052_ADC_DEVICE_NAME, &da9052_adc_fops);
> + if (ret < 0) {
> + printk(KERN_ERR "Unable to register %s\n",
> + DA9052_ADC_DEVICE_NAME);
> + return (-EFAULT);
> + } else {
> + /* Dyanmic major no allocation */
> + adc_major_number = ret;
> + printk(KERN_INFO "%s: Major number is: %d \n",
> + DA9052_ADC_DEVICE_NAME, adc_major_number);
> +
> + if (da9052_adc_hw_init()) {
> + /* Error in hw initialization */
> + unregister_chrdev(adc_major_number, \
> + DA9052_ADC_DEVICE_NAME);
> + return (-EFAULT);
> + } else {
> + return (SUCCESS);
> + }
> + }
> + return(SUCCESS);
> +}
> +
> +/**
> + * da9052_adc_remove: Called when ditaching device from driver
> + * @param void
> + * @return void
> + */
> +static s32 __devexit da9052_adc_remove(struct platform_device *dev)
> +{
> + DA9052_DEBUG(KERN_DEBUG "Removing %s \n", DA9052_ADC_DEVICE_NAME);
> + return (SUCCESS);
> +}
> +
> +/**
> + * static struct platform_driver da9052_adc_driver -
> + * This structure definition has to be defined here as an exception.
> + * @probe: Probe function for this device.
> + * @remove: Function to be called when removing this device from platform
> + * @driver: Contains glue logic to bind platform device and plarform driver
> + */
> +static struct platform_driver da9052_adc_driver = {
> + .probe = da9052_adc_probe,
> + .remove = __devexit_p(da9052_adc_remove),
> + .suspend = da9052_adc_suspend,
> + .resume = da9052_adc_resume,
> + .driver = {
> + .name = DA9052_ADC_DEVICE_NAME,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +/**
> + * da9052_adc_init: Initiales the driver
> + *
> + * @param void
> + * @return s32 Error Code, zero: no error
> + */
> +static s32 __init da9052_adc_init(void)
> +{
> + s32 retval;
> + printk(banner);
> +
> + da9052_adc_platform_device = platform_device_alloc("da9052_adc", 0);
> + if (!da9052_adc_platform_device)
> + return (-ENOMEM);
> +
> + retval = platform_device_add(da9052_adc_platform_device);
> + if (retval < 0) {
> + platform_device_put(da9052_adc_platform_device);
> + return (retval);
> + }
> +
> + retval = platform_driver_register(&da9052_adc_driver);
> + if (retval < 0)
> + platform_device_unregister(da9052_adc_platform_device);
> +
> + return (retval);
> +}
> +/**
> + * da9052_adc_exit:
> + *
> + * @param void
> + * @return void
> + */
> +static void __exit da9052_adc_exit(void)
> +{
> + da9052_adc_hw_deinit();
> + printk("DA9052: Unregistering ADC device.\n");
> + unregister_chrdev(adc_major_number, DA9052_ADC_DEVICE_NAME);
> +
> + /* Platform_Device */
> + platform_driver_unregister(&da9052_adc_driver);
> + platform_device_unregister(da9052_adc_platform_device);
> +}
> +
> +
> +module_init(da9052_adc_init);
> +module_exit(da9052_adc_exit);
> +
> +MODULE_AUTHOR("Dialog Semiconductor Ltd");
> +MODULE_DESCRIPTION("DA9052 ADC Device Driver");
> +MODULE_LICENSE("GPL");
> +
> +/*--------------------------------------------------------------------------*/
> +/* Exports */
> +/*--------------------------------------------------------------------------*/
> +
These belong immediately after the relevant function, not in a block at the end.

> +EXPORT_SYMBOL(da9052_set_adc_mode);
> +EXPORT_SYMBOL(da9052_configure_thresholds);
> +EXPORT_SYMBOL(da9052_set_adcin5_comparator);
> +EXPORT_SYMBOL(da9052_set_tbat_source_current);
> +EXPORT_SYMBOL(da9052_set_ad4_isrc_source_current);
> +EXPORT_SYMBOL(da9052_start_adc);
> +EXPORT_SYMBOL(da9052_stop_adc);
> +EXPORT_SYMBOL(da9052_read_adc);
> +EXPORT_SYMBOL(da9052_adc_set_sampling_interval);
> +EXPORT_SYMBOL(da9052_adc_read_vddout);
> +EXPORT_SYMBOL(da9052_adc_read_ich);
> +EXPORT_SYMBOL(da9052_adc_read_tbat);
> +EXPORT_SYMBOL(da9052_adc_read_vbat);
> +EXPORT_SYMBOL(da9052_adc_read_tjunc);
> +EXPORT_SYMBOL(da9052_adc_read_vbbat);
> +/* Mutex structure required for TSI */
> +EXPORT_SYMBOL(da9052_adc_manconv_lock);
> diff -Naur linux-2.6.33.2/drivers/mfd/da9052_eh.c linux-2.6.33.2_patch/drivers/mfd/da9052_eh.c
> --- linux-2.6.33.2/drivers/mfd/da9052_eh.c 1970-01-01 05:00:00.000000000 +0500
> +++ linux-2.6.33.2_patch/drivers/mfd/da9052_eh.c 2010-05-18 15:29:36.000000000 +0500
> @@ -0,0 +1,926 @@
> +/*
> + * Copyright(c) 2009 Dialog Semiconductor Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * da9052_eh.c: Event Handler module for DA9052 PM IC
> + *
> + * DA9052 has a single IRQ line which can be used to indicate 32 various events.
> + * Event Handler (EH) module has following primary functions.
> + *
> + * (1) To receive and acknowledge interrupts from DA9052.
> + * (2) Extract all events form DA9052 event registers after receiving interrupt.
> + * (3) To provide a mechanism to register call back for specific events of
> + * their interest, and call these call backs when corresponding event occurs.
> + * (4) To retrive, store and provide TSI data whenever requested by TSI module.
> + *
> + * History:
> + *
> + * 12/05/2009: Initial version
> + * 19/05/2009: First unit tested version
> + *
> + * 27/04/2010 Created initial draft for Linux community release
> + *
> + * Best Viewed with TabSize=8 and ColumnWidth=80
> + */
> +
> +/*-----------------------------------------------------

From: Randy Dunlap on 19 May 2010 13:20

---

On Wed, 19 May 2010 10:16:10 +0100 David Dajun Chen wrote:

Hi,
2 comments from a quick scan:



> diff -Naur linux-2.6.33.2/drivers/mfd/Kconfig linux-2.6.33.2_patch/drivers/mfd/Kconfig
> --- linux-2.6.33.2/drivers/mfd/Kconfig 2010-04-02 04:02:33.000000000 +0500
> +++ linux-2.6.33.2_patch/drivers/mfd/Kconfig 2010-05-18 15:30:37.000000000 +0500
> @@ -348,6 +348,35 @@
> read/write functions for the devices to get access to this chip.
> This chip embeds various other multimedia funtionalities as well.
>
> +config MFD_DA9052
> + bool "Dialog Semiconductor DA9052 PMIC Support"

a. why are all of these drivers "bool" (y/n) instead of tristate (y/m/n)?
i.e., why are they not buildable as loadable modules?


> + help
> + Say yes here to support for Dialog semiconductor Da9052, Power
> + Management IC.This option enables the SPI/I2C, EH, and ADC
> + features of DA9052.
> +choice
> + prompt "DA9052 SSC support"
> + depends on MFD_DA9052
> +config MFD_DA9052_SPI
> + bool "SPI"
> + select SPI
> + select GPIOLIB
> + help
> + Say Y to select SPI serial interface for DA9052 chip
> +
> +config MFD_DA9052_I2C
> + bool "I2C"
> + select I2C
> + help
> + Say Y to select I2C serial interface for DA9052 chip
> +endchoice
> +
> +config DA9052_ADC_ENABLE
> + bool "Dialog Semiconductor DA9052 ADC Driver"
> + depends on MFD_DA9052
> + help
> + Say Y to enable the ADC driver for the DA9052 chip
> +
> endmenu
>
> menu "Multimedia Capabilities Port drivers"


b. The .c and .h files have lots of comments in them that begin with "/**",
which in kernel-land means kernel-doc notation... but they are not in kernel-doc
notation. Please see Documentation/kernel-doc-nano-HOWTO.txt for info on how
to use kernel-doc notation.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Mark Brown on 20 May 2010 20:10

---

On Wed, May 19, 2010 at 10:16:10AM +0100, David Dajun Chen wrote:

> The attached is the MFD part of the device drivers newly developed for
> DA9052 Power Management IC from Dialog Semiconductor.

One thing that jumps out at me from this patch is that it seems like
it'd really benefit from being further split up so that the patch is not
so very large. This will make review much easier, usually the larger
the individual patch the more effort is required to review. For
example, the code to add ADC and IRQ support both look like very good
candidates for being split out of the core patch. Similarly, the
headers for things like the RTC would probably be best placed in the
patch that adds that driver.

Addressing some of the issues that have been raised by myself and others
regarding the use of typedefs and the addition of driver-specific ioctl
interfaces would also go a long way to making review easier. Much of
the code here appears to result from reinventing things that already
have standard implementations in Linux.

I'd also suggest using scripts/checkpatch.pl to check for coding style
issues.

> + * DA9052 has a single IRQ line which can be used to indicate 32 various events.
> + * Event Handler (EH) module has following primary functions.
> + *
> + * (1) To receive and acknowledge interrupts from DA9052.
> + * (2) Extract all events form DA9052 event registers after receiving interrupt.
> + * (3) To provide a mechanism to register call back for specific events of
> + * their interest, and call these call backs when corresponding event occurs.
> + * (4) To retrive, store and provide TSI data whenever requested by TSI module.

The best way to handle the interrupt controller in devices like this in
Linux is through the generic IRQ framework. This means a lot of the
event dispatch code is provided for you, gives you diagnostics and
allows good integration with the rest of the kernel (both for the
primary IRQ and also for the IRQs generated by GPIOs on the device).

Drivers such as 88pm860x and wm831x provide examples of doing this in
mainline.

> +static void da9052_eh_setup_irq(void)
> +{
> + /* Put your platform and board specific code here */
> +#if (ARCH_DEPENDENT_DA9052)
> + /* Set up GPI which is connected to nIRQ of DA9052 */
T> + s3c_gpio_setpull(DA9052_IRQ_PIN, S3C_GPIO_PULL_UP);
> +
> + /* Set GPI as LOW LEVEL interrupt source */
> + set_irq_type(DA9052_IRQ, IRQ_TYPE_LEVEL_LOW);

Use platform data for this - you can provide a callback function which
will be run when the device is registered which machines can use to do
any setup that needs to be done when the driver is starting up (or for
things like this they can just do it in the core machine startup).

> +/**
> + * da9052_eh_restore_irq:Function to free IRQ line which is connected to DA9052
> + * @param void
> + * @return void
> + */
> +static void da9052_eh_restore_irq(void)
> +{
> + /* Put your platform and board specific code here */
> + free_irq(DA9052_IRQ, NULL);

The IRQ used by the device should be passed in through the standard
mechanisms for the bus concerned - eg, the .irq field of the I2C board
info structure.

> +static irqreturn_t da9052_eh_isr(int irq, void *dev_id)
> +{
> + /* Schedule work to be done */
> + schedule_work(&eh_info.eh_isr_work);

Use request_threaded_irq() with IRQF_ONESHOT.

> +/**
> + * da9052_tsi_start_sampling: Called by TSI driver to indicate start of data
> + * sampling
> + * @param void
> + * @return int status
> + */
> +s32 da9052_tsi_start_sampling(void){

This and all the other touchscreen stuff in here almost certainly want
to be part of your touchscreen driver.

> +/**
> + * da9052_eh_resume: Power Management support function
> + *
> + * @param *dev pointer to platform device
> + * @return s32 status of resume operation
> + */
> +static s32 da9052_eh_resume(struct platform_device *dev)
> +{
> + /* Put your resume related operations here */
> + printk(KERN_INFO "%s: called\n", __FUNCTION__);
> + return (SUCCESS);
> +}

As with your other similar suspend and resume callbacks if this is
really needed you should provide platform data based callbacks rather
than expecting users to modify the driver - remember, people might want
to submit their boards to mainline. It would seem very unusual to use
most if not all of these, though.

> + depends on MFD_DA9052
> +config MFD_DA9052_SPI
> + bool "SPI"
> + select SPI
> + select GPIOLIB
> + help
> + Say Y to select SPI serial interface for DA9052 chip
> +
> +config MFD_DA9052_I2C
> + bool "I2C"
> + select I2C
> + help
> + Say Y to select I2C serial interface for DA9052 chip

There should be no reason not to support building both bus types into
the same kernel. Quite a few drivers in the kernel provide examples of
doing this.

> --- linux-2.6.33.2/include/linux/compiler-gcc4.h 2010-04-02 04:02:33.000000000 +0500
> +++ linux-2.6.33.2_patch/include/linux/compiler-gcc4.h 2010-05-18 15:29:36.000000000 +0500
> @@ -3,9 +3,19 @@
> #endif
>
> /* GCC 4.1.[01] miscompiles __weak */
> +#define GCC_VERSION (__GNUC__ * 10000 \
> + + __GNUC_MINOR__ * 100 \
> + + __GNUC_PATCHLEVEL__)
> +

The changes in here are totally unrelated to this device and should be
sumbitted separately along with an explanation of the intention.

> +
> +/* Channel Definations */
> +#define ADC_VDDOUT 0
> +#define ADC_ICH 1
> +#define ADC_TBAT 2

These any many of the other definitions in your headers are likely
collide with other users - they should be namespaced to avoid this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

 |  Next  |  Last
Pages: 1 2
Prev: [PATCH] POWER (BAT) of DA9052 Linux device drivers (4/9)
Next: [GIT PULL] Lockup detector updates