diff mbox series

[RFC,v2,2/4] iio: adc: ad7380: enable regmap cache

Message ID 20241224-ad7380-add-alert-support-v2-2-7c89b2bf7cb3@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: ad7380: add alert support | expand

Commit Message

Julien Stephan Dec. 24, 2024, 9:34 a.m. UTC
Enable regmap cache, to avoid useless access on spi bus.
Don't store anymore the oversampling ratio in private data structure.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 98 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 84 insertions(+), 14 deletions(-)

Comments

Uwe Kleine-König Dec. 27, 2024, 8:48 a.m. UTC | #1
On Tue, Dec 24, 2024 at 10:34:31AM +0100, Julien Stephan wrote:
> Enable regmap cache, to avoid useless access on spi bus.
> Don't store anymore the oversampling ratio in private data structure.

I would split that. There are a few changes in this patch that I don't
understand, e.g. why does the return type of ad7380_update_xfers()
change?

Best regards
Uwe
Jonathan Cameron Dec. 28, 2024, 2:02 p.m. UTC | #2
On Tue, 24 Dec 2024 10:34:31 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> Enable regmap cache, to avoid useless access on spi bus.
> Don't store anymore the oversampling ratio in private data structure.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>

A few really minor things inline,

Jonathan

>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> @@ -692,6 +709,37 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  	return ret;
>  }
>  
> +/**
> + * ad7380_regval_to_osr - convert OSR register value to ratio
> + * @regval: register value to check
> + *
> + * Returns: the ratio corresponding to the OSR register. If regval is not in
> + * bound, return 1 (oversampling disabled)
> + *
> + */
> +static int ad7380_regval_to_osr(int regval)

Make regval an unsigned int and yout can drop one test.
The FIELD_GET is never going to give you a signed value anyway.

> +{
> +	if (regval < 0 || regval >= ARRAY_SIZE(ad7380_oversampling_ratios))
> +		return 1;
> +
> +	return ad7380_oversampling_ratios[regval];
> +}
> +
> +static int ad7380_get_osr(struct ad7380_state *st, int *val)
> +{
> +	int tmp;
> +	int ret = 0;

No point in initializing.

> +
> +	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	*val = ad7380_regval_to_osr(FIELD_GET(AD7380_CONFIG1_OSR, tmp));
> +
> +	return 0;
> +}
> +
> +

Trivial but one blank line almost always enough!

>  /*
Jonathan Cameron Dec. 28, 2024, 2:07 p.m. UTC | #3
On Fri, 27 Dec 2024 09:48:35 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> On Tue, Dec 24, 2024 at 10:34:31AM +0100, Julien Stephan wrote:
> > Enable regmap cache, to avoid useless access on spi bus.
> > Don't store anymore the oversampling ratio in private data structure.  
> 
> I would split that. 

Splitting it probably makes sense, though the regcache enabling patch will be small
(which is fine).

> There are a few changes in this patch that I don't
> understand, e.g. why does the return type of ad7380_update_xfers()
> change?

On first call of this, the register contents may not be in the cache
so you might get an error from the bus read. Even if we known it is in the cache
(and from a quick glance I'm not sure we do as we haven't forced a full
fill of the regcache or accessed this register) from a local code point of view
it is correct to handle the potential error and pass it to higher layers.

Jonathan


> 
> Best regards
> Uwe
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 4e26e0e7ac1d5a1c4c67118dbc34f2921fc171a4..b49067c36fdd1bb0e760faf71d7fa0c8c1f610e9 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -582,7 +582,6 @@  struct ad7380_state {
 	const struct ad7380_chip_info *chip_info;
 	struct spi_device *spi;
 	struct regmap *regmap;
-	unsigned int oversampling_ratio;
 	bool resolution_boost_enabled;
 	unsigned int ch;
 	bool seq;
@@ -663,6 +662,20 @@  static int ad7380_regmap_reg_read(void *context, unsigned int reg,
 	return 0;
 }
 
+static const struct reg_default ad7380_reg_defaults[] = {
+	{ AD7380_REG_ADDR_ALERT_LOW_TH, 0x800 },
+	{ AD7380_REG_ADDR_ALERT_HIGH_TH, 0x7FF },
+};
+
+static const struct regmap_range ad7380_volatile_reg_ranges[] = {
+	regmap_reg_range(AD7380_REG_ADDR_CONFIG2, AD7380_REG_ADDR_ALERT),
+};
+
+static const struct regmap_access_table ad7380_volatile_regs = {
+	.yes_ranges = ad7380_volatile_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad7380_volatile_reg_ranges),
+};
+
 static const struct regmap_config ad7380_regmap_config = {
 	.reg_bits = 3,
 	.val_bits = 12,
@@ -670,6 +683,10 @@  static const struct regmap_config ad7380_regmap_config = {
 	.reg_write = ad7380_regmap_reg_write,
 	.max_register = AD7380_REG_ADDR_ALERT_HIGH_TH,
 	.can_sleep = true,
+	.reg_defaults = ad7380_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(ad7380_reg_defaults),
+	.volatile_table = &ad7380_volatile_regs,
+	.cache_type = REGCACHE_MAPLE,
 };
 
 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
@@ -692,6 +709,37 @@  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 	return ret;
 }
 
+/**
+ * ad7380_regval_to_osr - convert OSR register value to ratio
+ * @regval: register value to check
+ *
+ * Returns: the ratio corresponding to the OSR register. If regval is not in
+ * bound, return 1 (oversampling disabled)
+ *
+ */
+static int ad7380_regval_to_osr(int regval)
+{
+	if (regval < 0 || regval >= ARRAY_SIZE(ad7380_oversampling_ratios))
+		return 1;
+
+	return ad7380_oversampling_ratios[regval];
+}
+
+static int ad7380_get_osr(struct ad7380_state *st, int *val)
+{
+	int tmp;
+	int ret = 0;
+
+	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
+	if (ret)
+		return ret;
+
+	*val = ad7380_regval_to_osr(FIELD_GET(AD7380_CONFIG1_OSR, tmp));
+
+	return 0;
+}
+
+
 /*
  * When switching channel, the ADC require an additional settling time.
  * According to the datasheet, data is value on the third CS low. We already
@@ -707,11 +755,15 @@  static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
 			.unit = SPI_DELAY_UNIT_NSECS,
 		}
 	};
-	int ret;
+	int oversampling_ratio, ret;
 
 	if (st->ch == ch)
 		return 0;
 
+	ret = ad7380_get_osr(st, &oversampling_ratio);
+	if (ret)
+		return ret;
+
 	ret = regmap_update_bits(st->regmap,
 				 AD7380_REG_ADDR_CONFIG1,
 				 AD7380_CONFIG1_CH,
@@ -722,9 +774,9 @@  static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
 
 	st->ch = ch;
 
-	if (st->oversampling_ratio > 1)
+	if (oversampling_ratio > 1)
 		xfer.delay.value = T_CONVERT_0_NS +
-			T_CONVERT_X_NS * (st->oversampling_ratio - 1) *
+			T_CONVERT_X_NS * (oversampling_ratio - 1) *
 			st->chip_info->num_simult_channels / AD7380_NUM_SDO_LINES;
 
 	return spi_sync_transfer(st->spi, &xfer, 1);
@@ -735,20 +787,25 @@  static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
  * @st:		device instance specific state
  * @scan_type:	current scan type
  */
-static void ad7380_update_xfers(struct ad7380_state *st,
+static int ad7380_update_xfers(struct ad7380_state *st,
 				const struct iio_scan_type *scan_type)
 {
 	struct spi_transfer *xfer = st->seq ? st->seq_xfer : st->normal_xfer;
 	unsigned int t_convert = T_CONVERT_NS;
+	int oversampling_ratio, ret;
 
 	/*
 	 * In the case of oversampling, conversion time is higher than in normal
 	 * mode. Technically T_CONVERT_X_NS is lower for some chips, but we use
 	 * the maximum value for simplicity for now.
 	 */
-	if (st->oversampling_ratio > 1)
+	ret = ad7380_get_osr(st, &oversampling_ratio);
+	if (ret)
+		return ret;
+
+	if (oversampling_ratio > 1)
 		t_convert = T_CONVERT_0_NS + T_CONVERT_X_NS *
-			(st->oversampling_ratio - 1) *
+			(oversampling_ratio - 1) *
 			st->chip_info->num_simult_channels / AD7380_NUM_SDO_LINES;
 
 	if (st->seq) {
@@ -761,7 +818,7 @@  static void ad7380_update_xfers(struct ad7380_state *st,
 			st->chip_info->num_simult_channels;
 		xfer[3].rx_buf = xfer[2].rx_buf + xfer[2].len;
 		/* Additional delay required here when oversampling is enabled */
-		if (st->oversampling_ratio > 1)
+		if (oversampling_ratio > 1)
 			xfer[2].delay.value = t_convert;
 		else
 			xfer[2].delay.value = 0;
@@ -773,6 +830,8 @@  static void ad7380_update_xfers(struct ad7380_state *st,
 		xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
 			st->chip_info->num_simult_channels;
 	}
+
+	return 0;
 }
 
 static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
@@ -780,6 +839,7 @@  static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 	struct ad7380_state *st = iio_priv(indio_dev);
 	const struct iio_scan_type *scan_type;
 	struct spi_message *msg = &st->normal_msg;
+	int ret;
 
 	/*
 	 * Currently, we always read all channels at the same time. The scan_type
@@ -791,7 +851,6 @@  static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 
 	if (st->chip_info->has_mux) {
 		unsigned int index;
-		int ret;
 
 		/*
 		 * Depending on the requested scan_mask and current state,
@@ -822,7 +881,9 @@  static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 
 	}
 
-	ad7380_update_xfers(st, scan_type);
+	ret = ad7380_update_xfers(st, scan_type);
+	if (ret)
+		return ret;
 
 	return spi_optimize_message(st->spi, msg);
 }
@@ -895,7 +956,9 @@  static int ad7380_read_direct(struct ad7380_state *st, unsigned int scan_index,
 			return ret;
 	}
 
-	ad7380_update_xfers(st, scan_type);
+	ret = ad7380_update_xfers(st, scan_type);
+	if (ret)
+		return ret;
 
 	ret = spi_sync(st->spi, &st->normal_msg);
 	if (ret < 0)
@@ -973,7 +1036,16 @@  static int ad7380_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		*val = st->oversampling_ratio;
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7380_get_osr(st, val);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		if (ret)
+			return ret;
 
 		return IIO_VAL_INT;
 	default:
@@ -1049,7 +1121,6 @@  static int ad7380_write_raw(struct iio_dev *indio_dev,
 		if (ret)
 			goto err;
 
-		st->oversampling_ratio = val;
 		st->resolution_boost_enabled = boost;
 
 		/*
@@ -1109,7 +1180,6 @@  static int ad7380_init(struct ad7380_state *st, bool external_ref_en)
 	}
 
 	/* This is the default value after reset. */
-	st->oversampling_ratio = 1;
 	st->ch = 0;
 	st->seq = false;