Hi,
quick review below. Others may have additional comments, though.
On Thu, Feb 05, 2015 at 05:30:56PM +0100, Bartosz Golaszewski wrote:
> diff --git a/configure.ac b/configure.ac
> index 39766c6..6aba6dd 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -33,6 +33,9 @@ AC_CONFIG_HEADER([config.h])
> AC_CONFIG_MACRO_DIR([autostuff])
> AC_CONFIG_AUX_DIR([autostuff])
>
> +# We require this to know the target settings if we're cross-compiling.
> +AC_CANONICAL_SYSTEM
This looks unrelated. What's the error message or effect when this is
not present? Either way, it should probably be in an extra patch since
it's not ACME specific.
> +static const uint32_t devopts[] = {
> + SR_CONF_CONTINUOUS | SR_CONF_SET,
> + SR_CONF_LIMIT_SAMPLES | SR_CONF_GET | SR_CONF_SET,
> + SR_CONF_LIMIT_MSEC | SR_CONF_GET | SR_CONF_SET,
> + SR_CONF_SAMPLERATE | SR_CONF_GET | SR_CONF_SET | SR_CONF_LIST,
> +};
> +
> +#define MAX_SAMPLE_RATE 500 /* In HZ */
HZ -> Hz
> +static const uint32_t enrg_i2c_addrs[] = {
> + 0x40, 0x41, 0x44, 0x45, 0x42, 0x43, 0x46, 0x47,
> +};
> +
> +static const uint32_t temp_i2c_addrs[] = {
> + 0x0, 0x0, 0x0, 0x0, 0x4c, 0x49, 0x4f, 0x4b,
> +};
Is uint32_t intentional? Any reason not to use uint8_t?
> +struct channel_group_priv {
> + int prb_type;
-> probe_type?
> +static gboolean detect_probe(unsigned addr, int prb_num, const char
> *prb_name)
[...]
> + if (buf)
> + g_free(buf);
Not needed I think, g_free() can cope with NULL as input.
> +static int get_hwmon_index(unsigned addr)
> +{
> + int status, hwmon;
> + char path[256];
> + GError *err;
> + GDir *dir;
> +
> + probe_hwmon_path(addr, path, sizeof(path));
> + dir = g_dir_open(path, 0, &err);
> + if (dir == NULL) {
> + sr_err("Error opening %s: %s", path, err->message);
> + return -1;
> + }
> +
> + /*
> + * The directory should contain a single file named hwmonX where X
> + * is the hwmon index.
> + */
> + status = sscanf(g_dir_read_name(dir), "hwmon%d", &hwmon);
> + g_dir_close(dir);
> + if (status != 1) {
> + sr_err("Unable to determine the hwmon entry");
> + return -1;
> + }
> +
> + return hwmon;
> +}
> +
> +static void append_channel(struct sr_dev_inst *sdi,
> + struct sr_channel_group *chg,
Use "cg" for channel groups please, to match the rest of the codebase.
> + sr_err("Programmer goofed!");
Something more descriptive would be good here (same for the other
occurences).
> +static int config_get(uint32_t key, GVariant **data,
> + const struct sr_dev_inst *sdi,
> + const struct sr_channel_group *cg)
> +{
> + struct dev_context *devc;
> + int ret;
> +
> + (void)sdi;
> + (void)data;
> + (void)cg;
The lines for "sdi" and "data" are not needed, they're used below.
> +
> + devc = sdi->priv;
> +
> + ret = SR_OK;
> + switch (key) {
> + case SR_CONF_LIMIT_SAMPLES:
> + *data = g_variant_new_uint64(devc->limit_samples);
> + break;
> + case SR_CONF_LIMIT_MSEC:
> + *data = g_variant_new_uint64(devc->limit_msec);
> + break;
> + case SR_CONF_SAMPLERATE:
> + *data = g_variant_new_uint64(devc->samplerate);
> + break;
> + default:
> + return SR_ERR_NA;
> + }
> +
> + return ret;
> +}
> +
> +static int config_list(uint32_t key, GVariant **data,
> + const struct sr_dev_inst *sdi,
> + const struct sr_channel_group *cg)
> +{
> + GVariant *gvar;
> + GVariantBuilder gvb;
> + int ret;
> +
> + (void)sdi;
> + (void)data;
The "data" entry is not needed (used below).
> +static int get_hwmon_index(unsigned addr)
> +static int probe_name_path(unsigned addr, char *const buf, size_t buflen)
> +static int probe_hwmon_path(unsigned addr, char *const buf, size_t buflen)
> +static gboolean detect_probe(unsigned addr, int prb_num, const char
> *prb_name)
> +static int channel_to_mq(struct sr_channel *ch)
> +static int channel_to_unit(struct sr_channel *ch)
> +static float adjust_data(int val, int type)
> +static float read_sample(struct sr_channel *ch)
> +static int read_data(int fd, int revents, void *cb_data)
These and a few other non-API calls should probably be moved
to protocol.c (see my other mail). The api.c file would usually
more or less only contain the callbacks from the "struct sr_dev_driver
*_driver_info" struct at the bottom of the file.
> +static float read_sample(struct sr_channel *ch)
> +{
> + snprintf(path, sizeof(path),
> + "/sys/class/hwmon/hwmon%d/%s",
> + chp->probe->hwmon_num, file);
> + fd = open(path, O_RDONLY);
> + if (fd < 0) {
> + sr_err("Error opening %s: %s", path, strerror(errno));
> + ch->enabled = FALSE;
> + return -1.0;
> + }
> +
> + len = read(fd, buf, sizeof(buf));
> + close(fd);
> + if (len < 0) {
> + sr_err("error reading from %s: %s", path, strerror(errno));
> + ch->enabled = FALSE;
> + return -1.0;
> + }
The open/read here could also be done using glib functions as in the
rest of the file I guess?
> +static int read_data(int fd, int revents, void *cb_data)
> +{
> + time_to_sleep = 1000000lu / devc->samplerate - diff_time;
glib has G_USEC_PER_SEC for this, IIRC.
> + devc->samples_read++;
> + if (devc->limit_samples > 0 &&
> + devc->samples_read >= devc->limit_samples) {
> + sr_info("Requested number of samples reached.");
> + sdi->driver->dev_acquisition_stop(sdi, cb_data);
> + goto out;
> + } else if (devc->limit_msec > 0) {
> + cur_time = g_get_monotonic_time();
> + elapsed_time = cur_time - devc->start_time;
> +
> + if (elapsed_time >= devc->limit_msec) {
> + sr_info("Sampling time limit reached.");
> + sdi->driver->dev_acquisition_stop(sdi, cb_data);
> + goto out;
> + }
> + }
> +
> +out:
> + devc->last_sample_fin = g_get_monotonic_time();
> + return TRUE;
Hm, probably nicer to avoid the goto here, it's only saving one
(relatively simple) line of code.
Uwe.
--
http://hermann-uwe.de | http://randomprojects.org | http://sigrok.org
------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
