On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxx> 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(...);
> +}
--
With Best Regards,
Andy Shevchenko
