[PATCH 00/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[PATCH 00/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[Claudiu Beznea]

Hi,

The following series add support for temperature sensor available on
SAMA7G5.

Temperature sensor available on SAMA7G5 provides 2 outputs VTEMP and VBG.
VTEMP is proportional to the absolute temperature voltage and VBG is a
quasi-temperature independent voltage. Both are necessary in computing
the temperature (for better accuracy). Also, for better accuracy the
following settings were imposed when measusing the temperature:
oversampling rate of 256, sampling frequency of 10MHz, a startup time of
512 ticks, MR.tracktim=0xf, EMR.trackx=0x3.

For computing the temperature measured by ADC calibration data is
necessary. This is provided via OTP memory available on SAMA7G5.

Patches 1/16-3/16 provides some fixes.
Patches 3/16-12/16 prepares for the addition of temperature sensor
support.
Patch 13/16 adds the temperature sensor support.

Along with temperature sensor support I took the chance and added
runtime PM support in this series, too (handled in patch 15/16).

The rest of patches in this series are minor cleanups.

Thank you,
Claudiu Beznea

Claudiu Beznea (16):
  iio: adc: at91-sama5d2_adc: fix AT91_SAMA5D2_MR_TRACKTIM_MAX
  iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq
  iio: adc: at91-sama5d2_adc: exit from write_raw() when buffers are
    enabled
  iio: adc: at91-sama5d2_adc: handle different EMR.OSR for different hw
    versions
  iio: adc: at91-sama5d2_adc: adjust osr based on specific platform data
  iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio
  iio: adc: at91-sama5d2_adc: simplify the code in
    at91_adc_read_info_raw()
  iio: adc: at91-sama5d2_adc: move oversampling storage in its function
  iio: adc: at91-sama5d2_adc: update trackx on emr
  iio: adc: at91-sama5d2_adc: add startup and tracktim as parameter for
    at91_adc_setup_samp_freq()
  iio: adc: at91-sama5d2_adc: add locking parameter to
    at91_adc_read_info_raw()
  dt-bindings: iio: adc: at91-sama5d2_adc: add id for temperature
    channel
  iio: adc: at91-sama5d2_adc: add support for temperature sensor
  iio: adc: at91-sama5d2_adc: add empty line after functions
  iio: adc: at91-sama5d2_adc: add runtime pm support
  iio: adc: at91-sama5d2_adc: use pm_ptr()

 drivers/iio/adc/at91-sama5d2_adc.c            | 633 +++++++++++++++---
 .../dt-bindings/iio/adc/at91-sama5d2_adc.h    |   3 +
 2 files changed, 548 insertions(+), 88 deletions(-)

-- 
2.34.1

[PATCH 01/16] iio: adc: at91-sama5d2_adc: fix AT91_SAMA5D2_MR_TRACKTIM_MAX

[Claudiu Beznea]

All ADC HW versions handled by this driver (SAMA5D2, SAM9X60, SAMA7G5)
have MR.TRACKTIM on 4 bits. Fix AT91_SAMA5D2_MR_TRACKTIM_MAX to reflect
this.

Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index b764823ce57e..32b6f157b803 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -77,7 +77,7 @@ struct at91_adc_reg_layout {
 #define	AT91_SAMA5D2_MR_ANACH		BIT(23)
 /* Tracking Time */
 #define	AT91_SAMA5D2_MR_TRACKTIM(v)	((v) << 24)
-#define	AT91_SAMA5D2_MR_TRACKTIM_MAX	0xff
+#define	AT91_SAMA5D2_MR_TRACKTIM_MAX	0xf
 /* Transfer Time */
 #define	AT91_SAMA5D2_MR_TRANSFER(v)	((v) << 28)
 #define	AT91_SAMA5D2_MR_TRANSFER_MAX	0x3
-- 
2.34.1

[PATCH 02/16] iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq

[Claudiu Beznea]

.read_raw()/.write_raw() could be called asynchronously from user space
or other in kernel drivers. Without locking on st->lock these could be
called asynchronously while there is a conversion in progress. Read will
be harmless but changing registers while conversion is in progress may
lead to inconsistent results. Thus, to avoid this lock st->lock.

Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver")
Fixes: 6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling resolution")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 32b6f157b803..a672a520cdc0 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1542,10 +1542,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		ret = at91_adc_read_position(st, chan->channel,
 					     &tmp_val);
 		*val = tmp_val;
+		ret = at91_adc_adjust_val_osr(st, val);
 		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
-		return at91_adc_adjust_val_osr(st, val);
+		return ret;
 	}
 	if (chan->type == IIO_PRESSURE) {
 		ret = iio_device_claim_direct_mode(indio_dev);
@@ -1556,10 +1557,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		ret = at91_adc_read_pressure(st, chan->channel,
 					     &tmp_val);
 		*val = tmp_val;
+		ret = at91_adc_adjust_val_osr(st, val);
 		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
-		return at91_adc_adjust_val_osr(st, val);
+		return ret;
 	}
 
 	/* in this case we have a voltage channel */
@@ -1620,11 +1622,15 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_FRACTIONAL_LOG2;
 
 	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&st->lock);
 		*val = at91_adc_get_sample_freq(st);
+		mutex_unlock(&st->lock);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		mutex_lock(&st->lock);
 		*val = st->oversampling_ratio;
+		mutex_unlock(&st->lock);
 		return IIO_VAL_INT;
 
 	default:
@@ -1644,18 +1650,23 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 		    (val != AT91_OSR_16SAMPLES))
 			return -EINVAL;
 		/* if no change, optimize out */
+		mutex_lock(&st->lock);
 		if (val == st->oversampling_ratio)
-			return 0;
+			goto unlock;
 		st->oversampling_ratio = val;
 		/* update ratio */
 		at91_adc_config_emr(st);
+unlock:
+		mutex_unlock(&st->lock);
 		return 0;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val < st->soc_info.min_sample_rate ||
 		    val > st->soc_info.max_sample_rate)
 			return -EINVAL;
 
+		mutex_lock(&st->lock);
 		at91_adc_setup_samp_freq(indio_dev, val);
+		mutex_unlock(&st->lock);
 		return 0;
 	default:
 		return -EINVAL;
-- 
2.34.1

Re: [PATCH 02/16] iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:31:59 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> .read_raw()/.write_raw() could be called asynchronously from user space
> or other in kernel drivers. Without locking on st->lock these could be
> called asynchronously while there is a conversion in progress. Read will
> be harmless but changing registers while conversion is in progress may
> lead to inconsistent results. Thus, to avoid this lock st->lock.

The patch makes sense, but I'm not convinced all of the changes below
involve any changes to registers. E.g. at91_adc_adjust_val_osr()
is using the cached value of something in a register, but not the
register itself, so please update the description to mention cached state.

Other comments inline.
> 
> Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver")
> Fixes: 6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling resolution")
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 32b6f157b803..a672a520cdc0 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1542,10 +1542,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		ret = at91_adc_read_position(st, chan->channel,
>  					     &tmp_val);
>  		*val = tmp_val;
> +		ret = at91_adc_adjust_val_osr(st, val);
>  		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
> -		return at91_adc_adjust_val_osr(st, val);
> +		return ret;
>  	}
>  	if (chan->type == IIO_PRESSURE) {
>  		ret = iio_device_claim_direct_mode(indio_dev);
> @@ -1556,10 +1557,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		ret = at91_adc_read_pressure(st, chan->channel,
>  					     &tmp_val);
>  		*val = tmp_val;
> +		ret = at91_adc_adjust_val_osr(st, val);
>  		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
> -		return at91_adc_adjust_val_osr(st, val);
> +		return ret;
>  	}
>  
>  	/* in this case we have a voltage channel */
> @@ -1620,11 +1622,15 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&st->lock);
>  		*val = at91_adc_get_sample_freq(st);

So this is a straight read of a cached value.  The only thing you 'might'
arguably be protecting against is read/write tearing due to it in theory
being possible to write part of the value whilst reading.  I don't
see that being a concern for st->current_sample_rate

> +		mutex_unlock(&st->lock);
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		mutex_lock(&st->lock);
>  		*val = st->oversampling_ratio;
Likewise, what are you protecting against racing with this that can't
just occur before or after the lock?

> +		mutex_unlock(&st->lock);
>  		return IIO_VAL_INT;
>  
>  	default:
> @@ -1644,18 +1650,23 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  		    (val != AT91_OSR_16SAMPLES))
>  			return -EINVAL;
>  		/* if no change, optimize out */
> +		mutex_lock(&st->lock);
>  		if (val == st->oversampling_ratio)
> -			return 0;
It should be race free to check this outside the lock.

Definitely valid to lock around the cached value write and the config
write though.

> +			goto unlock;
If you did want to have locking as now then flip the logic

		if (val != st->oversampling_ratio) {
			st->oversampling_ratio = val;
			at91_adc_config_emr(st);
		}
		mutex_unlock()
..

Goto always have a cost in readability so if you can avoid them with
a simple flip of logic like this it is usually a good idea.
(exception is error code which should always be out of line as
that is more common so what we expect to see).

>  		st->oversampling_ratio = val;
>  		/* update ratio */
>  		at91_adc_config_emr(st);
> +unlock:
> +		mutex_unlock(&st->lock);
>  		return 0;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (val < st->soc_info.min_sample_rate ||
>  		    val > st->soc_info.max_sample_rate)
>  			return -EINVAL;
>  
> +		mutex_lock(&st->lock);
>  		at91_adc_setup_samp_freq(indio_dev, val);
> +		mutex_unlock(&st->lock);
>  		return 0;
>  	default:
>  		return -EINVAL;

Re: [PATCH 02/16] iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq

[Claudiu.Beznea]

On 11.06.2022 20:30, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:31:59 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> .read_raw()/.write_raw() could be called asynchronously from user space
>> or other in kernel drivers. Without locking on st->lock these could be
>> called asynchronously while there is a conversion in progress. Read will
>> be harmless but changing registers while conversion is in progress may
>> lead to inconsistent results. Thus, to avoid this lock st->lock.
> 
> The patch makes sense, but I'm not convinced all of the changes below
> involve any changes to registers. E.g. at91_adc_adjust_val_osr()
> is using the cached value of something in a register, but not the
> register itself, so please update the description to mention cached state.
> 
> Other comments inline.
>>
>> Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver")
>> Fixes: 6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling resolution")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 32b6f157b803..a672a520cdc0 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -1542,10 +1542,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>               ret = at91_adc_read_position(st, chan->channel,
>>                                            &tmp_val);
>>               *val = tmp_val;
>> +             ret = at91_adc_adjust_val_osr(st, val);
>>               mutex_unlock(&st->lock);
>>               iio_device_release_direct_mode(indio_dev);
>>
>> -             return at91_adc_adjust_val_osr(st, val);
>> +             return ret;
>>       }
>>       if (chan->type == IIO_PRESSURE) {
>>               ret = iio_device_claim_direct_mode(indio_dev);
>> @@ -1556,10 +1557,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>               ret = at91_adc_read_pressure(st, chan->channel,
>>                                            &tmp_val);
>>               *val = tmp_val;
>> +             ret = at91_adc_adjust_val_osr(st, val);
>>               mutex_unlock(&st->lock);
>>               iio_device_release_direct_mode(indio_dev);
>>
>> -             return at91_adc_adjust_val_osr(st, val);
>> +             return ret;
>>       }
>>
>>       /* in this case we have a voltage channel */
>> @@ -1620,11 +1622,15 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>               return IIO_VAL_FRACTIONAL_LOG2;
>>
>>       case IIO_CHAN_INFO_SAMP_FREQ:
>> +             mutex_lock(&st->lock);
>>               *val = at91_adc_get_sample_freq(st);
> 
> So this is a straight read of a cached value.  The only thing you 'might'
> arguably be protecting against is read/write tearing due to it in theory
> being possible to write part of the value whilst reading.  

Yes, for these kind of scenarios I kept the lock around cached values, too.

> I don't
> see that being a concern for st->current_sample_rate

I am not fully aware of all the user space tools that are retrieving this
and how this is used and thus I kept the lock also around the cached values
to protect the user space tools being polluted with wrong values, if any.

> 
>> +             mutex_unlock(&st->lock);
>>               return IIO_VAL_INT;
>>
>>       case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +             mutex_lock(&st->lock);
>>               *val = st->oversampling_ratio;
> Likewise, what are you protecting against racing with this that can't
> just occur before or after the lock?

Same as above.

> 
>> +             mutex_unlock(&st->lock);
>>               return IIO_VAL_INT;
>>
>>       default:
>> @@ -1644,18 +1650,23 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>                   (val != AT91_OSR_16SAMPLES))
>>                       return -EINVAL;
>>               /* if no change, optimize out */
>> +             mutex_lock(&st->lock);
>>               if (val == st->oversampling_ratio)
>> -                     return 0;
> It should be race free to check this outside the lock.
> 
> Definitely valid to lock around the cached value write and the config
> write though.
> 
>> +                     goto unlock;
> If you did want to have locking as now then flip the logic
> 
>                 if (val != st->oversampling_ratio) {
>                         st->oversampling_ratio = val;
>                         at91_adc_config_emr(st);
>                 }
>                 mutex_unlock()
> ..
> 

OK, thanks!

> Goto always have a cost in readability so if you can avoid them with
> a simple flip of logic like this it is usually a good idea.
> (exception is error code which should always be out of line as
> that is more common so what we expect to see).
> 
>>               st->oversampling_ratio = val;
>>               /* update ratio */
>>               at91_adc_config_emr(st);
>> +unlock:
>> +             mutex_unlock(&st->lock);
>>               return 0;
>>       case IIO_CHAN_INFO_SAMP_FREQ:
>>               if (val < st->soc_info.min_sample_rate ||
>>                   val > st->soc_info.max_sample_rate)
>>                       return -EINVAL;
>>
>> +             mutex_lock(&st->lock);
>>               at91_adc_setup_samp_freq(indio_dev, val);
>> +             mutex_unlock(&st->lock);
>>               return 0;
>>       default:
>>               return -EINVAL;
> 

[PATCH 03/16] iio: adc: at91-sama5d2_adc: exit from write_raw() when buffers are enabled

[Claudiu Beznea]

When buffers are enabled conversion may start asynchronously thus
allowing changes on actual hardware could lead to bad behavior. Thus
do not allow changing oversampling ratio and sample frequency when
buffers are enabled.

Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index a672a520cdc0..b76328da0cb2 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1644,6 +1644,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
-- 
2.34.1

Re: [PATCH 03/16] iio: adc: at91-sama5d2_adc: exit from write_raw() when buffers are enabled

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:00 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> When buffers are enabled conversion may start asynchronously thus
> allowing changes on actual hardware could lead to bad behavior. Thus
> do not allow changing oversampling ratio and sample frequency when
> buffers are enabled.

Less than desirable behavior perhaps, but broken?  I don't see this
as a fix from what you have mentioned - though I'm not against it.
(just drop the fixes tag)
It is an ABI change, but unlikely to be one any sane code hits.

> 
> Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index a672a520cdc0..b76328da0cb2 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1644,6 +1644,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;

This is racy as nothing stops buffers being enabled after this point.
Use the iio_device_claim_direct_mode() and release for this as they
protect against the race.


> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&

Re: [PATCH 03/16] iio: adc: at91-sama5d2_adc: exit from write_raw() when buffers are enabled

[Claudiu.Beznea]

On 11.06.2022 20:33, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:00 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> When buffers are enabled conversion may start asynchronously thus
>> allowing changes on actual hardware could lead to bad behavior. Thus
>> do not allow changing oversampling ratio and sample frequency when
>> buffers are enabled.
> 
> Less than desirable behavior perhaps, but broken?  I don't see this
> as a fix from what you have mentioned - though I'm not against it.
> (just drop the fixes tag)
> It is an ABI change, but unlikely to be one any sane code hits.
> 
>>
>> Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index a672a520cdc0..b76328da0cb2 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -1644,6 +1644,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>  {
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>>
>> +     if (iio_buffer_enabled(indio_dev))
>> +             return -EBUSY;
> 
> This is racy as nothing stops buffers being enabled after this point.
> Use the iio_device_claim_direct_mode() and release for this as they
> protect against the race.

OK, thanks!

> 
> 
>> +
>>       switch (mask) {
>>       case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>               if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
> 

[PATCH 04/16] iio: adc: at91-sama5d2_adc: handle different EMR.OSR for different hw versions

[Claudiu Beznea]

SAMA7G5 introduces 64 and 256 oversampling rates. Due to this EMR.OSR is 3
bits long. Change the code to reflect this. Commit prepares the code
for the addition of 64 and 256 oversampling rates.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 55 ++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index b76328da0cb2..1ceab097335c 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -138,8 +138,7 @@ struct at91_adc_reg_layout {
 /* Extended Mode Register */
 	u16				EMR;
 /* Extended Mode Register - Oversampling rate */
-#define AT91_SAMA5D2_EMR_OSR(V)			((V) << 16)
-#define AT91_SAMA5D2_EMR_OSR_MASK		GENMASK(17, 16)
+#define AT91_SAMA5D2_EMR_OSR(V, M)		(((V) << 16) & (M))
 #define AT91_SAMA5D2_EMR_OSR_1SAMPLES		0
 #define AT91_SAMA5D2_EMR_OSR_4SAMPLES		1
 #define AT91_SAMA5D2_EMR_OSR_16SAMPLES		2
@@ -403,6 +402,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
  * @max_index:		highest channel index (highest index may be higher
  *			than the total channel number)
  * @hw_trig_cnt:	number of possible hardware triggers
+ * @osr_mask:		oversampling ratio bitmask on EMR register
+ * @osr_vals:		available oversampling rates
  */
 struct at91_adc_platform {
 	const struct at91_adc_reg_layout	*layout;
@@ -414,6 +415,8 @@ struct at91_adc_platform {
 	unsigned int				max_channels;
 	unsigned int				max_index;
 	unsigned int				hw_trig_cnt;
+	unsigned int				osr_mask;
+	unsigned int				osr_vals;
 };
 
 /**
@@ -612,6 +615,10 @@ static const struct at91_adc_platform sama5d2_platform = {
 	.max_index = AT91_SAMA5D2_MAX_CHAN_IDX,
 #define AT91_SAMA5D2_HW_TRIG_CNT	3
 	.hw_trig_cnt = AT91_SAMA5D2_HW_TRIG_CNT,
+	.osr_mask = GENMASK(17, 16),
+	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
 };
 
 static const struct at91_adc_platform sama7g5_platform = {
@@ -627,6 +634,10 @@ static const struct at91_adc_platform sama7g5_platform = {
 	.max_index = AT91_SAMA7G5_MAX_CHAN_IDX,
 #define AT91_SAMA7G5_HW_TRIG_CNT	3
 	.hw_trig_cnt = AT91_SAMA7G5_HW_TRIG_CNT,
+	.osr_mask = GENMASK(18, 16),
+	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
 };
 
 static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
@@ -725,34 +736,45 @@ static void at91_adc_eoc_ena(struct at91_adc_state *st, unsigned int channel)
 		at91_adc_writel(st, EOC_IER, BIT(channel));
 }
 
-static void at91_adc_config_emr(struct at91_adc_state *st)
+static int at91_adc_config_emr(struct at91_adc_state *st,
+			       u32 oversampling_ratio)
 {
 	/* configure the extended mode register */
 	unsigned int emr = at91_adc_readl(st, EMR);
+	unsigned int osr_mask = st->soc_info.platform->osr_mask;
+	unsigned int osr_vals = st->soc_info.platform->osr_vals;
 
 	/* select oversampling per single trigger event */
 	emr |= AT91_SAMA5D2_EMR_ASTE(1);
 
 	/* delete leftover content if it's the case */
-	emr &= ~AT91_SAMA5D2_EMR_OSR_MASK;
+	emr &= ~osr_mask;
 
 	/* select oversampling ratio from configuration */
-	switch (st->oversampling_ratio) {
+	switch (oversampling_ratio) {
 	case AT91_OSR_1SAMPLES:
-		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES) &
-		       AT91_SAMA5D2_EMR_OSR_MASK;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES)))
+			return -EINVAL;
+		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES,
+					    osr_mask);
 		break;
 	case AT91_OSR_4SAMPLES:
-		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES) &
-		       AT91_SAMA5D2_EMR_OSR_MASK;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES)))
+			return -EINVAL;
+		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES,
+					    osr_mask);
 		break;
 	case AT91_OSR_16SAMPLES:
-		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES) &
-		       AT91_SAMA5D2_EMR_OSR_MASK;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES)))
+			return -EINVAL;
+		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
+					    osr_mask);
 		break;
 	}
 
 	at91_adc_writel(st, EMR, emr);
+
+	return 0;
 }
 
 static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
@@ -1643,6 +1665,7 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 			      int val, int val2, long mask)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret = 0;
 
 	if (iio_buffer_enabled(indio_dev))
 		return -EBUSY;
@@ -1656,12 +1679,14 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 		mutex_lock(&st->lock);
 		if (val == st->oversampling_ratio)
 			goto unlock;
-		st->oversampling_ratio = val;
 		/* update ratio */
-		at91_adc_config_emr(st);
+		ret = at91_adc_config_emr(st, val);
+		if (ret)
+			goto unlock;
+		st->oversampling_ratio = val;
 unlock:
 		mutex_unlock(&st->lock);
-		return 0;
+		return ret;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val < st->soc_info.min_sample_rate ||
 		    val > st->soc_info.max_sample_rate)
@@ -1834,7 +1859,7 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
 	at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate);
 
 	/* configure extended mode register */
-	at91_adc_config_emr(st);
+	at91_adc_config_emr(st, st->oversampling_ratio);
 }
 
 static ssize_t at91_adc_get_fifo_state(struct device *dev,
-- 
2.34.1

Re: [PATCH 04/16] iio: adc: at91-sama5d2_adc: handle different EMR.OSR for different hw versions

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:01 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> SAMA7G5 introduces 64 and 256 oversampling rates. Due to this EMR.OSR is 3
> bits long. Change the code to reflect this. Commit prepares the code
> for the addition of 64 and 256 oversampling rates.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 55 ++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index b76328da0cb2..1ceab097335c 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -138,8 +138,7 @@ struct at91_adc_reg_layout {
>  /* Extended Mode Register */
>  	u16				EMR;
>  /* Extended Mode Register - Oversampling rate */
> -#define AT91_SAMA5D2_EMR_OSR(V)			((V) << 16)
> -#define AT91_SAMA5D2_EMR_OSR_MASK		GENMASK(17, 16)
> +#define AT91_SAMA5D2_EMR_OSR(V, M)		(((V) << 16) & (M))
>  #define AT91_SAMA5D2_EMR_OSR_1SAMPLES		0
>  #define AT91_SAMA5D2_EMR_OSR_4SAMPLES		1
>  #define AT91_SAMA5D2_EMR_OSR_16SAMPLES		2
> @@ -403,6 +402,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>   * @max_index:		highest channel index (highest index may be higher
>   *			than the total channel number)
>   * @hw_trig_cnt:	number of possible hardware triggers
> + * @osr_mask:		oversampling ratio bitmask on EMR register
> + * @osr_vals:		available oversampling rates
>   */
>  struct at91_adc_platform {
>  	const struct at91_adc_reg_layout	*layout;
> @@ -414,6 +415,8 @@ struct at91_adc_platform {
>  	unsigned int				max_channels;
>  	unsigned int				max_index;
>  	unsigned int				hw_trig_cnt;
> +	unsigned int				osr_mask;
> +	unsigned int				osr_vals;
>  };
>  
>  /**
> @@ -612,6 +615,10 @@ static const struct at91_adc_platform sama5d2_platform = {
>  	.max_index = AT91_SAMA5D2_MAX_CHAN_IDX,
>  #define AT91_SAMA5D2_HW_TRIG_CNT	3
>  	.hw_trig_cnt = AT91_SAMA5D2_HW_TRIG_CNT,
> +	.osr_mask = GENMASK(17, 16),
> +	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
>  };
>  
>  static const struct at91_adc_platform sama7g5_platform = {
> @@ -627,6 +634,10 @@ static const struct at91_adc_platform sama7g5_platform = {
>  	.max_index = AT91_SAMA7G5_MAX_CHAN_IDX,
>  #define AT91_SAMA7G5_HW_TRIG_CNT	3
>  	.hw_trig_cnt = AT91_SAMA7G5_HW_TRIG_CNT,
> +	.osr_mask = GENMASK(18, 16),
> +	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
>  };
>  
>  static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
> @@ -725,34 +736,45 @@ static void at91_adc_eoc_ena(struct at91_adc_state *st, unsigned int channel)
>  		at91_adc_writel(st, EOC_IER, BIT(channel));
>  }
>  
> -static void at91_adc_config_emr(struct at91_adc_state *st)
> +static int at91_adc_config_emr(struct at91_adc_state *st,
> +			       u32 oversampling_ratio)
>  {
>  	/* configure the extended mode register */
>  	unsigned int emr = at91_adc_readl(st, EMR);
> +	unsigned int osr_mask = st->soc_info.platform->osr_mask;
> +	unsigned int osr_vals = st->soc_info.platform->osr_vals;
>  
>  	/* select oversampling per single trigger event */
>  	emr |= AT91_SAMA5D2_EMR_ASTE(1);
>  
>  	/* delete leftover content if it's the case */
> -	emr &= ~AT91_SAMA5D2_EMR_OSR_MASK;
> +	emr &= ~osr_mask;
>  
>  	/* select oversampling ratio from configuration */
> -	switch (st->oversampling_ratio) {
> +	switch (oversampling_ratio) {
>  	case AT91_OSR_1SAMPLES:
> -		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES) &
> -		       AT91_SAMA5D2_EMR_OSR_MASK;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES,
> +					    osr_mask);
>  		break;
>  	case AT91_OSR_4SAMPLES:
> -		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES) &
> -		       AT91_SAMA5D2_EMR_OSR_MASK;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES,
> +					    osr_mask);
>  		break;
>  	case AT91_OSR_16SAMPLES:
> -		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES) &
> -		       AT91_SAMA5D2_EMR_OSR_MASK;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
> +					    osr_mask);
>  		break;
>  	}
>  
>  	at91_adc_writel(st, EMR, emr);
> +
> +	return 0;
>  }
>  
>  static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
> @@ -1643,6 +1665,7 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  			      int val, int val2, long mask)
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret = 0;
>  
>  	if (iio_buffer_enabled(indio_dev))
>  		return -EBUSY;
> @@ -1656,12 +1679,14 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  		mutex_lock(&st->lock);
>  		if (val == st->oversampling_ratio)
>  			goto unlock;
> -		st->oversampling_ratio = val;
>  		/* update ratio */
> -		at91_adc_config_emr(st);
> +		ret = at91_adc_config_emr(st, val);
> +		if (ret)
> +			goto unlock;
> +		st->oversampling_ratio = val;

Good. I looked at the old ordering when reviewing earlier patch and thought
that doesn't look good :)

However, now you hae the value passed to at91_adc_config_emr() perhaps
you can drop the checking that it is a possible value from above this call
and move it to the default case on the switch statement in there?
(noticed on later patch, where that context is visible).

>  unlock:
>  		mutex_unlock(&st->lock);
> -		return 0;
> +		return ret;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (val < st->soc_info.min_sample_rate ||
>  		    val > st->soc_info.max_sample_rate)
> @@ -1834,7 +1859,7 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
>  	at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate);
>  
>  	/* configure extended mode register */
> -	at91_adc_config_emr(st);
> +	at91_adc_config_emr(st, st->oversampling_ratio);
>  }
>  
>  static ssize_t at91_adc_get_fifo_state(struct device *dev,

Re: [PATCH 04/16] iio: adc: at91-sama5d2_adc: handle different EMR.OSR for different hw versions

[Claudiu.Beznea]

On 11.06.2022 20:46, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:01 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> SAMA7G5 introduces 64 and 256 oversampling rates. Due to this EMR.OSR is 3
>> bits long. Change the code to reflect this. Commit prepares the code
>> for the addition of 64 and 256 oversampling rates.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 55 ++++++++++++++++++++++--------
>>  1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index b76328da0cb2..1ceab097335c 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -138,8 +138,7 @@ struct at91_adc_reg_layout {
>>  /* Extended Mode Register */
>>       u16                             EMR;
>>  /* Extended Mode Register - Oversampling rate */
>> -#define AT91_SAMA5D2_EMR_OSR(V)                      ((V) << 16)
>> -#define AT91_SAMA5D2_EMR_OSR_MASK            GENMASK(17, 16)
>> +#define AT91_SAMA5D2_EMR_OSR(V, M)           (((V) << 16) & (M))
>>  #define AT91_SAMA5D2_EMR_OSR_1SAMPLES                0
>>  #define AT91_SAMA5D2_EMR_OSR_4SAMPLES                1
>>  #define AT91_SAMA5D2_EMR_OSR_16SAMPLES               2
>> @@ -403,6 +402,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>>   * @max_index:               highest channel index (highest index may be higher
>>   *                   than the total channel number)
>>   * @hw_trig_cnt:     number of possible hardware triggers
>> + * @osr_mask:                oversampling ratio bitmask on EMR register
>> + * @osr_vals:                available oversampling rates
>>   */
>>  struct at91_adc_platform {
>>       const struct at91_adc_reg_layout        *layout;
>> @@ -414,6 +415,8 @@ struct at91_adc_platform {
>>       unsigned int                            max_channels;
>>       unsigned int                            max_index;
>>       unsigned int                            hw_trig_cnt;
>> +     unsigned int                            osr_mask;
>> +     unsigned int                            osr_vals;
>>  };
>>
>>  /**
>> @@ -612,6 +615,10 @@ static const struct at91_adc_platform sama5d2_platform = {
>>       .max_index = AT91_SAMA5D2_MAX_CHAN_IDX,
>>  #define AT91_SAMA5D2_HW_TRIG_CNT     3
>>       .hw_trig_cnt = AT91_SAMA5D2_HW_TRIG_CNT,
>> +     .osr_mask = GENMASK(17, 16),
>> +     .osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
>>  };
>>
>>  static const struct at91_adc_platform sama7g5_platform = {
>> @@ -627,6 +634,10 @@ static const struct at91_adc_platform sama7g5_platform = {
>>       .max_index = AT91_SAMA7G5_MAX_CHAN_IDX,
>>  #define AT91_SAMA7G5_HW_TRIG_CNT     3
>>       .hw_trig_cnt = AT91_SAMA7G5_HW_TRIG_CNT,
>> +     .osr_mask = GENMASK(18, 16),
>> +     .osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
>>  };
>>
>>  static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
>> @@ -725,34 +736,45 @@ static void at91_adc_eoc_ena(struct at91_adc_state *st, unsigned int channel)
>>               at91_adc_writel(st, EOC_IER, BIT(channel));
>>  }
>>
>> -static void at91_adc_config_emr(struct at91_adc_state *st)
>> +static int at91_adc_config_emr(struct at91_adc_state *st,
>> +                            u32 oversampling_ratio)
>>  {
>>       /* configure the extended mode register */
>>       unsigned int emr = at91_adc_readl(st, EMR);
>> +     unsigned int osr_mask = st->soc_info.platform->osr_mask;
>> +     unsigned int osr_vals = st->soc_info.platform->osr_vals;
>>
>>       /* select oversampling per single trigger event */
>>       emr |= AT91_SAMA5D2_EMR_ASTE(1);
>>
>>       /* delete leftover content if it's the case */
>> -     emr &= ~AT91_SAMA5D2_EMR_OSR_MASK;
>> +     emr &= ~osr_mask;
>>
>>       /* select oversampling ratio from configuration */
>> -     switch (st->oversampling_ratio) {
>> +     switch (oversampling_ratio) {
>>       case AT91_OSR_1SAMPLES:
>> -             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES) &
>> -                    AT91_SAMA5D2_EMR_OSR_MASK;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES)))
>> +                     return -EINVAL;
>> +             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES,
>> +                                         osr_mask);
>>               break;
>>       case AT91_OSR_4SAMPLES:
>> -             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES) &
>> -                    AT91_SAMA5D2_EMR_OSR_MASK;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES)))
>> +                     return -EINVAL;
>> +             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES,
>> +                                         osr_mask);
>>               break;
>>       case AT91_OSR_16SAMPLES:
>> -             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES) &
>> -                    AT91_SAMA5D2_EMR_OSR_MASK;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES)))
>> +                     return -EINVAL;
>> +             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>> +                                         osr_mask);
>>               break;
>>       }
>>
>>       at91_adc_writel(st, EMR, emr);
>> +
>> +     return 0;
>>  }
>>
>>  static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
>> @@ -1643,6 +1665,7 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>                             int val, int val2, long mask)
>>  {
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int ret = 0;
>>
>>       if (iio_buffer_enabled(indio_dev))
>>               return -EBUSY;
>> @@ -1656,12 +1679,14 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>               mutex_lock(&st->lock);
>>               if (val == st->oversampling_ratio)
>>                       goto unlock;
>> -             st->oversampling_ratio = val;
>>               /* update ratio */
>> -             at91_adc_config_emr(st);
>> +             ret = at91_adc_config_emr(st, val);
>> +             if (ret)
>> +                     goto unlock;
>> +             st->oversampling_ratio = val;
> 
> Good. I looked at the old ordering when reviewing earlier patch and thought
> that doesn't look good :)
> 
> However, now you hae the value passed to at91_adc_config_emr() perhaps
> you can drop the checking that it is a possible value from above this call
> and move it to the default case on the switch statement in there?
> (noticed on later patch, where that context is visible).

I'll check it and adapt it in next version.

> 
>>  unlock:
>>               mutex_unlock(&st->lock);
>> -             return 0;
>> +             return ret;
>>       case IIO_CHAN_INFO_SAMP_FREQ:
>>               if (val < st->soc_info.min_sample_rate ||
>>                   val > st->soc_info.max_sample_rate)
>> @@ -1834,7 +1859,7 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
>>       at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate);
>>
>>       /* configure extended mode register */
>> -     at91_adc_config_emr(st);
>> +     at91_adc_config_emr(st, st->oversampling_ratio);
>>  }
>>
>>  static ssize_t at91_adc_get_fifo_state(struct device *dev,
> 

[PATCH 05/16] iio: adc: at91-sama5d2_adc: adjust osr based on specific platform data

[Claudiu Beznea]

ADC captures data on 12 bits (if oversampling is not enabled). When using
oversampling captured data could go up to 14 bits for SAMA5D2 or up to
16 bits for SAMA7G5 (depending on oversampling settings). All the channels
that are subject of oversampling are registered as 14 or 16 real bits.
Depending on the oversampling settings the ADC converted value need to be
shifted up to 14 or 16 to cope with realbits value registered to IIO
subsystem. Commit adds platform specific information to know if we
run on a system with up to 14 or 16 bits ADC converted data.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 32 ++++++++++++++++++------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 1ceab097335c..7321a4b519af 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -404,6 +404,7 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
  * @hw_trig_cnt:	number of possible hardware triggers
  * @osr_mask:		oversampling ratio bitmask on EMR register
  * @osr_vals:		available oversampling rates
+ * @chan_realbits:	realbits for registered channels
  */
 struct at91_adc_platform {
 	const struct at91_adc_reg_layout	*layout;
@@ -417,6 +418,7 @@ struct at91_adc_platform {
 	unsigned int				hw_trig_cnt;
 	unsigned int				osr_mask;
 	unsigned int				osr_vals;
+	unsigned int				chan_realbits;
 };
 
 /**
@@ -619,6 +621,7 @@ static const struct at91_adc_platform sama5d2_platform = {
 	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
 		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
 		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
+	.chan_realbits = 14,
 };
 
 static const struct at91_adc_platform sama7g5_platform = {
@@ -638,6 +641,7 @@ static const struct at91_adc_platform sama7g5_platform = {
 	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
 		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
 		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
+	.chan_realbits = 16,
 };
 
 static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
@@ -779,19 +783,21 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 
 static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
 {
-	if (st->oversampling_ratio == AT91_OSR_1SAMPLES) {
-		/*
-		 * in this case we only have 12 bits of real data, but channel
-		 * is registered as 14 bits, so shift left two bits
-		 */
-		*val <<= 2;
-	} else if (st->oversampling_ratio == AT91_OSR_4SAMPLES) {
-		/*
-		 * in this case we have 13 bits of real data, but channel
-		 * is registered as 14 bits, so left shift one bit
-		 */
-		*val <<= 1;
-	}
+	int nbits, diff;
+
+	if (st->oversampling_ratio == AT91_OSR_1SAMPLES)
+		nbits = 12;
+	else if (st->oversampling_ratio == AT91_OSR_4SAMPLES)
+		nbits = 13;
+	else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
+		nbits = 14;
+
+	/*
+	 * We have nbits of real data and channel is registered as
+	 * st->soc_info.platform->chan_realbits, so shift left diff bits.
+	 */
+	diff = st->soc_info.platform->chan_realbits - nbits;
+	*val <<= diff;
 
 	return IIO_VAL_INT;
 }
-- 
2.34.1

[PATCH 06/16] iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio

[Claudiu Beznea]

Add 64 and 256 oversampling ratio support. It is necessary for temperature
sensor.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7321a4b519af..b52f1020feaf 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -142,6 +142,8 @@ struct at91_adc_reg_layout {
 #define AT91_SAMA5D2_EMR_OSR_1SAMPLES		0
 #define AT91_SAMA5D2_EMR_OSR_4SAMPLES		1
 #define AT91_SAMA5D2_EMR_OSR_16SAMPLES		2
+#define AT91_SAMA5D2_EMR_OSR_64SAMPLES		3
+#define AT91_SAMA5D2_EMR_OSR_256SAMPLES		4
 
 /* Extended Mode Register - Averaging on single trigger event */
 #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
@@ -308,6 +310,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
 #define AT91_OSR_1SAMPLES		1
 #define AT91_OSR_4SAMPLES		4
 #define AT91_OSR_16SAMPLES		16
+#define AT91_OSR_64SAMPLES		64
+#define AT91_OSR_256SAMPLES		256
 
 #define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr)			\
 	{								\
@@ -640,7 +644,9 @@ static const struct at91_adc_platform sama7g5_platform = {
 	.osr_mask = GENMASK(18, 16),
 	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
 		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
-		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
+		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
+		    BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
 	.chan_realbits = 16,
 };
 
@@ -774,6 +780,18 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
 					    osr_mask);
 		break;
+	case AT91_OSR_64SAMPLES:
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
+			return -EINVAL;
+		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
+					    osr_mask);
+		break;
+	case AT91_OSR_256SAMPLES:
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
+			return -EINVAL;
+		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
+					    osr_mask);
+		break;
 	}
 
 	at91_adc_writel(st, EMR, emr);
@@ -791,6 +809,10 @@ static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
 		nbits = 13;
 	else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
 		nbits = 14;
+	else if (st->oversampling_ratio == AT91_OSR_64SAMPLES)
+		nbits = 15;
+	else if (st->oversampling_ratio == AT91_OSR_256SAMPLES)
+		nbits = 16;
 
 	/*
 	 * We have nbits of real data and channel is registered as
@@ -1679,7 +1701,8 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
-		    (val != AT91_OSR_16SAMPLES))
+		    (val != AT91_OSR_16SAMPLES) && (val != AT91_OSR_64SAMPLES) &&
+		    (val != AT91_OSR_256SAMPLES))
 			return -EINVAL;
 		/* if no change, optimize out */
 		mutex_lock(&st->lock);
@@ -1897,7 +1920,9 @@ static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
 static IIO_CONST_ATTR(oversampling_ratio_available,
 		      __stringify(AT91_OSR_1SAMPLES) " "
 		      __stringify(AT91_OSR_4SAMPLES) " "
-		      __stringify(AT91_OSR_16SAMPLES));
+		      __stringify(AT91_OSR_16SAMPLES) " "
+		      __stringify(AT91_OSR_64SAMPLES) " "
+		      __stringify(AT91_OSR_256SAMPLES));
 
 static struct attribute *at91_adc_attributes[] = {
 	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
-- 
2.34.1

Re: [PATCH 06/16] iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:03 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> Add 64 and 256 oversampling ratio support. It is necessary for temperature
> sensor.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7321a4b519af..b52f1020feaf 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -142,6 +142,8 @@ struct at91_adc_reg_layout {
>  #define AT91_SAMA5D2_EMR_OSR_1SAMPLES		0
>  #define AT91_SAMA5D2_EMR_OSR_4SAMPLES		1
>  #define AT91_SAMA5D2_EMR_OSR_16SAMPLES		2
> +#define AT91_SAMA5D2_EMR_OSR_64SAMPLES		3
> +#define AT91_SAMA5D2_EMR_OSR_256SAMPLES		4
>  
>  /* Extended Mode Register - Averaging on single trigger event */
>  #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
> @@ -308,6 +310,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>  #define AT91_OSR_1SAMPLES		1
>  #define AT91_OSR_4SAMPLES		4
>  #define AT91_OSR_16SAMPLES		16
> +#define AT91_OSR_64SAMPLES		64
> +#define AT91_OSR_256SAMPLES		256

These defines seems a bit silly.  Better to use the values inline than
to have these.

>  
>  #define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr)			\
>  	{								\
> @@ -640,7 +644,9 @@ static const struct at91_adc_platform sama7g5_platform = {
>  	.osr_mask = GENMASK(18, 16),
>  	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
>  		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
> -		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
> +		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
>  	.chan_realbits = 16,
>  };
>  
> @@ -774,6 +780,18 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>  					    osr_mask);
>  		break;
> +	case AT91_OSR_64SAMPLES:
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
> +					    osr_mask);
> +		break;
> +	case AT91_OSR_256SAMPLES:
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
> +					    osr_mask);
> +		break;
>  	}
>  
>  	at91_adc_writel(st, EMR, emr);
> @@ -791,6 +809,10 @@ static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
>  		nbits = 13;
>  	else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
>  		nbits = 14;
> +	else if (st->oversampling_ratio == AT91_OSR_64SAMPLES)
> +		nbits = 15;
> +	else if (st->oversampling_ratio == AT91_OSR_256SAMPLES)
> +		nbits = 16;
>  
>  	/*
>  	 * We have nbits of real data and channel is registered as
> @@ -1679,7 +1701,8 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
> -		    (val != AT91_OSR_16SAMPLES))
> +		    (val != AT91_OSR_16SAMPLES) && (val != AT91_OSR_64SAMPLES) &&
> +		    (val != AT91_OSR_256SAMPLES))
Dropping this partial validity check and moving into a default in the switch statement
in config_emr() would be nice cleanup (I also replied to earlier patch based on what
is visible here).

>  			return -EINVAL;
>  		/* if no change, optimize out */
>  		mutex_lock(&st->lock);
> @@ -1897,7 +1920,9 @@ static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
>  static IIO_CONST_ATTR(oversampling_ratio_available,
>  		      __stringify(AT91_OSR_1SAMPLES) " "
>  		      __stringify(AT91_OSR_4SAMPLES) " "
> -		      __stringify(AT91_OSR_16SAMPLES));
> +		      __stringify(AT91_OSR_16SAMPLES) " "
> +		      __stringify(AT91_OSR_64SAMPLES) " "
> +		      __stringify(AT91_OSR_256SAMPLES));

At somepoint it would be good to move this over to the read_avail() callback rather than
hand rolling it.  We are slowly working through doing this for all the IIO drivers
but it will take a long time yet!

>  
>  static struct attribute *at91_adc_attributes[] = {
>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,

Re: [PATCH 06/16] iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio

[Claudiu.Beznea]

On 11.06.2022 20:47, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:03 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> Add 64 and 256 oversampling ratio support. It is necessary for temperature
>> sensor.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 7321a4b519af..b52f1020feaf 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -142,6 +142,8 @@ struct at91_adc_reg_layout {
>>  #define AT91_SAMA5D2_EMR_OSR_1SAMPLES                0
>>  #define AT91_SAMA5D2_EMR_OSR_4SAMPLES                1
>>  #define AT91_SAMA5D2_EMR_OSR_16SAMPLES               2
>> +#define AT91_SAMA5D2_EMR_OSR_64SAMPLES               3
>> +#define AT91_SAMA5D2_EMR_OSR_256SAMPLES              4
>>
>>  /* Extended Mode Register - Averaging on single trigger event */
>>  #define AT91_SAMA5D2_EMR_ASTE(V)             ((V) << 20)
>> @@ -308,6 +310,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>>  #define AT91_OSR_1SAMPLES            1
>>  #define AT91_OSR_4SAMPLES            4
>>  #define AT91_OSR_16SAMPLES           16
>> +#define AT91_OSR_64SAMPLES           64
>> +#define AT91_OSR_256SAMPLES          256
> 
> These defines seems a bit silly.  Better to use the values inline than
> to have these.
> 
>>
>>  #define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr)                   \
>>       {                                                               \
>> @@ -640,7 +644,9 @@ static const struct at91_adc_platform sama7g5_platform = {
>>       .osr_mask = GENMASK(18, 16),
>>       .osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
>>                   BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
>> -                 BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
>> +                 BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
>>       .chan_realbits = 16,
>>  };
>>
>> @@ -774,6 +780,18 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>>               emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>>                                           osr_mask);
>>               break;
>> +     case AT91_OSR_64SAMPLES:
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
>> +                     return -EINVAL;
>> +             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
>> +                                         osr_mask);
>> +             break;
>> +     case AT91_OSR_256SAMPLES:
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
>> +                     return -EINVAL;
>> +             emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
>> +                                         osr_mask);
>> +             break;
>>       }
>>
>>       at91_adc_writel(st, EMR, emr);
>> @@ -791,6 +809,10 @@ static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
>>               nbits = 13;
>>       else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
>>               nbits = 14;
>> +     else if (st->oversampling_ratio == AT91_OSR_64SAMPLES)
>> +             nbits = 15;
>> +     else if (st->oversampling_ratio == AT91_OSR_256SAMPLES)
>> +             nbits = 16;
>>
>>       /*
>>        * We have nbits of real data and channel is registered as
>> @@ -1679,7 +1701,8 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>       switch (mask) {
>>       case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>               if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
>> -                 (val != AT91_OSR_16SAMPLES))
>> +                 (val != AT91_OSR_16SAMPLES) && (val != AT91_OSR_64SAMPLES) &&
>> +                 (val != AT91_OSR_256SAMPLES))
> Dropping this partial validity check and moving into a default in the switch statement
> in config_emr() would be nice cleanup (I also replied to earlier patch based on what
> is visible here).

Sure, I'll check it.

> 
>>                       return -EINVAL;
>>               /* if no change, optimize out */
>>               mutex_lock(&st->lock);
>> @@ -1897,7 +1920,9 @@ static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
>>  static IIO_CONST_ATTR(oversampling_ratio_available,
>>                     __stringify(AT91_OSR_1SAMPLES) " "
>>                     __stringify(AT91_OSR_4SAMPLES) " "
>> -                   __stringify(AT91_OSR_16SAMPLES));
>> +                   __stringify(AT91_OSR_16SAMPLES) " "
>> +                   __stringify(AT91_OSR_64SAMPLES) " "
>> +                   __stringify(AT91_OSR_256SAMPLES));
> 
> At somepoint it would be good to move this over to the read_avail() callback rather than
> hand rolling it.  We are slowly working through doing this for all the IIO drivers
> but it will take a long time yet!

I'll check this, too.

> 
>>
>>  static struct attribute *at91_adc_attributes[] = {
>>       &iio_const_attr_oversampling_ratio_available.dev_attr.attr,
> 

[PATCH 07/16] iio: adc: at91-sama5d2_adc: simplify the code in at91_adc_read_info_raw()

[Claudiu Beznea]

Simplify a bit the code in at91_adc_read_info_raw() by reducing the
number of lines of code.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 35 +++++++++---------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index b52f1020feaf..fbb98e216e70 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1576,6 +1576,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 				  struct iio_chan_spec const *chan, int *val)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
+	int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;
 	u16 tmp_val;
 	int ret;
 
@@ -1583,29 +1584,18 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 	 * Keep in mind that we cannot use software trigger or touchscreen
 	 * if external trigger is enabled
 	 */
-	if (chan->type == IIO_POSITIONRELATIVE) {
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
-		mutex_lock(&st->lock);
-
-		ret = at91_adc_read_position(st, chan->channel,
-					     &tmp_val);
-		*val = tmp_val;
-		ret = at91_adc_adjust_val_osr(st, val);
-		mutex_unlock(&st->lock);
-		iio_device_release_direct_mode(indio_dev);
+	if (chan->type == IIO_POSITIONRELATIVE)
+		fn = at91_adc_read_position;
+	if (chan->type == IIO_PRESSURE)
+		fn = at91_adc_read_pressure;
 
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
 		return ret;
-	}
-	if (chan->type == IIO_PRESSURE) {
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
-		mutex_lock(&st->lock);
+	mutex_lock(&st->lock);
 
-		ret = at91_adc_read_pressure(st, chan->channel,
-					     &tmp_val);
+	if (fn) {
+		ret = fn(st, chan->channel, &tmp_val);
 		*val = tmp_val;
 		ret = at91_adc_adjust_val_osr(st, val);
 		mutex_unlock(&st->lock);
@@ -1616,11 +1606,6 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 
 	/* in this case we have a voltage channel */
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
-	mutex_lock(&st->lock);
-
 	st->chan = chan;
 
 	at91_adc_cor(st, chan);
-- 
2.34.1

Re: [PATCH 07/16] iio: adc: at91-sama5d2_adc: simplify the code in at91_adc_read_info_raw()

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:04 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> Simplify a bit the code in at91_adc_read_info_raw() by reducing the
> number of lines of code.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

I'm not convinced this is worth while, but there are some lesser
steps visible in this patch that probably are.

Given your earlier reorg to move at01_adc_adjust_val_osr() under the locks,
you can now move the locks to the caller, thus not needing to handle them
separately in all the exit paths.

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 35 +++++++++---------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index b52f1020feaf..fbb98e216e70 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1576,6 +1576,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  				  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;
>  	u16 tmp_val;
>  	int ret;
>  
> @@ -1583,29 +1584,18 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  	 * Keep in mind that we cannot use software trigger or touchscreen
>  	 * if external trigger is enabled
>  	 */
> -	if (chan->type == IIO_POSITIONRELATIVE) {
> -		ret = iio_device_claim_direct_mode(indio_dev);
> -		if (ret)
> -			return ret;

You can drop this out of the if statements as it happens in all paths.
Or even better, move it to the caller..

> -		mutex_lock(&st->lock);
> -
> -		ret = at91_adc_read_position(st, chan->channel,
> -					     &tmp_val);

huh? ret not checked? 

> -		*val = tmp_val;
> -		ret = at91_adc_adjust_val_osr(st, val);
Sure this is duplicated, but meh it's only a few lines.


> -		mutex_unlock(&st->lock);
> -		iio_device_release_direct_mode(indio_dev);

this early release (compared to the long path) is the only bit really
gets duplicated in all paths..

> +	if (chan->type == IIO_POSITIONRELATIVE)
> +		fn = at91_adc_read_position;
> +	if (chan->type == IIO_PRESSURE)
> +		fn = at91_adc_read_pressure;
>  
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
>  		return ret;
> -	}
> -	if (chan->type == IIO_PRESSURE) {
this should always have been an else if () as the chan->type couldn't be both.

> -		ret = iio_device_claim_direct_mode(indio_dev);
> -		if (ret)
> -			return ret;
> -		mutex_lock(&st->lock);
hence this lot can be shared with the above.

> +	mutex_lock(&st->lock);
>  
> -		ret = at91_adc_read_pressure(st, chan->channel,
> -					     &tmp_val);
> +	if (fn) {
> +		ret = fn(st, chan->channel, &tmp_val);
>  		*val = tmp_val;
>  		ret = at91_adc_adjust_val_osr(st, val);
>  		mutex_unlock(&st->lock);
> @@ -1616,11 +1606,6 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  
>  	/* in this case we have a voltage channel */
>  
> -	ret = iio_device_claim_direct_mode(indio_dev);
> -	if (ret)
> -		return ret;
> -	mutex_lock(&st->lock);
> -
>  	st->chan = chan;
>  
>  	at91_adc_cor(st, chan);

Re: [PATCH 07/16] iio: adc: at91-sama5d2_adc: simplify the code in at91_adc_read_info_raw()

[Claudiu.Beznea]

On 11.06.2022 20:54, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:04 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> Simplify a bit the code in at91_adc_read_info_raw() by reducing the
>> number of lines of code.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> I'm not convinced this is worth while, but there are some lesser
> steps visible in this patch that probably are.
> 
> Given your earlier reorg to move at01_adc_adjust_val_osr() under the locks,
> you can now move the locks to the caller, thus not needing to handle them
> separately in all the exit paths.

OK, I'll give it a try. With this, would you prefer to still keep this patch?

> 
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 35 +++++++++---------------------
>>  1 file changed, 10 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index b52f1020feaf..fbb98e216e70 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -1576,6 +1576,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>                                 struct iio_chan_spec const *chan, int *val)
>>  {
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;
>>       u16 tmp_val;
>>       int ret;
>>
>> @@ -1583,29 +1584,18 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>        * Keep in mind that we cannot use software trigger or touchscreen
>>        * if external trigger is enabled
>>        */
>> -     if (chan->type == IIO_POSITIONRELATIVE) {
>> -             ret = iio_device_claim_direct_mode(indio_dev);
>> -             if (ret)
>> -                     return ret;
> 
> You can drop this out of the if statements as it happens in all paths.
> Or even better, move it to the caller..
> 
>> -             mutex_lock(&st->lock);
>> -
>> -             ret = at91_adc_read_position(st, chan->channel,
>> -                                          &tmp_val);
> 
> huh? ret not checked?

Yep, this should have been missed...

> 
>> -             *val = tmp_val;
>> -             ret = at91_adc_adjust_val_osr(st, val);
> Sure this is duplicated, but meh it's only a few lines.
> 
> 
>> -             mutex_unlock(&st->lock);
>> -             iio_device_release_direct_mode(indio_dev);
> 
> this early release (compared to the long path) is the only bit really
> gets duplicated in all paths..
> 
>> +     if (chan->type == IIO_POSITIONRELATIVE)
>> +             fn = at91_adc_read_position;
>> +     if (chan->type == IIO_PRESSURE)
>> +             fn = at91_adc_read_pressure;
>>
>> +     ret = iio_device_claim_direct_mode(indio_dev);
>> +     if (ret)
>>               return ret;
>> -     }
>> -     if (chan->type == IIO_PRESSURE) {
> this should always have been an else if () as the chan->type couldn't be both.
> 
>> -             ret = iio_device_claim_direct_mode(indio_dev);
>> -             if (ret)
>> -                     return ret;
>> -             mutex_lock(&st->lock);
> hence this lot can be shared with the above.

To be sure of what I've understood correctly: in the end you prefer to have
a patch with the point you suggested rather then the initial patch?

Thank you,
Claudiu Beznea

> 
>> +     mutex_lock(&st->lock);
>>
>> -             ret = at91_adc_read_pressure(st, chan->channel,
>> -                                          &tmp_val);
>> +     if (fn) {
>> +             ret = fn(st, chan->channel, &tmp_val);
>>               *val = tmp_val;
>>               ret = at91_adc_adjust_val_osr(st, val);
>>               mutex_unlock(&st->lock);
>> @@ -1616,11 +1606,6 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>
>>       /* in this case we have a voltage channel */
>>
>> -     ret = iio_device_claim_direct_mode(indio_dev);
>> -     if (ret)
>> -             return ret;
>> -     mutex_lock(&st->lock);
>> -
>>       st->chan = chan;
>>
>>       at91_adc_cor(st, chan);
> 

Re: [PATCH 07/16] iio: adc: at91-sama5d2_adc: simplify the code in at91_adc_read_info_raw()

[Jonathan Cameron]

On Tue, 14 Jun 2022 08:49:03 +0000
<Claudiu.Beznea@microchip.com> wrote:

> On 11.06.2022 20:54, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, 9 Jun 2022 11:32:04 +0300
> > Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> >   
> >> Simplify a bit the code in at91_adc_read_info_raw() by reducing the
> >> number of lines of code.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>  
> > 
> > I'm not convinced this is worth while, but there are some lesser
> > steps visible in this patch that probably are.
> > 
> > Given your earlier reorg to move at01_adc_adjust_val_osr() under the locks,
> > you can now move the locks to the caller, thus not needing to handle them
> > separately in all the exit paths.  
> 
> OK, I'll give it a try. With this, would you prefer to still keep this patch?
> 
No. I don't think it will bring enough benefit for the loss of readability.
Having moved the locking, there will only be a few repeated lines.

Jonathan

[PATCH 08/16] iio: adc: at91-sama5d2_adc: move oversampling storage in its function

[Claudiu Beznea]

Move the storage of oversampling_ratio in at91_adc_config_emr().
This prepares for the next commits.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index fbb98e216e70..76367cc948e8 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -796,6 +796,8 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 
 	at91_adc_writel(st, EMR, emr);
 
+	st->oversampling_ratio = oversampling_ratio;
+
 	return 0;
 }
 
@@ -1695,9 +1697,6 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 			goto unlock;
 		/* update ratio */
 		ret = at91_adc_config_emr(st, val);
-		if (ret)
-			goto unlock;
-		st->oversampling_ratio = val;
 unlock:
 		mutex_unlock(&st->lock);
 		return ret;
-- 
2.34.1

[PATCH 09/16] iio: adc: at91-sama5d2_adc: update trackx on emr

[Claudiu Beznea]

Add support for updating trackx bits of EMR register. Having different
values of EMR.TRACKX when measuring temperature give a better accuracy.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 76367cc948e8..22cd8d33c9bf 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -145,6 +145,10 @@ struct at91_adc_reg_layout {
 #define AT91_SAMA5D2_EMR_OSR_64SAMPLES		3
 #define AT91_SAMA5D2_EMR_OSR_256SAMPLES		4
 
+/* Extended Mode Register - TRACKX */
+#define AT91_SAMA5D2_TRACKX_MASK		GENMASK(23, 22)
+#define AT91_SAMA5D2_TRACKX(x)			(((x) << 22) & \
+						 AT91_SAMA5D2_TRACKX_MASK)
 /* Extended Mode Register - Averaging on single trigger event */
 #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
 
@@ -747,7 +751,7 @@ static void at91_adc_eoc_ena(struct at91_adc_state *st, unsigned int channel)
 }
 
 static int at91_adc_config_emr(struct at91_adc_state *st,
-			       u32 oversampling_ratio)
+			       u32 oversampling_ratio, u32 trackx)
 {
 	/* configure the extended mode register */
 	unsigned int emr = at91_adc_readl(st, EMR);
@@ -758,7 +762,7 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 	emr |= AT91_SAMA5D2_EMR_ASTE(1);
 
 	/* delete leftover content if it's the case */
-	emr &= ~osr_mask;
+	emr &= ~(osr_mask | AT91_SAMA5D2_TRACKX_MASK);
 
 	/* select oversampling ratio from configuration */
 	switch (oversampling_ratio) {
@@ -794,6 +798,8 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 		break;
 	}
 
+	/* Update trackx. */
+	emr |= AT91_SAMA5D2_TRACKX(trackx);
 	at91_adc_writel(st, EMR, emr);
 
 	st->oversampling_ratio = oversampling_ratio;
@@ -1696,7 +1702,7 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 		if (val == st->oversampling_ratio)
 			goto unlock;
 		/* update ratio */
-		ret = at91_adc_config_emr(st, val);
+		ret = at91_adc_config_emr(st, val, 0);
 unlock:
 		mutex_unlock(&st->lock);
 		return ret;
@@ -1872,7 +1878,7 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
 	at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate);
 
 	/* configure extended mode register */
-	at91_adc_config_emr(st, st->oversampling_ratio);
+	at91_adc_config_emr(st, st->oversampling_ratio, 0);
 }
 
 static ssize_t at91_adc_get_fifo_state(struct device *dev,
-- 
2.34.1

[PATCH 10/16] iio: adc: at91-sama5d2_adc: add startup and tracktim as parameter for at91_adc_setup_samp_freq()

[Claudiu Beznea]

Add startup and tracktim as parameter for at91_adc_setup_samp_freq()
function. In case of temperature sensor being enabled these parameters
will be configured on temperature read request to improve the accuracy
of the read temperature.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 22cd8d33c9bf..1283bcf4e682 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1435,7 +1435,9 @@ static unsigned at91_adc_startup_time(unsigned startup_time_min,
 	return i;
 }
 
-static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq)
+static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq,
+				     unsigned int startup_time,
+				     unsigned int tracktim)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 	unsigned f_per, prescal, startup, mr;
@@ -1443,17 +1445,17 @@ static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq)
 	f_per = clk_get_rate(st->per_clk);
 	prescal = (f_per / (2 * freq)) - 1;
 
-	startup = at91_adc_startup_time(st->soc_info.startup_time,
-					freq / 1000);
+	startup = at91_adc_startup_time(startup_time, freq / 1000);
 
 	mr = at91_adc_readl(st, MR);
 	mr &= ~(AT91_SAMA5D2_MR_STARTUP_MASK | AT91_SAMA5D2_MR_PRESCAL_MASK);
 	mr |= AT91_SAMA5D2_MR_STARTUP(startup);
 	mr |= AT91_SAMA5D2_MR_PRESCAL(prescal);
+	mr |= AT91_SAMA5D2_MR_TRACKTIM(tracktim);
 	at91_adc_writel(st, MR, mr);
 
-	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
-		freq, startup, prescal);
+	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u, tracktim=%u\n",
+		freq, startup, prescal, tracktim);
 	st->current_sample_rate = freq;
 }
 
@@ -1712,7 +1714,8 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 
 		mutex_lock(&st->lock);
-		at91_adc_setup_samp_freq(indio_dev, val);
+		at91_adc_setup_samp_freq(indio_dev, val,
+					 st->soc_info.startup_time, 0);
 		mutex_unlock(&st->lock);
 		return 0;
 	default:
@@ -1875,7 +1878,8 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
 	at91_adc_writel(st, MR,
 			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
 
-	at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate);
+	at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate,
+				 st->soc_info.startup_time, 0);
 
 	/* configure extended mode register */
 	at91_adc_config_emr(st, st->oversampling_ratio, 0);
-- 
2.34.1

[PATCH 11/16] iio: adc: at91-sama5d2_adc: add locking parameter to at91_adc_read_info_raw()

[Claudiu Beznea]

Add a parameter to at91_adc_read_info_raw() to specify if st->lock mutex
need to be acquired. This prepares for the addition of temperature sensor
code which will re-use at91_adc_read_info_raw() function to read 2 voltages
for determining the real temperature.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 1283bcf4e682..8f8fef42de84 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1583,7 +1583,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 }
 
 static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
-				  struct iio_chan_spec const *chan, int *val)
+				  struct iio_chan_spec const *chan, int *val,
+				  bool lock)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 	int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;
@@ -1602,13 +1603,15 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 	ret = iio_device_claim_direct_mode(indio_dev);
 	if (ret)
 		return ret;
-	mutex_lock(&st->lock);
+	if (lock)
+		mutex_lock(&st->lock);
 
 	if (fn) {
 		ret = fn(st, chan->channel, &tmp_val);
 		*val = tmp_val;
 		ret = at91_adc_adjust_val_osr(st, val);
-		mutex_unlock(&st->lock);
+		if (lock)
+			mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
 		return ret;
@@ -1644,7 +1647,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 	/* Needed to ACK the DRDY interruption */
 	at91_adc_readl(st, LCDR);
 
-	mutex_unlock(&st->lock);
+	if (lock)
+		mutex_unlock(&st->lock);
 
 	iio_device_release_direct_mode(indio_dev);
 	return ret;
@@ -1658,7 +1662,8 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		return at91_adc_read_info_raw(indio_dev, chan, val);
+		return at91_adc_read_info_raw(indio_dev, chan, val, true);
+
 	case IIO_CHAN_INFO_SCALE:
 		*val = st->vref_uv / 1000;
 		if (chan->differential)
-- 
2.34.1

Re: [PATCH 11/16] iio: adc: at91-sama5d2_adc: add locking parameter to at91_adc_read_info_raw()

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:08 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> Add a parameter to at91_adc_read_info_raw() to specify if st->lock mutex
> need to be acquired. This prepares for the addition of temperature sensor
> code which will re-use at91_adc_read_info_raw() function to read 2 voltages
> for determining the real temperature.

This looks like a potential lock dependency issue.
iio_device_claim_direct_mode() takes an internal iio lock, and
you then take st->lock.

If you are going to invert that locking order in another path
you have a deadlock.

So rethink this. If you want to reuse the code you'll need to factor
it out to a separate function that takes none of the locks then
take all locks needed in each call path (in the same order).

Jonathan


> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 1283bcf4e682..8f8fef42de84 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1583,7 +1583,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  }
>  
>  static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> -				  struct iio_chan_spec const *chan, int *val)
> +				  struct iio_chan_spec const *chan, int *val,
> +				  bool lock)
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  	int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;
> @@ -1602,13 +1603,15 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  	ret = iio_device_claim_direct_mode(indio_dev);
>  	if (ret)
>  		return ret;
> -	mutex_lock(&st->lock);
> +	if (lock)
> +		mutex_lock(&st->lock);
>  
>  	if (fn) {
>  		ret = fn(st, chan->channel, &tmp_val);
>  		*val = tmp_val;
>  		ret = at91_adc_adjust_val_osr(st, val);
> -		mutex_unlock(&st->lock);
> +		if (lock)
> +			mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
>  		return ret;
> @@ -1644,7 +1647,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  	/* Needed to ACK the DRDY interruption */
>  	at91_adc_readl(st, LCDR);
>  
> -	mutex_unlock(&st->lock);
> +	if (lock)
> +		mutex_unlock(&st->lock);
>  
>  	iio_device_release_direct_mode(indio_dev);
>  	return ret;
> @@ -1658,7 +1662,8 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		return at91_adc_read_info_raw(indio_dev, chan, val);
> +		return at91_adc_read_info_raw(indio_dev, chan, val, true);
> +
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = st->vref_uv / 1000;
>  		if (chan->differential)

Re: [PATCH 11/16] iio: adc: at91-sama5d2_adc: add locking parameter to at91_adc_read_info_raw()

[Claudiu.Beznea]

On 11.06.2022 20:58, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:08 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> Add a parameter to at91_adc_read_info_raw() to specify if st->lock mutex
>> need to be acquired. This prepares for the addition of temperature sensor
>> code which will re-use at91_adc_read_info_raw() function to read 2 voltages
>> for determining the real temperature.
> 
> This looks like a potential lock dependency issue.
> iio_device_claim_direct_mode() takes an internal iio lock, and
> you then take st->lock.
> 
> If you are going to invert that locking order in another path
> you have a deadlock.
> 
> So rethink this. If you want to reuse the code you'll need to factor
> it out to a separate function that takes none of the locks then
> take all locks needed in each call path (in the same order).

OK, I'll check it.

> 
> Jonathan
> 
> 
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 1283bcf4e682..8f8fef42de84 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -1583,7 +1583,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>>  }
>>
>>  static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>> -                               struct iio_chan_spec const *chan, int *val)
>> +                               struct iio_chan_spec const *chan, int *val,
>> +                               bool lock)
>>  {
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>>       int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;
>> @@ -1602,13 +1603,15 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>       ret = iio_device_claim_direct_mode(indio_dev);
>>       if (ret)
>>               return ret;
>> -     mutex_lock(&st->lock);
>> +     if (lock)
>> +             mutex_lock(&st->lock);
>>
>>       if (fn) {
>>               ret = fn(st, chan->channel, &tmp_val);
>>               *val = tmp_val;
>>               ret = at91_adc_adjust_val_osr(st, val);
>> -             mutex_unlock(&st->lock);
>> +             if (lock)
>> +                     mutex_unlock(&st->lock);
>>               iio_device_release_direct_mode(indio_dev);
>>
>>               return ret;
>> @@ -1644,7 +1647,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>       /* Needed to ACK the DRDY interruption */
>>       at91_adc_readl(st, LCDR);
>>
>> -     mutex_unlock(&st->lock);
>> +     if (lock)
>> +             mutex_unlock(&st->lock);
>>
>>       iio_device_release_direct_mode(indio_dev);
>>       return ret;
>> @@ -1658,7 +1662,8 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>
>>       switch (mask) {
>>       case IIO_CHAN_INFO_RAW:
>> -             return at91_adc_read_info_raw(indio_dev, chan, val);
>> +             return at91_adc_read_info_raw(indio_dev, chan, val, true);
>> +
>>       case IIO_CHAN_INFO_SCALE:
>>               *val = st->vref_uv / 1000;
>>               if (chan->differential)
> 

Re: [PATCH 11/16] iio: adc: at91-sama5d2_adc: add locking parameter to at91_adc_read_info_raw()

[Jonathan Cameron]

On Tue, 14 Jun 2022 08:50:14 +0000
<Claudiu.Beznea@microchip.com> wrote:

> On 11.06.2022 20:58, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, 9 Jun 2022 11:32:08 +0300
> > Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> >   
> >> Add a parameter to at91_adc_read_info_raw() to specify if st->lock mutex
> >> need to be acquired. This prepares for the addition of temperature sensor
> >> code which will re-use at91_adc_read_info_raw() function to read 2 voltages
> >> for determining the real temperature.  
> > 
> > This looks like a potential lock dependency issue.
> > iio_device_claim_direct_mode() takes an internal iio lock, and
> > you then take st->lock.
> > 
> > If you are going to invert that locking order in another path
> > you have a deadlock.
> > 
> > So rethink this. If you want to reuse the code you'll need to factor
> > it out to a separate function that takes none of the locks then
> > take all locks needed in each call path (in the same order).  
> 
> OK, I'll check it.

Hi Claudia,

Minor kernel mailing list etiquette thing is that there is no need
to reply to say you'll check something or that you agree with review
feedback.  Just generates more emails to read.  Reviewers assume
anything you don't comment on their feedback will be addressed in
next version of the code!

Thanks,

Jonathan

> 
> > 
> > Jonathan
> > 
> >   
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >> ---
> >>  drivers/iio/adc/at91-sama5d2_adc.c | 15 ++++++++++-----
> >>  1 file changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >> index 1283bcf4e682..8f8fef42de84 100644
> >> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >> @@ -1583,7 +1583,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> >>  }
> >>
> >>  static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >> -                               struct iio_chan_spec const *chan, int *val)
> >> +                               struct iio_chan_spec const *chan, int *val,
> >> +                               bool lock)
> >>  {
> >>       struct at91_adc_state *st = iio_priv(indio_dev);
> >>       int (*fn)(struct at91_adc_state *, int, u16 *) = NULL;
> >> @@ -1602,13 +1603,15 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >>       ret = iio_device_claim_direct_mode(indio_dev);
> >>       if (ret)
> >>               return ret;
> >> -     mutex_lock(&st->lock);
> >> +     if (lock)
> >> +             mutex_lock(&st->lock);
> >>
> >>       if (fn) {
> >>               ret = fn(st, chan->channel, &tmp_val);
> >>               *val = tmp_val;
> >>               ret = at91_adc_adjust_val_osr(st, val);
> >> -             mutex_unlock(&st->lock);
> >> +             if (lock)
> >> +                     mutex_unlock(&st->lock);
> >>               iio_device_release_direct_mode(indio_dev);
> >>
> >>               return ret;
> >> @@ -1644,7 +1647,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >>       /* Needed to ACK the DRDY interruption */
> >>       at91_adc_readl(st, LCDR);
> >>
> >> -     mutex_unlock(&st->lock);
> >> +     if (lock)
> >> +             mutex_unlock(&st->lock);
> >>
> >>       iio_device_release_direct_mode(indio_dev);
> >>       return ret;
> >> @@ -1658,7 +1662,8 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
> >>
> >>       switch (mask) {
> >>       case IIO_CHAN_INFO_RAW:
> >> -             return at91_adc_read_info_raw(indio_dev, chan, val);
> >> +             return at91_adc_read_info_raw(indio_dev, chan, val, true);
> >> +
> >>       case IIO_CHAN_INFO_SCALE:
> >>               *val = st->vref_uv / 1000;
> >>               if (chan->differential)  
> >   
> 

[PATCH 12/16] dt-bindings: iio: adc: at91-sama5d2_adc: add id for temperature channel

[Claudiu Beznea]

Add ID for temperature channel of AT91 SAMA5D2 ADC.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 include/dt-bindings/iio/adc/at91-sama5d2_adc.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/iio/adc/at91-sama5d2_adc.h b/include/dt-bindings/iio/adc/at91-sama5d2_adc.h
index 70f99dbdbb42..866d36530583 100644
--- a/include/dt-bindings/iio/adc/at91-sama5d2_adc.h
+++ b/include/dt-bindings/iio/adc/at91-sama5d2_adc.h
@@ -13,4 +13,7 @@
 /* pressure channel index */
 #define AT91_SAMA5D2_ADC_P_CHANNEL		26
 
+/* SAMA7G5 Temperature sensor channel index. */
+#define AT91_SAMA7G5_ADC_TEMP_CHANNEL		31
+
 #endif
-- 
2.34.1

Re: [PATCH 12/16] dt-bindings: iio: adc: at91-sama5d2_adc: add id for temperature channel

[Rob Herring]

On Thu, 09 Jun 2022 11:32:09 +0300, Claudiu Beznea wrote:
> Add ID for temperature channel of AT91 SAMA5D2 ADC.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  include/dt-bindings/iio/adc/at91-sama5d2_adc.h | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 13/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[Claudiu Beznea]

SAMA7G5 has a temperature sensor embedded that is connected to channel 31
of ADC. Temperature sensor provides 2 outputs: VTEMP and VBG. VTEMP is
proportional to the absolute temperature voltage, VBG is quasi-temperature
independent voltage. The calibration data for temperature sensor are
retrieved from OTP memory specific to SAMA7G5. The formula to calculate
the junction temperature is as follows:

P1 + (Vref * (Vtemp - P6 - P4 * Vbg)) / (Vbg * VTEMP_DT)

where Pi are calibration data retrieved from OTP memory.

For better resolution before reading the temperature certain settings
for oversampling ratio, sample frequency, EMR.TRACKX, MR.TRACKTIM are
applied. The initial settings are reapplied at the end of temperature
reading. Current support is not integrated with trigger buffers.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 252 ++++++++++++++++++++++++++++-
 1 file changed, 247 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 8f8fef42de84..67ced1369754 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -26,9 +26,12 @@
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/regulator/consumer.h>
 
+#include <dt-bindings/iio/adc/at91-sama5d2_adc.h>
+
 struct at91_adc_reg_layout {
 /* Control Register */
 	u16				CR;
@@ -73,10 +76,13 @@ struct at91_adc_reg_layout {
 /* Startup Time */
 #define	AT91_SAMA5D2_MR_STARTUP(v)	((v) << 16)
 #define AT91_SAMA5D2_MR_STARTUP_MASK	GENMASK(19, 16)
+/* Minimum startup time for temperature sensor */
+#define AT91_SAMA5D2_MR_STARTUP_TS_MIN	(50)
 /* Analog Change */
 #define	AT91_SAMA5D2_MR_ANACH		BIT(23)
 /* Tracking Time */
 #define	AT91_SAMA5D2_MR_TRACKTIM(v)	((v) << 24)
+#define	AT91_SAMA5D2_MR_TRACKTIM_TS	6
 #define	AT91_SAMA5D2_MR_TRACKTIM_MAX	0xf
 /* Transfer Time */
 #define	AT91_SAMA5D2_MR_TRANSFER(v)	((v) << 28)
@@ -149,6 +155,9 @@ struct at91_adc_reg_layout {
 #define AT91_SAMA5D2_TRACKX_MASK		GENMASK(23, 22)
 #define AT91_SAMA5D2_TRACKX(x)			(((x) << 22) & \
 						 AT91_SAMA5D2_TRACKX_MASK)
+/* TRACKX for temperature sensor. */
+#define AT91_SAMA5D2_TRACKX_TS			(1)
+
 /* Extended Mode Register - Averaging on single trigger event */
 #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
 
@@ -164,6 +173,8 @@ struct at91_adc_reg_layout {
 	u16				ACR;
 /* Analog Control Register - Pen detect sensitivity mask */
 #define AT91_SAMA5D2_ACR_PENDETSENS_MASK        GENMASK(1, 0)
+/* Analog Control Register - Source last channel */
+#define AT91_SAMA5D2_ACR_SRCLCH		BIT(16)
 
 /* Touchscreen Mode Register */
 	u16				TSMR;
@@ -231,6 +242,10 @@ struct at91_adc_reg_layout {
 	u16				WPSR;
 /* Version Register */
 	u16				VERSION;
+/* Temperature Sensor Mode Register */
+	u16				TEMPMR;
+/* Temperature Sensor Mode - Temperature sensor on */
+#define AT91_SAMA5D2_TEMPMR_TEMPON	BIT(0)
 };
 
 static const struct at91_adc_reg_layout sama5d2_layout = {
@@ -285,6 +300,7 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
 	.EOC_IDR =		0x38,
 	.EOC_IMR =		0x3c,
 	.EOC_ISR =		0x40,
+	.TEMPMR =		0x44,
 	.OVER =			0x4c,
 	.EMR =			0x50,
 	.CWR =			0x54,
@@ -390,6 +406,23 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
 		.datasheet_name = name,					\
 	}
 
+#define AT91_SAMA5D2_CHAN_TEMP(num, name, addr)				\
+	{								\
+		.type = IIO_TEMP,					\
+		.channel = num,						\
+		.address =  addr,					\
+		.scan_index = num,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 16,					\
+			.storagebits = 16,				\
+		},							\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),	\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_PROCESSED) | \
+					BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),\
+		.datasheet_name = name,					\
+	}
+
 #define at91_adc_readl(st, reg)						\
 	readl_relaxed((st)->base + (st)->soc_info.platform->layout->reg)
 #define at91_adc_read_chan(st, reg)					\
@@ -413,6 +446,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
  * @osr_mask:		oversampling ratio bitmask on EMR register
  * @osr_vals:		available oversampling rates
  * @chan_realbits:	realbits for registered channels
+ * @temp_chan:		temperature channel index
+ * @temp_sensor:	temperature sensor supported
  */
 struct at91_adc_platform {
 	const struct at91_adc_reg_layout	*layout;
@@ -427,20 +462,54 @@ struct at91_adc_platform {
 	unsigned int				osr_mask;
 	unsigned int				osr_vals;
 	unsigned int				chan_realbits;
+	unsigned int				temp_chan;
+	bool					temp_sensor;
+};
+
+/**
+ * struct at91_adc_temp_sensor_clb - at91-sama5d2 temperature sensor
+ * calibration data structure
+ * @p1: P1 calibration temperature
+ * @p4: P4 calibration voltage
+ * @p6: P6 calibration voltage
+ */
+struct at91_adc_temp_sensor_clb {
+	u32 p1;
+	u32 p4;
+	u32 p6;
+};
+
+/**
+ * enum at91_adc_ts_clb_idx - calibration indexes in NVMEM buffer
+ * @AT91_ADC_TS_CLB_IDX_P1: index for P1
+ * @AT91_ADC_TS_CLB_IDX_P4: index for P4
+ * @AT91_ADC_TS_CLB_IDX_P6: index for P6
+ * @AT91_ADC_TS_CLB_IDX_MAX: max index for temperature calibration packet in OTP
+ */
+enum at91_adc_ts_clb_idx {
+	AT91_ADC_TS_CLB_IDX_P1 = 2,
+	AT91_ADC_TS_CLB_IDX_P4 = 5,
+	AT91_ADC_TS_CLB_IDX_P6 = 7,
+	AT91_ADC_TS_CLB_IDX_MAX = 19,
 };
 
+/* Temperature sensor calibration - Vtemp voltage sensitivity to temperature. */
+#define AT91_ADC_TS_VTEMP_DT		(2080U)
+
 /**
  * struct at91_adc_soc_info - at91-sama5d2 soc information struct
  * @startup_time:	device startup time
  * @min_sample_rate:	minimum sample rate in Hz
  * @max_sample_rate:	maximum sample rate in Hz
  * @platform:		pointer to the platform structure
+ * @temp_sensor_clb:	temperature sensor calibration data structure
  */
 struct at91_adc_soc_info {
 	unsigned			startup_time;
 	unsigned			min_sample_rate;
 	unsigned			max_sample_rate;
 	const struct at91_adc_platform	*platform;
+	struct at91_adc_temp_sensor_clb	temp_sensor_clb;
 };
 
 struct at91_adc_trigger {
@@ -488,6 +557,20 @@ struct at91_adc_touch {
 	struct work_struct		workq;
 };
 
+/**
+ * struct at91_adc_temp - at91-sama5d2 temperature information structure
+ * @sample_period_val:	sample period value
+ * @saved_sample_rate:	saved sample rate
+ * @saved_oversampling:	saved oversampling
+ * @init:		temperature sensor initialized
+ */
+struct at91_adc_temp {
+	u16				sample_period_val;
+	u16				saved_sample_rate;
+	u16				saved_oversampling;
+	bool				init;
+};
+
 /*
  * Buffer size requirements:
  * No channels * bytes_per_channel(2) + timestamp bytes (8)
@@ -515,6 +598,7 @@ struct at91_adc_state {
 	wait_queue_head_t		wq_data_available;
 	struct at91_adc_dma		dma_st;
 	struct at91_adc_touch		touch_st;
+	struct at91_adc_temp		temp_st;
 	struct iio_dev			*indio_dev;
 	/* Ensure naturally aligned timestamp */
 	u16				buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
@@ -604,6 +688,7 @@ static const struct iio_chan_spec at91_sama7g5_adc_channels[] = {
 	AT91_SAMA5D2_CHAN_DIFF(22, 12, 13, 0x90),
 	AT91_SAMA5D2_CHAN_DIFF(23, 14, 15, 0x98),
 	IIO_CHAN_SOFT_TIMESTAMP(24),
+	AT91_SAMA5D2_CHAN_TEMP(AT91_SAMA7G5_ADC_TEMP_CHANNEL, "temp", 0xdc),
 };
 
 static const struct at91_adc_platform sama5d2_platform = {
@@ -637,10 +722,13 @@ static const struct at91_adc_platform sama7g5_platform = {
 	.adc_channels = &at91_sama7g5_adc_channels,
 #define AT91_SAMA7G5_SINGLE_CHAN_CNT	16
 #define AT91_SAMA7G5_DIFF_CHAN_CNT	8
+#define AT91_SAMA7G5_TEMP_CHAN_CNT	1
 	.nr_channels = AT91_SAMA7G5_SINGLE_CHAN_CNT +
-		       AT91_SAMA7G5_DIFF_CHAN_CNT,
+		       AT91_SAMA7G5_DIFF_CHAN_CNT +
+		       AT91_SAMA7G5_TEMP_CHAN_CNT,
 #define AT91_SAMA7G5_MAX_CHAN_IDX	(AT91_SAMA7G5_SINGLE_CHAN_CNT + \
-					AT91_SAMA7G5_DIFF_CHAN_CNT)
+					AT91_SAMA7G5_DIFF_CHAN_CNT + \
+					AT91_SAMA7G5_TEMP_CHAN_CNT)
 	.max_channels = ARRAY_SIZE(at91_sama7g5_adc_channels),
 	.max_index = AT91_SAMA7G5_MAX_CHAN_IDX,
 #define AT91_SAMA7G5_HW_TRIG_CNT	3
@@ -652,6 +740,8 @@ static const struct at91_adc_platform sama7g5_platform = {
 		    BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
 		    BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
 	.chan_realbits = 16,
+	.temp_sensor = true,
+	.temp_chan = AT91_SAMA7G5_ADC_TEMP_CHANNEL,
 };
 
 static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
@@ -1193,7 +1283,8 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
 			continue;
 		/* these channel types cannot be handled by this trigger */
 		if (chan->type == IIO_POSITIONRELATIVE ||
-		    chan->type == IIO_PRESSURE)
+		    chan->type == IIO_PRESSURE ||
+		    chan->type == IIO_TEMP)
 			continue;
 
 		at91_adc_cor(st, chan);
@@ -1235,7 +1326,8 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
 			continue;
 		/* these channel types are virtual, no need to do anything */
 		if (chan->type == IIO_POSITIONRELATIVE ||
-		    chan->type == IIO_PRESSURE)
+		    chan->type == IIO_PRESSURE ||
+		    chan->type == IIO_TEMP)
 			continue;
 
 		at91_adc_writel(st, CHDR, BIT(chan->channel));
@@ -1617,12 +1709,19 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		return ret;
 	}
 
-	/* in this case we have a voltage channel */
+	/* in this case we have a voltage or temperature channel */
 
 	st->chan = chan;
 
 	at91_adc_cor(st, chan);
 	at91_adc_writel(st, CHER, BIT(chan->channel));
+	/*
+	 * TEMPMR.TEMPON needs to update after CHER otherwise if none
+	 * of the channels are enabled and TEMPMR.TEMPON = 1 will
+	 * trigger DRDY interruption while preparing for temperature read.
+	 */
+	if (chan->type == IIO_TEMP)
+		at91_adc_writel(st, TEMPMR, AT91_SAMA5D2_TEMPMR_TEMPON);
 	at91_adc_eoc_ena(st, chan->channel);
 	at91_adc_writel(st, CR, AT91_SAMA5D2_CR_START);
 
@@ -1642,6 +1741,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 	}
 
 	at91_adc_eoc_dis(st, st->chan->channel);
+	if (chan->type == IIO_TEMP)
+		at91_adc_writel(st, TEMPMR, 0U);
 	at91_adc_writel(st, CHDR, BIT(chan->channel));
 
 	/* Needed to ACK the DRDY interruption */
@@ -1654,6 +1755,91 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static void at91_adc_temp_sensor_configure(struct at91_adc_state *st,
+					   bool start)
+{
+	u32 sample_rate, oversampling_ratio;
+	u32 startup_time, tracktim, trackx;
+
+	if (start) {
+		/*
+		 * Configure the sensor for best accuracy: 10MHz frequency,
+		 * oversampling rate of 256, tracktim=0xf and trackx=1.
+		 */
+		sample_rate = 10000000U;
+		oversampling_ratio = AT91_OSR_256SAMPLES;
+		startup_time = AT91_SAMA5D2_MR_STARTUP_TS_MIN;
+		tracktim = AT91_SAMA5D2_MR_TRACKTIM_TS;
+		trackx = AT91_SAMA5D2_TRACKX_TS;
+
+		st->temp_st.saved_sample_rate = st->current_sample_rate;
+		st->temp_st.saved_oversampling = st->oversampling_ratio;
+	} else {
+		/* Go back to previous settings. */
+		sample_rate = st->temp_st.saved_sample_rate;
+		oversampling_ratio = st->temp_st.saved_oversampling;
+		startup_time = st->soc_info.startup_time;
+		tracktim = 0;
+		trackx = 0;
+	}
+
+	at91_adc_setup_samp_freq(st->indio_dev, sample_rate, startup_time,
+				 tracktim);
+	at91_adc_config_emr(st, oversampling_ratio, trackx);
+}
+
+static int at91_adc_read_temp(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int *val)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
+	u64 div1, div2;
+	u32 tmp;
+	int ret, vbg, vtemp;
+
+	if (!st->soc_info.platform->temp_sensor || !st->temp_st.init)
+		return -EPERM;
+
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+
+	mutex_lock(&st->lock);
+
+	at91_adc_temp_sensor_configure(st, true);
+
+	/* Read VBG. */
+	tmp = at91_adc_readl(st, ACR);
+	tmp |= AT91_SAMA5D2_ACR_SRCLCH;
+	at91_adc_writel(st, ACR, tmp);
+	ret = at91_adc_read_info_raw(indio_dev, chan, &vbg, false);
+	if (ret < 0)
+		goto unlock;
+
+	/* Read VTEMP. */
+	tmp &= ~AT91_SAMA5D2_ACR_SRCLCH;
+	at91_adc_writel(st, ACR, tmp);
+	ret = at91_adc_read_info_raw(indio_dev, chan, &vtemp, false);
+
+unlock:
+	/* Revert previous settings. */
+	at91_adc_temp_sensor_configure(st, false);
+	mutex_unlock(&st->lock);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Temp[milli] = p1[milli] + (vtemp * clb->p6 - clb->p4 * vbg)/
+	 *			     (vbg * AT91_ADC_TS_VTEMP_DT)
+	 */
+	div1 = DIV_ROUND_CLOSEST_ULL(((u64)vtemp * clb->p6), vbg);
+	div1 = DIV_ROUND_CLOSEST_ULL((div1 * 1000), AT91_ADC_TS_VTEMP_DT);
+	div2 = DIV_ROUND_CLOSEST_ULL((u64)clb->p4, AT91_ADC_TS_VTEMP_DT);
+	div2 *= 1000;
+	*val = clb->p1 + (int)div1 - (int)div2;
+
+	return ret;
+}
+
 static int at91_adc_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val, int *val2, long mask)
@@ -1671,6 +1857,11 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 		*val2 = chan->scan_type.realbits;
 		return IIO_VAL_FRACTIONAL_LOG2;
 
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type != IIO_TEMP)
+			return -EINVAL;
+		return at91_adc_read_temp(indio_dev, chan, val);
+
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		mutex_lock(&st->lock);
 		*val = at91_adc_get_sample_freq(st);
@@ -1987,6 +2178,55 @@ static int at91_adc_buffer_and_trigger_init(struct device *dev,
 	return 0;
 }
 
+static void at91_adc_temp_sensor_init(struct at91_adc_state *st,
+				      struct device *dev)
+{
+	struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
+	struct nvmem_cell *temp_calib;
+	u32 *buf;
+	size_t len;
+
+	if (!st->soc_info.platform->temp_sensor)
+		return;
+
+	st->temp_st.init = false;
+
+	/* Get the calibration data from NVMEM. */
+	temp_calib = devm_nvmem_cell_get(dev, "temperature_calib");
+	if (IS_ERR(temp_calib)) {
+		if (PTR_ERR(temp_calib) != -ENOENT)
+			dev_err(dev, "Failed to get temperature_calib cell!\n");
+		return;
+	}
+
+	buf = nvmem_cell_read(temp_calib, &len);
+	if (IS_ERR(buf)) {
+		dev_err(dev, "Failed to read calibration data!\n");
+		return;
+	}
+	if (len < AT91_ADC_TS_CLB_IDX_MAX * 4) {
+		dev_err(dev, "Invalid calibration data!\n");
+		goto free_buf;
+	}
+
+	/* Store calibration data for later use. */
+	clb->p1 = buf[AT91_ADC_TS_CLB_IDX_P1];
+	clb->p4 = buf[AT91_ADC_TS_CLB_IDX_P4];
+	clb->p6 = buf[AT91_ADC_TS_CLB_IDX_P6];
+
+	/*
+	 * We prepare here the conversion to milli and also add constant
+	 * factor (5 degrees Celsius) to p1 here to avoid doing it on
+	 * hotpath.
+	 */
+	clb->p1 = clb->p1 * 1000 + 5000;
+
+	st->temp_st.init = true;
+
+free_buf:
+	kfree(buf);
+}
+
 static int at91_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
@@ -2120,6 +2360,8 @@ static int at91_adc_probe(struct platform_device *pdev)
 	if (ret)
 		goto vref_disable;
 
+	at91_adc_temp_sensor_init(st, &pdev->dev);
+
 	at91_adc_hw_init(indio_dev);
 
 	platform_set_drvdata(pdev, indio_dev);
-- 
2.34.1

Re: [PATCH 13/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:10 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> SAMA7G5 has a temperature sensor embedded that is connected to channel 31
> of ADC. Temperature sensor provides 2 outputs: VTEMP and VBG. VTEMP is
> proportional to the absolute temperature voltage, VBG is quasi-temperature
> independent voltage. The calibration data for temperature sensor are
> retrieved from OTP memory specific to SAMA7G5. The formula to calculate
> the junction temperature is as follows:
> 
> P1 + (Vref * (Vtemp - P6 - P4 * Vbg)) / (Vbg * VTEMP_DT)
> 
> where Pi are calibration data retrieved from OTP memory.
> 
> For better resolution before reading the temperature certain settings
> for oversampling ratio, sample frequency, EMR.TRACKX, MR.TRACKTIM are
> applied. The initial settings are reapplied at the end of temperature
> reading. Current support is not integrated with trigger buffers.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

This is a complex driver, so I got a bit lost figuring out what happens
about buffered capture of this channel.  What ends up in the buffer?
Most processed channels are not useable with that mode (and hence have
a scanindex == -1 which ensures they aren't exposed as an option for
userspace to enable).

Other comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 252 ++++++++++++++++++++++++++++-
>  1 file changed, 247 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 8f8fef42de84..67ced1369754 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -26,9 +26,12 @@
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <dt-bindings/iio/adc/at91-sama5d2_adc.h>
> +
>  struct at91_adc_reg_layout {
>  /* Control Register */
>  	u16				CR;
> @@ -73,10 +76,13 @@ struct at91_adc_reg_layout {
>  /* Startup Time */
>  #define	AT91_SAMA5D2_MR_STARTUP(v)	((v) << 16)
>  #define AT91_SAMA5D2_MR_STARTUP_MASK	GENMASK(19, 16)
> +/* Minimum startup time for temperature sensor */
> +#define AT91_SAMA5D2_MR_STARTUP_TS_MIN	(50)
>  /* Analog Change */
>  #define	AT91_SAMA5D2_MR_ANACH		BIT(23)
>  /* Tracking Time */
>  #define	AT91_SAMA5D2_MR_TRACKTIM(v)	((v) << 24)
> +#define	AT91_SAMA5D2_MR_TRACKTIM_TS	6
>  #define	AT91_SAMA5D2_MR_TRACKTIM_MAX	0xf
>  /* Transfer Time */
>  #define	AT91_SAMA5D2_MR_TRANSFER(v)	((v) << 28)
> @@ -149,6 +155,9 @@ struct at91_adc_reg_layout {
>  #define AT91_SAMA5D2_TRACKX_MASK		GENMASK(23, 22)
>  #define AT91_SAMA5D2_TRACKX(x)			(((x) << 22) & \
>  						 AT91_SAMA5D2_TRACKX_MASK)
> +/* TRACKX for temperature sensor. */
> +#define AT91_SAMA5D2_TRACKX_TS			(1)
> +
>  /* Extended Mode Register - Averaging on single trigger event */
>  #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
>  
> @@ -164,6 +173,8 @@ struct at91_adc_reg_layout {
>  	u16				ACR;
>  /* Analog Control Register - Pen detect sensitivity mask */
>  #define AT91_SAMA5D2_ACR_PENDETSENS_MASK        GENMASK(1, 0)
> +/* Analog Control Register - Source last channel */
> +#define AT91_SAMA5D2_ACR_SRCLCH		BIT(16)
>  
>  /* Touchscreen Mode Register */
>  	u16				TSMR;
> @@ -231,6 +242,10 @@ struct at91_adc_reg_layout {
>  	u16				WPSR;
>  /* Version Register */
>  	u16				VERSION;
> +/* Temperature Sensor Mode Register */
> +	u16				TEMPMR;
> +/* Temperature Sensor Mode - Temperature sensor on */
> +#define AT91_SAMA5D2_TEMPMR_TEMPON	BIT(0)
>  };
>  
>  static const struct at91_adc_reg_layout sama5d2_layout = {
> @@ -285,6 +300,7 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>  	.EOC_IDR =		0x38,
>  	.EOC_IMR =		0x3c,
>  	.EOC_ISR =		0x40,
> +	.TEMPMR =		0x44,
>  	.OVER =			0x4c,
>  	.EMR =			0x50,
>  	.CWR =			0x54,
> @@ -390,6 +406,23 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>  		.datasheet_name = name,					\
>  	}
>  
> +#define AT91_SAMA5D2_CHAN_TEMP(num, name, addr)				\
> +	{								\
> +		.type = IIO_TEMP,					\
> +		.channel = num,						\
> +		.address =  addr,					\
> +		.scan_index = num,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\

So this is unusual.  Normally a processed channel isn't suitable for buffered
capture because they tend not to fit in nice compact storage.  In this case
what actually goes in the buffer?  Perhaps a comment would be useful here.

> +		},							\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),	\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_PROCESSED) | \
> +					BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),\
> +		.datasheet_name = name,					\
> +	}
> +
>  #define at91_adc_readl(st, reg)						\
>  	readl_relaxed((st)->base + (st)->soc_info.platform->layout->reg)
>  #define at91_adc_read_chan(st, reg)					\
> @@ -413,6 +446,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>   * @osr_mask:		oversampling ratio bitmask on EMR register
>   * @osr_vals:		available oversampling rates
>   * @chan_realbits:	realbits for registered channels
> + * @temp_chan:		temperature channel index
> + * @temp_sensor:	temperature sensor supported
>   */
>  struct at91_adc_platform {
>  	const struct at91_adc_reg_layout	*layout;
> @@ -427,20 +462,54 @@ struct at91_adc_platform {
>  	unsigned int				osr_mask;
>  	unsigned int				osr_vals;
>  	unsigned int				chan_realbits;
> +	unsigned int				temp_chan;
> +	bool					temp_sensor;
> +};
> +
> +/**
> + * struct at91_adc_temp_sensor_clb - at91-sama5d2 temperature sensor
> + * calibration data structure
> + * @p1: P1 calibration temperature
> + * @p4: P4 calibration voltage
> + * @p6: P6 calibration voltage
> + */
> +struct at91_adc_temp_sensor_clb {
> +	u32 p1;
> +	u32 p4;
> +	u32 p6;
> +};
> +
> +/**
> + * enum at91_adc_ts_clb_idx - calibration indexes in NVMEM buffer
> + * @AT91_ADC_TS_CLB_IDX_P1: index for P1
> + * @AT91_ADC_TS_CLB_IDX_P4: index for P4
> + * @AT91_ADC_TS_CLB_IDX_P6: index for P6
> + * @AT91_ADC_TS_CLB_IDX_MAX: max index for temperature calibration packet in OTP
> + */
> +enum at91_adc_ts_clb_idx {
> +	AT91_ADC_TS_CLB_IDX_P1 = 2,
> +	AT91_ADC_TS_CLB_IDX_P4 = 5,
> +	AT91_ADC_TS_CLB_IDX_P6 = 7,
> +	AT91_ADC_TS_CLB_IDX_MAX = 19,
>  };
>  
> +/* Temperature sensor calibration - Vtemp voltage sensitivity to temperature. */
> +#define AT91_ADC_TS_VTEMP_DT		(2080U)
> +
>  /**
>   * struct at91_adc_soc_info - at91-sama5d2 soc information struct
>   * @startup_time:	device startup time
>   * @min_sample_rate:	minimum sample rate in Hz
>   * @max_sample_rate:	maximum sample rate in Hz
>   * @platform:		pointer to the platform structure
> + * @temp_sensor_clb:	temperature sensor calibration data structure
>   */
>  struct at91_adc_soc_info {
>  	unsigned			startup_time;
>  	unsigned			min_sample_rate;
>  	unsigned			max_sample_rate;
>  	const struct at91_adc_platform	*platform;
> +	struct at91_adc_temp_sensor_clb	temp_sensor_clb;
>  };
>  
>  struct at91_adc_trigger {
> @@ -488,6 +557,20 @@ struct at91_adc_touch {
>  	struct work_struct		workq;
>  };
>  
> +/**
> + * struct at91_adc_temp - at91-sama5d2 temperature information structure
> + * @sample_period_val:	sample period value
> + * @saved_sample_rate:	saved sample rate
> + * @saved_oversampling:	saved oversampling
> + * @init:		temperature sensor initialized
> + */
> +struct at91_adc_temp {
> +	u16				sample_period_val;
> +	u16				saved_sample_rate;
> +	u16				saved_oversampling;
> +	bool				init;
> +};
> +
>  /*
>   * Buffer size requirements:
>   * No channels * bytes_per_channel(2) + timestamp bytes (8)
> @@ -515,6 +598,7 @@ struct at91_adc_state {
>  	wait_queue_head_t		wq_data_available;
>  	struct at91_adc_dma		dma_st;
>  	struct at91_adc_touch		touch_st;
> +	struct at91_adc_temp		temp_st;
>  	struct iio_dev			*indio_dev;
>  	/* Ensure naturally aligned timestamp */
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
> @@ -604,6 +688,7 @@ static const struct iio_chan_spec at91_sama7g5_adc_channels[] = {
>  	AT91_SAMA5D2_CHAN_DIFF(22, 12, 13, 0x90),
>  	AT91_SAMA5D2_CHAN_DIFF(23, 14, 15, 0x98),
>  	IIO_CHAN_SOFT_TIMESTAMP(24),
> +	AT91_SAMA5D2_CHAN_TEMP(AT91_SAMA7G5_ADC_TEMP_CHANNEL, "temp", 0xdc),
>  };
>  
>  static const struct at91_adc_platform sama5d2_platform = {
> @@ -637,10 +722,13 @@ static const struct at91_adc_platform sama7g5_platform = {
>  	.adc_channels = &at91_sama7g5_adc_channels,
>  #define AT91_SAMA7G5_SINGLE_CHAN_CNT	16
>  #define AT91_SAMA7G5_DIFF_CHAN_CNT	8
> +#define AT91_SAMA7G5_TEMP_CHAN_CNT	1
>  	.nr_channels = AT91_SAMA7G5_SINGLE_CHAN_CNT +
> -		       AT91_SAMA7G5_DIFF_CHAN_CNT,
> +		       AT91_SAMA7G5_DIFF_CHAN_CNT +
> +		       AT91_SAMA7G5_TEMP_CHAN_CNT,
>  #define AT91_SAMA7G5_MAX_CHAN_IDX	(AT91_SAMA7G5_SINGLE_CHAN_CNT + \
> -					AT91_SAMA7G5_DIFF_CHAN_CNT)
> +					AT91_SAMA7G5_DIFF_CHAN_CNT + \
> +					AT91_SAMA7G5_TEMP_CHAN_CNT)
>  	.max_channels = ARRAY_SIZE(at91_sama7g5_adc_channels),
>  	.max_index = AT91_SAMA7G5_MAX_CHAN_IDX,
>  #define AT91_SAMA7G5_HW_TRIG_CNT	3
> @@ -652,6 +740,8 @@ static const struct at91_adc_platform sama7g5_platform = {
>  		    BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
>  		    BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
>  	.chan_realbits = 16,
> +	.temp_sensor = true,
> +	.temp_chan = AT91_SAMA7G5_ADC_TEMP_CHANNEL,
>  };
>  
>  static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
> @@ -1193,7 +1283,8 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
>  			continue;
>  		/* these channel types cannot be handled by this trigger */
>  		if (chan->type == IIO_POSITIONRELATIVE ||
> -		    chan->type == IIO_PRESSURE)
> +		    chan->type == IIO_PRESSURE ||
> +		    chan->type == IIO_TEMP)
>  			continue;
>  
>  		at91_adc_cor(st, chan);
> @@ -1235,7 +1326,8 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>  			continue;
>  		/* these channel types are virtual, no need to do anything */
>  		if (chan->type == IIO_POSITIONRELATIVE ||
> -		    chan->type == IIO_PRESSURE)
> +		    chan->type == IIO_PRESSURE ||
> +		    chan->type == IIO_TEMP)
>  			continue;
>  
>  		at91_adc_writel(st, CHDR, BIT(chan->channel));
> @@ -1617,12 +1709,19 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		return ret;
>  	}
>  
> -	/* in this case we have a voltage channel */
> +	/* in this case we have a voltage or temperature channel */
>  
>  	st->chan = chan;
>  
>  	at91_adc_cor(st, chan);
>  	at91_adc_writel(st, CHER, BIT(chan->channel));
> +	/*
> +	 * TEMPMR.TEMPON needs to update after CHER otherwise if none
> +	 * of the channels are enabled and TEMPMR.TEMPON = 1 will
> +	 * trigger DRDY interruption while preparing for temperature read.
> +	 */
> +	if (chan->type == IIO_TEMP)
> +		at91_adc_writel(st, TEMPMR, AT91_SAMA5D2_TEMPMR_TEMPON);
>  	at91_adc_eoc_ena(st, chan->channel);
>  	at91_adc_writel(st, CR, AT91_SAMA5D2_CR_START);
>  
> @@ -1642,6 +1741,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  	}
>  
>  	at91_adc_eoc_dis(st, st->chan->channel);
> +	if (chan->type == IIO_TEMP)
> +		at91_adc_writel(st, TEMPMR, 0U);
>  	at91_adc_writel(st, CHDR, BIT(chan->channel));
>  
>  	/* Needed to ACK the DRDY interruption */
> @@ -1654,6 +1755,91 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static void at91_adc_temp_sensor_configure(struct at91_adc_state *st,
> +					   bool start)
> +{
> +	u32 sample_rate, oversampling_ratio;
> +	u32 startup_time, tracktim, trackx;
> +
> +	if (start) {
> +		/*
> +		 * Configure the sensor for best accuracy: 10MHz frequency,
> +		 * oversampling rate of 256, tracktim=0xf and trackx=1.
> +		 */
> +		sample_rate = 10000000U;
> +		oversampling_ratio = AT91_OSR_256SAMPLES;
> +		startup_time = AT91_SAMA5D2_MR_STARTUP_TS_MIN;
> +		tracktim = AT91_SAMA5D2_MR_TRACKTIM_TS;
> +		trackx = AT91_SAMA5D2_TRACKX_TS;
> +
> +		st->temp_st.saved_sample_rate = st->current_sample_rate;
> +		st->temp_st.saved_oversampling = st->oversampling_ratio;
> +	} else {
> +		/* Go back to previous settings. */
> +		sample_rate = st->temp_st.saved_sample_rate;
> +		oversampling_ratio = st->temp_st.saved_oversampling;
> +		startup_time = st->soc_info.startup_time;
> +		tracktim = 0;
> +		trackx = 0;
> +	}
> +
> +	at91_adc_setup_samp_freq(st->indio_dev, sample_rate, startup_time,
> +				 tracktim);
> +	at91_adc_config_emr(st, oversampling_ratio, trackx);
> +}
> +
> +static int at91_adc_read_temp(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
> +	u64 div1, div2;
> +	u32 tmp;
> +	int ret, vbg, vtemp;
> +
> +	if (!st->soc_info.platform->temp_sensor || !st->temp_st.init)
> +		return -EPERM;

You shouldn't register the channel if it's not readable.  Hence this
should never happen.

> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;

claim_direct - never check the buffer is enabled as it's racy - nothing
stops it being enabled immediately after you check it (only exception
is before interfaces are exposed such as _resume() where it should be safe
to check).
Which fixes the locking issue in the previous patch.  Move the
iio_device_claim_direct() and mutex_lock() to all callers and you won't
have a problem any more.


> +
> +	mutex_lock(&st->lock);
> +
> +	at91_adc_temp_sensor_configure(st, true);
> +
> +	/* Read VBG. */
> +	tmp = at91_adc_readl(st, ACR);
> +	tmp |= AT91_SAMA5D2_ACR_SRCLCH;
> +	at91_adc_writel(st, ACR, tmp);
> +	ret = at91_adc_read_info_raw(indio_dev, chan, &vbg, false);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	/* Read VTEMP. */
> +	tmp &= ~AT91_SAMA5D2_ACR_SRCLCH;
> +	at91_adc_writel(st, ACR, tmp);
> +	ret = at91_adc_read_info_raw(indio_dev, chan, &vtemp, false);
> +
> +unlock:
> +	/* Revert previous settings. */
> +	at91_adc_temp_sensor_configure(st, false);
> +	mutex_unlock(&st->lock);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Temp[milli] = p1[milli] + (vtemp * clb->p6 - clb->p4 * vbg)/
> +	 *			     (vbg * AT91_ADC_TS_VTEMP_DT)
> +	 */
> +	div1 = DIV_ROUND_CLOSEST_ULL(((u64)vtemp * clb->p6), vbg);
> +	div1 = DIV_ROUND_CLOSEST_ULL((div1 * 1000), AT91_ADC_TS_VTEMP_DT);
> +	div2 = DIV_ROUND_CLOSEST_ULL((u64)clb->p4, AT91_ADC_TS_VTEMP_DT);
> +	div2 *= 1000;
> +	*val = clb->p1 + (int)div1 - (int)div2;
> +
> +	return ret;
> +}
> +
>  static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int *val, int *val2, long mask)
> @@ -1671,6 +1857,11 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
> +	case IIO_CHAN_INFO_PROCESSED:
> +		if (chan->type != IIO_TEMP)
> +			return -EINVAL;
> +		return at91_adc_read_temp(indio_dev, chan, val);
> +
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		mutex_lock(&st->lock);
>  		*val = at91_adc_get_sample_freq(st);
> @@ -1987,6 +2178,55 @@ static int at91_adc_buffer_and_trigger_init(struct device *dev,
>  	return 0;
>  }
>  
> +static void at91_adc_temp_sensor_init(struct at91_adc_state *st,
> +				      struct device *dev)
> +{
> +	struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
> +	struct nvmem_cell *temp_calib;
> +	u32 *buf;
> +	size_t len;
> +
> +	if (!st->soc_info.platform->temp_sensor)
> +		return;
> +
> +	st->temp_st.init = false;

that structure is kzalloc so this isn't really needed unless you think it's
providing 'documentation'.

> +
> +	/* Get the calibration data from NVMEM. */
> +	temp_calib = devm_nvmem_cell_get(dev, "temperature_calib");
> +	if (IS_ERR(temp_calib)) {
> +		if (PTR_ERR(temp_calib) != -ENOENT)
> +			dev_err(dev, "Failed to get temperature_calib cell!\n");
> +		return;
> +	}
> +
> +	buf = nvmem_cell_read(temp_calib, &len);
> +	if (IS_ERR(buf)) {
> +		dev_err(dev, "Failed to read calibration data!\n");
> +		return;
> +	}
> +	if (len < AT91_ADC_TS_CLB_IDX_MAX * 4) {
> +		dev_err(dev, "Invalid calibration data!\n");
> +		goto free_buf;
> +	}
> +
> +	/* Store calibration data for later use. */
> +	clb->p1 = buf[AT91_ADC_TS_CLB_IDX_P1];
> +	clb->p4 = buf[AT91_ADC_TS_CLB_IDX_P4];
> +	clb->p6 = buf[AT91_ADC_TS_CLB_IDX_P6];
> +
> +	/*
> +	 * We prepare here the conversion to milli and also add constant
> +	 * factor (5 degrees Celsius) to p1 here to avoid doing it on
> +	 * hotpath.
> +	 */
> +	clb->p1 = clb->p1 * 1000 + 5000;
> +
> +	st->temp_st.init = true;
> +
> +free_buf:
> +	kfree(buf);
> +}
> +

Re: [PATCH 13/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[Claudiu.Beznea]

On 11.06.2022 21:15, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:10 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> SAMA7G5 has a temperature sensor embedded that is connected to channel 31
>> of ADC. Temperature sensor provides 2 outputs: VTEMP and VBG. VTEMP is
>> proportional to the absolute temperature voltage, VBG is quasi-temperature
>> independent voltage. The calibration data for temperature sensor are
>> retrieved from OTP memory specific to SAMA7G5. The formula to calculate
>> the junction temperature is as follows:
>>
>> P1 + (Vref * (Vtemp - P6 - P4 * Vbg)) / (Vbg * VTEMP_DT)
>>
>> where Pi are calibration data retrieved from OTP memory.
>>
>> For better resolution before reading the temperature certain settings
>> for oversampling ratio, sample frequency, EMR.TRACKX, MR.TRACKTIM are
>> applied. The initial settings are reapplied at the end of temperature
>> reading. Current support is not integrated with trigger buffers.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> This is a complex driver, so I got a bit lost figuring out what happens
> about buffered capture of this channel.  What ends up in the buffer?

With this implementation nothing should end up in the buffer as the patch
skips channel enable/disable if its about an IIO_TEMP channel (see
functions functions at91_adc_buffer_prepare(),
at91_adc_buffer_postdisable()). More details above.

According to datasheet the temperature channel behaves the same as the
other channels. On temperature channel are multiplexed both VBG and VTEMP.

          `
          | \      +-----+
VBG   --->|  |ch31 |     |
Vtemp --->|  |---->| ADC |
          |  /     +-----+
          | /
          .

And both are needed to be measured in order to determine the correct in SoC
temperature.

At a moment of time only one of these could be measured, the selection
being done with bit SRCLCH bit of ACR register.

According to datasheet there is no special treatment for channel 31 of ADC
in case triggers are enabled. So, in case of a buffer capture the buffer
for channel 31 will contain either the converted value for VBG or VTEMP
(depending on the value of bit SRCLCH bit of ACR register), if channel 31
is enabled for that. But on this implementation we skip the enable/disable
of IIO_TEMP channels (functions at91_adc_buffer_prepare(),
at91_adc_buffer_postdisable()).

Hardware implements a special mechanism for reading the temperature when
triggers are enabled as follows: the hardware provides a RTC trigger wich
fires every 1 second and starts a conversion on channel 31. This could
permit to have a temperature value once every 2 seconds (in the 1st RTC
trigger VBG could be read, in the 2nd RTC trigger Vtemp could be read, or
the other way arround). For this, configuration needs to be done propertly
in the RTC memory spaces and on Linux side something should be implemented
for the interaction b/w RTC and IIO subsystems. But this is for future
development.

> Most processed channels are not useable with that mode (and hence have
> a scanindex == -1 which ensures they aren't exposed as an option for
> userspace to enable).

OK, I haven't been aware of that. I only did some basic research on how
this could be achieved. As we are using the thermal support on SAMA7G5 with
driver at drivers/thermal/thermal-generic-adc.c which reads processed value
at periodic intervals one idea was to take advantage of the RTC trigger
mechanism for channel 31 and have the converted values of VBG and VTEMP
kept only inside the at91-sama5d2_adc.c thus when receiving requests from
themal-generic-adc.c and buffers are enabled to use those cached values in
computation formula.

> 
> Other comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 252 ++++++++++++++++++++++++++++-
>>  1 file changed, 247 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 8f8fef42de84..67ced1369754 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -26,9 +26,12 @@
>>  #include <linux/iio/trigger.h>
>>  #include <linux/iio/trigger_consumer.h>
>>  #include <linux/iio/triggered_buffer.h>
>> +#include <linux/nvmem-consumer.h>
>>  #include <linux/pinctrl/consumer.h>
>>  #include <linux/regulator/consumer.h>
>>
>> +#include <dt-bindings/iio/adc/at91-sama5d2_adc.h>
>> +
>>  struct at91_adc_reg_layout {
>>  /* Control Register */
>>       u16                             CR;
>> @@ -73,10 +76,13 @@ struct at91_adc_reg_layout {
>>  /* Startup Time */
>>  #define      AT91_SAMA5D2_MR_STARTUP(v)      ((v) << 16)
>>  #define AT91_SAMA5D2_MR_STARTUP_MASK GENMASK(19, 16)
>> +/* Minimum startup time for temperature sensor */
>> +#define AT91_SAMA5D2_MR_STARTUP_TS_MIN       (50)
>>  /* Analog Change */
>>  #define      AT91_SAMA5D2_MR_ANACH           BIT(23)
>>  /* Tracking Time */
>>  #define      AT91_SAMA5D2_MR_TRACKTIM(v)     ((v) << 24)
>> +#define      AT91_SAMA5D2_MR_TRACKTIM_TS     6
>>  #define      AT91_SAMA5D2_MR_TRACKTIM_MAX    0xf
>>  /* Transfer Time */
>>  #define      AT91_SAMA5D2_MR_TRANSFER(v)     ((v) << 28)
>> @@ -149,6 +155,9 @@ struct at91_adc_reg_layout {
>>  #define AT91_SAMA5D2_TRACKX_MASK             GENMASK(23, 22)
>>  #define AT91_SAMA5D2_TRACKX(x)                       (((x) << 22) & \
>>                                                AT91_SAMA5D2_TRACKX_MASK)
>> +/* TRACKX for temperature sensor. */
>> +#define AT91_SAMA5D2_TRACKX_TS                       (1)
>> +
>>  /* Extended Mode Register - Averaging on single trigger event */
>>  #define AT91_SAMA5D2_EMR_ASTE(V)             ((V) << 20)
>>
>> @@ -164,6 +173,8 @@ struct at91_adc_reg_layout {
>>       u16                             ACR;
>>  /* Analog Control Register - Pen detect sensitivity mask */
>>  #define AT91_SAMA5D2_ACR_PENDETSENS_MASK        GENMASK(1, 0)
>> +/* Analog Control Register - Source last channel */
>> +#define AT91_SAMA5D2_ACR_SRCLCH              BIT(16)
>>
>>  /* Touchscreen Mode Register */
>>       u16                             TSMR;
>> @@ -231,6 +242,10 @@ struct at91_adc_reg_layout {
>>       u16                             WPSR;
>>  /* Version Register */
>>       u16                             VERSION;
>> +/* Temperature Sensor Mode Register */
>> +     u16                             TEMPMR;
>> +/* Temperature Sensor Mode - Temperature sensor on */
>> +#define AT91_SAMA5D2_TEMPMR_TEMPON   BIT(0)
>>  };
>>
>>  static const struct at91_adc_reg_layout sama5d2_layout = {
>> @@ -285,6 +300,7 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>>       .EOC_IDR =              0x38,
>>       .EOC_IMR =              0x3c,
>>       .EOC_ISR =              0x40,
>> +     .TEMPMR =               0x44,
>>       .OVER =                 0x4c,
>>       .EMR =                  0x50,
>>       .CWR =                  0x54,
>> @@ -390,6 +406,23 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>>               .datasheet_name = name,                                 \
>>       }
>>
>> +#define AT91_SAMA5D2_CHAN_TEMP(num, name, addr)                              \
>> +     {                                                               \
>> +             .type = IIO_TEMP,                                       \
>> +             .channel = num,                                         \
>> +             .address =  addr,                                       \
>> +             .scan_index = num,                                      \
>> +             .scan_type = {                                          \
>> +                     .sign = 'u',                                    \
>> +                     .realbits = 16,                                 \
>> +                     .storagebits = 16,                              \
> 
> So this is unusual.  Normally a processed channel isn't suitable for buffered
> capture because they tend not to fit in nice compact storage.  In this case
> what actually goes in the buffer?  Perhaps a comment would be useful here.

At the moment we don't allow the enabling of channel 31 when enabling the
buffers (we skip IIO_TEMP channels in at91_adc_buffer_prepare(),
at91_adc_buffer_postdisable()). At the moment when buffers are enabled the
IIO_TEMP consumer (drivers/thermal/thermal-generic-adc.c) will fail on
readings due to iio_buffer_enabled() in at91_adc_read_temp() or
iio_device_claim_direct_mode() in at91_adc_read_info_raw().

> 
>> +             },                                                      \
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),     \
>> +             .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_PROCESSED) | \
>> +                                     BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),\
>> +             .datasheet_name = name,                                 \
>> +     }
>> +
>>  #define at91_adc_readl(st, reg)                                              \
>>       readl_relaxed((st)->base + (st)->soc_info.platform->layout->reg)
>>  #define at91_adc_read_chan(st, reg)                                  \
>> @@ -413,6 +446,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>>   * @osr_mask:                oversampling ratio bitmask on EMR register
>>   * @osr_vals:                available oversampling rates
>>   * @chan_realbits:   realbits for registered channels
>> + * @temp_chan:               temperature channel index
>> + * @temp_sensor:     temperature sensor supported
>>   */
>>  struct at91_adc_platform {
>>       const struct at91_adc_reg_layout        *layout;
>> @@ -427,20 +462,54 @@ struct at91_adc_platform {
>>       unsigned int                            osr_mask;
>>       unsigned int                            osr_vals;
>>       unsigned int                            chan_realbits;
>> +     unsigned int                            temp_chan;
>> +     bool                                    temp_sensor;
>> +};
>> +
>> +/**
>> + * struct at91_adc_temp_sensor_clb - at91-sama5d2 temperature sensor
>> + * calibration data structure
>> + * @p1: P1 calibration temperature
>> + * @p4: P4 calibration voltage
>> + * @p6: P6 calibration voltage
>> + */
>> +struct at91_adc_temp_sensor_clb {
>> +     u32 p1;
>> +     u32 p4;
>> +     u32 p6;
>> +};
>> +
>> +/**
>> + * enum at91_adc_ts_clb_idx - calibration indexes in NVMEM buffer
>> + * @AT91_ADC_TS_CLB_IDX_P1: index for P1
>> + * @AT91_ADC_TS_CLB_IDX_P4: index for P4
>> + * @AT91_ADC_TS_CLB_IDX_P6: index for P6
>> + * @AT91_ADC_TS_CLB_IDX_MAX: max index for temperature calibration packet in OTP
>> + */
>> +enum at91_adc_ts_clb_idx {
>> +     AT91_ADC_TS_CLB_IDX_P1 = 2,
>> +     AT91_ADC_TS_CLB_IDX_P4 = 5,
>> +     AT91_ADC_TS_CLB_IDX_P6 = 7,
>> +     AT91_ADC_TS_CLB_IDX_MAX = 19,
>>  };
>>
>> +/* Temperature sensor calibration - Vtemp voltage sensitivity to temperature. */
>> +#define AT91_ADC_TS_VTEMP_DT         (2080U)
>> +
>>  /**
>>   * struct at91_adc_soc_info - at91-sama5d2 soc information struct
>>   * @startup_time:    device startup time
>>   * @min_sample_rate: minimum sample rate in Hz
>>   * @max_sample_rate: maximum sample rate in Hz
>>   * @platform:                pointer to the platform structure
>> + * @temp_sensor_clb: temperature sensor calibration data structure
>>   */
>>  struct at91_adc_soc_info {
>>       unsigned                        startup_time;
>>       unsigned                        min_sample_rate;
>>       unsigned                        max_sample_rate;
>>       const struct at91_adc_platform  *platform;
>> +     struct at91_adc_temp_sensor_clb temp_sensor_clb;
>>  };
>>
>>  struct at91_adc_trigger {
>> @@ -488,6 +557,20 @@ struct at91_adc_touch {
>>       struct work_struct              workq;
>>  };
>>
>> +/**
>> + * struct at91_adc_temp - at91-sama5d2 temperature information structure
>> + * @sample_period_val:       sample period value
>> + * @saved_sample_rate:       saved sample rate
>> + * @saved_oversampling:      saved oversampling
>> + * @init:            temperature sensor initialized
>> + */
>> +struct at91_adc_temp {
>> +     u16                             sample_period_val;
>> +     u16                             saved_sample_rate;
>> +     u16                             saved_oversampling;
>> +     bool                            init;
>> +};
>> +
>>  /*
>>   * Buffer size requirements:
>>   * No channels * bytes_per_channel(2) + timestamp bytes (8)
>> @@ -515,6 +598,7 @@ struct at91_adc_state {
>>       wait_queue_head_t               wq_data_available;
>>       struct at91_adc_dma             dma_st;
>>       struct at91_adc_touch           touch_st;
>> +     struct at91_adc_temp            temp_st;
>>       struct iio_dev                  *indio_dev;
>>       /* Ensure naturally aligned timestamp */
>>       u16                             buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
>> @@ -604,6 +688,7 @@ static const struct iio_chan_spec at91_sama7g5_adc_channels[] = {
>>       AT91_SAMA5D2_CHAN_DIFF(22, 12, 13, 0x90),
>>       AT91_SAMA5D2_CHAN_DIFF(23, 14, 15, 0x98),
>>       IIO_CHAN_SOFT_TIMESTAMP(24),
>> +     AT91_SAMA5D2_CHAN_TEMP(AT91_SAMA7G5_ADC_TEMP_CHANNEL, "temp", 0xdc),
>>  };
>>
>>  static const struct at91_adc_platform sama5d2_platform = {
>> @@ -637,10 +722,13 @@ static const struct at91_adc_platform sama7g5_platform = {
>>       .adc_channels = &at91_sama7g5_adc_channels,
>>  #define AT91_SAMA7G5_SINGLE_CHAN_CNT 16
>>  #define AT91_SAMA7G5_DIFF_CHAN_CNT   8
>> +#define AT91_SAMA7G5_TEMP_CHAN_CNT   1
>>       .nr_channels = AT91_SAMA7G5_SINGLE_CHAN_CNT +
>> -                    AT91_SAMA7G5_DIFF_CHAN_CNT,
>> +                    AT91_SAMA7G5_DIFF_CHAN_CNT +
>> +                    AT91_SAMA7G5_TEMP_CHAN_CNT,
>>  #define AT91_SAMA7G5_MAX_CHAN_IDX    (AT91_SAMA7G5_SINGLE_CHAN_CNT + \
>> -                                     AT91_SAMA7G5_DIFF_CHAN_CNT)
>> +                                     AT91_SAMA7G5_DIFF_CHAN_CNT + \
>> +                                     AT91_SAMA7G5_TEMP_CHAN_CNT)
>>       .max_channels = ARRAY_SIZE(at91_sama7g5_adc_channels),
>>       .max_index = AT91_SAMA7G5_MAX_CHAN_IDX,
>>  #define AT91_SAMA7G5_HW_TRIG_CNT     3
>> @@ -652,6 +740,8 @@ static const struct at91_adc_platform sama7g5_platform = {
>>                   BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
>>                   BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
>>       .chan_realbits = 16,
>> +     .temp_sensor = true,
>> +     .temp_chan = AT91_SAMA7G5_ADC_TEMP_CHANNEL,
>>  };
>>
>>  static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
>> @@ -1193,7 +1283,8 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
>>                       continue;
>>               /* these channel types cannot be handled by this trigger */
>>               if (chan->type == IIO_POSITIONRELATIVE ||
>> -                 chan->type == IIO_PRESSURE)
>> +                 chan->type == IIO_PRESSURE ||
>> +                 chan->type == IIO_TEMP)
>>                       continue;
>>
>>               at91_adc_cor(st, chan);
>> @@ -1235,7 +1326,8 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>                       continue;
>>               /* these channel types are virtual, no need to do anything */
>>               if (chan->type == IIO_POSITIONRELATIVE ||
>> -                 chan->type == IIO_PRESSURE)
>> +                 chan->type == IIO_PRESSURE ||
>> +                 chan->type == IIO_TEMP)
>>                       continue;
>>
>>               at91_adc_writel(st, CHDR, BIT(chan->channel));
>> @@ -1617,12 +1709,19 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>               return ret;
>>       }
>>
>> -     /* in this case we have a voltage channel */
>> +     /* in this case we have a voltage or temperature channel */
>>
>>       st->chan = chan;
>>
>>       at91_adc_cor(st, chan);
>>       at91_adc_writel(st, CHER, BIT(chan->channel));
>> +     /*
>> +      * TEMPMR.TEMPON needs to update after CHER otherwise if none
>> +      * of the channels are enabled and TEMPMR.TEMPON = 1 will
>> +      * trigger DRDY interruption while preparing for temperature read.
>> +      */
>> +     if (chan->type == IIO_TEMP)
>> +             at91_adc_writel(st, TEMPMR, AT91_SAMA5D2_TEMPMR_TEMPON);
>>       at91_adc_eoc_ena(st, chan->channel);
>>       at91_adc_writel(st, CR, AT91_SAMA5D2_CR_START);
>>
>> @@ -1642,6 +1741,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>       }
>>
>>       at91_adc_eoc_dis(st, st->chan->channel);
>> +     if (chan->type == IIO_TEMP)
>> +             at91_adc_writel(st, TEMPMR, 0U);
>>       at91_adc_writel(st, CHDR, BIT(chan->channel));
>>
>>       /* Needed to ACK the DRDY interruption */
>> @@ -1654,6 +1755,91 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>       return ret;
>>  }
>>
>> +static void at91_adc_temp_sensor_configure(struct at91_adc_state *st,
>> +                                        bool start)
>> +{
>> +     u32 sample_rate, oversampling_ratio;
>> +     u32 startup_time, tracktim, trackx;
>> +
>> +     if (start) {
>> +             /*
>> +              * Configure the sensor for best accuracy: 10MHz frequency,
>> +              * oversampling rate of 256, tracktim=0xf and trackx=1.
>> +              */
>> +             sample_rate = 10000000U;
>> +             oversampling_ratio = AT91_OSR_256SAMPLES;
>> +             startup_time = AT91_SAMA5D2_MR_STARTUP_TS_MIN;
>> +             tracktim = AT91_SAMA5D2_MR_TRACKTIM_TS;
>> +             trackx = AT91_SAMA5D2_TRACKX_TS;
>> +
>> +             st->temp_st.saved_sample_rate = st->current_sample_rate;
>> +             st->temp_st.saved_oversampling = st->oversampling_ratio;
>> +     } else {
>> +             /* Go back to previous settings. */
>> +             sample_rate = st->temp_st.saved_sample_rate;
>> +             oversampling_ratio = st->temp_st.saved_oversampling;
>> +             startup_time = st->soc_info.startup_time;
>> +             tracktim = 0;
>> +             trackx = 0;
>> +     }
>> +
>> +     at91_adc_setup_samp_freq(st->indio_dev, sample_rate, startup_time,
>> +                              tracktim);
>> +     at91_adc_config_emr(st, oversampling_ratio, trackx);
>> +}
>> +
>> +static int at91_adc_read_temp(struct iio_dev *indio_dev,
>> +                           struct iio_chan_spec const *chan, int *val)
>> +{
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +     struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
>> +     u64 div1, div2;
>> +     u32 tmp;
>> +     int ret, vbg, vtemp;
>> +
>> +     if (!st->soc_info.platform->temp_sensor || !st->temp_st.init)
>> +             return -EPERM;
> 
> You shouldn't register the channel if it's not readable.  Hence this
> should never happen.

I kept this as an indicator for temperature consumer that something wrong
happend on probing path of temperature sensor. In function
at91_adc_temp_sensor_init() the following could fail:
- devm_nvmem_cell_get()
- nvmem_cell_read()
- invalid length for NVMEM cell
Thus to keep the ADC probe going on in case temperature sensor probing init
failed I've added this st->temp_st.init. On the field there might be
devices that don't have proper information in NVMEM memory for temperature
sensor.

> 
>> +
>> +     if (iio_buffer_enabled(indio_dev))
>> +             return -EBUSY;
> 
> claim_direct - never check the buffer is enabled as it's racy - nothing
> stops it being enabled immediately after you check it (only exception
> is before interfaces are exposed such as _resume() where it should be safe
> to check).
> Which fixes the locking issue in the previous patch.  Move the
> iio_device_claim_direct() and mutex_lock() to all callers and you won't
> have a problem any more.

OK

> 
> 
>> +
>> +     mutex_lock(&st->lock);
>> +
>> +     at91_adc_temp_sensor_configure(st, true);
>> +
>> +     /* Read VBG. */
>> +     tmp = at91_adc_readl(st, ACR);
>> +     tmp |= AT91_SAMA5D2_ACR_SRCLCH;
>> +     at91_adc_writel(st, ACR, tmp);
>> +     ret = at91_adc_read_info_raw(indio_dev, chan, &vbg, false);
>> +     if (ret < 0)
>> +             goto unlock;
>> +
>> +     /* Read VTEMP. */
>> +     tmp &= ~AT91_SAMA5D2_ACR_SRCLCH;
>> +     at91_adc_writel(st, ACR, tmp);
>> +     ret = at91_adc_read_info_raw(indio_dev, chan, &vtemp, false);
>> +
>> +unlock:
>> +     /* Revert previous settings. */
>> +     at91_adc_temp_sensor_configure(st, false);
>> +     mutex_unlock(&st->lock);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /*
>> +      * Temp[milli] = p1[milli] + (vtemp * clb->p6 - clb->p4 * vbg)/
>> +      *                           (vbg * AT91_ADC_TS_VTEMP_DT)
>> +      */
>> +     div1 = DIV_ROUND_CLOSEST_ULL(((u64)vtemp * clb->p6), vbg);
>> +     div1 = DIV_ROUND_CLOSEST_ULL((div1 * 1000), AT91_ADC_TS_VTEMP_DT);
>> +     div2 = DIV_ROUND_CLOSEST_ULL((u64)clb->p4, AT91_ADC_TS_VTEMP_DT);
>> +     div2 *= 1000;
>> +     *val = clb->p1 + (int)div1 - (int)div2;
>> +
>> +     return ret;
>> +}
>> +
>>  static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>                            struct iio_chan_spec const *chan,
>>                            int *val, int *val2, long mask)
>> @@ -1671,6 +1857,11 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>               *val2 = chan->scan_type.realbits;
>>               return IIO_VAL_FRACTIONAL_LOG2;
>>
>> +     case IIO_CHAN_INFO_PROCESSED:
>> +             if (chan->type != IIO_TEMP)
>> +                     return -EINVAL;
>> +             return at91_adc_read_temp(indio_dev, chan, val);
>> +
>>       case IIO_CHAN_INFO_SAMP_FREQ:
>>               mutex_lock(&st->lock);
>>               *val = at91_adc_get_sample_freq(st);
>> @@ -1987,6 +2178,55 @@ static int at91_adc_buffer_and_trigger_init(struct device *dev,
>>       return 0;
>>  }
>>
>> +static void at91_adc_temp_sensor_init(struct at91_adc_state *st,
>> +                                   struct device *dev)
>> +{
>> +     struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
>> +     struct nvmem_cell *temp_calib;
>> +     u32 *buf;
>> +     size_t len;
>> +
>> +     if (!st->soc_info.platform->temp_sensor)
>> +             return;
>> +
>> +     st->temp_st.init = false;
> 
> that structure is kzalloc so this isn't really needed unless you think it's
> providing 'documentation'.

As mentioned above, I keep this as an indicator for temperature consumer
that temperature sendor was correctly probed.

> 
>> +
>> +     /* Get the calibration data from NVMEM. */
>> +     temp_calib = devm_nvmem_cell_get(dev, "temperature_calib");
>> +     if (IS_ERR(temp_calib)) {
>> +             if (PTR_ERR(temp_calib) != -ENOENT)
>> +                     dev_err(dev, "Failed to get temperature_calib cell!\n");
>> +             return;
>> +     }
>> +
>> +     buf = nvmem_cell_read(temp_calib, &len);
>> +     if (IS_ERR(buf)) {
>> +             dev_err(dev, "Failed to read calibration data!\n");
>> +             return;
>> +     }
>> +     if (len < AT91_ADC_TS_CLB_IDX_MAX * 4) {
>> +             dev_err(dev, "Invalid calibration data!\n");
>> +             goto free_buf;
>> +     }
>> +
>> +     /* Store calibration data for later use. */
>> +     clb->p1 = buf[AT91_ADC_TS_CLB_IDX_P1];
>> +     clb->p4 = buf[AT91_ADC_TS_CLB_IDX_P4];
>> +     clb->p6 = buf[AT91_ADC_TS_CLB_IDX_P6];
>> +
>> +     /*
>> +      * We prepare here the conversion to milli and also add constant
>> +      * factor (5 degrees Celsius) to p1 here to avoid doing it on
>> +      * hotpath.
>> +      */
>> +     clb->p1 = clb->p1 * 1000 + 5000;
>> +
>> +     st->temp_st.init = true;
>> +
>> +free_buf:
>> +     kfree(buf);
>> +}
>> +
> 

Re: [PATCH 13/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[Jonathan Cameron]

On Tue, 14 Jun 2022 10:13:17 +0000
<Claudiu.Beznea@microchip.com> wrote:

> On 11.06.2022 21:15, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, 9 Jun 2022 11:32:10 +0300
> > Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> >   
> >> SAMA7G5 has a temperature sensor embedded that is connected to channel 31
> >> of ADC. Temperature sensor provides 2 outputs: VTEMP and VBG. VTEMP is
> >> proportional to the absolute temperature voltage, VBG is quasi-temperature
> >> independent voltage. The calibration data for temperature sensor are
> >> retrieved from OTP memory specific to SAMA7G5. The formula to calculate
> >> the junction temperature is as follows:
> >>
> >> P1 + (Vref * (Vtemp - P6 - P4 * Vbg)) / (Vbg * VTEMP_DT)
> >>
> >> where Pi are calibration data retrieved from OTP memory.
> >>
> >> For better resolution before reading the temperature certain settings
> >> for oversampling ratio, sample frequency, EMR.TRACKX, MR.TRACKTIM are
> >> applied. The initial settings are reapplied at the end of temperature
> >> reading. Current support is not integrated with trigger buffers.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>  
> > 
> > This is a complex driver, so I got a bit lost figuring out what happens
> > about buffered capture of this channel.  What ends up in the buffer?  
> 
> With this implementation nothing should end up in the buffer as the patch
> skips channel enable/disable if its about an IIO_TEMP channel (see
> functions functions at91_adc_buffer_prepare(),
> at91_adc_buffer_postdisable()). More details above.
> 
> According to datasheet the temperature channel behaves the same as the
> other channels. On temperature channel are multiplexed both VBG and VTEMP.
> 
>           `
>           | \      +-----+
> VBG   --->|  |ch31 |     |
> Vtemp --->|  |---->| ADC |
>           |  /     +-----+
>           | /
>           .
> 
> And both are needed to be measured in order to determine the correct in SoC
> temperature.
> 
> At a moment of time only one of these could be measured, the selection
> being done with bit SRCLCH bit of ACR register.
> 
> According to datasheet there is no special treatment for channel 31 of ADC
> in case triggers are enabled. So, in case of a buffer capture the buffer
> for channel 31 will contain either the converted value for VBG or VTEMP
> (depending on the value of bit SRCLCH bit of ACR register), if channel 31
> is enabled for that. But on this implementation we skip the enable/disable
> of IIO_TEMP channels (functions at91_adc_buffer_prepare(),
> at91_adc_buffer_postdisable()).

Great explanation. Perhaps we can capture some of it either as comments,
or patch description etc.

> 
> Hardware implements a special mechanism for reading the temperature when
> triggers are enabled as follows: the hardware provides a RTC trigger wich
> fires every 1 second and starts a conversion on channel 31. This could
> permit to have a temperature value once every 2 seconds (in the 1st RTC
> trigger VBG could be read, in the 2nd RTC trigger Vtemp could be read, or
> the other way arround). For this, configuration needs to be done propertly
> in the RTC memory spaces and on Linux side something should be implemented
> for the interaction b/w RTC and IIO subsystems. But this is for future
> development.
> 
> > Most processed channels are not useable with that mode (and hence have
> > a scanindex == -1 which ensures they aren't exposed as an option for
> > userspace to enable).  
> 
> OK, I haven't been aware of that. I only did some basic research on how
> this could be achieved. As we are using the thermal support on SAMA7G5 with
> driver at drivers/thermal/thermal-generic-adc.c which reads processed value
> at periodic intervals one idea was to take advantage of the RTC trigger
> mechanism for channel 31 and have the converted values of VBG and VTEMP
> kept only inside the at91-sama5d2_adc.c thus when receiving requests from
> themal-generic-adc.c and buffers are enabled to use those cached values in
> computation formula.
Sure, that might work ok. It's a bit of a hack, but would let you keep the
more interesting stuff hidden way in one place.

>

...


> >> +#define AT91_SAMA5D2_CHAN_TEMP(num, name, addr)                              \
> >> +     {                                                               \
> >> +             .type = IIO_TEMP,                                       \
> >> +             .channel = num,                                         \
> >> +             .address =  addr,                                       \
> >> +             .scan_index = num,                                      \
> >> +             .scan_type = {                                          \
> >> +                     .sign = 'u',                                    \
> >> +                     .realbits = 16,                                 \
> >> +                     .storagebits = 16,                              \  
> > 
> > So this is unusual.  Normally a processed channel isn't suitable for buffered
> > capture because they tend not to fit in nice compact storage.  In this case
> > what actually goes in the buffer?  Perhaps a comment would be useful here.  
> 
> At the moment we don't allow the enabling of channel 31 when enabling the
> buffers (we skip IIO_TEMP channels in at91_adc_buffer_prepare(),
> at91_adc_buffer_postdisable()). At the moment when buffers are enabled the
> IIO_TEMP consumer (drivers/thermal/thermal-generic-adc.c) will fail on
> readings due to iio_buffer_enabled() in at91_adc_read_temp() or
> iio_device_claim_direct_mode() in at91_adc_read_info_raw().

I suspected as much. If so, a bunch of this makes not sense. Channels that
can't be in that buffer have magic scan_index = -1 and .scan_type is
probably not used (occasionally it is helpful for non buffered paths, though
rarely all the info in there).

> 

> >> +static int at91_adc_read_temp(struct iio_dev *indio_dev,
> >> +                           struct iio_chan_spec const *chan, int *val)
> >> +{
> >> +     struct at91_adc_state *st = iio_priv(indio_dev);
> >> +     struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
> >> +     u64 div1, div2;
> >> +     u32 tmp;
> >> +     int ret, vbg, vtemp;
> >> +
> >> +     if (!st->soc_info.platform->temp_sensor || !st->temp_st.init)
> >> +             return -EPERM;  
> > 
> > You shouldn't register the channel if it's not readable.  Hence this
> > should never happen.  
> 
> I kept this as an indicator for temperature consumer that something wrong
> happend on probing path of temperature sensor. In function
> at91_adc_temp_sensor_init() the following could fail:
> - devm_nvmem_cell_get()
> - nvmem_cell_read()
> - invalid length for NVMEM cell
> Thus to keep the ADC probe going on in case temperature sensor probing init
> failed I've added this st->temp_st.init. On the field there might be
> devices that don't have proper information in NVMEM memory for temperature
> sensor.

If those fail, don't register the channel.  It should just be invisible
to userspace / in kernel consumers.  That may require reorganizing how
you register channels to know if these worked or not before the
point of registering channels.


Thanks,

Jonathan

[PATCH 14/16] iio: adc: at91-sama5d2_adc: add empty line after functions

[Claudiu Beznea]

Add empty line after function.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 67ced1369754..1a6788566969 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1374,6 +1374,7 @@ static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
 
 	return trig;
 }
+
 static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
 					   struct iio_poll_func *pf)
 {
-- 
2.34.1

[PATCH 15/16] iio: adc: at91-sama5d2_adc: add runtime pm support

[Claudiu Beznea]

Add runtime PM support by disabling/enabling ADC's peripheral clock.
On simple conversion the ADC's clock is kept enabled just while the
conversion is in progress. This includes also temperature conversion.
For triggers and touch conversions the ADC clock is kept enabled while
the triggers or touch are enabled.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 201 +++++++++++++++++++++++++----
 1 file changed, 173 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 1a6788566969..5d9ad51d0920 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -28,6 +28,7 @@
 #include <linux/iio/triggered_buffer.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 #include <dt-bindings/iio/adc/at91-sama5d2_adc.h>
@@ -600,6 +601,7 @@ struct at91_adc_state {
 	struct at91_adc_touch		touch_st;
 	struct at91_adc_temp		temp_st;
 	struct iio_dev			*indio_dev;
+	struct device			*dev;
 	/* Ensure naturally aligned timestamp */
 	u16				buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
 	/*
@@ -844,10 +846,16 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 			       u32 oversampling_ratio, u32 trackx)
 {
 	/* configure the extended mode register */
-	unsigned int emr = at91_adc_readl(st, EMR);
+	unsigned int emr;
 	unsigned int osr_mask = st->soc_info.platform->osr_mask;
 	unsigned int osr_vals = st->soc_info.platform->osr_vals;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return ret;
 
+	emr = at91_adc_readl(st, EMR);
 	/* select oversampling per single trigger event */
 	emr |= AT91_SAMA5D2_EMR_ASTE(1);
 
@@ -857,32 +865,42 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 	/* select oversampling ratio from configuration */
 	switch (oversampling_ratio) {
 	case AT91_OSR_1SAMPLES:
-		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES)))
-			return -EINVAL;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES))) {
+			ret = -EINVAL;
+			goto pm_runtime_put;
+		}
 		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES,
 					    osr_mask);
 		break;
 	case AT91_OSR_4SAMPLES:
-		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES)))
-			return -EINVAL;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES))) {
+			ret = -EINVAL;
+			goto pm_runtime_put;
+		}
 		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES,
 					    osr_mask);
 		break;
 	case AT91_OSR_16SAMPLES:
-		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES)))
-			return -EINVAL;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES))) {
+			ret = -EINVAL;
+			goto pm_runtime_put;
+		}
 		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
 					    osr_mask);
 		break;
 	case AT91_OSR_64SAMPLES:
-		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
-			return -EINVAL;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES))) {
+			ret = -EINVAL;
+			goto pm_runtime_put;
+		}
 		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
 					    osr_mask);
 		break;
 	case AT91_OSR_256SAMPLES:
-		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
-			return -EINVAL;
+		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES))) {
+			ret = -EINVAL;
+			goto pm_runtime_put;
+		}
 		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
 					    osr_mask);
 		break;
@@ -894,7 +912,10 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
 
 	st->oversampling_ratio = oversampling_ratio;
 
-	return 0;
+pm_runtime_put:
+	pm_runtime_mark_last_busy(st->dev);
+	pm_runtime_put_autosuspend(st->dev);
+	return ret;
 }
 
 static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
@@ -947,15 +968,22 @@ static void at91_adc_adjust_val_osr_array(struct at91_adc_state *st, void *buf,
 static int at91_adc_configure_touch(struct at91_adc_state *st, bool state)
 {
 	u32 clk_khz = st->current_sample_rate / 1000;
-	int i = 0;
+	int i = 0, ret;
 	u16 pendbc;
 	u32 tsmr, acr;
 
-	if (!state) {
+	if (state) {
+		ret = pm_runtime_resume_and_get(st->dev);
+		if (ret < 0)
+			return ret;
+	} else {
 		/* disabling touch IRQs and setting mode to no touch enabled */
 		at91_adc_writel(st, IDR,
 				AT91_SAMA5D2_IER_PEN | AT91_SAMA5D2_IER_NOPEN);
 		at91_adc_writel(st, TSMR, 0);
+
+		pm_runtime_mark_last_busy(st->dev);
+		pm_runtime_put_autosuspend(st->dev);
 		return 0;
 	}
 	/*
@@ -1100,8 +1128,16 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
 	struct at91_adc_state *st = iio_priv(indio);
-	u32 status = at91_adc_readl(st, TRGR);
+	u32 status;
+	int ret;
+
+	if (state) {
+		ret = pm_runtime_resume_and_get(st->dev);
+		if (ret < 0)
+			return ret;
+	}
 
+	status = at91_adc_readl(st, TRGR);
 	/* clear TRGMOD */
 	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
 
@@ -1111,6 +1147,11 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 	/* set/unset hw trigger */
 	at91_adc_writel(st, TRGR, status);
 
+	if (!state) {
+		pm_runtime_mark_last_busy(st->dev);
+		pm_runtime_put_autosuspend(st->dev);
+	}
+
 	return 0;
 }
 
@@ -1268,11 +1309,15 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
 	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return ret;
+
 	/* we continue with the triggered buffer */
 	ret = at91_adc_dma_start(indio_dev);
 	if (ret) {
 		dev_err(&indio_dev->dev, "buffer prepare failed\n");
-		return ret;
+		goto pm_runtime_put;
 	}
 
 	for_each_set_bit(bit, indio_dev->active_scan_mask,
@@ -1295,12 +1340,16 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
 	if (at91_adc_buffer_check_use_irq(indio_dev, st))
 		at91_adc_writel(st, IER, AT91_SAMA5D2_IER_DRDY);
 
-	return 0;
+pm_runtime_put:
+	pm_runtime_mark_last_busy(st->dev);
+	pm_runtime_put_autosuspend(st->dev);
+	return ret;
 }
 
 static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
 	u8 bit;
 
 	/* check if we are disabling triggered buffer or the touchscreen */
@@ -1311,6 +1360,10 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
 	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * For each enable channel we must disable it in hardware.
 	 * In the case of DMA, we must read the last converted value
@@ -1346,6 +1399,9 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
 	if (st->dma_st.dma_chan)
 		dmaengine_terminate_sync(st->dma_st.dma_chan);
 
+	pm_runtime_mark_last_busy(st->dev);
+	pm_runtime_put_autosuspend(st->dev);
+
 	return 0;
 }
 
@@ -1534,12 +1590,17 @@ static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq,
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 	unsigned f_per, prescal, startup, mr;
+	int ret;
 
 	f_per = clk_get_rate(st->per_clk);
 	prescal = (f_per / (2 * freq)) - 1;
 
 	startup = at91_adc_startup_time(startup_time, freq / 1000);
 
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return;
+
 	mr = at91_adc_readl(st, MR);
 	mr &= ~(AT91_SAMA5D2_MR_STARTUP_MASK | AT91_SAMA5D2_MR_PRESCAL_MASK);
 	mr |= AT91_SAMA5D2_MR_STARTUP(startup);
@@ -1547,6 +1608,9 @@ static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq,
 	mr |= AT91_SAMA5D2_MR_TRACKTIM(tracktim);
 	at91_adc_writel(st, MR, mr);
 
+	pm_runtime_mark_last_busy(st->dev);
+	pm_runtime_put_autosuspend(st->dev);
+
 	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u, tracktim=%u\n",
 		freq, startup, prescal, tracktim);
 	st->current_sample_rate = freq;
@@ -1684,6 +1748,10 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 	u16 tmp_val;
 	int ret;
 
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * Keep in mind that we cannot use software trigger or touchscreen
 	 * if external trigger is enabled
@@ -1695,7 +1763,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 
 	ret = iio_device_claim_direct_mode(indio_dev);
 	if (ret)
-		return ret;
+		goto pm_runtime_put;
 	if (lock)
 		mutex_lock(&st->lock);
 
@@ -1707,7 +1775,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 			mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
-		return ret;
+		goto pm_runtime_put;
 	}
 
 	/* in this case we have a voltage or temperature channel */
@@ -1753,6 +1821,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&st->lock);
 
 	iio_device_release_direct_mode(indio_dev);
+
+pm_runtime_put:
+	pm_runtime_mark_last_busy(st->dev);
+	pm_runtime_put_autosuspend(st->dev);
+
 	return ret;
 }
 
@@ -1804,6 +1877,10 @@ static int at91_adc_read_temp(struct iio_dev *indio_dev,
 	if (iio_buffer_enabled(indio_dev))
 		return -EBUSY;
 
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return ret;
+
 	mutex_lock(&st->lock);
 
 	at91_adc_temp_sensor_configure(st, true);
@@ -1825,6 +1902,10 @@ static int at91_adc_read_temp(struct iio_dev *indio_dev,
 	/* Revert previous settings. */
 	at91_adc_temp_sensor_configure(st, false);
 	mutex_unlock(&st->lock);
+
+	pm_runtime_mark_last_busy(st->dev);
+	pm_runtime_put_autosuspend(st->dev);
+
 	if (ret < 0)
 		return ret;
 
@@ -2363,13 +2444,19 @@ static int at91_adc_probe(struct platform_device *pdev)
 
 	at91_adc_temp_sensor_init(st, &pdev->dev);
 
-	at91_adc_hw_init(indio_dev);
-
 	platform_set_drvdata(pdev, indio_dev);
+	st->dev = &pdev->dev;
+	pm_runtime_set_autosuspend_delay(st->dev, 500);
+	pm_runtime_use_autosuspend(st->dev);
+	pm_runtime_set_active(st->dev);
+	pm_runtime_enable(st->dev);
+	pm_runtime_get_noresume(st->dev);
+
+	at91_adc_hw_init(indio_dev);
 
 	ret = at91_adc_buffer_and_trigger_init(&pdev->dev, indio_dev);
 	if (ret < 0)
-		goto per_clk_disable_unprepare;
+		goto err_pm_disable;
 
 	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
 		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
@@ -2385,10 +2472,18 @@ static int at91_adc_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "version: %x\n",
 		 readl_relaxed(st->base + st->soc_info.platform->layout->VERSION));
 
+	pm_runtime_mark_last_busy(st->dev);
+	pm_runtime_put_autosuspend(st->dev);
+
 	return 0;
 
 dma_disable:
 	at91_adc_dma_disable(st);
+err_pm_disable:
+	pm_runtime_put_noidle(st->dev);
+	pm_runtime_disable(st->dev);
+	pm_runtime_set_suspended(st->dev);
+	pm_runtime_dont_use_autosuspend(st->dev);
 per_clk_disable_unprepare:
 	clk_disable_unprepare(st->per_clk);
 vref_disable:
@@ -2402,11 +2497,18 @@ static int at91_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return ret;
 
 	iio_device_unregister(indio_dev);
 
 	at91_adc_dma_disable(st);
 
+	pm_runtime_disable(st->dev);
+	pm_runtime_put_noidle(st->dev);
 	clk_disable_unprepare(st->per_clk);
 
 	regulator_disable(st->vref);
@@ -2419,6 +2521,11 @@ static __maybe_unused int at91_adc_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(st->dev);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * Do a sofware reset of the ADC before we go to suspend.
@@ -2428,7 +2535,8 @@ static __maybe_unused int at91_adc_suspend(struct device *dev)
 	 */
 	at91_adc_writel(st, CR, AT91_SAMA5D2_CR_SWRST);
 
-	clk_disable_unprepare(st->per_clk);
+	pm_runtime_force_suspend(st->dev);
+	clk_unprepare(st->per_clk);
 	regulator_disable(st->vref);
 	regulator_disable(st->reg);
 
@@ -2453,25 +2561,40 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
 	if (ret)
 		goto reg_disable_resume;
 
-	ret = clk_prepare_enable(st->per_clk);
+	ret = clk_prepare(st->per_clk);
 	if (ret)
 		goto vref_disable_resume;
 
+	ret = pm_runtime_force_resume(st->dev);
+	if (ret < 0)
+		goto clk_unprepare_resume;
+
 	at91_adc_hw_init(indio_dev);
 
 	/* reconfiguring trigger hardware state */
 	if (!iio_buffer_enabled(indio_dev))
-		return 0;
+		goto pm_runtime_put;
 
 	/* check if we are enabling triggered buffer or the touchscreen */
 	if (at91_adc_current_chan_is_touch(indio_dev))
-		return at91_adc_configure_touch(st, true);
+		ret = at91_adc_configure_touch(st, true);
 	else
-		return at91_adc_configure_trigger(st->trig, true);
+		ret = at91_adc_configure_trigger(st->trig, true);
+
+pm_runtime_put:
+	pm_runtime_mark_last_busy(st->dev);
+	if (ret < 0) {
+		pm_runtime_put_sync_suspend(st->dev);
+		goto clk_unprepare_resume;
+	} else {
+		pm_runtime_put_autosuspend(st->dev);
+	}
 
 	/* not needed but more explicit */
 	return 0;
 
+clk_unprepare_resume:
+	clk_unprepare(st->per_clk);
 vref_disable_resume:
 	regulator_disable(st->vref);
 reg_disable_resume:
@@ -2481,7 +2604,29 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
 	return ret;
 }
 
-static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
+static int __maybe_unused at91_adc_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	clk_disable(st->per_clk);
+
+	return 0;
+}
+
+static int __maybe_unused at91_adc_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	return clk_enable(st->per_clk);
+}
+
+static const struct dev_pm_ops __maybe_unused at91_adc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(at91_adc_suspend, at91_adc_resume)
+	SET_RUNTIME_PM_OPS(at91_adc_runtime_suspend, at91_adc_runtime_resume,
+			   NULL)
+};
 
 static const struct of_device_id at91_adc_dt_match[] = {
 	{
-- 
2.34.1

Re: [PATCH 15/16] iio: adc: at91-sama5d2_adc: add runtime pm support

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:12 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> Add runtime PM support by disabling/enabling ADC's peripheral clock.
> On simple conversion the ADC's clock is kept enabled just while the
> conversion is in progress. This includes also temperature conversion.
> For triggers and touch conversions the ADC clock is kept enabled while
> the triggers or touch are enabled.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Various comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 201 +++++++++++++++++++++++++----
>  1 file changed, 173 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 1a6788566969..5d9ad51d0920 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -28,6 +28,7 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/nvmem-consumer.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  
>  #include <dt-bindings/iio/adc/at91-sama5d2_adc.h>
> @@ -600,6 +601,7 @@ struct at91_adc_state {
>  	struct at91_adc_touch		touch_st;
>  	struct at91_adc_temp		temp_st;
>  	struct iio_dev			*indio_dev;
> +	struct device			*dev;
>  	/* Ensure naturally aligned timestamp */
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
>  	/*
> @@ -844,10 +846,16 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>  			       u32 oversampling_ratio, u32 trackx)
>  {
>  	/* configure the extended mode register */
> -	unsigned int emr = at91_adc_readl(st, EMR);
> +	unsigned int emr;
>  	unsigned int osr_mask = st->soc_info.platform->osr_mask;
>  	unsigned int osr_vals = st->soc_info.platform->osr_vals;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;

In this particular case, it might be cleaner to introduce a wrapper
function that deals with the power management and then calls this one.
Gets rid of the ugly gotos out of the switch statement.

>  
> +	emr = at91_adc_readl(st, EMR);
>  	/* select oversampling per single trigger event */
>  	emr |= AT91_SAMA5D2_EMR_ASTE(1);
>  
> @@ -857,32 +865,42 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>  	/* select oversampling ratio from configuration */
>  	switch (oversampling_ratio) {
>  	case AT91_OSR_1SAMPLES:
> -		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES)))
> -			return -EINVAL;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES))) {
> +			ret = -EINVAL;
> +			goto pm_runtime_put;
> +		}
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES,
>  					    osr_mask);
>  		break;
>  	case AT91_OSR_4SAMPLES:
> -		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES)))
> -			return -EINVAL;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES))) {
> +			ret = -EINVAL;
> +			goto pm_runtime_put;
> +		}
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES,
>  					    osr_mask);
>  		break;
>  	case AT91_OSR_16SAMPLES:
> -		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES)))
> -			return -EINVAL;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES))) {
> +			ret = -EINVAL;
> +			goto pm_runtime_put;
> +		}
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>  					    osr_mask);
>  		break;
>  	case AT91_OSR_64SAMPLES:
> -		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
> -			return -EINVAL;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES))) {
> +			ret = -EINVAL;
> +			goto pm_runtime_put;
> +		}
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
>  					    osr_mask);
>  		break;
>  	case AT91_OSR_256SAMPLES:
> -		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
> -			return -EINVAL;
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES))) {
> +			ret = -EINVAL;
> +			goto pm_runtime_put;
> +		}
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
>  					    osr_mask);
>  		break;
> @@ -894,7 +912,10 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>  
>  	st->oversampling_ratio = oversampling_ratio;
>  
> -	return 0;
> +pm_runtime_put:
> +	pm_runtime_mark_last_busy(st->dev);
> +	pm_runtime_put_autosuspend(st->dev);
> +	return ret;
>  }
>  
>  static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
> @@ -947,15 +968,22 @@ static void at91_adc_adjust_val_osr_array(struct at91_adc_state *st, void *buf,
>  static int at91_adc_configure_touch(struct at91_adc_state *st, bool state)
>  {
>  	u32 clk_khz = st->current_sample_rate / 1000;
> -	int i = 0;
> +	int i = 0, ret;
>  	u16 pendbc;
>  	u32 tsmr, acr;
>  
> -	if (!state) {
> +	if (state) {
> +		ret = pm_runtime_resume_and_get(st->dev);
> +		if (ret < 0)
> +			return ret;
> +	} else {
>  		/* disabling touch IRQs and setting mode to no touch enabled */
>  		at91_adc_writel(st, IDR,
>  				AT91_SAMA5D2_IER_PEN | AT91_SAMA5D2_IER_NOPEN);
>  		at91_adc_writel(st, TSMR, 0);
> +
> +		pm_runtime_mark_last_busy(st->dev);
> +		pm_runtime_put_autosuspend(st->dev);
>  		return 0;
>  	}
>  	/*
> @@ -1100,8 +1128,16 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>  	struct at91_adc_state *st = iio_priv(indio);
> -	u32 status = at91_adc_readl(st, TRGR);
> +	u32 status;
> +	int ret;
> +
> +	if (state) {
> +		ret = pm_runtime_resume_and_get(st->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
> +	status = at91_adc_readl(st, TRGR);
>  	/* clear TRGMOD */
>  	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
>  
> @@ -1111,6 +1147,11 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  	/* set/unset hw trigger */
>  	at91_adc_writel(st, TRGR, status);
>  
> +	if (!state) {
> +		pm_runtime_mark_last_busy(st->dev);
> +		pm_runtime_put_autosuspend(st->dev);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1268,11 +1309,15 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
>  	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
>  		return -EINVAL;
>  
> +	ret = pm_runtime_resume_and_get(st->dev);

This seems unusual.  I'd normally expect to see runtime pm left on whenever
a buffer is in use, but in this case you seem to let it autosuspend.

That 'might' be fine as you might hit it often enough that it stays up whilst
doing DMA but it certainly seems odd and less than efficient.
Or possibly the use of the trigger is enough to keep it up.

> +	if (ret < 0)
> +		return ret;
> +
>  	/* we continue with the triggered buffer */
>  	ret = at91_adc_dma_start(indio_dev);
>  	if (ret) {
>  		dev_err(&indio_dev->dev, "buffer prepare failed\n");
> -		return ret;
> +		goto pm_runtime_put;
>  	}
>  
>  	for_each_set_bit(bit, indio_dev->active_scan_mask,
> @@ -1295,12 +1340,16 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
>  	if (at91_adc_buffer_check_use_irq(indio_dev, st))
>  		at91_adc_writel(st, IER, AT91_SAMA5D2_IER_DRDY);
>  
> -	return 0;
> +pm_runtime_put:
> +	pm_runtime_mark_last_busy(st->dev);
> +	pm_runtime_put_autosuspend(st->dev);
> +	return ret;
>  }
>  
>  static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
>  	u8 bit;
>  
>  	/* check if we are disabling triggered buffer or the touchscreen */
> @@ -1311,6 +1360,10 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>  	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
>  		return -EINVAL;
>  
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	/*
>  	 * For each enable channel we must disable it in hardware.
>  	 * In the case of DMA, we must read the last converted value
> @@ -1346,6 +1399,9 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>  	if (st->dma_st.dma_chan)
>  		dmaengine_terminate_sync(st->dma_st.dma_chan);
>  
> +	pm_runtime_mark_last_busy(st->dev);
> +	pm_runtime_put_autosuspend(st->dev);
> +
>  	return 0;
>  }
>  
> @@ -1534,12 +1590,17 @@ static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq,
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  	unsigned f_per, prescal, startup, mr;
> +	int ret;
>  
>  	f_per = clk_get_rate(st->per_clk);
>  	prescal = (f_per / (2 * freq)) - 1;
>  
>  	startup = at91_adc_startup_time(startup_time, freq / 1000);
>  
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return;
> +
>  	mr = at91_adc_readl(st, MR);
>  	mr &= ~(AT91_SAMA5D2_MR_STARTUP_MASK | AT91_SAMA5D2_MR_PRESCAL_MASK);
>  	mr |= AT91_SAMA5D2_MR_STARTUP(startup);
> @@ -1547,6 +1608,9 @@ static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq,
>  	mr |= AT91_SAMA5D2_MR_TRACKTIM(tracktim);
>  	at91_adc_writel(st, MR, mr);
>  
> +	pm_runtime_mark_last_busy(st->dev);
> +	pm_runtime_put_autosuspend(st->dev);
> +
>  	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u, tracktim=%u\n",
>  		freq, startup, prescal, tracktim);
>  	st->current_sample_rate = freq;
> @@ -1684,6 +1748,10 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  	u16 tmp_val;
>  	int ret;
>  
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	/*
>  	 * Keep in mind that we cannot use software trigger or touchscreen
>  	 * if external trigger is enabled
> @@ -1695,7 +1763,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  
>  	ret = iio_device_claim_direct_mode(indio_dev);
>  	if (ret)
> -		return ret;
> +		goto pm_runtime_put;
>  	if (lock)
>  		mutex_lock(&st->lock);
>  
> @@ -1707,7 +1775,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  			mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
> -		return ret;
> +		goto pm_runtime_put;
>  	}
>  
>  	/* in this case we have a voltage or temperature channel */
> @@ -1753,6 +1821,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		mutex_unlock(&st->lock);
>  
>  	iio_device_release_direct_mode(indio_dev);
> +
> +pm_runtime_put:
> +	pm_runtime_mark_last_busy(st->dev);
> +	pm_runtime_put_autosuspend(st->dev);
> +
>  	return ret;
>  }
>  
> @@ -1804,6 +1877,10 @@ static int at91_adc_read_temp(struct iio_dev *indio_dev,
>  	if (iio_buffer_enabled(indio_dev))
>  		return -EBUSY;
>  
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	mutex_lock(&st->lock);
>  
>  	at91_adc_temp_sensor_configure(st, true);
> @@ -1825,6 +1902,10 @@ static int at91_adc_read_temp(struct iio_dev *indio_dev,
>  	/* Revert previous settings. */
>  	at91_adc_temp_sensor_configure(st, false);
>  	mutex_unlock(&st->lock);
> +
> +	pm_runtime_mark_last_busy(st->dev);
> +	pm_runtime_put_autosuspend(st->dev);
> +
>  	if (ret < 0)
>  		return ret;
>  
> @@ -2363,13 +2444,19 @@ static int at91_adc_probe(struct platform_device *pdev)
>  
>  	at91_adc_temp_sensor_init(st, &pdev->dev);
>  
> -	at91_adc_hw_init(indio_dev);
> -
>  	platform_set_drvdata(pdev, indio_dev);
> +	st->dev = &pdev->dev;
> +	pm_runtime_set_autosuspend_delay(st->dev, 500);
> +	pm_runtime_use_autosuspend(st->dev);
> +	pm_runtime_set_active(st->dev);
> +	pm_runtime_enable(st->dev);
> +	pm_runtime_get_noresume(st->dev);
> +
> +	at91_adc_hw_init(indio_dev);
>  
>  	ret = at91_adc_buffer_and_trigger_init(&pdev->dev, indio_dev);
>  	if (ret < 0)
> -		goto per_clk_disable_unprepare;
> +		goto err_pm_disable;
>  
>  	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
>  		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
> @@ -2385,10 +2472,18 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	dev_info(&pdev->dev, "version: %x\n",
>  		 readl_relaxed(st->base + st->soc_info.platform->layout->VERSION));
>  
> +	pm_runtime_mark_last_busy(st->dev);
> +	pm_runtime_put_autosuspend(st->dev);
> +
>  	return 0;
>  
>  dma_disable:
>  	at91_adc_dma_disable(st);
> +err_pm_disable:
> +	pm_runtime_put_noidle(st->dev);
> +	pm_runtime_disable(st->dev);
> +	pm_runtime_set_suspended(st->dev);
> +	pm_runtime_dont_use_autosuspend(st->dev);
>  per_clk_disable_unprepare:
>  	clk_disable_unprepare(st->per_clk);
>  vref_disable:
> @@ -2402,11 +2497,18 @@ static int at91_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;

There isn't much useful that can be done with a return of error from
a remove() function. Uwe Klein-Konig is busy removing all these returns
(and eventually changing the prototypes to return void), so don't introduce a new one
or Uwe will be grumpy :)

>  
>  	iio_device_unregister(indio_dev);
>  
>  	at91_adc_dma_disable(st);
>  
> +	pm_runtime_disable(st->dev);
> +	pm_runtime_put_noidle(st->dev);
>  	clk_disable_unprepare(st->per_clk);
>  
>  	regulator_disable(st->vref);
> @@ -2419,6 +2521,11 @@ static __maybe_unused int at91_adc_suspend(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(st->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	/*
>  	 * Do a sofware reset of the ADC before we go to suspend.
> @@ -2428,7 +2535,8 @@ static __maybe_unused int at91_adc_suspend(struct device *dev)
>  	 */
>  	at91_adc_writel(st, CR, AT91_SAMA5D2_CR_SWRST);
>  
> -	clk_disable_unprepare(st->per_clk);
> +	pm_runtime_force_suspend(st->dev);

This confuses me a bit because we know we are already awake (because of
the pm_runtime_resume_and_get() so we will definitely suspend here and
I'm fairly sure that means we definitely resume in the _resume()
below.  Basically our usage counters are I think off by one. If you
could verify that it doesn't turn back on if we don't have the buffered
enabled and were previously in runtime suspend state that would
prove me wrong (which is more than possible - this stuff always gives
me a headache)


> +	clk_unprepare(st->per_clk);
>  	regulator_disable(st->vref);
>  	regulator_disable(st->reg);
>  
> @@ -2453,25 +2561,40 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>  	if (ret)
>  		goto reg_disable_resume;
>  
> -	ret = clk_prepare_enable(st->per_clk);
> +	ret = clk_prepare(st->per_clk);
>  	if (ret)
>  		goto vref_disable_resume;
>  
> +	ret = pm_runtime_force_resume(st->dev);
> +	if (ret < 0)
> +		goto clk_unprepare_resume;
> +
>  	at91_adc_hw_init(indio_dev);
>  
>  	/* reconfiguring trigger hardware state */
>  	if (!iio_buffer_enabled(indio_dev))
(see below)
	flip this check so the next block is indented givne
	exit path will be shared (thus removing the goto).
	if (iio_buffer_enabled(indio_dev)) {
		/* check ...


> -		return 0;
> +		goto pm_runtime_put;
>  
>  	/* check if we are enabling triggered buffer or the touchscreen */
>  	if (at91_adc_current_chan_is_touch(indio_dev))
> -		return at91_adc_configure_touch(st, true);
> +		ret = at91_adc_configure_touch(st, true);
>  	else
> -		return at91_adc_configure_trigger(st->trig, true);
> +		ret = at91_adc_configure_trigger(st->trig, true);
> +
(see below)
	
		if (ret < 0)
			goto pm_runtime_put;
	}

	pm_runtime_mark_last_busy(st->dev);
	pm_runtime_put_autosuspend(st->dev);
	return 0;
			
> +pm_runtime_put:

I think this would be easier to follow if you break this up into the
different cases.

pm_runtime_put:
	pm_runtime_mark_last_busy(st->dev);
	pm_runtime_put_sync_suspend(st->dev);
clk_unprepare_resume:
	...

> +	pm_runtime_mark_last_busy(st->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_sync_suspend(st->dev);
> +		goto clk_unprepare_resume;
> +	} else {
> +		pm_runtime_put_autosuspend(st->dev);
> +	}
>  
>  	/* not needed but more explicit */
>  	return 0;
>  
> +clk_unprepare_resume:
> +	clk_unprepare(st->per_clk);
>  vref_disable_resume:
>  	regulator_disable(st->vref);
>  reg_disable_resume:
> @@ -2481,7 +2604,29 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>  	return ret;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
> +static int __maybe_unused at91_adc_runtime_suspend(struct device *dev)

See below, but these no longer need to be marked __maybe_unused.

> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	clk_disable(st->per_clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused at91_adc_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	return clk_enable(st->per_clk);
> +}
> +
> +static const struct dev_pm_ops __maybe_unused at91_adc_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(at91_adc_suspend, at91_adc_resume)
> +	SET_RUNTIME_PM_OPS(at91_adc_runtime_suspend, at91_adc_runtime_resume,
> +			   NULL)
Use the new SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() + drop the __maybe_unused.

Squash the next patch into here so that the pm_ptr() magic will allow the compiler
to clean these out if not used.

Paul Cercueil recently did some work to simplify all this stuff.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/pm.h?id=0ae101fdd3297b7165755340e05386f1e1379709

> +};
>  
>  static const struct of_device_id at91_adc_dt_match[] = {
>  	{

Re: [PATCH 15/16] iio: adc: at91-sama5d2_adc: add runtime pm support

[Claudiu.Beznea]

On 11.06.2022 19:48, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:32:12 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> Add runtime PM support by disabling/enabling ADC's peripheral clock.
>> On simple conversion the ADC's clock is kept enabled just while the
>> conversion is in progress. This includes also temperature conversion.
>> For triggers and touch conversions the ADC clock is kept enabled while
>> the triggers or touch are enabled.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Various comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 201 +++++++++++++++++++++++++----
>>  1 file changed, 173 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 1a6788566969..5d9ad51d0920 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/iio/triggered_buffer.h>
>>  #include <linux/nvmem-consumer.h>
>>  #include <linux/pinctrl/consumer.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/regulator/consumer.h>
>>
>>  #include <dt-bindings/iio/adc/at91-sama5d2_adc.h>
>> @@ -600,6 +601,7 @@ struct at91_adc_state {
>>       struct at91_adc_touch           touch_st;
>>       struct at91_adc_temp            temp_st;
>>       struct iio_dev                  *indio_dev;
>> +     struct device                   *dev;
>>       /* Ensure naturally aligned timestamp */
>>       u16                             buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
>>       /*
>> @@ -844,10 +846,16 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>>                              u32 oversampling_ratio, u32 trackx)
>>  {
>>       /* configure the extended mode register */
>> -     unsigned int emr = at91_adc_readl(st, EMR);
>> +     unsigned int emr;
>>       unsigned int osr_mask = st->soc_info.platform->osr_mask;
>>       unsigned int osr_vals = st->soc_info.platform->osr_vals;
>> +     int ret;
>> +
>> +     ret = pm_runtime_resume_and_get(st->dev);
>> +     if (ret < 0)
>> +             return ret;
> 
> In this particular case, it might be cleaner to introduce a wrapper
> function that deals with the power management and then calls this one.
> Gets rid of the ugly gotos out of the switch statement.

Hm.. I'll see how I can do it.

> 
>>
>> +     emr = at91_adc_readl(st, EMR);
>>       /* select oversampling per single trigger event */
>>       emr |= AT91_SAMA5D2_EMR_ASTE(1);
>>
>> @@ -857,32 +865,42 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>>       /* select oversampling ratio from configuration */
>>       switch (oversampling_ratio) {
>>       case AT91_OSR_1SAMPLES:
>> -             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES)))
>> -                     return -EINVAL;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES))) {
>> +                     ret = -EINVAL;
>> +                     goto pm_runtime_put;
>> +             }
>>               emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_1SAMPLES,
>>                                           osr_mask);
>>               break;
>>       case AT91_OSR_4SAMPLES:
>> -             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES)))
>> -                     return -EINVAL;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES))) {
>> +                     ret = -EINVAL;
>> +                     goto pm_runtime_put;
>> +             }
>>               emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_4SAMPLES,
>>                                           osr_mask);
>>               break;
>>       case AT91_OSR_16SAMPLES:
>> -             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES)))
>> -                     return -EINVAL;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES))) {
>> +                     ret = -EINVAL;
>> +                     goto pm_runtime_put;
>> +             }
>>               emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>>                                           osr_mask);
>>               break;
>>       case AT91_OSR_64SAMPLES:
>> -             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
>> -                     return -EINVAL;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES))) {
>> +                     ret = -EINVAL;
>> +                     goto pm_runtime_put;
>> +             }
>>               emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
>>                                           osr_mask);
>>               break;
>>       case AT91_OSR_256SAMPLES:
>> -             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
>> -                     return -EINVAL;
>> +             if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES))) {
>> +                     ret = -EINVAL;
>> +                     goto pm_runtime_put;
>> +             }
>>               emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
>>                                           osr_mask);
>>               break;
>> @@ -894,7 +912,10 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>>
>>       st->oversampling_ratio = oversampling_ratio;
>>
>> -     return 0;
>> +pm_runtime_put:
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     pm_runtime_put_autosuspend(st->dev);
>> +     return ret;
>>  }
>>
>>  static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
>> @@ -947,15 +968,22 @@ static void at91_adc_adjust_val_osr_array(struct at91_adc_state *st, void *buf,
>>  static int at91_adc_configure_touch(struct at91_adc_state *st, bool state)
>>  {
>>       u32 clk_khz = st->current_sample_rate / 1000;
>> -     int i = 0;
>> +     int i = 0, ret;
>>       u16 pendbc;
>>       u32 tsmr, acr;
>>
>> -     if (!state) {
>> +     if (state) {
>> +             ret = pm_runtime_resume_and_get(st->dev);
>> +             if (ret < 0)
>> +                     return ret;
>> +     } else {
>>               /* disabling touch IRQs and setting mode to no touch enabled */
>>               at91_adc_writel(st, IDR,
>>                               AT91_SAMA5D2_IER_PEN | AT91_SAMA5D2_IER_NOPEN);
>>               at91_adc_writel(st, TSMR, 0);
>> +
>> +             pm_runtime_mark_last_busy(st->dev);
>> +             pm_runtime_put_autosuspend(st->dev);
>>               return 0;
>>       }
>>       /*
>> @@ -1100,8 +1128,16 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>>  {
>>       struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>>       struct at91_adc_state *st = iio_priv(indio);
>> -     u32 status = at91_adc_readl(st, TRGR);
>> +     u32 status;
>> +     int ret;
>> +
>> +     if (state) {
>> +             ret = pm_runtime_resume_and_get(st->dev);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }
>>
>> +     status = at91_adc_readl(st, TRGR);
>>       /* clear TRGMOD */
>>       status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
>>
>> @@ -1111,6 +1147,11 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>>       /* set/unset hw trigger */
>>       at91_adc_writel(st, TRGR, status);
>>
>> +     if (!state) {
>> +             pm_runtime_mark_last_busy(st->dev);
>> +             pm_runtime_put_autosuspend(st->dev);
>> +     }
>> +
>>       return 0;
>>  }
>>
>> @@ -1268,11 +1309,15 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
>>       if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
>>               return -EINVAL;
>>
>> +     ret = pm_runtime_resume_and_get(st->dev);
> 
> This seems unusual.  I'd normally expect to see runtime pm left on whenever
> a buffer is in use, but in this case you seem to let it autosuspend.
> 
> That 'might' be fine as you might hit it often enough that it stays up whilst
> doing DMA but it certainly seems odd and less than efficient.
> Or possibly the use of the trigger is enough to keep it up.

This is here because at91_adc_buffer_prepare() is called though
.hwfifo_set_watermark which is called in iio_enable_buffers() before
iio_trigger_attach_poll_func() which calls in turn
at91_adc_configure_trigger() that turns on the power for the whole duration
the buffers are enabled.

It is necessary to have runtime resume here to be able to do proper
register settings.

> 
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       /* we continue with the triggered buffer */
>>       ret = at91_adc_dma_start(indio_dev);
>>       if (ret) {
>>               dev_err(&indio_dev->dev, "buffer prepare failed\n");
>> -             return ret;
>> +             goto pm_runtime_put;
>>       }
>>
>>       for_each_set_bit(bit, indio_dev->active_scan_mask,
>> @@ -1295,12 +1340,16 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
>>       if (at91_adc_buffer_check_use_irq(indio_dev, st))
>>               at91_adc_writel(st, IER, AT91_SAMA5D2_IER_DRDY);
>>
>> -     return 0;
>> +pm_runtime_put:
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     pm_runtime_put_autosuspend(st->dev);
>> +     return ret;
>>  }
>>
>>  static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>  {
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int ret;
>>       u8 bit;
>>
>>       /* check if we are disabling triggered buffer or the touchscreen */
>> @@ -1311,6 +1360,10 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>       if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
>>               return -EINVAL;
>>
>> +     ret = pm_runtime_resume_and_get(st->dev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       /*
>>        * For each enable channel we must disable it in hardware.
>>        * In the case of DMA, we must read the last converted value
>> @@ -1346,6 +1399,9 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>       if (st->dma_st.dma_chan)
>>               dmaengine_terminate_sync(st->dma_st.dma_chan);
>>
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     pm_runtime_put_autosuspend(st->dev);
>> +
>>       return 0;
>>  }
>>
>> @@ -1534,12 +1590,17 @@ static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq,
>>  {
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>>       unsigned f_per, prescal, startup, mr;
>> +     int ret;
>>
>>       f_per = clk_get_rate(st->per_clk);
>>       prescal = (f_per / (2 * freq)) - 1;
>>
>>       startup = at91_adc_startup_time(startup_time, freq / 1000);
>>
>> +     ret = pm_runtime_resume_and_get(st->dev);
>> +     if (ret < 0)
>> +             return;
>> +
>>       mr = at91_adc_readl(st, MR);
>>       mr &= ~(AT91_SAMA5D2_MR_STARTUP_MASK | AT91_SAMA5D2_MR_PRESCAL_MASK);
>>       mr |= AT91_SAMA5D2_MR_STARTUP(startup);
>> @@ -1547,6 +1608,9 @@ static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq,
>>       mr |= AT91_SAMA5D2_MR_TRACKTIM(tracktim);
>>       at91_adc_writel(st, MR, mr);
>>
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     pm_runtime_put_autosuspend(st->dev);
>> +
>>       dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u, tracktim=%u\n",
>>               freq, startup, prescal, tracktim);
>>       st->current_sample_rate = freq;
>> @@ -1684,6 +1748,10 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>       u16 tmp_val;
>>       int ret;
>>
>> +     ret = pm_runtime_resume_and_get(st->dev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       /*
>>        * Keep in mind that we cannot use software trigger or touchscreen
>>        * if external trigger is enabled
>> @@ -1695,7 +1763,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>
>>       ret = iio_device_claim_direct_mode(indio_dev);
>>       if (ret)
>> -             return ret;
>> +             goto pm_runtime_put;
>>       if (lock)
>>               mutex_lock(&st->lock);
>>
>> @@ -1707,7 +1775,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>                       mutex_unlock(&st->lock);
>>               iio_device_release_direct_mode(indio_dev);
>>
>> -             return ret;
>> +             goto pm_runtime_put;
>>       }
>>
>>       /* in this case we have a voltage or temperature channel */
>> @@ -1753,6 +1821,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>               mutex_unlock(&st->lock);
>>
>>       iio_device_release_direct_mode(indio_dev);
>> +
>> +pm_runtime_put:
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     pm_runtime_put_autosuspend(st->dev);
>> +
>>       return ret;
>>  }
>>
>> @@ -1804,6 +1877,10 @@ static int at91_adc_read_temp(struct iio_dev *indio_dev,
>>       if (iio_buffer_enabled(indio_dev))
>>               return -EBUSY;
>>
>> +     ret = pm_runtime_resume_and_get(st->dev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       mutex_lock(&st->lock);
>>
>>       at91_adc_temp_sensor_configure(st, true);
>> @@ -1825,6 +1902,10 @@ static int at91_adc_read_temp(struct iio_dev *indio_dev,
>>       /* Revert previous settings. */
>>       at91_adc_temp_sensor_configure(st, false);
>>       mutex_unlock(&st->lock);
>> +
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     pm_runtime_put_autosuspend(st->dev);
>> +
>>       if (ret < 0)
>>               return ret;
>>
>> @@ -2363,13 +2444,19 @@ static int at91_adc_probe(struct platform_device *pdev)
>>
>>       at91_adc_temp_sensor_init(st, &pdev->dev);
>>
>> -     at91_adc_hw_init(indio_dev);
>> -
>>       platform_set_drvdata(pdev, indio_dev);
>> +     st->dev = &pdev->dev;
>> +     pm_runtime_set_autosuspend_delay(st->dev, 500);
>> +     pm_runtime_use_autosuspend(st->dev);
>> +     pm_runtime_set_active(st->dev);
>> +     pm_runtime_enable(st->dev);
>> +     pm_runtime_get_noresume(st->dev);
>> +
>> +     at91_adc_hw_init(indio_dev);
>>
>>       ret = at91_adc_buffer_and_trigger_init(&pdev->dev, indio_dev);
>>       if (ret < 0)
>> -             goto per_clk_disable_unprepare;
>> +             goto err_pm_disable;
>>
>>       if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
>>               dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
>> @@ -2385,10 +2472,18 @@ static int at91_adc_probe(struct platform_device *pdev)
>>       dev_info(&pdev->dev, "version: %x\n",
>>                readl_relaxed(st->base + st->soc_info.platform->layout->VERSION));
>>
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     pm_runtime_put_autosuspend(st->dev);
>> +
>>       return 0;
>>
>>  dma_disable:
>>       at91_adc_dma_disable(st);
>> +err_pm_disable:
>> +     pm_runtime_put_noidle(st->dev);
>> +     pm_runtime_disable(st->dev);
>> +     pm_runtime_set_suspended(st->dev);
>> +     pm_runtime_dont_use_autosuspend(st->dev);
>>  per_clk_disable_unprepare:
>>       clk_disable_unprepare(st->per_clk);
>>  vref_disable:
>> @@ -2402,11 +2497,18 @@ static int at91_adc_remove(struct platform_device *pdev)
>>  {
>>       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     ret = pm_runtime_resume_and_get(st->dev);
>> +     if (ret < 0)
>> +             return ret;
> 
> There isn't much useful that can be done with a return of error from
> a remove() function. Uwe Klein-Konig is busy removing all these returns
> (and eventually changing the prototypes to return void), so don't introduce a new one
> or Uwe will be grumpy :)

Ah, right. Thanks for pointing it.

> 
>>
>>       iio_device_unregister(indio_dev);
>>
>>       at91_adc_dma_disable(st);
>>
>> +     pm_runtime_disable(st->dev);
>> +     pm_runtime_put_noidle(st->dev);
>>       clk_disable_unprepare(st->per_clk);
>>
>>       regulator_disable(st->vref);
>> @@ -2419,6 +2521,11 @@ static __maybe_unused int at91_adc_suspend(struct device *dev)
>>  {
>>       struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>       struct at91_adc_state *st = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     ret = pm_runtime_resume_and_get(st->dev);
>> +     if (ret < 0)
>> +             return ret;
>>
>>       /*
>>        * Do a sofware reset of the ADC before we go to suspend.
>> @@ -2428,7 +2535,8 @@ static __maybe_unused int at91_adc_suspend(struct device *dev)
>>        */
>>       at91_adc_writel(st, CR, AT91_SAMA5D2_CR_SWRST);
>>
>> -     clk_disable_unprepare(st->per_clk);
>> +     pm_runtime_force_suspend(st->dev);
> 
> This confuses me a bit because we know we are already awake (because of
> the pm_runtime_resume_and_get() so we will definitely suspend here and
> I'm fairly sure that means we definitely resume in the _resume()
> below.  Basically our usage counters are I think off by one. If you
> could verify that it doesn't turn back on if we don't have the buffered
> enabled and were previously in runtime suspend state that would
> prove me wrong (which is more than possible - this stuff always gives
> me a headache)

I remember I checked this with continuous suspend/resume having buffers
disabled and no conversion in progress and all went good. I'll double check
it again to be sure all good.

> 
> 
>> +     clk_unprepare(st->per_clk);
>>       regulator_disable(st->vref);
>>       regulator_disable(st->reg);
>>
>> @@ -2453,25 +2561,40 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>>       if (ret)
>>               goto reg_disable_resume;
>>
>> -     ret = clk_prepare_enable(st->per_clk);
>> +     ret = clk_prepare(st->per_clk);
>>       if (ret)
>>               goto vref_disable_resume;
>>
>> +     ret = pm_runtime_force_resume(st->dev);
>> +     if (ret < 0)
>> +             goto clk_unprepare_resume;
>> +
>>       at91_adc_hw_init(indio_dev);
>>
>>       /* reconfiguring trigger hardware state */
>>       if (!iio_buffer_enabled(indio_dev))
> (see below)
>         flip this check so the next block is indented givne
>         exit path will be shared (thus removing the goto).
>         if (iio_buffer_enabled(indio_dev)) {
>                 /* check ...

ok

> 
> 
>> -             return 0;
>> +             goto pm_runtime_put;
>>
>>       /* check if we are enabling triggered buffer or the touchscreen */
>>       if (at91_adc_current_chan_is_touch(indio_dev))
>> -             return at91_adc_configure_touch(st, true);
>> +             ret = at91_adc_configure_touch(st, true);
>>       else
>> -             return at91_adc_configure_trigger(st->trig, true);
>> +             ret = at91_adc_configure_trigger(st->trig, true);
>> +
> (see below)
> 
>                 if (ret < 0)
>                         goto pm_runtime_put;
>         }
> 
>         pm_runtime_mark_last_busy(st->dev);
>         pm_runtime_put_autosuspend(st->dev);
>         return 0;
> 
>> +pm_runtime_put:
> 
> I think this would be easier to follow if you break this up into the
> different cases.

I'll check it.

> 
> pm_runtime_put:
>         pm_runtime_mark_last_busy(st->dev);
>         pm_runtime_put_sync_suspend(st->dev);
> clk_unprepare_resume:
>         ...
> 
>> +     pm_runtime_mark_last_busy(st->dev);
>> +     if (ret < 0) {
>> +             pm_runtime_put_sync_suspend(st->dev);
>> +             goto clk_unprepare_resume;
>> +     } else {
>> +             pm_runtime_put_autosuspend(st->dev);
>> +     }
>>
>>       /* not needed but more explicit */
>>       return 0;
>>
>> +clk_unprepare_resume:
>> +     clk_unprepare(st->per_clk);
>>  vref_disable_resume:
>>       regulator_disable(st->vref);
>>  reg_disable_resume:
>> @@ -2481,7 +2604,29 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>>       return ret;
>>  }
>>
>> -static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
>> +static int __maybe_unused at91_adc_runtime_suspend(struct device *dev)
> 
> See below, but these no longer need to be marked __maybe_unused.
> 
>> +{
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +
>> +     clk_disable(st->per_clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static int __maybe_unused at91_adc_runtime_resume(struct device *dev)
>> +{
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct at91_adc_state *st = iio_priv(indio_dev);
>> +
>> +     return clk_enable(st->per_clk);
>> +}
>> +
>> +static const struct dev_pm_ops __maybe_unused at91_adc_pm_ops = {
>> +     SET_SYSTEM_SLEEP_PM_OPS(at91_adc_suspend, at91_adc_resume)
>> +     SET_RUNTIME_PM_OPS(at91_adc_runtime_suspend, at91_adc_runtime_resume,
>> +                        NULL)
> Use the new SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() + drop the __maybe_unused.
> 
> Squash the next patch into here so that the pm_ptr() magic will allow the compiler
> to clean these out if not used.
> 
> Paul Cercueil recently did some work to simplify all this stuff.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/pm.h?id=0ae101fdd3297b7165755340e05386f1e1379709

Nice, thanks for poining it.

> 
>> +};
>>
>>  static const struct of_device_id at91_adc_dt_match[] = {
>>       {
> 

Re: [PATCH 15/16] iio: adc: at91-sama5d2_adc: add runtime pm support

[Jonathan Cameron]

...


> >> @@ -1268,11 +1309,15 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
> >>       if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
> >>               return -EINVAL;
> >>
> >> +     ret = pm_runtime_resume_and_get(st->dev);  
> > 
> > This seems unusual.  I'd normally expect to see runtime pm left on whenever
> > a buffer is in use, but in this case you seem to let it autosuspend.
> > 
> > That 'might' be fine as you might hit it often enough that it stays up whilst
> > doing DMA but it certainly seems odd and less than efficient.
> > Or possibly the use of the trigger is enough to keep it up.  
> 
> This is here because at91_adc_buffer_prepare() is called though
> .hwfifo_set_watermark which is called in iio_enable_buffers() before
> iio_trigger_attach_poll_func() which calls in turn
> at91_adc_configure_trigger() that turns on the power for the whole duration
> the buffers are enabled.
> 
> It is necessary to have runtime resume here to be able to do proper
> register settings.

Runtime pm is reference counted so I'd take the view that either
* buffer being enabled or
* trigger being enabled

are reasons to leave the device powered up.  That'll result in
needing it to be powered down in both buffer and trigger disables
but that shoudl be fine.

...

Thanks,

Jonathan

[PATCH 16/16] iio: adc: at91-sama5d2_adc: use pm_ptr()

[Claudiu Beznea]

Use pm_ptr() on the assignment of at91_adc_pm_ops to
at91_adc_driver.driver.pm.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 5d9ad51d0920..221c09532b38 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -2647,7 +2647,7 @@ static struct platform_driver at91_adc_driver = {
 	.driver = {
 		.name = "at91-sama5d2_adc",
 		.of_match_table = at91_adc_dt_match,
-		.pm = &at91_adc_pm_ops,
+		.pm = pm_ptr(&at91_adc_pm_ops),
 	},
 };
 module_platform_driver(at91_adc_driver)
-- 
2.34.1

Re: [PATCH 16/16] iio: adc: at91-sama5d2_adc: use pm_ptr()

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:32:13 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> Use pm_ptr() on the assignment of at91_adc_pm_ops to
> at91_adc_driver.driver.pm.

Squash this into the previous patch.

> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 5d9ad51d0920..221c09532b38 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -2647,7 +2647,7 @@ static struct platform_driver at91_adc_driver = {
>  	.driver = {
>  		.name = "at91-sama5d2_adc",
>  		.of_match_table = at91_adc_dt_match,
> -		.pm = &at91_adc_pm_ops,
> +		.pm = pm_ptr(&at91_adc_pm_ops),
>  	},
>  };
>  module_platform_driver(at91_adc_driver)

Re: [PATCH 00/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[Jonathan Cameron]

On Thu, 9 Jun 2022 11:31:57 +0300
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> Hi,
> 
> The following series add support for temperature sensor available on
> SAMA7G5.
> 
> Temperature sensor available on SAMA7G5 provides 2 outputs VTEMP and VBG.
> VTEMP is proportional to the absolute temperature voltage and VBG is a
> quasi-temperature independent voltage. Both are necessary in computing
> the temperature (for better accuracy). Also, for better accuracy the
> following settings were imposed when measusing the temperature:
> oversampling rate of 256, sampling frequency of 10MHz, a startup time of
> 512 ticks, MR.tracktim=0xf, EMR.trackx=0x3.
> 
> For computing the temperature measured by ADC calibration data is
> necessary. This is provided via OTP memory available on SAMA7G5.
> 
> Patches 1/16-3/16 provides some fixes.
> Patches 3/16-12/16 prepares for the addition of temperature sensor
> support.
> Patch 13/16 adds the temperature sensor support.
> 
> Along with temperature sensor support I took the chance and added
> runtime PM support in this series, too (handled in patch 15/16).
> 
> The rest of patches in this series are minor cleanups.
> 
> Thank you,
> Claudiu Beznea

Hi CLaudiu,

Those patches I haven't replied to individually look good to me.

Thanks,

Jonathan

> 
> Claudiu Beznea (16):
>   iio: adc: at91-sama5d2_adc: fix AT91_SAMA5D2_MR_TRACKTIM_MAX
>   iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq
>   iio: adc: at91-sama5d2_adc: exit from write_raw() when buffers are
>     enabled
>   iio: adc: at91-sama5d2_adc: handle different EMR.OSR for different hw
>     versions
>   iio: adc: at91-sama5d2_adc: adjust osr based on specific platform data
>   iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio
>   iio: adc: at91-sama5d2_adc: simplify the code in
>     at91_adc_read_info_raw()
>   iio: adc: at91-sama5d2_adc: move oversampling storage in its function
>   iio: adc: at91-sama5d2_adc: update trackx on emr
>   iio: adc: at91-sama5d2_adc: add startup and tracktim as parameter for
>     at91_adc_setup_samp_freq()
>   iio: adc: at91-sama5d2_adc: add locking parameter to
>     at91_adc_read_info_raw()
>   dt-bindings: iio: adc: at91-sama5d2_adc: add id for temperature
>     channel
>   iio: adc: at91-sama5d2_adc: add support for temperature sensor
>   iio: adc: at91-sama5d2_adc: add empty line after functions
>   iio: adc: at91-sama5d2_adc: add runtime pm support
>   iio: adc: at91-sama5d2_adc: use pm_ptr()
> 
>  drivers/iio/adc/at91-sama5d2_adc.c            | 633 +++++++++++++++---
>  .../dt-bindings/iio/adc/at91-sama5d2_adc.h    |   3 +
>  2 files changed, 548 insertions(+), 88 deletions(-)
> 

Re: [PATCH 00/16] iio: adc: at91-sama5d2_adc: add support for temperature sensor

[Claudiu.Beznea]

On 11.06.2022 21:16, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:31:57 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> Hi,
>>
>> The following series add support for temperature sensor available on
>> SAMA7G5.
>>
>> Temperature sensor available on SAMA7G5 provides 2 outputs VTEMP and VBG.
>> VTEMP is proportional to the absolute temperature voltage and VBG is a
>> quasi-temperature independent voltage. Both are necessary in computing
>> the temperature (for better accuracy). Also, for better accuracy the
>> following settings were imposed when measusing the temperature:
>> oversampling rate of 256, sampling frequency of 10MHz, a startup time of
>> 512 ticks, MR.tracktim=0xf, EMR.trackx=0x3.
>>
>> For computing the temperature measured by ADC calibration data is
>> necessary. This is provided via OTP memory available on SAMA7G5.
>>
>> Patches 1/16-3/16 provides some fixes.
>> Patches 3/16-12/16 prepares for the addition of temperature sensor
>> support.
>> Patch 13/16 adds the temperature sensor support.
>>
>> Along with temperature sensor support I took the chance and added
>> runtime PM support in this series, too (handled in patch 15/16).
>>
>> The rest of patches in this series are minor cleanups.
>>
>> Thank you,
>> Claudiu Beznea
> 
> Hi CLaudiu,
> 
> Those patches I haven't replied to individually look good to me.

Hi, Jonathan,

Thank you for your review!

> > Thanks,
> 
> Jonathan
> 
>>
>> Claudiu Beznea (16):
>>   iio: adc: at91-sama5d2_adc: fix AT91_SAMA5D2_MR_TRACKTIM_MAX
>>   iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq
>>   iio: adc: at91-sama5d2_adc: exit from write_raw() when buffers are
>>     enabled
>>   iio: adc: at91-sama5d2_adc: handle different EMR.OSR for different hw
>>     versions
>>   iio: adc: at91-sama5d2_adc: adjust osr based on specific platform data
>>   iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio
>>   iio: adc: at91-sama5d2_adc: simplify the code in
>>     at91_adc_read_info_raw()
>>   iio: adc: at91-sama5d2_adc: move oversampling storage in its function
>>   iio: adc: at91-sama5d2_adc: update trackx on emr
>>   iio: adc: at91-sama5d2_adc: add startup and tracktim as parameter for
>>     at91_adc_setup_samp_freq()
>>   iio: adc: at91-sama5d2_adc: add locking parameter to
>>     at91_adc_read_info_raw()
>>   dt-bindings: iio: adc: at91-sama5d2_adc: add id for temperature
>>     channel
>>   iio: adc: at91-sama5d2_adc: add support for temperature sensor
>>   iio: adc: at91-sama5d2_adc: add empty line after functions
>>   iio: adc: at91-sama5d2_adc: add runtime pm support
>>   iio: adc: at91-sama5d2_adc: use pm_ptr()
>>
>>  drivers/iio/adc/at91-sama5d2_adc.c            | 633 +++++++++++++++---
>>  .../dt-bindings/iio/adc/at91-sama5d2_adc.h    |   3 +
>>  2 files changed, 548 insertions(+), 88 deletions(-)
>>
>