lkml.org 
[lkml]   [2021]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 3/5] iio: adc: ad_sigma_delta: Add sequencer support
On Wed, 10 Nov 2021 13:17:48 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
>
> Some sigma-delta chips support sampling of multiple
> channels in continuous mode.
>
> When the operating with more than one channel enabled,
> the channel sequencer cycles through the enabled channels
> in sequential order, from first channel to the last one.
> If a channel is disabled, it is skipped by the sequencer.
>
> If more than one channel is used in continuous mode,
> instruct the device to append the status to the SPI transfer
> (1 extra byte) every time we receive a sample.
> All sigma-delta chips possessing a sampling sequencer have
> this ability. Inside the status register there will be
> the number of the converted channel. In this way, even
> if the CPU won't keep up with the sampling rate, it won't
> send to userspace wrong channel samples.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>

Hi Alexandru,

A few comments inline. Approach looks good.

Jonathan

> ---
> drivers/iio/adc/ad_sigma_delta.c | 106 ++++++++++++++++++++-----
> include/linux/iio/adc/ad_sigma_delta.h | 22 +++++
> 2 files changed, 110 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 1d652d9b2f5c..d595e7972975 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -342,33 +342,48 @@ EXPORT_SYMBOL_GPL(ad_sigma_delta_single_conversion);
> static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
> {
> struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> - unsigned int channel;
> + unsigned int i, slot;
> int ret;
>
> - channel = find_first_bit(indio_dev->active_scan_mask,
> - indio_dev->masklength);
> - ret = ad_sigma_delta_set_channel(sigma_delta,
> - indio_dev->channels[channel].address);
> - if (ret)
> - return ret;
> + slot = 0;
> + for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> + sigma_delta->slots[slot] = indio_dev->channels[i].address;
> + ret = ad_sigma_delta_set_channel(sigma_delta, indio_dev->channels[i].address);
> + if (ret)
> + return ret;
> + slot++;
> + }
> +
> + sigma_delta->active_slots = slot;
> + sigma_delta->current_slot = 0;
> +
> + if (sigma_delta->active_slots > 1) {
> + ret = ad_sigma_delta_append_status(sigma_delta, true);
> + if (ret)
> + return ret;
> + }
> +
> + kfree(sigma_delta->samples_buf);

krealloc() preferred. It might not be necessary to actually do an allocation after all
if we happen to have one we can already use.

> + sigma_delta->samples_buf = kzalloc(slot * indio_dev->channels[0].scan_type.storagebits,
> + GFP_KERNEL);

I think this needs space for an aligned timestamp give you pass it directly
to iio_push_to_buffer_with_timestamp();

I haven't checked, but I wonder if the fact you are no longer directly using rx_buf
would allow you to relax the alignment constraint on that. (see below though as I
think you should consider keeping the original path to avoid a copy for the simple
devices).


> + if (!sigma_delta->samples_buf)
> + return -ENOMEM;
>
> spi_bus_lock(sigma_delta->spi->master);
> sigma_delta->bus_locked = true;
> sigma_delta->keep_cs_asserted = true;
>
> ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS);
> - if (ret)
> - goto err_unlock;
> + if (ret) {
> + kfree(sigma_delta->samples_buf);
> + spi_bus_unlock(sigma_delta->spi->master);

From style point of view preferred to keep to the goto style previously used.
Also, unlock then kfree() buffer + given you pass samples_buf to kfree() on next
run of this function you need to set it to NULL to avoid a double free.


> + return ret;
> + }
>
> sigma_delta->irq_dis = false;
> enable_irq(sigma_delta->spi->irq);
>
> return 0;
> -
> -err_unlock:
> - spi_bus_unlock(sigma_delta->spi->master);
> -
> - return ret;
> }
>
> static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
> @@ -386,6 +401,9 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
> sigma_delta->keep_cs_asserted = false;
> ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
>
> + if (sigma_delta->status_appended)
> + ad_sigma_delta_append_status(sigma_delta, false);
> +
> sigma_delta->bus_locked = false;
> return spi_bus_unlock(sigma_delta->spi->master);
> }
> @@ -396,6 +414,10 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
> struct iio_dev *indio_dev = pf->indio_dev;
> struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> uint8_t *data = sigma_delta->rx_buf;
> + unsigned int transfer_size;
> + unsigned int sample_size;
> + unsigned int sample_pos;
> + unsigned int status_pos;
> unsigned int reg_size;
> unsigned int data_reg;
>
> @@ -408,20 +430,55 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
> else
> data_reg = AD_SD_REG_DATA;
>
> + /* Status word will be appended to the sample during transfer */
> + if (sigma_delta->status_appended)
> + transfer_size = reg_size + 1;
> + else
> + transfer_size = reg_size;
> +
> switch (reg_size) {
> case 4:
> case 2:
> case 1:
> - ad_sd_read_reg_raw(sigma_delta, data_reg, reg_size, &data[0]);
> + status_pos = reg_size;
> + ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[0]);
> break;
> case 3:
> + status_pos = reg_size + 1;
> /* We store 24 bit samples in a 32 bit word. Keep the upper
> * byte set to zero. */
> - ad_sd_read_reg_raw(sigma_delta, data_reg, reg_size, &data[1]);
> + ad_sd_read_reg_raw(sigma_delta, data_reg, transfer_size, &data[1]);
> break;
> }
>
> - iio_push_to_buffers_with_timestamp(indio_dev, data, pf->timestamp);

Given lots of drivers don't support scan mode, perhaps you want to keep a fast path that
avoids the extra copies used here? i.e. keep old code in place if active_slots == 1

> + if (sigma_delta->status_appended) {
> + u8 converted_channel;
> +
> + converted_channel = data[status_pos] & sigma_delta->info->status_ch_mask;
> + if (converted_channel != sigma_delta->slots[sigma_delta->current_slot]) {
> + /* Desynq occurred during continuous sampling of multiple channels.
/*
* Desync

preferred though i see we have some other comments in here not using that style.

> + * Drop this incomplete sample and start from first channel again.
> + */
> +
> + sigma_delta->current_slot = 0;
> + iio_trigger_notify_done(indio_dev->trig);
> + sigma_delta->irq_dis = false;
> + enable_irq(sigma_delta->spi->irq);
> +
> + return IRQ_HANDLED;
> + }
> + }
> +
> + sample_size = indio_dev->channels[0].scan_type.storagebits / 8;
> + sample_pos = sample_size * sigma_delta->current_slot;
> + memcpy(&sigma_delta->samples_buf[sample_pos], data, sample_size);
> + sigma_delta->current_slot++;
> +
> + if (sigma_delta->current_slot == sigma_delta->active_slots) {
> + sigma_delta->current_slot = 0;
> + iio_push_to_buffers_with_timestamp(indio_dev, sigma_delta->samples_buf,
> + pf->timestamp);
> + }
>
> iio_trigger_notify_done(indio_dev->trig);
> sigma_delta->irq_dis = false;
> @@ -430,10 +487,17 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
> return IRQ_HANDLED;
> }
>
> +static bool ad_sd_validate_scan_mask(struct iio_dev *indio_dev, const unsigned long *mask)
> +{
> + struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> +
> + return bitmap_weight(mask, indio_dev->masklength) <= sigma_delta->info->num_slots;

I think this requires all existing drivers to have num_slots == 1 to do the same as
iio_validate_scan_mask_onehot. I'm not seeing that change in this patch, or any setting
of the default to 1.

> +}
> +
> static const struct iio_buffer_setup_ops ad_sd_buffer_setup_ops = {
> .postenable = &ad_sd_buffer_postenable,
> .postdisable = &ad_sd_buffer_postdisable,
> - .validate_scan_mask = &iio_validate_scan_mask_onehot,
> + .validate_scan_mask = &ad_sd_validate_scan_mask,
> };
>
> static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
> @@ -517,8 +581,14 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
> */
> int devm_ad_sd_setup_buffer_and_trigger(struct device *dev, struct iio_dev *indio_dev)
> {
> + struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> int ret;
>
> + sigma_delta->slots = devm_kcalloc(dev, sigma_delta->info->num_slots,
> + sizeof(*sigma_delta->slots), GFP_KERNEL);
> + if (!sigma_delta->slots)
> + return -ENOMEM;
> +
> ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> &iio_pollfunc_store_time,
> &ad_sd_trigger_handler,
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index c525fd51652f..758ff25590c3 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -32,6 +32,7 @@ struct iio_dev;
> /**
> * struct ad_sigma_delta_info - Sigma Delta driver specific callbacks and options
> * @set_channel: Will be called to select the current channel, may be NULL.
> + * @append_status: Will be called to enable status append at the end of the sample, may be NULL.
> * @set_mode: Will be called to select the current mode, may be NULL.
> * @postprocess_sample: Is called for each sampled data word, can be used to
> * modify or drop the sample data, it, may be NULL.
> @@ -39,19 +40,24 @@ struct iio_dev;
> * if there is just one read-only sample data shift register.
> * @addr_shift: Shift of the register address in the communications register.
> * @read_mask: Mask for the communications register having the read bit set.
> + * @status_ch_mask: Mask for the channel number stored in status register.
> * @data_reg: Address of the data register, if 0 the default address of 0x3 will
> * be used.
> * @irq_flags: flags for the interrupt used by the triggered buffer
> + * @num_slots: Number of sequencer slots
> */
> struct ad_sigma_delta_info {
> int (*set_channel)(struct ad_sigma_delta *, unsigned int channel);
> + int (*append_status)(struct ad_sigma_delta *, bool append);
> int (*set_mode)(struct ad_sigma_delta *, enum ad_sigma_delta_mode mode);
> int (*postprocess_sample)(struct ad_sigma_delta *, unsigned int raw_sample);
> bool has_registers;
> unsigned int addr_shift;
> unsigned int read_mask;
> + unsigned int status_ch_mask;
> unsigned int data_reg;
> unsigned long irq_flags;
> + unsigned int num_slots;
> };
>
> /**
> @@ -76,6 +82,12 @@ struct ad_sigma_delta {
> uint8_t comm;
>
> const struct ad_sigma_delta_info *info;
> + unsigned int active_slots;
> + unsigned int current_slot;
> + bool status_appended;
> + /* map slots to channels in order to know what to expect from devices */
> + unsigned int *slots;
> + uint8_t *samples_buf;
>
> /*
> * DMA (thus cache coherency maintenance) requires the
> @@ -97,6 +109,16 @@ static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,
> return 0;
> }
>
> +static inline int ad_sigma_delta_append_status(struct ad_sigma_delta *sd, bool append)
> +{
> + if (sd->info->append_status) {
> + sd->status_appended = append;
> + return sd->info->append_status(sd, append);
> + }
> +
> + return 0;
> +}
> +
> static inline int ad_sigma_delta_set_mode(struct ad_sigma_delta *sd,
> unsigned int mode)
> {

\
 
 \ /
  Last update: 2021-11-12 18:10    [W:0.068 / U:0.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site