From: Gerhard Sittig Date: Mon, 24 Jan 2022 20:30:11 +0000 (+0100) Subject: kingst-la2016: more checks on configured rate/depth/channels X-Git-Url: https://sigrok.org/gitaction?a=commitdiff_plain;h=ea436ba7c5dc89e87c24200850fe458d6e6576d1;p=libsigrok.git kingst-la2016: more checks on configured rate/depth/channels Extend the list of supported samplerates, start from 10kHz (used to start at 20kHz before). Comment on hardware constraints which limit the available rates, for awareness during maintenance. Move the check for enabled channels from api.c to protocol.c, it was only used for the trigger configuration. Rename the routine since it never configured any channels, only got the list of currently enabled channels to pass on to the device. Extend the range checks for the rate and depth on acquisition start, also check the lower limits. Moving codes which references declarations required that definitions also moved to public location of greater scope. This is acceptable since all of them remain local to the kingst-la2016 driver. Stick with names as they are used right now to reduce the diff and help reviewers, these can get unified in a later commit. --- diff --git a/src/hardware/kingst-la2016/api.c b/src/hardware/kingst-la2016/api.c index 45262c1e..f11c21ec 100644 --- a/src/hardware/kingst-la2016/api.c +++ b/src/hardware/kingst-la2016/api.c @@ -65,7 +65,17 @@ static const char *channel_names[] = { "CH8", "CH9", "CH10", "CH11", "CH12", "CH13", "CH14", "CH15", }; +/* + * The hardware uses a (model dependent) 100/200/500MHz base clock and + * a 16bit divider (common across all models). The range from 10kHz to + * 100/200/500MHz should be applicable to all devices. High rates may + * suffer from coarse resolution (e.g. in the "500MHz div 2" case) and + * may not provide the desired 1/2/5 steps. This is not an issue now, + * the 500MHz model is not supported yet by this driver. + */ + static const uint64_t samplerates_la2016[] = { + SR_KHZ(10), SR_KHZ(20), SR_KHZ(50), SR_KHZ(100), @@ -84,6 +94,7 @@ static const uint64_t samplerates_la2016[] = { }; static const uint64_t samplerates_la1016[] = { + SR_KHZ(10), SR_KHZ(20), SR_KHZ(50), SR_KHZ(100), @@ -571,25 +582,6 @@ static int config_list(uint32_t key, GVariant **data, return SR_OK; } -static int configure_channels(const struct sr_dev_inst *sdi) -{ - struct dev_context *devc; - GSList *l; - struct sr_channel *ch; - - devc = sdi->priv; - - devc->cur_channels = 0; - for (l = sdi->channels; l; l = l->next) { - ch = l->data; - if (!ch->enabled) - continue; - devc->cur_channels |= 1UL << ch->index; - } - - return SR_OK; -} - static int dev_acquisition_start(const struct sr_dev_inst *sdi) { struct sr_dev_driver *di; @@ -603,11 +595,6 @@ static int dev_acquisition_start(const struct sr_dev_inst *sdi) ctx = drvc->sr_ctx;; devc = sdi->priv; - if (configure_channels(sdi) != SR_OK) { - sr_err("Cannot configure channels."); - return SR_ERR; - } - devc->convbuffer_size = LA2016_CONVBUFFER_SIZE; devc->convbuffer = g_try_malloc(devc->convbuffer_size); if (!devc->convbuffer) { diff --git a/src/hardware/kingst-la2016/protocol.c b/src/hardware/kingst-la2016/protocol.c index 3ef10f6e..f21a2ba0 100644 --- a/src/hardware/kingst-la2016/protocol.c +++ b/src/hardware/kingst-la2016/protocol.c @@ -34,13 +34,6 @@ #define FPGA_FW_LA1016 "kingst-la1016-fpga.bitstream" #define FPGA_FW_LA1016A "kingst-la1016a1-fpga.bitstream" -/* Maximum device capabilities. May differ between models. */ -#define MAX_SAMPLE_RATE_LA2016 SR_MHZ(200) -#define MAX_SAMPLE_RATE_LA1016 SR_MHZ(100) -#define MAX_SAMPLE_DEPTH 10e9 -#define MAX_PWM_FREQ SR_MHZ(20) -#define PWM_CLOCK SR_MHZ(200) /* 200MHz for both LA2016 and LA1016 */ - /* * Default device configuration. Must be applicable to any of the * supported devices (no model specific default values yet). Specific @@ -51,14 +44,6 @@ #define LA2016_DFLT_SAMPLEDEPTH (5 * 1000 * 1000) #define LA2016_DFLT_CAPT_RATIO 5 /* Capture ratio, in percent. */ -/* TODO - * What is the origin and motivation of that 128Mi literal? What is its - * unit? How does it relate to a device's hardware capabilities? How to - * map the 1GiB of RAM of an LA2016 (at 16 channels) to the 128Mi value? - * It cannot be sample count. Is it memory size in bytes perhaps? - */ -#define LA2016_PRE_MEM_LIMIT_BASE (128 * 1024 * 1024) - /* USB vendor class control requests, executed by the Cypress FX2 MCU. */ #define CMD_FPGA_ENABLE 0x10 #define CMD_FPGA_SPI 0x20 /* R/W access to FPGA registers via SPI. */ @@ -514,6 +499,25 @@ static int set_defaults(const struct sr_dev_inst *sdi) return SR_OK; } +static uint16_t get_channels_mask(const struct sr_dev_inst *sdi) +{ + uint16_t channels; + GSList *l; + struct sr_channel *ch; + + channels = 0; + for (l = sdi->channels; l; l = l->next) { + ch = l->data; + if (ch->type != SR_CHANNEL_LOGIC) + continue; + if (!ch->enabled) + continue; + channels |= 1UL << ch->index; + } + + return channels; +} + static int set_trigger_config(const struct sr_dev_inst *sdi) { struct dev_context *devc; @@ -533,7 +537,7 @@ static int set_trigger_config(const struct sr_dev_inst *sdi) memset(&cfg, 0, sizeof(cfg)); - cfg.channels = devc->cur_channels; + cfg.channels = get_channels_mask(sdi); if (trigger && trigger->stages) { stages = trigger->stages; @@ -623,7 +627,12 @@ static int set_sample_config(const struct sr_dev_inst *sdi) if (devc->cur_samplerate > devc->max_samplerate) { sr_err("Too high a sample rate: %" PRIu64 ".", devc->cur_samplerate); - return SR_ERR; + return SR_ERR_ARG; + } + if (devc->cur_samplerate < MIN_SAMPLE_RATE_LA2016) { + sr_err("Too low a sample rate: %" PRIu64 ".", + devc->cur_samplerate); + return SR_ERR_ARG; } clock_divisor = devc->max_samplerate / (double)devc->cur_samplerate; @@ -632,11 +641,16 @@ static int set_sample_config(const struct sr_dev_inst *sdi) divider_u16 = (uint16_t)(clock_divisor + 0.5); devc->cur_samplerate = devc->max_samplerate / divider_u16; - if (devc->limit_samples > MAX_SAMPLE_DEPTH) { + if (devc->limit_samples > LA2016_NUM_SAMPLES_MAX) { sr_err("Too high a sample depth: %" PRIu64 ".", devc->limit_samples); return SR_ERR; } + if (devc->limit_samples < LA2016_NUM_SAMPLES_MIN) { + sr_err("Too low a sample depth: %" PRIu64 ".", + devc->limit_samples); + return SR_ERR; + } /* * The acquisition configuration communicates "pre-trigger" diff --git a/src/hardware/kingst-la2016/protocol.h b/src/hardware/kingst-la2016/protocol.h index d5c967ad..9eb483ce 100644 --- a/src/hardware/kingst-la2016/protocol.h +++ b/src/hardware/kingst-la2016/protocol.h @@ -69,8 +69,23 @@ #define LA2016_THR_VOLTAGE_MIN 0.40 #define LA2016_THR_VOLTAGE_MAX 4.00 -#define LA2016_NUM_SAMPLES_MIN 256 -#define LA2016_NUM_SAMPLES_MAX (10UL * 1000 * 1000 * 1000) +#define LA2016_NUM_SAMPLES_MIN (UINT64_C(256)) +#define LA2016_NUM_SAMPLES_MAX (UINT64_C(10 * 1000 * 1000 * 1000)) + +/* Maximum device capabilities. May differ between models. */ +#define MAX_SAMPLE_RATE_LA2016 SR_MHZ(200) +#define MAX_SAMPLE_RATE_LA1016 SR_MHZ(100) +#define MIN_SAMPLE_RATE_LA2016 SR_KHZ(10) +#define MAX_PWM_FREQ SR_MHZ(20) +#define PWM_CLOCK SR_MHZ(200) /* 200MHz for both LA2016 and LA1016 */ + +/* TODO + * What is the origin and motivation of that 128Mi literal? What is its + * unit? How does it relate to a device's hardware capabilities? How to + * map the 1GiB of RAM of an LA2016 (at 16 channels) to the 128Mi value? + * It cannot be sample count. Is it memory size in bytes perhaps? + */ +#define LA2016_PRE_MEM_LIMIT_BASE (128 * 1024 * 1024) #define LA2016_NUM_PWMCH_MAX 2 @@ -114,7 +129,6 @@ struct dev_context { uint64_t cur_samplerate; uint64_t limit_samples; uint64_t capture_ratio; - uint16_t cur_channels; /* Internal acquisition and download state. */ gboolean trigger_involved;