| Message ID | 20241122113322.242875-20-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: ad7124: Various fixes | expand |
On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > It can happen if a previous conversion was aborted the ADC pulls down > the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling > the irq might immediatly fire (depending on the irq controller's immediately > capabilities) and even with a rdy-gpio isn't identified as an unrelated > one. > > To cure that problem check for a pending event before the measurement is > started and clear it if needed. ... > +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta) > +{ > + int ret = 0; Unneeded assignment, see below. > + bool pending_event; > + > + /* > + * read RDY pin (if possible) or status register to check if there is an > + * old event. > + */ > + if (sigma_delta->rdy_gpiod) { > + pending_event = gpiod_get_value(sigma_delta->rdy_gpiod); > + } else { > + unsigned status_reg; > + > + ret = ad_sd_read_reg(sigma_delta, AD_SD_REG_STATUS, 1, &status_reg); > + if (ret) > + return ret; Side note: in the initial logic the 0 assigned above is overwritten by 0 here. While it's not a technical problem, it might affect the workflow in the future. > + pending_event = !(status_reg & AD_SD_REG_STATUS_RDY); > + } > + > + if (pending_event) { So, check for the error condition first pattern? if (!pending_event) return 0; This among other benefits makes the below code less indented and hence less LoCs to be occupied. > + /* > + * In general the size of the data register is unknown. It > + * varies from device to device, might be one byte longer if > + * CONTROL.DATA_STATUS is set and even varies on some devices > + * depending on which input is selected. So send one byte to > + * start reading the data register and then just clock for some > + * bytes with DIN (aka MOSI) high to not confuse the register > + * access state machine after the data register was completely > + * read. Note however that the sequence length must be shorter > + * than the reset procedure. > + */ > + unsigned int data_read_len = DIV_ROUND_UP(sigma_delta->info->num_resetclks, 8); BITS_TO_BYTES() > + uint8_t data[9]; Why not u8? > + struct spi_transfer t[] = { > + { > + .tx_buf = data, > + .len = 1, > + }, { > + .tx_buf = data + 1, > + .len = data_read_len, > + } > + }; > + struct spi_message m; > + > + /* Oh, back out instead of overflowing data[] */ > + if (data_read_len > sizeof(data) - 1) > + return -EINVAL; > + > + spi_message_init(&m); > + if (sigma_delta->info->has_registers) { > + unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA; > + > + data[0] = data_reg << sigma_delta->info->addr_shift; > + data[0] |= sigma_delta->info->read_mask; > + data[0] |= sigma_delta->comm; > + spi_message_add_tail(&t[0], &m); > + } > + > + /* > + * The first transferred byte is part of the real data register, > + * so this doesn't need to be 0xff. In the remaining > + * `data_read_len - 1` bytes are less than $num_resetclks ones. > + */ > + data[1] = 0x00; > + memset(data + 2, 0xff, data_read_len - 1); > + spi_message_add_tail(&t[1], &m); Instead you can also use if (...) spi_message_init_with_transfers(..., 2); else spi_message_init_with_transfers(..., 1); > + ret = spi_sync_locked(sigma_delta->spi, &m); > + } > + > + return ret; return spi_sync_locked(...); > +}
Hello Andy, On Fri, Nov 22, 2024 at 10:17:51PM +0200, Andy Shevchenko wrote: > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> wrote: > > +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta) > > +{ > > + int ret = 0; > > Unneeded assignment, see below. With the changes you suggested below this can get a more local scope, too. That's what I did for v4. > > + spi_message_add_tail(&t[1], &m); > > Instead you can also use > > if (...) > spi_message_init_with_transfers(..., 2); > else > spi_message_init_with_transfers(..., 1); I kept it as is to match the register read function. Implemented your other suggestions, stay tuned for v4. Best regards Uwe
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index 19cb9b7b62c6..8bc652b71019 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -29,8 +29,11 @@ #define AD_SD_COMM_CHAN_MASK 0x3 #define AD_SD_REG_COMM 0x00 +#define AD_SD_REG_STATUS 0x00 #define AD_SD_REG_DATA 0x03 +#define AD_SD_REG_STATUS_RDY 0x80 + /** * ad_sd_set_comm() - Set communications register * @@ -222,6 +225,82 @@ static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta) enable_irq(sigma_delta->irq_line); } +/* Called with `sigma_delta->bus_locked == true` only. */ +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta) +{ + int ret = 0; + bool pending_event; + + /* + * read RDY pin (if possible) or status register to check if there is an + * old event. + */ + if (sigma_delta->rdy_gpiod) { + pending_event = gpiod_get_value(sigma_delta->rdy_gpiod); + } else { + unsigned status_reg; + + ret = ad_sd_read_reg(sigma_delta, AD_SD_REG_STATUS, 1, &status_reg); + if (ret) + return ret; + + pending_event = !(status_reg & AD_SD_REG_STATUS_RDY); + } + + if (pending_event) { + /* + * In general the size of the data register is unknown. It + * varies from device to device, might be one byte longer if + * CONTROL.DATA_STATUS is set and even varies on some devices + * depending on which input is selected. So send one byte to + * start reading the data register and then just clock for some + * bytes with DIN (aka MOSI) high to not confuse the register + * access state machine after the data register was completely + * read. Note however that the sequence length must be shorter + * than the reset procedure. + */ + unsigned int data_read_len = DIV_ROUND_UP(sigma_delta->info->num_resetclks, 8); + uint8_t data[9]; + struct spi_transfer t[] = { + { + .tx_buf = data, + .len = 1, + }, { + .tx_buf = data + 1, + .len = data_read_len, + } + }; + struct spi_message m; + + /* Oh, back out instead of overflowing data[] */ + if (data_read_len > sizeof(data) - 1) + return -EINVAL; + + spi_message_init(&m); + if (sigma_delta->info->has_registers) { + unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA; + + data[0] = data_reg << sigma_delta->info->addr_shift; + data[0] |= sigma_delta->info->read_mask; + data[0] |= sigma_delta->comm; + spi_message_add_tail(&t[0], &m); + } + + /* + * The first transferred byte is part of the real data register, + * so this doesn't need to be 0xff. In the remaining + * `data_read_len - 1` bytes are less than $num_resetclks ones. + */ + data[1] = 0x00; + memset(data + 2, 0xff, data_read_len - 1); + spi_message_add_tail(&t[1], &m); + + ret = spi_sync_locked(sigma_delta->spi, &m); + } + + return ret; +} + int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, unsigned int mode, unsigned int channel) { @@ -237,6 +316,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, sigma_delta->keep_cs_asserted = true; reinit_completion(&sigma_delta->completion); + ret = ad_sigma_delta_clear_pending_event(sigma_delta); + if (ret) + return ret; + ret = ad_sigma_delta_set_mode(sigma_delta, mode); if (ret < 0) goto out; @@ -310,6 +393,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, sigma_delta->keep_cs_asserted = true; reinit_completion(&sigma_delta->completion); + ret = ad_sigma_delta_clear_pending_event(sigma_delta); + if (ret) + return ret; + ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE); ad_sd_enable_irq(sigma_delta); @@ -406,6 +493,10 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev) sigma_delta->bus_locked = true; sigma_delta->keep_cs_asserted = true; + ret = ad_sigma_delta_clear_pending_event(sigma_delta); + if (ret) + return ret; + ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS); if (ret) goto err_unlock;
It can happen if a previous conversion was aborted the ADC pulls down the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling the irq might immediatly fire (depending on the irq controller's capabilities) and even with a rdy-gpio isn't identified as an unrelated one. To cure that problem check for a pending event before the measurement is started and clear it if needed. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad_sigma_delta.c | 91 ++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+)