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 [off-list ref] wrote:

quoted

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 <redacted>
---
 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.

quoted

 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,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel