Kumar Abhishek wrote:
> These patches add BeagleLogic support to libsigrok.
Patch 1 (.gitignore) should be sent independently of BeagleLogic, if
at all. Please clarify if it will be accepted at all.
You're making good progress.
Please squash patches 2-4 into a single commit which directly adds a
complete and functional driver.
Kumar Abhishek wrote:
> +++ b/hardware/beaglelogic/beaglelogic.h
> @@ -0,0 +1,366 @@
> +/*
> + * This file is part of the libsigrok and the BeagleLogic project.
This doesn't make sense. The file is clearly in libsigrok.
> +/* ioctl calls that can be issued on /dev/beaglelogic */
> +#define IOCTL_BL_GET_VERSION _IOR('k', 0x20, uint32_t)
> +#define IOCTL_BL_GET_SAMPLE_RATE _IOR('k', 0x21, uint32_t)
> +#define IOCTL_BL_SET_SAMPLE_RATE _IOW('k', 0x21, uint32_t)
> +#define IOCTL_BL_GET_SAMPLE_UNIT _IOR('k', 0x22, uint32_t)
> +#define IOCTL_BL_SET_SAMPLE_UNIT _IOW('k', 0x22, uint32_t)
> +#define IOCTL_BL_GET_TRIGGER_FLAGS _IOR('k', 0x23, uint32_t)
> +#define IOCTL_BL_SET_TRIGGER_FLAGS _IOW('k', 0x23, uint32_t)
> +#define IOCTL_BL_CACHE_INVALIDATE _IO('k', 0x25)
> +#define IOCTL_BL_GET_BUFFER_SIZE _IOR('k', 0x26, uint32_t)
> +#define IOCTL_BL_SET_BUFFER_SIZE _IOW('k', 0x26, uint32_t)
> +#define IOCTL_BL_GET_BUFUNIT_SIZE _IOR('k', 0x27, uint32_t)
> +#define IOCTL_BL_FILL_TEST_PATTERN _IO('k', 0x28)
> +#define IOCTL_BL_START _IO('k', 0x29)
> +#define IOCTL_BL_STOP _IO('k', 0x2A)
This will never be accepted into the kernel. ioctls are quite scarce.
If you absolutely cannot avoid using an ioctl then you need to only
use a single one, passing a pointer to a request/response struct as
parameter, and having the command be a member of that struct.
Also, these defines should go into the kernel source, and libsigrok
should #include <linux/beaglelogic.h> or such. It will take a while
until distributions have that header available in /usr/include so
make sure that there's a way to specify an include path manually when
configuring libsigrok.
> +SR_PRIV int beaglelogic_open(void);
> +SR_PRIV int beaglelogic_open_nonblock(void);
> +SR_PRIV int beaglelogic_close(int fd);
> +SR_PRIV int beaglelogic_read(int fd, void *buf, size_t bytes);
> +SR_PRIV int beaglelogic_get_buffersize(int fd, uint32_t *bufsize);
> +SR_PRIV int beaglelogic_set_buffersize(int fd, uint32_t bufsize);
> +SR_PRIV int beaglelogic_get_samplerate(int fd, uint32_t *samplerate);
> +SR_PRIV int beaglelogic_set_samplerate(int fd, uint32_t samplerate);
> +SR_PRIV int beaglelogic_get_sampleunit(int fd,
> + enum beaglelogic_sampleunit *sampleunit);
> +SR_PRIV int beaglelogic_set_sampleunit(int fd,
> + enum beaglelogic_sampleunit sampleunit);
> +SR_PRIV int beaglelogic_get_triggerflags(int fd,
> + enum beaglelogic_triggerflags *triggerflags);
> +SR_PRIV int beaglelogic_set_triggerflags(int fd,
> + enum beaglelogic_triggerflags triggerflags);
> +SR_PRIV inline int beaglelogic_start(int fd);
> +SR_PRIV inline int beaglelogic_stop(int fd) ;
Note the whitespace issue before the semicolon in the above line.
> +SR_PRIV inline int beaglelogic_memcacheinvalidate(int fd);
> +SR_PRIV int beaglelogic_get_bufunitsize(int fd);
> +SR_PRIV void * beaglelogic_mmap(int fd);
> +SR_PRIV int beaglelogic_munmap(int fd, void *addr);
All the above functions deal with an fd for the open device.
This makes a lot of sense if the fd is a single identifier for one
instance of the hardware. However...
> +SR_PRIV int beaglelogic_getlasterror(void);
> + * Parameters:
> + * none. This uses sysfs attributes, hence no params are required
> +SR_PRIV int beaglelogic_waitfornextbuffer(void);
These two functions do *not* deal with any fd for the open device.
This might work because the hardware is in fact incapable of multiple
instances. That means there is no need to deal with an fd anywhere.
The libsigrok code can and should keep all state in static variables.
If the kernel driver doesn't support being opened multiple times
then the libsigrok code needs to take that into account.
> +SR_PRIV int beaglelogic_getlasterror(void) {
> + int fd = open(BEAGLELOGIC_SYSFS_ATTR(lasterror), O_RDONLY);
> + char buf[16];
> + char *endptr;
> + int lasterror, ret;
> +
> + if (!fd)
> + return -1;
open() returns -1 on errors, not 0.
> + if ((ret = read(fd, buf, 16)) < 0)
> + return -1;
So it is not an error if EOF (0) or less than 16 bytes are returned?
And what if 16 bytes are returned? Is the kernel guaranteed to add
the terminating zero byte?
> + close(fd);
> + lasterror = strtoul(buf, &endptr, 10);
If you don't use endptr to check for a successful conversion then
pass NULL instead. Best is of course to check for successful
conversion.
In general, please look into to what libsigrok device driver SR_PRIV
functions can actually return. I don't believe -1 is appropriate, but
that you should be using SR_* result values.
Kumar Abhishek wrote:
> +static struct dev_context * beaglelogic_devc_alloc(void)
> +{
> + struct dev_context *devc;
> +
> + /* Allocate the zeroed structure */
> + if (!(devc = g_try_malloc0(sizeof(*devc)))) {
> + sr_err("Device context alloc failed.");
> + return NULL;
> + }
> +
> + /* Default non-zero values (if any) */
> + devc->fd = -1;
So here's an fd memeber in a device context structure. This seems
like something the driver should utilize.
> @@ -38,8 +100,37 @@ static GSList *scan(GSList *options)
..
> + /* Get a little information from BeagleLogic */
> + if ((fd = beaglelogic_open()) == -1)
> + return NULL;
> + beaglelogic_get_sampleunit(fd, (uint32_t *)&i);
No error checking?
> @@ -56,31 +147,76 @@ static int dev_clear(void)
>
> static int dev_open(struct sr_dev_inst *sdi)
> {
> - (void)sdi;
> -
> - /* TODO: get handle from sdi->conn and open it. */
> + int fd;
> + struct dev_context *devc = sdi->priv;
> +
> + /* Open BeagleLogic */
> + fd = beaglelogic_open_nonblock();
> + if (fd == -1)
> + return SR_ERR;
> +
> + /* Set fd and local attributes */
> + devc->fd = fd;
Great that this saves the open fd.
> + /* Get default attributes */
> + beaglelogic_get_samplerate(fd, (uint32_t *)&devc->cur_samplerate);
> + beaglelogic_get_sampleunit(fd, (void *)&devc->sampleunit);
> + beaglelogic_get_triggerflags(fd, &devc->triggerflags);
> + beaglelogic_get_buffersize(fd, &devc->buffersize);
Notice how all these functions use both the fd and something from devc,
and notice that the fd is already present within devc. Why not change
the functions to simply take a pointer to devc, where fd is anyway
available, and let them use the corresponding member themselves?
Especially if the nasty casting is indeed required then that really
should happen within the respective function and not at every single
call site.
> +/* Define maximum possible feasible buffer size on the BeagleBone Black
> + * May be extended to >= 300 MB, but may leave system unstable
> + * Change this to 128 MB if compiling for the BeagleBone White */
> +#define MAX_MEM (256 * 1024 * 1024)
Why does this need to be set at build time? Why can't the driver
autodetect this at runtime instead?
It doesn't seem at all practical to require different binaries for
black and white.
> static int config_set(int key, GVariant *data, const struct sr_dev_inst *sdi,
> const struct sr_channel_group *cg)
..
> +#if 0
> + /* Try to allocate that buffer statically if possible. If not,
> then
> + * set continous mode on and fall back to 64 MB buffers */
..
> +#endif
I guess this block of code should just be removed at some point, or
included?
> +++ b/hardware/beaglelogic/protocol.c
..
> +#define min(a,b) ((a) < (b) ? (a) : (b))
This is usually called MIN() and is already used elsewhere in
libsigrok, probably defined by glib. Don't add it also in your file.
> +/* This implementation is zero copy from the libsigrok side.
That's nice.
> SR_PRIV int beaglelogic_receive_data(int fd, int revents, void *cb_data)
..
> if (revents == G_IO_IN) {
..
> + /* Dummy read the data into a null pointer. The kernel
> + * module detects this and just updates its internal
> + * read cursor for poll() to work properly */
> + read(fd, NULL, copysize);
This is really nasty. Why not use lseek(whence==SEEK_END)?
It's very tempting to start overloading the fd functions when writing
a kernel driver, but you should resist that urge and make the
character device behave like everyone else does. That way you
increase the chances of the kernel code being usable also outside of
the single usecase you have in mind at the moment.
> +++ b/hardware/beaglelogic/protocol.h
..
> +#define DEFAULT_SAMPLERATE SR_MHZ(50)
Doesn't this need to be included in the list of samplerates then?
//Peter
------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
