mfd: mc13xxx adc fixes and enhancements

mfd: mc13xxx adc fixes and enhancements

[Marc Reilly]

This series provides some small fixes to the mc13xxx adc conversion funtion to
make it more robust. Readout of single channel conversions is fixed so that the
read out order corresponds to the sampling order (Critical for dedicated 
battery reading).

A new conversion function is introduced which allows the caller to enable the
dedicated readings and other adc functionality by providing a mask/ value pair
for the adc0,1 registers.

These patches apply onto my 'add I2C support' series [1].
(I think only the first patch is required)

Cheers,
Marc

[1] http://lists.arm.linux.org.uk/lurker/message/20120119.010131.616dd8a6.en.html

[PATCH 1/6] mfd: mc13xxx-core: ADC conv: ack existing pending irqs before requesting

[Marc Reilly]

This acks any existing pending ADCDONE irqs before requesting one for the
new conversion, rather than after.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 1fd265d..8cb83ef 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -561,9 +561,9 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 	}
 
 	dev_dbg(mc13xxx->dev, "%s: request irq\n", __func__);
+	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
 	mc13xxx_irq_request(mc13xxx, MC13XXX_IRQ_ADCDONE,
 			mc13xxx_handler_adcdone, __func__, &adcdone_data);
-	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
 
 	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, adc0);
 	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
-- 
1.7.3.4

[PATCH 1/6] mfd: mc13xxx-core: ADC conv: ack existing pending irqs before requesting

[Uwe Kleine-König]

On Mon, Jan 30, 2012 at 09:33:23AM +1100, Marc Reilly wrote:
> This acks any existing pending ADCDONE irqs before requesting one for the
> new conversion, rather than after.
> 
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mfd/mc13xxx-core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index 1fd265d..8cb83ef 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -561,9 +561,9 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
>  	}
>  
>  	dev_dbg(mc13xxx->dev, "%s: request irq\n", __func__);
> +	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
>  	mc13xxx_irq_request(mc13xxx, MC13XXX_IRQ_ADCDONE,
>  			mc13xxx_handler_adcdone, __func__, &adcdone_data);
> -	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
>  
>  	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, adc0);
>  	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
hmm, I wonder if the irq should be acked if it's not requested. The code
doesn't do error checking, but consider mc13xxx_irq_request fails
because the irq is already requested by someone else. Then acking before
requesting is wrong, isn't it? Maybe irq_request should ack the irq or
maybe better introduce a function mc13xxx_irq_request_and_ack that acks
only if the request part is OK?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/6] mfd: mc13xxx-core: ADC conv: ack existing pending irqs before requesting

[Marc Reilly]

On Monday, January 30, 2012 06:15:02 PM Uwe Kleine-K?nig wrote:
> On Mon, Jan 30, 2012 at 09:33:23AM +1100, Marc Reilly wrote:
> > This acks any existing pending ADCDONE irqs before requesting one for the
> > new conversion, rather than after.
> > 
> > Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> > ---
> > 
> >  drivers/mfd/mc13xxx-core.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> > index 1fd265d..8cb83ef 100644
> > --- a/drivers/mfd/mc13xxx-core.c
> > +++ b/drivers/mfd/mc13xxx-core.c
> > @@ -561,9 +561,9 @@ int mc13xxx_adc_do_conversion(struct mc13xxx
> > *mc13xxx, unsigned int mode,
> > 
> >  	}
> >  	
> >  	dev_dbg(mc13xxx->dev, "%s: request irq\n", __func__);
> > 
> > +	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
> > 
> >  	mc13xxx_irq_request(mc13xxx, MC13XXX_IRQ_ADCDONE,
> >  	
> >  			mc13xxx_handler_adcdone, __func__, &adcdone_data);
> > 
> > -	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
> > 
> >  	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, adc0);
> >  	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
> 
> hmm, I wonder if the irq should be acked if it's not requested. The code
> doesn't do error checking, but consider mc13xxx_irq_request fails
> because the irq is already requested by someone else. Then acking before
> requesting is wrong, isn't it?

I see your point.

> Maybe irq_request should ack the irq or
> maybe better introduce a function mc13xxx_irq_request_and_ack that acks
> only if the request part is OK?

I'd say that just acking in mc13xxx_irq_request is good enough. Ack happens 
after the request, before unmasking. Do you see any specific need for handling 
a pre-existing interrupt?

Cheers,
Marc

[PATCH 2/6] mfd: mc13xxx-core: ADC conv: wait_for_completion returns a long

[Marc Reilly]

Use the correct return type for wait_for_completion, as long may be
larger than int.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 8cb83ef..afff892 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -513,6 +513,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 {
 	u32 adc0, adc1, old_adc0;
 	int i, ret;
+	long timeout;
 	struct mc13xxx_adcdone_data adcdone_data = {
 		.mc13xxx = mc13xxx,
 	};
@@ -566,20 +567,23 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 			mc13xxx_handler_adcdone, __func__, &adcdone_data);
 
 	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, adc0);
-	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
+	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
 
 	mc13xxx_unlock(mc13xxx);
 
-	ret = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
+	timeout = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
 
-	if (!ret)
+	if (timeout <= 0) {
+		dev_warn(mc13xxx->dev,
+				"timed out waiting for ADC completion\n");
 		ret = -ETIMEDOUT;
+	}
 
 	mc13xxx_lock(mc13xxx);
 
 	mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_ADCDONE, &adcdone_data);
 
-	if (ret > 0)
+	if (!ret)
 		for (i = 0; i < 4; ++i) {
 			ret = mc13xxx_reg_read(mc13xxx,
 					MC13XXX_ADC2, &sample[i]);
-- 
1.7.3.4

[PATCH 2/6] mfd: mc13xxx-core: ADC conv: wait_for_completion returns a long

[Uwe Kleine-König]

On Mon, Jan 30, 2012 at 09:33:24AM +1100, Marc Reilly wrote:
> Use the correct return type for wait_for_completion, as long may be
> larger than int.
That's a theoretical problem only because the return value should be in
the range -ESOMETHING ... HZ which fits into an int.

> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mfd/mc13xxx-core.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index 8cb83ef..afff892 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -513,6 +513,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
>  {
>  	u32 adc0, adc1, old_adc0;
>  	int i, ret;
> +	long timeout;
>  	struct mc13xxx_adcdone_data adcdone_data = {
>  		.mc13xxx = mc13xxx,
>  	};
> @@ -566,20 +567,23 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
>  			mc13xxx_handler_adcdone, __func__, &adcdone_data);
>  
>  	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, adc0);
> -	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
> +	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
Is this change intended? I guess without it you get a warning that ret
is used uninitialized, but if mc13xxx_reg_write fails you should IMHO
return at once.
 
>  	mc13xxx_unlock(mc13xxx);
>  
> -	ret = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
> +	timeout = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
>  
> -	if (!ret)
> +	if (timeout <= 0) {
> +		dev_warn(mc13xxx->dev,
> +				"timed out waiting for ADC completion\n");
>  		ret = -ETIMEDOUT;
> +	}
I think this is wrong. wait_for_completion_interruptible_timeout returns
-ERESTARTSYS if it was interrupted. That's not a timeout and
-ERESTARTSYS should be propagated then. !ret is the correct test for
timeout.
>  
>  	mc13xxx_lock(mc13xxx);
>  
>  	mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_ADCDONE, &adcdone_data);
>  
> -	if (ret > 0)
> +	if (!ret)
This is wrong, too, isn't it?

>  		for (i = 0; i < 4; ++i) {
>  			ret = mc13xxx_reg_read(mc13xxx,
>  					MC13XXX_ADC2, &sample[i]);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/6] mfd: mc13xxx-core: ADC conv: wait_for_completion returns a long

[Marc Reilly]

Hi Uwe,

Thanks for reviewing.

On Monday, January 30, 2012 06:24:53 PM Uwe Kleine-K?nig wrote:
> On Mon, Jan 30, 2012 at 09:33:24AM +1100, Marc Reilly wrote:
> > Use the correct return type for wait_for_completion, as long may be
> > larger than int.
> 
> That's a theoretical problem only because the return value should be in
> the range -ESOMETHING ... HZ which fits into an int.

It _should_ be ok, but I propose that it is generally better practice to match 
up the types. 


> > @@ -566,20 +567,23 @@ int mc13xxx_adc_do_conversion(struct mc13xxx
> > *mc13xxx, unsigned int mode,
> > 
> >  			mc13xxx_handler_adcdone, __func__, &adcdone_data);
> >  	
> >  	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, adc0);
> > 
> > -	mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
> > +	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
> 
> Is this change intended? I guess without it you get a warning that ret
> is used uninitialized, 

This was intended, ret is then either 0 after a successful write to start the 
conversion off, or negative from a write error (or non-completion below).


> but if mc13xxx_reg_write fails you should IMHO
> return at once.

I guess there should be a lot more checking for failure in other places too. 
You are right about returning at once.


> 
> >  	mc13xxx_unlock(mc13xxx);
> > 
> > -	ret = wait_for_completion_interruptible_timeout(&adcdone_data.done,
> > HZ); +	timeout =
> > wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
> > 
> > -	if (!ret)
> > +	if (timeout <= 0) {
> > +		dev_warn(mc13xxx->dev,
> > +				"timed out waiting for ADC completion\n");
> > 
> >  		ret = -ETIMEDOUT;
> > 
> > +	}
> 
> I think this is wrong. wait_for_completion_interruptible_timeout returns
> -ERESTARTSYS if it was interrupted. That's not a timeout and
> -ERESTARTSYS should be propagated then. !ret is the correct test for
> timeout.

It took me a little while to get your point here, and I guess I missed that in 
my original understanding of the code, (which may be more of a reflection on 
me :) )
 
I still think the way it was before is subtle, and would prefer something more 
explicit, perhaps:

if (timeout == 0)
	ret = -ETIMEDOUT;
else if (timeout < 0)
	ret = timeout;


> 
> >  	mc13xxx_lock(mc13xxx);
> >  	
> >  	mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_ADCDONE, &adcdone_data);
> > 
> > -	if (ret > 0)
> > +	if (!ret)
> 
> This is wrong, too, isn't it?

This is right I think. ret is return code from the mc13xxx_* call, so 0 is 
success.


Cheers,
Marc

[PATCH 2/6] mfd: mc13xxx-core: ADC conv: wait_for_completion returns a long

[Uwe Kleine-König]

Hello Marc,

On Tue, Jan 31, 2012 at 08:40:55AM +1100, Marc Reilly wrote:
> On Monday, January 30, 2012 06:24:53 PM Uwe Kleine-K?nig wrote:
> > On Mon, Jan 30, 2012 at 09:33:24AM +1100, Marc Reilly wrote:
> > > Use the correct return type for wait_for_completion, as long may be
> > > larger than int.
> > 
> > That's a theoretical problem only because the return value should be in
> > the range -ESOMETHING ... HZ which fits into an int.
> 
> It _should_ be ok, but I propose that it is generally better practice to match 
> up the types. 
Agreed, but then only change the type and don't touch the logic in the
same commit. (Or at least mention it in the change log.)
 
> > 
> > >  	mc13xxx_unlock(mc13xxx);
> > > 
> > > -	ret = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
> > > +	timeout = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
> > > 
> > > -	if (!ret)
> > > +	if (timeout <= 0) {
> > > +		dev_warn(mc13xxx->dev,
> > > +				"timed out waiting for ADC completion\n");
> > > 
> > >  		ret = -ETIMEDOUT;
> > > 
> > > +	}
> > 
> > I think this is wrong. wait_for_completion_interruptible_timeout returns
> > -ERESTARTSYS if it was interrupted. That's not a timeout and
> > -ERESTARTSYS should be propagated then. !ret is the correct test for
> > timeout.
> 
> It took me a little while to get your point here, and I guess I missed that in 
> my original understanding of the code, (which may be more of a reflection on 
> me :) )
>  
> I still think the way it was before is subtle, and would prefer something more 
> explicit, perhaps:
> 
> if (timeout == 0)
> 	ret = -ETIMEDOUT;
> else if (timeout < 0)
> 	ret = timeout;
Yeah, that's better than the original as it propagates an eventual
-ERESTARTSYS from wait_for_completion_interruptible_timeout. Don't know
if/how the upper layer handle that though.
 
> > 
> > >  	mc13xxx_lock(mc13xxx);
> > >  	
> > >  	mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_ADCDONE, &adcdone_data);
> > > 
> > > -	if (ret > 0)
> > > +	if (!ret)
> > 
> > This is wrong, too, isn't it?
> 
> This is right I think. ret is return code from the mc13xxx_* call, so 0 is 
> success.
Ah, I thought ret still holds the return value of
wait_for_completion_interruptible_timeout. You're right.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/6] mfd: mc13xxx-core: ADC conv: wait_for_completion returns a long

[Uwe Kleine-König]

Hello again,

On Tue, Jan 31, 2012 at 09:07:59AM +0100, Uwe Kleine-K?nig wrote:
> > > > -	if (!ret)
> > > > +	if (timeout <= 0) {
> > > > +		dev_warn(mc13xxx->dev,
> > > > +				"timed out waiting for ADC completion\n");
> > > > 
> > > >  		ret = -ETIMEDOUT;
> > > > 
> > > > +	}
> > > 
> > > I think this is wrong. wait_for_completion_interruptible_timeout returns
> > > -ERESTARTSYS if it was interrupted. That's not a timeout and
> > > -ERESTARTSYS should be propagated then. !ret is the correct test for
> > > timeout.
> > 
> > It took me a little while to get your point here, and I guess I missed that in 
> > my original understanding of the code, (which may be more of a reflection on 
> > me :) )
> >  
> > I still think the way it was before is subtle, and would prefer something more 
> > explicit, perhaps:
> > 
> > if (timeout == 0)
> > 	ret = -ETIMEDOUT;
> > else if (timeout < 0)
> > 	ret = timeout;
> Yeah, that's better than the original as it propagates an eventual
> -ERESTARTSYS from wait_for_completion_interruptible_timeout. Don't know
> if/how the upper layer handle that though.
Just a small note, after starring again at the original code,
-ERESTARTSYS is propagated correctly. So it's only about style.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 3/6] mfd: mc13xxx-core: ADC conv: setup readout for single channel

[Marc Reilly]

The ADC always does 8 conversions, for single channel mode one channel
is converted 8 times and the results are placed in the channels 0..7
This change resets the channel indexes to 0 and 4 so that the 8 individual
samples are read out correctly.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index afff892..56e09ea 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -583,13 +583,26 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 
 	mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_ADCDONE, &adcdone_data);
 
-	if (!ret)
+	if (!ret) {
+		if (mode == MC13XXX_ADC_MODE_SINGLE_CHAN) {
+			/*
+			 * Set the channels for read back same as for multi
+			 * channel. Don't use adc1 var from above, or another
+			 * conversion would be started.
+			 */
+			u32 mask = (0x7 << MC13XXX_ADC1_CHAN0_SHIFT) |
+					(0x7 << MC13XXX_ADC1_CHAN1_SHIFT);
+			mc13xxx_reg_rmw(mc13xxx, MC13XXX_ADC1, mask,
+					4 << MC13XXX_ADC1_CHAN1_SHIFT);
+		}
+
 		for (i = 0; i < 4; ++i) {
 			ret = mc13xxx_reg_read(mc13xxx,
 					MC13XXX_ADC2, &sample[i]);
 			if (ret)
 				break;
 		}
+	}
 
 	if (mode == MC13XXX_ADC_MODE_TS)
 		/* restore TSMOD */
-- 
1.7.3.4

[PATCH 4/6] mfd: mc13xxx-core: ADC conv: clear ADC_WORKING flag for invalid mode

[Marc Reilly]

Requesting a conversion for and invalid mode would mean that the
MC13XXX_ADC_WORKING flag never gets cleared.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 56e09ea..61a767d 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -557,8 +557,9 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 		break;
 
 	default:
-		mc13xxx_unlock(mc13xxx);
-		return -EINVAL;
+		dev_warn(mc13xxx->dev, "%s: bad ADC mode requested\n", __func__);
+		ret = -EINVAL;
+		goto out_flag;
 	}
 
 	dev_dbg(mc13xxx->dev, "%s: request irq\n", __func__);
@@ -608,6 +609,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 		/* restore TSMOD */
 		mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, old_adc0);
 
+out_flag:
 	mc13xxx->adcflags &= ~MC13XXX_ADC_WORKING;
 out:
 	mc13xxx_unlock(mc13xxx);
-- 
1.7.3.4

[PATCH 4/6] mfd: mc13xxx-core: ADC conv: clear ADC_WORKING flag for invalid mode

[Uwe Kleine-König]

On Mon, Jan 30, 2012 at 09:33:26AM +1100, Marc Reilly wrote:
> Requesting a conversion for and invalid mode would mean that the
s/and/an/

> MC13XXX_ADC_WORKING flag never gets cleared.
> 
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mfd/mc13xxx-core.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index 56e09ea..61a767d 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -557,8 +557,9 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
>  		break;
>  
>  	default:
> -		mc13xxx_unlock(mc13xxx);
> -		return -EINVAL;
> +		dev_warn(mc13xxx->dev, "%s: bad ADC mode requested\n", __func__);
> +		ret = -EINVAL;
> +		goto out_flag;
I would prefer this to be named in a more descritive way. Maybe
"out_clear_working"?

Other than that, ack.

Uwe
>  	}
>  
>  	dev_dbg(mc13xxx->dev, "%s: request irq\n", __func__);
> @@ -608,6 +609,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
>  		/* restore TSMOD */
>  		mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, old_adc0);
>  
> +out_flag:
>  	mc13xxx->adcflags &= ~MC13XXX_ADC_WORKING;
>  out:
>  	mc13xxx_unlock(mc13xxx);
> -- 
> 1.7.3.4
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 5/6] mfd: mc13xxx-core: ADC conv: only preserve TSMOD if TS in interrupt mode.

[Marc Reilly]

This change keeps the TS touch interrupt enabled only if the TS is in
interrupt mode (ie only TSMOD0 is set).
This ensures we never inadvertently allow a touchscreen reading in place
of a regular single or multi channel reading (if TSMOD1 is somehow already
set for example).

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 61a767d..09d03f5 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -535,24 +535,34 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 	adc0 = MC13XXX_ADC0_ADINC1 | MC13XXX_ADC0_ADINC2;
 	adc1 = MC13XXX_ADC1_ADEN | MC13XXX_ADC1_ADTRIGIGN | MC13XXX_ADC1_ASC;
 
+	/*
+	 * For the mc13892 not in TS mode the touchscreen inputs
+	 * are being sampled, but channels [8..11] will read 0
+	 */
 	if (channel > 7)
 		adc1 |= MC13XXX_ADC1_ADSEL;
 
 	switch (mode) {
 	case MC13XXX_ADC_MODE_TS:
+		/*
+		 * for mc13873 this uses position mode,
+		 * mc13892 doesn't care about TSMOD0 when TSMOD1 is set
+		 */
 		adc0 |= MC13XXX_ADC0_ADREFEN | MC13XXX_ADC0_TSMOD0 |
 			MC13XXX_ADC0_TSMOD1;
 		adc1 |= 4 << MC13XXX_ADC1_CHAN1_SHIFT;
 		break;
 
 	case MC13XXX_ADC_MODE_SINGLE_CHAN:
-		adc0 |= old_adc0 & MC13XXX_ADC0_TSMOD_MASK;
+		if ((old_adc0 & MC13XXX_ADC0_TSMOD_MASK) == MC13XXX_ADC0_TSMOD0)
+			adc0 |= MC13XXX_ADC0_TSMOD0;
 		adc1 |= (channel & 0x7) << MC13XXX_ADC1_CHAN0_SHIFT;
 		adc1 |= MC13XXX_ADC1_RAND;
 		break;
 
 	case MC13XXX_ADC_MODE_MULT_CHAN:
-		adc0 |= old_adc0 & MC13XXX_ADC0_TSMOD_MASK;
+		if ((old_adc0 & MC13XXX_ADC0_TSMOD_MASK) == MC13XXX_ADC0_TSMOD0)
+			adc0 |= MC13XXX_ADC0_TSMOD0;
 		adc1 |= 4 << MC13XXX_ADC1_CHAN1_SHIFT;
 		break;
 
-- 
1.7.3.4

[PATCH 6/6] mfd: mc13xxx-core: ADC conversion with extra capabilities

[Marc Reilly]

Allows extra flags to be given at conversion time. This will allow
other drivers to utilize the special channel readings and tweak the adc
operation to their specific needs.
Bit definitions for the ADC0 and ADC0 registers common to both mc13892
and mc13783 are added to include/linux/mfd/mc13xxx.h.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c  |   23 +++++++++++++++++++++--
 include/linux/mfd/mc13xxx.h |   12 ++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 09d03f5..431d324 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -508,8 +508,9 @@ static irqreturn_t mc13xxx_handler_adcdone(int irq, void *data)
 
 #define MC13XXX_ADC_WORKING (1 << 0)
 
-int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
-		unsigned int channel, unsigned int *sample)
+int mc13xxx_adc_do_conversion_ex(struct mc13xxx *mc13xxx, unsigned int mode,
+		unsigned int channel, unsigned int *sample,
+		u32 adc0mask, u32 adc0val, u32 adc1mask, u32 adc1val)
 {
 	u32 adc0, adc1, old_adc0;
 	int i, ret;
@@ -535,6 +536,16 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 	adc0 = MC13XXX_ADC0_ADINC1 | MC13XXX_ADC0_ADINC2;
 	adc1 = MC13XXX_ADC1_ADEN | MC13XXX_ADC1_ADTRIGIGN | MC13XXX_ADC1_ASC;
 
+	if (adc0mask) {
+		adc0 &= ~(adc0mask & ~adc0val);
+		adc0 |= (adc0mask & adc0val);
+	}
+
+	if (adc1mask) {
+		adc1 &= ~(adc1mask & ~adc1val);
+		adc1 |= (adc1mask & adc1val);
+	}
+
 	/*
 	 * For the mc13892 not in TS mode the touchscreen inputs
 	 * are being sampled, but channels [8..11] will read 0
@@ -626,6 +637,14 @@ out:
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mc13xxx_adc_do_conversion_ex);
+
+int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
+		unsigned int channel, unsigned int *sample)
+{
+	return mc13xxx_adc_do_conversion_ex(mc13xxx, mode, channel, sample,
+			0, 0, 0, 0);
+}
 EXPORT_SYMBOL_GPL(mc13xxx_adc_do_conversion);
 
 static int mc13xxx_add_subdevice_pdata(struct mc13xxx *mc13xxx,
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index df91dd9..0d3d7b3 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -38,6 +38,10 @@ int mc13xxx_get_flags(struct mc13xxx *mc13xxx);
 int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx,
 		unsigned int mode, unsigned int channel, unsigned int *sample);
 
+int mc13xxx_adc_do_conversion_ex(struct mc13xxx *mc13xxx,
+		unsigned int mode, unsigned int channel, unsigned int *sample,
+		u32 adc0mask, u32 adc0val, u32 adc1mask, u32 adc1val);
+
 #define MC13XXX_IRQ_ADCDONE	0
 #define MC13XXX_IRQ_ADCBISDONE	1
 #define MC13XXX_IRQ_TS		2
@@ -171,10 +175,14 @@ struct mc13xxx_platform_data {
 #define MC13XXX_ADC_MODE_MULT_CHAN	3
 
 #define MC13XXX_ADC0		43
+#define MC13XXX_ADC0_LICELLCON		(1 << 0)
+#define MC13XXX_ADC0_CHRGICON		(1 << 1)
+#define MC13XXX_ADC0_BATTICON		(1 << 2)
 #define MC13XXX_ADC0_ADREFEN		(1 << 10)
 #define MC13XXX_ADC0_TSMOD0		(1 << 12)
 #define MC13XXX_ADC0_TSMOD1		(1 << 13)
 #define MC13XXX_ADC0_TSMOD2		(1 << 14)
+#define MC13XXX_ADC0_CHRGRAWDIV		(1 << 15)
 #define MC13XXX_ADC0_ADINC1		(1 << 16)
 #define MC13XXX_ADC0_ADINC2		(1 << 17)
 
@@ -182,4 +190,8 @@ struct mc13xxx_platform_data {
 					MC13XXX_ADC0_TSMOD1 | \
 					MC13XXX_ADC0_TSMOD2)
 
+#define MC13XXX_ADC1_ATO_SHIFT		11
+#define MC13XXX_ADC1_ATO_MASK		(0xff << MC13XXX_ADC1_ATO_SHIFT)
+#define MC13XXX_ADC1_ATOX		(1 << 19)
+
 #endif /* ifndef __LINUX_MFD_MC13XXX_H */
-- 
1.7.3.4

mfd: mc13xxx adc fixes and enhancements

[Uwe Kleine-König]

Hello,

On Mon, Jan 30, 2012 at 09:33:22AM +1100, Marc Reilly wrote:
> This series provides some small fixes to the mc13xxx adc conversion funtion to
> make it more robust. Readout of single channel conversions is fixed so that the
> read out order corresponds to the sampling order (Critical for dedicated 
> battery reading).
> 
> A new conversion function is introduced which allows the caller to enable the
> dedicated readings and other adc functionality by providing a mask/ value pair
> for the adc0,1 registers.
> 
> These patches apply onto my 'add I2C support' series [1].
> (I think only the first patch is required)
I wonder if Robin van der Gracht (added to Cc:) can say something to
this series as he seems to have used the adc recently.

@Robin: tell me if I should bounce you the series.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

mfd: mc13xxx adc fixes and enhancements

[Robin van der Gracht]

On 01/30/2012 09:07 AM, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Mon, Jan 30, 2012 at 09:33:22AM +1100, Marc Reilly wrote:
>> This series provides some small fixes to the mc13xxx adc conversion funtion to
>> make it more robust. Readout of single channel conversions is fixed so that the
>> read out order corresponds to the sampling order (Critical for dedicated
>> battery reading).
>>
>> A new conversion function is introduced which allows the caller to enable the
>> dedicated readings and other adc functionality by providing a mask/ value pair
>> for the adc0,1 registers.
>>
>> These patches apply onto my 'add I2C support' series [1].
>> (I think only the first patch is required)
> I wonder if Robin van der Gracht (added to Cc:) can say something to
> this series as he seems to have used the adc recently.
>
> @Robin: tell me if I should bounce you the series.
>
> Best regards
> Uwe
>
Uwe,
I would like to recieve the new series,

Regards,

-- 
Robin van der Gracht
Protonic Holland.
tel.: +31 (0) 229 212928
fax.: +31 (0) 229 210930
Factorij 36 / 1689 AL Zwaag