[PATCH v2 2/6] iio: adc: ti-ads1298: fix power-up and clock handling

[Stephan Costa Barros <stephanbrrs8@gmail.com>]

Fix three issues with the power-up sequence and the device clock:

- The datasheet powers up the supplies first, then the clock, then
  waits t_POR = 2^18 clock cycles (128 ms at 2.048 MHz) before the
  reset. The driver enabled an external clock and the vref supply
  before AVDD, reset right away, and did not wait for the 150 ms
  start-up of the internal reference. Follow the datasheet. Probe takes
  about 280 ms longer.

- The binding describes one optional clock and no clock-names, but the
  driver asked for the clock named "clk". With a device tree that
  follows the binding, the lookup fails, the optional getter returns
  NULL, and a clock given in the device tree is never enabled, while the
  sampling frequency is computed from the nominal 2.048 MHz. Ask for the
  first clock of the node; a device tree with clock-names = "clk" still
  gets the same clock.

- A command takes 4 cycles of the device clock to decode, the minimum
  reset pulse is 2 cycles, and the reset takes 18 more. These were
  converted with the nominal 2.048 MHz, so a command ends after a fixed
  2 us, short of 4 cycles with an external clock below 2 MHz, which the
  ADS129x allows down to 1.94 MHz. Convert them with the rate of the
  clock in use. At 2.048 MHz the delays do not change.

The clock lookup was seen to fail on a Raspberry Pi 4, with a
fixed-clock in the device tree. The power-up order was found by review
and reproduced on an emulated device only. The decode time was found
with an emulated device; it is not seen on hardware running at
2.048 MHz.

Fixes: 00ef7708fa60 ("iio: adc: ti-ads1298: Add driver")
Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
---
 drivers/iio/adc/ti-ads1298.c | 73 +++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index 1d32b1f47..f5965b994 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -84,8 +84,12 @@
 #define ADS1298_MAX_CHANNELS	8
 #define ADS1298_BITS_PER_SAMPLE	24
 #define ADS1298_CLK_RATE_HZ	2048000
-#define ADS1298_CLOCKS_TO_USECS(x) \
-		(DIV_ROUND_UP((x) * MICROHZ_PER_HZ, ADS1298_CLK_RATE_HZ))
+/* A command takes 4 clock cycles to decode, before the next one may start */
+#define ADS1298_DECODE_CLOCKS	4
+/* Power-on reset: wait 2^18 clock cycles before the reset */
+#define ADS1298_TPOR_CLOCKS	BIT(18)
+/* Start-up time of the internal reference */
+#define ADS1298_REF_START_UP_MS	150
 /*
  * Read/write register commands require 4 clocks to decode, for speeds above
  * 2x the clock rate, this would require extra time between the command byte and
@@ -106,6 +110,7 @@ struct ads1298_private {
 	struct regulator *reg_avdd;
 	struct regulator *reg_vref;
 	struct clk *clk;
+	unsigned long clk_rate;
 	struct regmap *regmap;
 	struct completion completion;
 	struct iio_trigger *trig;
@@ -180,6 +185,12 @@ static const struct iio_chan_spec ads1298_channels[] = {
 	ADS1298_CHAN(7),
 };
 
+static unsigned int ads1298_clocks_to_usecs(struct ads1298_private *priv,
+					    unsigned int clocks)
+{
+	return DIV_ROUND_UP(clocks * USEC_PER_SEC, priv->clk_rate);
+}
+
 static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
 {
 	struct spi_transfer xfer = {
@@ -188,7 +199,7 @@ static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
 		.len = 1,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
-			.value = 2,
+			.value = ads1298_clocks_to_usecs(priv, ADS1298_DECODE_CLOCKS),
 			.unit = SPI_DELAY_UNIT_USECS,
 		},
 	};
@@ -433,7 +444,7 @@ static int ads1298_reg_write(void *context, unsigned int reg, unsigned int val)
 		.len = 3,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
-			.value = 2,
+			.value = ads1298_clocks_to_usecs(priv, ADS1298_DECODE_CLOCKS),
 			.unit = SPI_DELAY_UNIT_USECS,
 		},
 	};
@@ -454,7 +465,7 @@ static int ads1298_reg_read(void *context, unsigned int reg, unsigned int *val)
 		.len = 3,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
-			.value = 2,
+			.value = ads1298_clocks_to_usecs(priv, ADS1298_DECODE_CLOCKS),
 			.unit = SPI_DELAY_UNIT_USECS,
 		},
 	};
@@ -730,7 +741,14 @@ static int ads1298_init(struct iio_dev *indio_dev)
 		if (regulator_get_voltage(priv->reg_avdd) >= 4400000)
 			val |= ADS1298_MASK_CONFIG3_VREF_4V;
 	}
-	return regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
+	ret = regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
+	if (ret)
+		return ret;
+
+	if (!priv->reg_vref)
+		msleep(ADS1298_REF_START_UP_MS);
+
+	return 0;
 }
 
 static int ads1298_probe(struct spi_device *spi)
@@ -739,6 +757,7 @@ static int ads1298_probe(struct spi_device *spi)
 	struct iio_dev *indio_dev;
 	struct device *dev = &spi->dev;
 	struct gpio_desc *reset_gpio;
+	unsigned long clk_rate;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
@@ -753,6 +772,19 @@ static int ads1298_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(reset_gpio),
 				     "Cannot get reset GPIO\n");
 
+	priv->reg_avdd = devm_regulator_get(dev, "avdd");
+	if (IS_ERR(priv->reg_avdd))
+		return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
+				     "Failed to get avdd regulator\n");
+
+	ret = regulator_enable(priv->reg_avdd);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable avdd regulator\n");
+
+	ret = devm_add_action_or_reset(dev, ads1298_reg_disable, priv->reg_avdd);
+	if (ret)
+		return ret;
+
 	/* VREF can be supplied externally, otherwise use internal reference */
 	priv->reg_vref = devm_regulator_get_optional(dev, "vref");
 	if (IS_ERR(priv->reg_vref)) {
@@ -771,22 +803,15 @@ static int ads1298_probe(struct spi_device *spi)
 			return ret;
 	}
 
-	priv->clk = devm_clk_get_optional_enabled(dev, "clk");
+	/* The clock follows the supplies */
+	priv->clk = devm_clk_get_optional_enabled(dev, NULL);
 	if (IS_ERR(priv->clk))
 		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clk\n");
 
-	priv->reg_avdd = devm_regulator_get(dev, "avdd");
-	if (IS_ERR(priv->reg_avdd))
-		return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
-				     "Failed to get avdd regulator\n");
-
-	ret = regulator_enable(priv->reg_avdd);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to enable avdd regulator\n");
-
-	ret = devm_add_action_or_reset(dev, ads1298_reg_disable, priv->reg_avdd);
-	if (ret)
-		return ret;
+	clk_rate = priv->clk ? clk_get_rate(priv->clk) : 0;
+	if (!clk_rate)
+		clk_rate = ADS1298_CLK_RATE_HZ;
+	priv->clk_rate = clk_rate;
 
 	priv->spi = spi;
 	init_completion(&priv->completion);
@@ -800,12 +825,15 @@ static int ads1298_probe(struct spi_device *spi)
 	indio_dev->channels = ads1298_channels;
 	indio_dev->info = &ads1298_info;
 
+	/* Power-on reset time, counted from when the clock is valid */
+	msleep(DIV_ROUND_UP(ADS1298_TPOR_CLOCKS * MILLI, clk_rate));
+
 	if (reset_gpio) {
 		/*
 		 * Deassert reset now that clock and power are active.
 		 * Minimum reset pulsewidth is 2 clock cycles.
 		 */
-		fsleep(ADS1298_CLOCKS_TO_USECS(2));
+		fsleep(ads1298_clocks_to_usecs(priv, 2));
 		gpiod_set_value_cansleep(reset_gpio, 0);
 	} else {
 		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
@@ -813,7 +841,7 @@ static int ads1298_probe(struct spi_device *spi)
 			return dev_err_probe(dev, ret, "RESET failed\n");
 	}
 	/* Wait 18 clock cycles for reset command to complete */
-	fsleep(ADS1298_CLOCKS_TO_USECS(18));
+	fsleep(ads1298_clocks_to_usecs(priv, 18));
 
 	ret = ads1298_init(indio_dev);
 	if (ret)
@@ -824,7 +852,8 @@ static int ads1298_probe(struct spi_device *spi)
 	priv->rdata_xfer.rx_buf = priv->rx_buffer;
 	priv->rdata_xfer.len = ADS1298_SPI_RDATA_BUFFER_SIZE(indio_dev->num_channels);
 	/* Must keep CS low for 4 clocks */
-	priv->rdata_xfer.delay.value = 2;
+	priv->rdata_xfer.delay.value = ads1298_clocks_to_usecs(priv,
+							       ADS1298_DECODE_CLOCKS);
 	priv->rdata_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
 	spi_message_init_with_transfers(&priv->rdata_msg, &priv->rdata_xfer, 1);
 	priv->rdata_msg.complete = &ads1298_rdata_complete;
-- 
2.43.0