* [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements
@ 2024-11-11 15:59 Trevor Gamblin
2024-11-11 15:59 ` [PATCH 1/3] iio: adc: ad4695: fix buffered read timing in ad4695_buffer_preenable() Trevor Gamblin
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Trevor Gamblin @ 2024-11-11 15:59 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
David Lechner, Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, linux-kernel, Trevor Gamblin
The AD4695 driver currently operates all SPI reads/writes at the speed
appropriate for register access, rather than the max rate for the bus.
Data reads should ideally operate at the latter speed, but making this
change universally makes it possible for data to be corrupted during use
and for unexpected behavior to occur on driver subsequent driver
binds/unbinds. To solve this, introduce custom regmap bus callbacks for
the driver that explicitly set a lower speed only for these operations.
The first patch in this series is a fix introduced after discovering the
corresponding issue during testing of the callbacks. This is a timing
fix that ensures the AD4695 datasheet's timing specs are met, as before
the busy signal would sometimes fail to toggle again following the end
of the conversion sequence. Adding an extra delay in the form of a blank
transfer before every CS deassert in ad4695_buffer_preenable() allows
this requirement to be met.
The second patch is an improvement that increases the robustness of the
exit message in ad4695_exit_conversion_mode(), this time by adding a
delay before the actual exit command. This helps avoid the possibility
that the exit message will be read as data, causing corruption on some
buffered reads.
For additional context, see:
https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/
Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
Trevor Gamblin (3):
iio: adc: ad4695: fix buffered read timing in ad4695_buffer_preenable()
iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust
iio: adc: ad4695: add custom regmap bus callbacks
drivers/iio/adc/Kconfig | 2 +-
drivers/iio/adc/ad4695.c | 135 ++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 118 insertions(+), 19 deletions(-)
---
base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
change-id: 20241111-tgamblin-ad4695_improvements-7a32a6268c26
Best regards,
--
Trevor Gamblin <tgamblin@baylibre.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] iio: adc: ad4695: fix buffered read timing in ad4695_buffer_preenable()
2024-11-11 15:59 [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
@ 2024-11-11 15:59 ` Trevor Gamblin
2024-11-11 15:59 ` [PATCH 2/3] iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust Trevor Gamblin
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Trevor Gamblin @ 2024-11-11 15:59 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
David Lechner, Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, linux-kernel, Trevor Gamblin
Modify ad4695_buffer_preenable() by adding an extra SPI transfer after
each data read to help ensure that the timing requirement between the
last SCLK rising edge and the next CNV rising edge is met. This requires
a restructure of the buf_read_xfer array in ad4695_state. Also define
AD4695_T_SCK_CNV_DELAY_NS to use for each added transfer. Without this
change it is possible for the data to become corrupted on sequential
buffered reads due to the device not properly exiting conversion mode.
Fixes: 6cc7e4bf2e08 ("iio: adc: ad4695: implement triggered buffer")
Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
drivers/iio/adc/ad4695.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 595ec4158e73..82e930b21c69 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -91,6 +91,7 @@
#define AD4695_T_WAKEUP_SW_MS 3
#define AD4695_T_REFBUF_MS 100
#define AD4695_T_REGCONFIG_NS 20
+#define AD4695_T_SCK_CNV_DELAY_NS 80
#define AD4695_REG_ACCESS_SCLK_HZ (10 * MEGA)
/* Max number of voltage input channels. */
@@ -132,8 +133,13 @@ struct ad4695_state {
unsigned int vref_mv;
/* Common mode input pin voltage. */
unsigned int com_mv;
- /* 1 per voltage and temperature chan plus 1 xfer to trigger 1st CNV */
- struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS + 2];
+ /*
+ * 2 per voltage and temperature chan plus 1 xfer to trigger 1st
+ * CNV. Excluding the trigger xfer, every 2nd xfer only serves
+ * to control CS and add a delay between the last SCLK and next
+ * CNV rising edges.
+ */
+ struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS * 2 + 3];
struct spi_message buf_read_msg;
/* Raw conversion data received. */
u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
@@ -451,9 +457,6 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
xfer->bits_per_word = 16;
xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
xfer->len = 2;
- xfer->cs_change = 1;
- xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
- xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
if (bit == temp_chan_bit) {
temp_en = 1;
@@ -468,6 +471,20 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
}
num_xfer++;
+
+ /*
+ * We need to add a blank xfer in data reads, to meet
+ * the timing requirement of a minimum delay between the
+ * last SCLK rising edge and the CS deassert.
+ */
+ xfer = &st->buf_read_xfer[num_xfer];
+ xfer->delay.value = AD4695_T_SCK_CNV_DELAY_NS;
+ xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
+ xfer->cs_change = 1;
+ xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
+ xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+
+ num_xfer++;
}
/*
--
2.39.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust
2024-11-11 15:59 [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
2024-11-11 15:59 ` [PATCH 1/3] iio: adc: ad4695: fix buffered read timing in ad4695_buffer_preenable() Trevor Gamblin
@ 2024-11-11 15:59 ` Trevor Gamblin
2024-11-11 15:59 ` [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks Trevor Gamblin
2024-11-13 21:18 ` [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
3 siblings, 0 replies; 7+ messages in thread
From: Trevor Gamblin @ 2024-11-11 15:59 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
David Lechner, Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, linux-kernel, Trevor Gamblin
Ensure that conversion mode is successfully exited when the command is
issued by adding an extra transfer beforehand, matching the minimum CNV
high and low times from the AD4695 datasheet. The AD4695 has a quirk
where the exit command only works during a conversion, so guarantee this
happens by triggering a conversion in ad4695_exit_conversion_mode().
Then make this even more robust by ensuring that the exit command is run
at AD4695_REG_ACCESS_SCLK_HZ rather than the bus maximum.
Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
drivers/iio/adc/ad4695.c | 34 ++++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 82e930b21c69..f36c1a1db886 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -92,6 +92,8 @@
#define AD4695_T_REFBUF_MS 100
#define AD4695_T_REGCONFIG_NS 20
#define AD4695_T_SCK_CNV_DELAY_NS 80
+#define AD4695_T_CNVL_NS 80
+#define AD4695_T_CNVH_NS 10
#define AD4695_REG_ACCESS_SCLK_HZ (10 * MEGA)
/* Max number of voltage input channels. */
@@ -364,11 +366,31 @@ static int ad4695_enter_advanced_sequencer_mode(struct ad4695_state *st, u32 n)
*/
static int ad4695_exit_conversion_mode(struct ad4695_state *st)
{
- struct spi_transfer xfer = {
- .tx_buf = &st->cnv_cmd2,
- .len = 1,
- .delay.value = AD4695_T_REGCONFIG_NS,
- .delay.unit = SPI_DELAY_UNIT_NSECS,
+ /*
+ * An extra transfer is needed to trigger a conversion here so
+ * that we can be 100% sure the command will be processed by the
+ * ADC, rather than relying on it to be in the correct state
+ * when this function is called (this chip has a quirk where the
+ * command only works when reading a conversion, and if the
+ * previous conversion was already read then it won't work). The
+ * actual conversion command is then run at the slower
+ * AD4695_REG_ACCESS_SCLK_HZ speed to guarantee this works.
+ */
+ struct spi_transfer xfers[] = {
+ {
+ .delay.value = AD4695_T_CNVL_NS,
+ .delay.unit = SPI_DELAY_UNIT_NSECS,
+ .cs_change = 1,
+ .cs_change_delay.value = AD4695_T_CNVH_NS,
+ .cs_change_delay.unit = SPI_DELAY_UNIT_NSECS,
+ },
+ {
+ .speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+ .tx_buf = &st->cnv_cmd2,
+ .len = 1,
+ .delay.value = AD4695_T_REGCONFIG_NS,
+ .delay.unit = SPI_DELAY_UNIT_NSECS,
+ },
};
/*
@@ -377,7 +399,7 @@ static int ad4695_exit_conversion_mode(struct ad4695_state *st)
*/
st->cnv_cmd2 = AD4695_CMD_EXIT_CNV_MODE << 3;
- return spi_sync_transfer(st->spi, &xfer, 1);
+ return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
}
static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv)
--
2.39.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks
2024-11-11 15:59 [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
2024-11-11 15:59 ` [PATCH 1/3] iio: adc: ad4695: fix buffered read timing in ad4695_buffer_preenable() Trevor Gamblin
2024-11-11 15:59 ` [PATCH 2/3] iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust Trevor Gamblin
@ 2024-11-11 15:59 ` Trevor Gamblin
2024-11-11 20:31 ` Trevor Gamblin
2024-11-11 22:51 ` Trevor Gamblin
2024-11-13 21:18 ` [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
3 siblings, 2 replies; 7+ messages in thread
From: Trevor Gamblin @ 2024-11-11 15:59 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
David Lechner, Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, linux-kernel, Trevor Gamblin
Add a custom implementation of regmap read/write callbacks using the SPI
bus. This allows them to be performed at a lower SCLK rate than data
reads. Previously, all SPI transfers were being performed at a lower
speed, but with this change sample data is read at the max bus speed
while the register reads/writes remain at the lower rate.
Also remove .can_multi_write from the AD4695 driver's regmap_configs, as
this isn't implemented or needed.
For some background context, see:
https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/
Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
drivers/iio/adc/Kconfig | 2 +-
drivers/iio/adc/ad4695.c | 74 +++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 68 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6c4e74420fd2..e0f9d01ce37d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -51,9 +51,9 @@ config AD4130
config AD4695
tristate "Analog Device AD4695 ADC Driver"
depends on SPI
- select REGMAP_SPI
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
+ select REGMAP
help
Say yes here to build support for Analog Devices AD4695 and similar
analog to digital converters (ADC).
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index f36c1a1db886..180a0fd4f03c 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -150,6 +150,8 @@ struct ad4695_state {
/* Commands to send for single conversion. */
u16 cnv_cmd;
u8 cnv_cmd2;
+ /* Buffer for storing data from regmap bus reads/writes */
+ u8 regmap_bus_data[4];
};
static const struct regmap_range ad4695_regmap_rd_ranges[] = {
@@ -194,7 +196,6 @@ static const struct regmap_config ad4695_regmap_config = {
.max_register = AD4695_REG_AS_SLOT(127),
.rd_table = &ad4695_regmap_rd_table,
.wr_table = &ad4695_regmap_wr_table,
- .can_multi_write = true,
};
static const struct regmap_range ad4695_regmap16_rd_ranges[] = {
@@ -226,7 +227,67 @@ static const struct regmap_config ad4695_regmap16_config = {
.max_register = AD4695_REG_GAIN_IN(15),
.rd_table = &ad4695_regmap16_rd_table,
.wr_table = &ad4695_regmap16_wr_table,
- .can_multi_write = true,
+};
+
+static int ad4695_regmap_bus_reg_write(void *context, const void *data,
+ size_t count)
+{
+ struct ad4695_state *st = context;
+ struct spi_transfer xfer = {
+ .speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+ .len = count,
+ .tx_buf = st->regmap_bus_data,
+ };
+
+ if (count > ARRAY_SIZE(st->regmap_bus_data))
+ return -EINVAL;
+
+ memcpy(st->regmap_bus_data, data, count);
+
+ return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
+static int ad4695_regmap_bus_reg_read(void *context, const void *reg,
+ size_t reg_size, void *val,
+ size_t val_size)
+{
+ struct ad4695_state *st = context;
+ struct spi_transfer xfers[] = {
+ {
+ .speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+ .len = reg_size,
+ .tx_buf = &st->regmap_bus_data[0],
+ }, {
+ .speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+ .len = val_size,
+ .rx_buf = &st->regmap_bus_data[2],
+ },
+ };
+ int ret;
+
+ if (reg_size > 2)
+ return -EINVAL;
+
+ if (val_size > 2)
+ return -EINVAL;
+
+ memcpy(&st->regmap_bus_data[0], reg, reg_size);
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret)
+ return ret;
+
+ memcpy(val, &st->regmap_bus_data[2], val_size);
+
+ return 0;
+}
+
+static const struct regmap_bus ad4695_regmap_bus = {
+ .write = ad4695_regmap_bus_reg_write,
+ .read = ad4695_regmap_bus_reg_read,
+ .read_flag_mask = 0x80,
+ .reg_format_endian_default = REGMAP_ENDIAN_BIG,
+ .val_format_endian_default = REGMAP_ENDIAN_BIG,
};
static const struct iio_chan_spec ad4695_channel_template = {
@@ -1040,15 +1101,14 @@ static int ad4695_probe(struct spi_device *spi)
if (!st->chip_info)
return -EINVAL;
- /* Registers cannot be read at the max allowable speed */
- spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ;
-
- st->regmap = devm_regmap_init_spi(spi, &ad4695_regmap_config);
+ st->regmap = devm_regmap_init(dev, &ad4695_regmap_bus, st,
+ &ad4695_regmap_config);
if (IS_ERR(st->regmap))
return dev_err_probe(dev, PTR_ERR(st->regmap),
"Failed to initialize regmap\n");
- st->regmap16 = devm_regmap_init_spi(spi, &ad4695_regmap16_config);
+ st->regmap16 = devm_regmap_init(dev, &ad4695_regmap_bus, st,
+ &ad4695_regmap_config);
if (IS_ERR(st->regmap16))
return dev_err_probe(dev, PTR_ERR(st->regmap16),
"Failed to initialize regmap16\n");
--
2.39.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks
2024-11-11 15:59 ` [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks Trevor Gamblin
@ 2024-11-11 20:31 ` Trevor Gamblin
2024-11-11 22:51 ` Trevor Gamblin
1 sibling, 0 replies; 7+ messages in thread
From: Trevor Gamblin @ 2024-11-11 20:31 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
David Lechner, Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, linux-kernel
On 2024-11-11 10:59, Trevor Gamblin wrote:
> Add a custom implementation of regmap read/write callbacks using the SPI
> bus. This allows them to be performed at a lower SCLK rate than data
> reads. Previously, all SPI transfers were being performed at a lower
> speed, but with this change sample data is read at the max bus speed
> while the register reads/writes remain at the lower rate.
>
> Also remove .can_multi_write from the AD4695 driver's regmap_configs, as
> this isn't implemented or needed.
>
> For some background context, see:
>
> https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/
>
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
> drivers/iio/adc/Kconfig | 2 +-
> drivers/iio/adc/ad4695.c | 74 +++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 68 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 6c4e74420fd2..e0f9d01ce37d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -51,9 +51,9 @@ config AD4130
> config AD4695
> tristate "Analog Device AD4695 ADC Driver"
> depends on SPI
> - select REGMAP_SPI
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
> + select REGMAP
> help
> Say yes here to build support for Analog Devices AD4695 and similar
> analog to digital converters (ADC).
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index f36c1a1db886..180a0fd4f03c 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -150,6 +150,8 @@ struct ad4695_state {
> /* Commands to send for single conversion. */
> u16 cnv_cmd;
> u8 cnv_cmd2;
> + /* Buffer for storing data from regmap bus reads/writes */
> + u8 regmap_bus_data[4];
> };
>
> static const struct regmap_range ad4695_regmap_rd_ranges[] = {
> @@ -194,7 +196,6 @@ static const struct regmap_config ad4695_regmap_config = {
> .max_register = AD4695_REG_AS_SLOT(127),
> .rd_table = &ad4695_regmap_rd_table,
> .wr_table = &ad4695_regmap_wr_table,
> - .can_multi_write = true,
> };
>
> static const struct regmap_range ad4695_regmap16_rd_ranges[] = {
> @@ -226,7 +227,67 @@ static const struct regmap_config ad4695_regmap16_config = {
> .max_register = AD4695_REG_GAIN_IN(15),
> .rd_table = &ad4695_regmap16_rd_table,
> .wr_table = &ad4695_regmap16_wr_table,
> - .can_multi_write = true,
> +};
> +
> +static int ad4695_regmap_bus_reg_write(void *context, const void *data,
> + size_t count)
> +{
> + struct ad4695_state *st = context;
> + struct spi_transfer xfer = {
> + .speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
> + .len = count,
> + .tx_buf = st->regmap_bus_data,
> + };
> +
> + if (count > ARRAY_SIZE(st->regmap_bus_data))
> + return -EINVAL;
> +
> + memcpy(st->regmap_bus_data, data, count);
> +
> + return spi_sync_transfer(st->spi, &xfer, 1);
> +}
> +
> +static int ad4695_regmap_bus_reg_read(void *context, const void *reg,
> + size_t reg_size, void *val,
> + size_t val_size)
> +{
> + struct ad4695_state *st = context;
> + struct spi_transfer xfers[] = {
> + {
> + .speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
> + .len = reg_size,
> + .tx_buf = &st->regmap_bus_data[0],
> + }, {
> + .speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
> + .len = val_size,
> + .rx_buf = &st->regmap_bus_data[2],
> + },
> + };
> + int ret;
> +
> + if (reg_size > 2)
> + return -EINVAL;
> +
> + if (val_size > 2)
> + return -EINVAL;
> +
> + memcpy(&st->regmap_bus_data[0], reg, reg_size);
> +
> + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> + if (ret)
> + return ret;
> +
> + memcpy(val, &st->regmap_bus_data[2], val_size);
> +
> + return 0;
> +}
> +
> +static const struct regmap_bus ad4695_regmap_bus = {
> + .write = ad4695_regmap_bus_reg_write,
> + .read = ad4695_regmap_bus_reg_read,
> + .read_flag_mask = 0x80,
> + .reg_format_endian_default = REGMAP_ENDIAN_BIG,
> + .val_format_endian_default = REGMAP_ENDIAN_BIG,
> };
>
> static const struct iio_chan_spec ad4695_channel_template = {
> @@ -1040,15 +1101,14 @@ static int ad4695_probe(struct spi_device *spi)
> if (!st->chip_info)
> return -EINVAL;
>
> - /* Registers cannot be read at the max allowable speed */
> - spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ;
> -
> - st->regmap = devm_regmap_init_spi(spi, &ad4695_regmap_config);
> + st->regmap = devm_regmap_init(dev, &ad4695_regmap_bus, st,
> + &ad4695_regmap_config);
> if (IS_ERR(st->regmap))
> return dev_err_probe(dev, PTR_ERR(st->regmap),
> "Failed to initialize regmap\n");
>
> - st->regmap16 = devm_regmap_init_spi(spi, &ad4695_regmap16_config);
> + st->regmap16 = devm_regmap_init(dev, &ad4695_regmap_bus, st,
> + &ad4695_regmap_config);
Note that there's a bug here - that should be
+ st->regmap16 = devm_regmap_init(dev, &ad4695_regmap_bus, st,
+ &ad4695_regmap16_config);
I can fix this in v2, unless it would just be easier to fix during merge
(assuming there is no other major feedback).
> if (IS_ERR(st->regmap16))
> return dev_err_probe(dev, PTR_ERR(st->regmap16),
> "Failed to initialize regmap16\n");
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks
2024-11-11 15:59 ` [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks Trevor Gamblin
2024-11-11 20:31 ` Trevor Gamblin
@ 2024-11-11 22:51 ` Trevor Gamblin
1 sibling, 0 replies; 7+ messages in thread
From: Trevor Gamblin @ 2024-11-11 22:51 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
David Lechner, Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, linux-kernel
On 2024-11-11 10:59, Trevor Gamblin wrote:
> Add a custom implementation of regmap read/write callbacks using the SPI
> bus. This allows them to be performed at a lower SCLK rate than data
> reads. Previously, all SPI transfers were being performed at a lower
> speed, but with this change sample data is read at the max bus speed
> while the register reads/writes remain at the lower rate.
>
> Also remove .can_multi_write from the AD4695 driver's regmap_configs, as
> this isn't implemented or needed.
>
> For some background context, see:
>
> https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/
>
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
Found a couple more bugs after some testing. I'll wait until EOD on
Wednesday to send a v2, in case there are any other review comments.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements
2024-11-11 15:59 [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
` (2 preceding siblings ...)
2024-11-11 15:59 ` [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks Trevor Gamblin
@ 2024-11-13 21:18 ` Trevor Gamblin
3 siblings, 0 replies; 7+ messages in thread
From: Trevor Gamblin @ 2024-11-13 21:18 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
David Lechner, Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, linux-kernel
On 2024-11-11 10:59, Trevor Gamblin wrote:
> The AD4695 driver currently operates all SPI reads/writes at the speed
> appropriate for register access, rather than the max rate for the bus.
> Data reads should ideally operate at the latter speed, but making this
> change universally makes it possible for data to be corrupted during use
> and for unexpected behavior to occur on driver subsequent driver
> binds/unbinds. To solve this, introduce custom regmap bus callbacks for
> the driver that explicitly set a lower speed only for these operations.
>
> The first patch in this series is a fix introduced after discovering the
> corresponding issue during testing of the callbacks. This is a timing
> fix that ensures the AD4695 datasheet's timing specs are met, as before
> the busy signal would sometimes fail to toggle again following the end
> of the conversion sequence. Adding an extra delay in the form of a blank
> transfer before every CS deassert in ad4695_buffer_preenable() allows
> this requirement to be met.
>
> The second patch is an improvement that increases the robustness of the
> exit message in ad4695_exit_conversion_mode(), this time by adding a
> delay before the actual exit command. This helps avoid the possibility
> that the exit message will be read as data, causing corruption on some
> buffered reads.
>
> For additional context, see:
> https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/
>
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
> Trevor Gamblin (3):
> iio: adc: ad4695: fix buffered read timing in ad4695_buffer_preenable()
> iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust
> iio: adc: ad4695: add custom regmap bus callbacks
>
> drivers/iio/adc/Kconfig | 2 +-
> drivers/iio/adc/ad4695.c | 135 ++++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 118 insertions(+), 19 deletions(-)
> ---
> base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
> change-id: 20241111-tgamblin-ad4695_improvements-7a32a6268c26
>
> Best regards,
FYI, there is now a v2, so if anyone's currently reviewing this, please
don't spend any additional time. v2:
https://lore.kernel.org/linux-iio/20241113-tgamblin-ad4695_improvements-v2-0-b6bb7c758fc4@baylibre.com/T/#
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-11-13 21:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-11 15:59 [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
2024-11-11 15:59 ` [PATCH 1/3] iio: adc: ad4695: fix buffered read timing in ad4695_buffer_preenable() Trevor Gamblin
2024-11-11 15:59 ` [PATCH 2/3] iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust Trevor Gamblin
2024-11-11 15:59 ` [PATCH 3/3] iio: adc: ad4695: add custom regmap bus callbacks Trevor Gamblin
2024-11-11 20:31 ` Trevor Gamblin
2024-11-11 22:51 ` Trevor Gamblin
2024-11-13 21:18 ` [PATCH 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements Trevor Gamblin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox