diff mbox

ti-soc-thermal: implement eocz bit to make driver useful on omap3

Message ID 20150103114632.GA21883@amd
State Changes Requested
Delegated to: Eduardo Valentin
Headers show

Commit Message

Pavel Machek Jan. 3, 2015, 11:46 a.m. UTC
For omap3, proper implementation of eocz bit is needed. It was
actually a TODO in the driver.

When periodic mode is not enabled, it is neccessary to force reads.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

Comments

Eduardo Valentin Jan. 3, 2015, 12:26 p.m. UTC | #1
On Sat, Jan 03, 2015 at 12:46:32PM +0100, Pavel Machek wrote:
> For omap3, proper implementation of eocz bit is needed. It was
> actually a TODO in the driver.
> 
> When periodic mode is not enabled, it is neccessary to force reads.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> index 634b6ce..ee63d08 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> @@ -43,6 +43,8 @@
>  
>  #include "ti-bandgap.h"
>  
> +static int ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id);
> +
>  /***   Helper functions to access registers and their bitfields   ***/
>  
>  /**
> @@ -852,14 +831,20 @@ int ti_bandgap_read_temperature(struct ti_bandgap *bgp, int id,
>  	if (ret)
>  		return ret;
>  
> +	if (!TI_BANDGAP_HAS(bgp, MODE_CONFIG)) {
> +		ret = ti_bandgap_force_single_read(bgp, id);

not sure MODE_CONFIG is sufficient condition for single read on all OMAP
versions.

> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock(&bgp->lock);
>  	temp = ti_bandgap_read_temp(bgp, id);
>  	spin_unlock(&bgp->lock);
>  
> -	ret |= ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> +	ret = ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);

this one should be part of your clean up patch

>  	if (ret)
>  		return -EIO;
>  
>  	*temperature = temp;
>  
>  	return 0;
> @@ -917,7 +903,8 @@ void *ti_bandgap_get_sensor_data(struct ti_bandgap *bgp, int id)
>  static int
>  ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
>  {
> -	u32 temp = 0, counter = 1000;
> +	u32 counter = 1000;
> +	struct temp_sensor_registers *tsr;
>  
>  	/* Select single conversion mode */
>  	if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
> @@ -925,16 +912,27 @@ ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
>  
>  	/* Start of Conversion = 1 */
>  	RMW_BITS(bgp, id, temp_sensor_ctrl, bgap_soc_mask, 1);
> -	/* Wait until DTEMP is updated */
> -	temp = ti_bandgap_read_temp(bgp, id);
>  
> -	while ((temp == 0) && --counter)
> -		temp = ti_bandgap_read_temp(bgp, id);
> -	/* REVISIT: Check correct condition for end of conversion */
> +	/* Wait for EOCZ going up */
> +	tsr = bgp->conf->sensors[id].registers;
> +
> +	while (--counter) {
> +		if (ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
> +		    tsr->bgap_eocz_mask)
> +			break;
> +	}
>  
>  	/* Start of Conversion = 0 */
>  	RMW_BITS(bgp, id, temp_sensor_ctrl, bgap_soc_mask, 0);
>  
> +	/* Wait for EOCZ going down */
> +	counter = 1000;
> +	while (--counter) {
> +		if (!(ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
> +		      tsr->bgap_eocz_mask))
> +			break;
> +	}
> +
>  	return 0;
>  }
>  
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek Jan. 3, 2015, 4:22 p.m. UTC | #2
Hi!

> > When periodic mode is not enabled, it is neccessary to force reads.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 

> > --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > @@ -43,6 +43,8 @@
> >  
> >  #include "ti-bandgap.h"
> >  
> > +static int ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id);
> > +
> >  /***   Helper functions to access registers and their bitfields   ***/
> >  
> >  /**
> > @@ -852,14 +831,20 @@ int ti_bandgap_read_temperature(struct ti_bandgap *bgp, int id,
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (!TI_BANDGAP_HAS(bgp, MODE_CONFIG)) {
> > +		ret = ti_bandgap_force_single_read(bgp, id);
> 
> not sure MODE_CONFIG is sufficient condition for single read on all OMAP
> versions.

Ok, what do you suggest? AFAICT, without MODE_CONFIG, continuous ADC
mode is not available, so we have to force it periodically, so this
should be correct.

> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	spin_lock(&bgp->lock);
> >  	temp = ti_bandgap_read_temp(bgp, id);
> >  	spin_unlock(&bgp->lock);
> >  
> > -	ret |= ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> > +	ret = ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> 
> this one should be part of your clean up patch

Ok, can you apply the cleanup patch and I'll prepare one on the top of
it?

									Pavel
Eduardo Valentin Jan. 5, 2015, 9:35 p.m. UTC | #3
On Sat, Jan 03, 2015 at 05:22:42PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > When periodic mode is not enabled, it is neccessary to force reads.
> > > 
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > 
> 
> > > --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > > +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > > @@ -43,6 +43,8 @@
> > >  
> > >  #include "ti-bandgap.h"
> > >  
> > > +static int ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id);
> > > +
> > >  /***   Helper functions to access registers and their bitfields   ***/
> > >  
> > >  /**
> > > @@ -852,14 +831,20 @@ int ti_bandgap_read_temperature(struct ti_bandgap *bgp, int id,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	if (!TI_BANDGAP_HAS(bgp, MODE_CONFIG)) {
> > > +		ret = ti_bandgap_force_single_read(bgp, id);
> > 
> > not sure MODE_CONFIG is sufficient condition for single read on all OMAP
> > versions.
> 
> Ok, what do you suggest? AFAICT, without MODE_CONFIG, continuous ADC
> mode is not available, so we have to force it periodically, so this
> should be correct.

I will have a better look and let you know. for now, adding a single
read should not hurt ( but I will double check)

> 
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > >  	spin_lock(&bgp->lock);
> > >  	temp = ti_bandgap_read_temp(bgp, id);
> > >  	spin_unlock(&bgp->lock);
> > >  
> > > -	ret |= ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> > > +	ret = ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> > 
> > this one should be part of your clean up patch
> 
> Ok, can you apply the cleanup patch and I'll prepare one on the top of
> it?

I mean, you should resend the cleanup patch including the above '|=' removal, as you are already doing in the cleanup patch.

> 
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek Jan. 18, 2015, 8:18 p.m. UTC | #4
Hi!

> > Ok, what do you suggest? AFAICT, without MODE_CONFIG, continuous ADC
> > mode is not available, so we have to force it periodically, so this
> > should be correct.
> 
> I will have a better look and let you know. for now, adding a single
> read should not hurt ( but I will double check)

Any news there?

> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > >  	spin_lock(&bgp->lock);
> > > >  	temp = ti_bandgap_read_temp(bgp, id);
> > > >  	spin_unlock(&bgp->lock);
> > > >  
> > > > -	ret |= ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> > > > +	ret = ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> > > 
> > > this one should be part of your clean up patch
> > 
> > Ok, can you apply the cleanup patch and I'll prepare one on the top of
> > it?
> 
> I mean, you should resend the cleanup patch including the above '|=' removal, as you are already doing in the cleanup patch.
> 

Ok, little patch-editing can not hurt :-).

									Pavel
Eduardo Valentin Jan. 21, 2015, 5:21 a.m. UTC | #5
On Sun, Jan 18, 2015 at 09:18:21PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Ok, what do you suggest? AFAICT, without MODE_CONFIG, continuous ADC
> > > mode is not available, so we have to force it periodically, so this
> > > should be correct.
> > 
> > I will have a better look and let you know. for now, adding a single
> > read should not hurt ( but I will double check)
> 
> Any news there?

I believe it is OK to keep your code the way you posted.

Thanks.

> 
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > >  	spin_lock(&bgp->lock);
> > > > >  	temp = ti_bandgap_read_temp(bgp, id);
> > > > >  	spin_unlock(&bgp->lock);
> > > > >  
> > > > > -	ret |= ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> > > > > +	ret = ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
> > > > 
> > > > this one should be part of your clean up patch
> > > 
> > > Ok, can you apply the cleanup patch and I'll prepare one on the top of
> > > it?
> > 
> > I mean, you should resend the cleanup patch including the above '|=' removal, as you are already doing in the cleanup patch.
> > 
> 
> Ok, little patch-editing can not hurt :-).
> 
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
diff mbox

Patch

diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
index 634b6ce..ee63d08 100644
--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
@@ -43,6 +43,8 @@ 
 
 #include "ti-bandgap.h"
 
+static int ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id);
+
 /***   Helper functions to access registers and their bitfields   ***/
 
 /**
@@ -852,14 +831,20 @@  int ti_bandgap_read_temperature(struct ti_bandgap *bgp, int id,
 	if (ret)
 		return ret;
 
+	if (!TI_BANDGAP_HAS(bgp, MODE_CONFIG)) {
+		ret = ti_bandgap_force_single_read(bgp, id);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock(&bgp->lock);
 	temp = ti_bandgap_read_temp(bgp, id);
 	spin_unlock(&bgp->lock);
 
-	ret |= ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
+	ret = ti_bandgap_adc_to_mcelsius(bgp, temp, &temp);
 	if (ret)
 		return -EIO;
 
 	*temperature = temp;
 
 	return 0;
@@ -917,7 +903,8 @@  void *ti_bandgap_get_sensor_data(struct ti_bandgap *bgp, int id)
 static int
 ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
 {
-	u32 temp = 0, counter = 1000;
+	u32 counter = 1000;
+	struct temp_sensor_registers *tsr;
 
 	/* Select single conversion mode */
 	if (TI_BANDGAP_HAS(bgp, MODE_CONFIG))
@@ -925,16 +912,27 @@  ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id)
 
 	/* Start of Conversion = 1 */
 	RMW_BITS(bgp, id, temp_sensor_ctrl, bgap_soc_mask, 1);
-	/* Wait until DTEMP is updated */
-	temp = ti_bandgap_read_temp(bgp, id);
 
-	while ((temp == 0) && --counter)
-		temp = ti_bandgap_read_temp(bgp, id);
-	/* REVISIT: Check correct condition for end of conversion */
+	/* Wait for EOCZ going up */
+	tsr = bgp->conf->sensors[id].registers;
+
+	while (--counter) {
+		if (ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
+		    tsr->bgap_eocz_mask)
+			break;
+	}
 
 	/* Start of Conversion = 0 */
 	RMW_BITS(bgp, id, temp_sensor_ctrl, bgap_soc_mask, 0);
 
+	/* Wait for EOCZ going down */
+	counter = 1000;
+	while (--counter) {
+		if (!(ti_bandgap_readl(bgp, tsr->temp_sensor_ctrl) &
+		      tsr->bgap_eocz_mask))
+			break;
+	}
+
 	return 0;
 }