RE: TDM bus support in Linux Kernel [PATCH]

RE: TDM bus support in Linux Kernel [PATCH]

[Kurachkin Michail]

[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]

Hi Greg,

I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.

Regrads,
Michail
________________________________________
From: Greg Kroah-Hartman [gregkh@linuxfoundation.org]
Sent: Thursday, November 29, 2012 1:38
To: Kurachkin Michail
Cc: linux-kernel@vger.kernel.org; Kuten Ivan; Josh Triplett
Subject: Re: TDM bus support in Linux Kernel

On Mon, Nov 19, 2012 at 11:00:12AM +0000, Kurachkin Michail wrote:
> Hi Greg,
>
> During the work on developing VoIP network router I designed TDM bus subsystem and wrote SLIC driver for si3226x. This code was developed for the commercial project, and now I am willing to release it under GPL.
> Please review my sources (or give me a clue on what to do next) and give some feedback on my work.
> https://github.com/stelhs/slic_tdm.git

It's really hard to review a random git tree somewhere, and having it in
this form doesn't make it easy to add to the kernel tree itself.

So, could you take your git tree and convert it to a patch (or patches)
to be applied to the kernel.org tree and send it in email so we can see
what it looks like that way?

I'd recommend putting everything under drivers/staging/ for now, to make
it easier to accept at the moment, and it makes it easier to clean up
and fix up any issues in-kernel, instead of having to keep huge patches
outside of the tree.

Take a look at the file, Documentation/SubmittingPatches for how to
create a patch and what I need in order for it to be able to be applied.

thanks,

greg k-h

[-- Attachment #2: tdm_slic.patch --]
[-- Type: application/octet-stream, Size: 172437 bytes --]

Re: TDM bus support in Linux Kernel [PATCH]

[Oliver Neukum]

On Wednesday 30 January 2013 12:37:25 Kurachkin Michail wrote:
> Hi Greg,
> 
> I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.

Hi,

given the size of the thing I hope you don't mind a review in parts.
Here we go.


Do you really need all those helpers? At a minimum they should become static
to not pollute common namespaces.

	Regards
		Oliver

+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include "fifo.h"
+
+static void inc_pointer(int *pointer, int item_size, int count_items);
+
+
+/**
+ * Init circular fifo buffer
+ * @param buf - buffer descriptor
+ * @param item_size - item size in bytes
+ * @param count - count items in buffer
+ * @return 0 - ok
+ */
+int init_fifo(struct fifo_buffer *fifo, int item_size, int count)
+{
+	fifo->read_pointer = 0;
+	fifo->write_pointer = 0;
+
+	fifo->buf = kzalloc(item_size * count, GFP_KERNEL);
+	if(!fifo->buf)
+		return -ENOMEM;
+
+	fifo->count = count;
+	fifo->item_size = item_size;
+	memset(fifo->buf, 0, item_size * count);
+	return 0;
+}
+
+
+/**
+ * free fifo buffer
+ * @param fifo
+ */
+void free_fifo(struct fifo_buffer *fifo)
+{
+	if(!fifo)
+		return;
+
+	if(!fifo->buf) {
+		kfree(fifo);
+		return;
+	}
+
+	kfree(fifo->buf);
+}
+
+
+/**
+ * Push into fifo
+ * @param fifo - fifo descriptor
+ * @param item - pointer to item
+ * @return 0 - ok, 1 - item pushed and buffer is full
+ */
+int fifo_push(struct fifo_buffer *fifo, void *item)
+{
+	memcpy(fifo->buf + fifo->write_pointer, item, fifo->item_size);
+
+	inc_pointer(&fifo->write_pointer, fifo->item_size, fifo->count);
+
+	if(fifo->write_pointer == fifo->read_pointer) { // If fifo is full
+		inc_pointer(&fifo->read_pointer, fifo->item_size, fifo->count);
+		return 1;
+	}
+
+	return 0;
+}
+
+
+/**
+ * Check fifo buffer is full
+ * @param fifo - fifo descriptor
+ * @return 1 - buffer is full
+ */
+int fifo_is_full(struct fifo_buffer *fifo)
+{
+	int p;
+	p = fifo->write_pointer;
+	inc_pointer(&p, fifo->item_size, fifo->count);
+	if(p == fifo->read_pointer)
+		return 1;
+
+	return 0;
+}
+
+
+/**
+ * Check fifo is empty
+ * @param fifo - fifo descriptor
+ * @return 1 - buffer is empty
+ */
+int fifo_is_empty(struct fifo_buffer *fifo)
+{
+	return fifo->read_pointer == fifo->write_pointer;
+}
+
+
+/**
+ * Pop from fifo buffer
+ * @param fifo - fifo descriptor
+ * @param item - pointer to poped item
+ * @return 0 - ok, 1 - buffer is empty
+ */
+int fifo_pop(struct fifo_buffer *fifo, void *item)
+{
+	if(fifo->read_pointer == fifo->write_pointer)
+		return 1;
+
+	memcpy(item, fifo->buf + fifo->read_pointer, fifo->item_size);
+	inc_pointer(&fifo->read_pointer, fifo->item_size, fifo->count);
+	return 0;
+}
+
+
+/**
+ * Increment pointer in circular buffer
+ * @param pointer - pointer in circular buffer
+ * @param item_size - item size in bytes
+ * @param count_items - count items in fifo
+ */
+static void inc_pointer(int *pointer, int item_size, int count_items)
+{
+	(*pointer) += item_size;
+
+	if(*pointer >= (count_items * item_size))
+		*pointer = 0;
+}
+

Re: TDM bus support in Linux Kernel [PATCH]

[Oliver Neukum]

On Wednesday 30 January 2013 12:37:25 Kurachkin Michail wrote:
> Hi Greg,
> 
> I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.

Part #2

+/**
+ * slic controls over ioctl
+ * @param slic - slic descriptor
+ * @param cmd - command
+ * @param arg - argument
+ * @return 0 - ok
+ */
+int slic_control(struct si3226x_slic *slic, unsigned int cmd, unsigned long arg)
+{
+	int rc;
+	struct si3226x_line *line = slic->lines;
+	int i;
+
+	switch(cmd) {
+	case SI3226X_SET_COMPANDING_MODE:
+		if(	arg != SI_ALAW && arg !=	SI_MLAW)
+			return -EINVAL;
+
+		for (i = 0; i < SI3226X_MAX_CHANNELS; i++, line++) {
+			if (line->state == SI_LINE_DISABLE)
+				continue;
+
+			line->companding_mode = arg;
+
+			rc = slic_setup_audio(line);
+			if (rc)
+				return rc;

This interface is not well thought out. If you get an error, the command
has been partially executed, but you have no idea how far.

+		}
+		break;
+
+	case SI3226X_SET_CALLERID_MODE:
+		if(	arg != SI_FSK_BELLCORE && arg != SI_FSK_ETSI
+		    && arg != SI_CALLERID_DTMF)
+			return -EINVAL;
+
+		for (i = 0; i < SI3226X_MAX_CHANNELS; i++, line++) {
+			if (line->state == SI_LINE_DISABLE)
+				continue;
+
+			line->callerid_mode = arg;
+		}
+		break;
+	}
+
+	return 0;
+}

+/**
+ * slic line controls over ioctl
+ * @param line - line descriptor
+ * @param cmd - command
+ * @param arg - argument
+ * @return 0 - ok
+ */
+int line_control(struct si3226x_line *line, unsigned int cmd, unsigned long arg)
+{
+	int rc = 0;
+	u8 data;
+	struct si3226x_caller_id caller_id;
+	u8 callerid_buffer[CALLERID_BUF_SIZE];
+
+	switch(cmd) {
+	case SI3226X_SET_CALLERID_MODE:
+		if( arg != SI_FSK_BELLCORE && arg != SI_FSK_ETSI
+		    && arg != SI_CALLERID_DTMF && arg != SI_CALLERID_NONE)
+			return -EINVAL;
+
+		line->callerid_mode = arg;
+		break;
+
+	case SI3226X_SET_ECHO_CANCELATION:
+		if(arg)
+			rc = slic_enable_echo(line);
+		else
+			rc = slic_disable_echo(line);
+		break;
+
+	case SI3226X_SET_LINE_STATE:
+		rc = slic_set_line_state(line, arg);
+		break;
+
+	case SI3226X_CALL:
+		copy_from_user(&caller_id, (__u8 __user *)arg, sizeof caller_id);

must handle errors

+
+		if(caller_id.size > CALLERID_BUF_SIZE)
+			caller_id.size = CALLERID_BUF_SIZE;
+
+		copy_from_user(callerid_buffer, (__u8 __user *)caller_id.data, caller_id.size);

must handle errors

+		rc = slic_line_call(line, callerid_buffer, caller_id.size);
+		break;
+
+	case SI3226X_SEND_DTMF:
+		rc = slic_send_dtmf_digit(line, arg);
+		break;
+
+	case SI3226X_GET_HOOK_STATE:
+		__put_user(line->hook_state, (__u8 __user *)arg);

must handle errors

+		break;
+
+	case SI3226X_GET_DTMF_DIGIT:
+		rc = fifo_pop(&line->dtmf, &data);
+		if (rc)
+			return -ENODATA;
+
+		__put_user(data, (__u8 __user *)arg);

must handle errors

+		break;
+
+	case SI3226X_GET_AUDIO_BLOCK_SIZE:
+		__put_user(line->audio_buffer_size, (__u8 __user *)arg);

must handle errors

+		break;
+
+	case SI3226X_SET_COMPANDING_MODE:
+		if(	arg != SI_ALAW && arg != SI_MLAW)
+			return -EINVAL;
+
+		line->companding_mode = arg;
+
+		rc = slic_setup_audio(line);
+		if (rc)
+			return rc;

a bit pointless

+
+		break;
+
+	case SI3226X_ENABLE_AUDIO:
+		if (line->tdm_dev == NULL)
+			return -ENODEV;
+
+		/* TODO: workaround to fix sound distortion after asterisk restart */
+		if (!line->audio_start_flag) {
+			rc = tdm_run_audio(line->tdm_dev);
+			if (!rc)
+				line->audio_start_flag = 1;
+		}
+
+		break;
+
+	case SI3226X_DISABLE_AUDIO:
+		if (line->tdm_dev == NULL)
+			return -ENODEV;
+
+		rc = tdm_stop_audio(line->tdm_dev);
+		break;
+	}
+
+	return rc;
+}

Re: TDM bus support in Linux Kernel [PATCH]

[Oliver Neukum]

On Wednesday 30 January 2013 12:37:25 Kurachkin Michail wrote:
> Hi Greg,
> 
> I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.

Part #3

+/*
+ * method "open" for character device
+ */
+static int slic_chr_open(struct inode *inode, struct file *file)
+{
+	struct slic_chr_dev *chr_dev;
+	struct si3226x_slic *slic;
+	struct si3226x_line *line;
+	struct tdm_device *tdm_dev;
+	struct tdm_voice_channel *ch;
+	int status = -ENXIO;
+
+	mutex_lock(&slic_chr_dev_lock);
+
+	list_for_each_entry(chr_dev, &slic_chr_dev_list, list) {
+		switch (chr_dev->type) {
+		case SLIC_CHR_DEV:
+			slic = dev_get_drvdata(chr_dev->dev);
+
+			if (slic->devt != inode->i_rdev)
+				continue;;
+
+			if (slic->file_opened) {
+				status = -EBUSY;
+				goto out;
+			}
+
+			slic->file_opened = 1;
+			status = 0;
+			break;
+
+		case LINE_CHR_DEV:
+			line = dev_get_drvdata(chr_dev->dev);
+			tdm_dev = line->tdm_dev;
+
+			if (line->devt != inode->i_rdev)
+				continue;
+
+			if (line->file_opened) {
+				status = -EBUSY;
+				goto out;
+			}
+
+			line->audio_buffer_size = tdm_get_voice_block_size(tdm_dev);
+
+			line->rx_buffer = kzalloc(line->audio_buffer_size, GFP_KERNEL);
+			if (!line->rx_buffer) {
+				status = -ENOMEM;
+				goto out;
+			}
+
+			line->tx_buffer = kzalloc(line->audio_buffer_size, GFP_KERNEL);
+			if (!line->tx_buffer) {
+				status = -ENOMEM;

memory leak of line->rx_buffer

+				goto out;
+			}
+
+			/* store pointer to transmit wait_queue_head_t for use in poll() */
+			ch = tdm_dev->ch;
+			line->tx_wait_queue = &ch->tx_queue;
+			line->rx_wait_queue = &ch->rx_queue;
+			line->file_opened = 1;
+			status = 0;
+
+			break;
+		}
+
+		file->private_data = chr_dev;
+		status = 0;
+		goto out;

a bit pointless

+	}
+
+out:
+	mutex_unlock(&slic_chr_dev_lock);
+	return status;
+}

+/*
+ * method "ioctl" for character device
+ */
+static long
+slic_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct slic_chr_dev *chr_dev = file->private_data;
+	struct si3226x_slic *slic;
+	struct si3226x_line *line;
+	int status = -ENXIO;
+
+	if (_IOC_TYPE(cmd) != SI3226X_IOC_MAGIC)
+		return -ENOTTY;
+
+	mutex_lock(&slic_chr_dev_lock);
+
+	if (!file->private_data) {
+		status = -EPERM;

-EPERM?

+		goto out;
+	}
+
+	switch(chr_dev->type) {
+	case SLIC_CHR_DEV:
+		slic = dev_get_drvdata(chr_dev->dev);
+		if (!slic->file_opened) {
+			status = -ENODEV;
+			goto out;
+		}
+
+		status = slic_control(slic, cmd, arg);
+		break;
+
+	case LINE_CHR_DEV:
+		line = dev_get_drvdata(chr_dev->dev);
+		if (!line->file_opened) {
+			status = -ENODEV;
+			goto out;
+		}
+
+		status = line_control(line, cmd, arg);
+		break;
+	}
+
+out:
+	mutex_unlock(&slic_chr_dev_lock);
+	return status;
+}


+/*
+ * method "read" for character device
+ */
+static ssize_t
+slic_chr_read(struct file *file, char *buff, size_t count, loff_t *offp)
+{
+	struct slic_chr_dev *chr_dev = file->private_data;
+	struct si3226x_line *line;
+	struct tdm_device *tdm_dev;
+	int rc;
+
+	if (!file->private_data)
+		return -EPERM;
+
+	if(chr_dev->type != LINE_CHR_DEV)
+		return -EPERM;
+
+	mutex_lock(&slic_chr_dev_lock);
+
+	line = dev_get_drvdata(chr_dev->dev);
+	tdm_dev = line->tdm_dev;
+
+	if (count != line->audio_buffer_size) {
+		rc = -ENODATA;
+		goto out;
+	}
+
+	rc = tdm_recv(tdm_dev, line->rx_buffer);
+	if (rc) {
+		rc = -EFAULT;

EFAULT is usually not what you return upon a read error.

+		goto out;
+	}
+
+	rc = copy_to_user(buff, line->rx_buffer, count);
+	if (rc) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	rc = count;
+out:
+	mutex_unlock(&slic_chr_dev_lock);
+	return rc;
+}

Re: TDM bus support in Linux Kernel [PATCH]

[Oliver Neukum]

On Wednesday 30 January 2013 12:37:25 Kurachkin Michail wrote:
> Hi Greg,
> 
> I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.

Part #4

+/**
+ * add to list slic character device.
+ * @param dev - character device
+ * @param type - type of character device
+ * @return 0 - ok
+ */
+static int add_slic_chr_dev(struct device *dev, enum chr_dev_type type)
+{
+	struct slic_chr_dev *chr_dev;
+
+	chr_dev = kzalloc(sizeof(*chr_dev), GFP_KERNEL);
+	if (!chr_dev)
+		return -ENOMEM;
+
+	chr_dev->dev = dev;
+	chr_dev->type = type;

Here we have a memory reordering bug. Once you put
this on the list, other CPUs can see it. Yet you have not
made sure that the fields initialised before have been
flushed to RAM.

+	list_add(&chr_dev->list, &slic_chr_dev_list);
+
+	return 0;
+}

+/**
+ * Init slic driver
+ * @return 0 - ok
+ */
+static int init_slic_drv(struct si3226x_slic *slic)
+{
+	struct platform_device *pdev = slic->pdev;
+	struct si3226x_platform_data *plat = 	pdev->dev.platform_data;
+	struct si3226x_line *line = slic->lines;
+	struct device *chr_dev;
+	int rc;
+	int i;
+
+	dev_info(&pdev->dev, "run Initialization slic driver\n");
+
+	/* set default companding_mode for all lines */
+	for (i = 0; i < SI3226X_MAX_CHANNELS; i++, line++)
+		line->companding_mode = plat->companding_mode;
+
+	/* request reset GPIO */
+	rc = gpio_request(slic->reset_gpio, DRIVER_NAME "_reset");
+	if (rc < 0) {
+		dev_err(&pdev->dev, "failed to request " DRIVER_NAME "_reset\n");
+		goto out0;
+	}
+	gpio_direction_output(slic->reset_gpio, 1);
+
+	/* request interrupt GPIO */
+	rc = gpio_request(slic->int_gpio, DRIVER_NAME "_irq");
+	if (rc < 0) {
+		dev_err(&pdev->dev, "failed to request " DRIVER_NAME "_irq\n");
+		goto out1;
+	}
+	gpio_direction_input(slic->int_gpio);
+
+	dev_info(&pdev->dev, "GPIO requested\n");
+
+	slic->irq = gpio_to_irq(slic->int_gpio);
+
+	dev_info(&pdev->dev, "slic->int_gpio = %d\n", slic->int_gpio);
+	dev_info(&pdev->dev, "slic->irq = %d\n", slic->irq);
+
+	INIT_WORK(&slic->irq_work, slic_irq_callback);
+
+#ifdef CONFIG_SI3226X_POLLING
+	INIT_DELAYED_WORK(&slic->delayed_work, slic_delayed_work);
+#endif
+
+
+	/* register slic character device */
+	rc = register_chrdev(SI3226X_MAJOR, DRIVER_NAME, &slic_cnt_ops);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "can not register character device\n");
+		goto out2;
+	}

This is a race condition. You register a device before you are
finished initialising it properly.

+	/* added class device and create /dev/si3226x_cnt.X */
+	slic->devt = MKDEV(SI3226X_MAJOR, pdev->id);
+	chr_dev = device_create(slic_class, &pdev->dev, slic->devt,
+	                        slic, DRIVER_NAME "_cnt.%d", pdev->id);
+
+	if (IS_ERR(chr_dev)) {
+		dev_err(&pdev->dev, "can not added class device\n");
+		goto out3;
+	}
+
+	/*  added character device into device list
+	    for use in file operations
+	*/
+	rc = add_slic_chr_dev(chr_dev, SLIC_CHR_DEV);
+	if (rc) {
+		dev_err(&pdev->dev, "can not added character device\n");
+		goto out4;
+	}
+
+	line = slic->lines;
+	for (i = 0; i < SI3226X_MAX_CHANNELS; i++, line++) {
+		if (plat->fxs_tdm_ch[i] < 0) {
+			line->state = SI_LINE_DISABLE;
+			continue;
+		}
+
+		INIT_WORK(&line->line_call_work, do_line_call);
+
+		line->state = SI_LINE_SILENCE;
+
+		/* added class device and create /dev/si3226x_fxsX.X */
+		line->devt = MKDEV(SI3226X_MAJOR, pdev->id + 1 + i);
+		chr_dev = device_create(slic_class, &pdev->dev, line->devt,
+		                        line, DRIVER_NAME "_fxs%d.%d", i, pdev->id);
+
+		if (IS_ERR(chr_dev)) {
+			dev_err(&pdev->dev, "can not added class device\n");
+			goto out4;
+		}
+
+		/*  added created character device into device list
+		    for use in file operations
+		*/
+		rc = add_slic_chr_dev(chr_dev, LINE_CHR_DEV);
+		if (rc) {
+			dev_err(&pdev->dev, "can't added character device\n");
+			goto out4;
+		}
+	}
+
+	rc = init_slic(slic);
+	if (rc) {
+		dev_err(&pdev->dev, "slic initialization fail\n");
+		goto out4;
+	}
+
+#ifdef CONFIG_SI3226X_POLLING
+	dev_info(&pdev->dev, "schedule delayed work\n");
+	schedule_delayed_work(&slic->delayed_work, MSEC(50));
+#else
+	dev_info(&pdev->dev, "request irq\n");
+
+	/* set interrupt callback slic_irq */
+	rc = request_irq(slic->irq, slic_irq,
+	                 IRQF_TRIGGER_FALLING | IRQF_DISABLED,
+	                 dev_name(&slic->pdev->dev), slic);
+
+	if (rc) {
+		dev_err(&pdev->dev, "can not request IRQ\n");
+		rc = -EINVAL;
+		goto out5;
+	}
+#endif
+
+	dev_info(&pdev->dev, "success initialization slic driver\n");
+	return 0;
+
+out5:
+	free_irq(slic->irq, slic);
+	deinit_slic(slic);
+
+out4:
+	{
+		/* remove all character devices */
+		struct slic_chr_dev *chr_dev, *chr_dev_tmp;
+		list_for_each_entry_safe(chr_dev, chr_dev_tmp,
+		                         &slic_chr_dev_list, list) {
+			device_del(chr_dev->dev);
+			del_slic_chr_dev(chr_dev->dev);
+		}
+	}
+
+out3:
+	unregister_chrdev(SI3226X_MAJOR, DRIVER_NAME);
+
+out2:
+	gpio_free(slic->int_gpio);
+
+out1:
+	gpio_free(slic->reset_gpio);
+
+out0:
+	return rc;
+}

+/**
+ * allocate memory and configure slic descriptor.
+ * @param pdev - platform device
+ * @return allocated slic controller descriptor
+ */
+struct si3226x_slic *alloc_slic(struct platform_device *pdev) {
+	int i;
+	struct si3226x_slic 	*slic;
+	struct si3226x_line *line;
+
+	slic = kzalloc(sizeof *slic, GFP_KERNEL);
+	if (!slic)
+		return NULL;
+
+	memset(slic, 0, sizeof *slic);

kzalloc and memset is overkill

+	platform_set_drvdata(pdev, slic);
+
+	slic->pdev = pdev;
+
+	line = slic->lines;
+	for (i = 0; i < SI3226X_MAX_CHANNELS; i++, line++) {
+		line->ch = i;
+		line->audio_start_flag = 0;
+	}
+
+	return slic;
+}
+

Re: TDM bus support in Linux Kernel [PATCH]

[Oliver Neukum]

On Wednesday 30 January 2013 12:37:25 Kurachkin Michail wrote:
> Hi Greg,
> 
> I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.
> 
Part #5

+/**
+ * Reset SLIC
+ */
+static int slic_reset(struct si3226x_slic *slic)
+{
+	unsigned long timeout;
+
+	gpio_set_value(slic->reset_gpio, 0);
+
+	timeout = jiffies + MSEC(RESET_SLIC_PERIOD);
+
+	while(!(time_after(jiffies, timeout)))
+		schedule();

Ouch. We have a delay primitive.

+
+	gpio_set_value(slic->reset_gpio, 1);
+
+	return 0;
+}

+/**
+ * Receive voice data block from TDM voice channel controller.
+ * @param ch - voice channel attendant to transmit data in TDM frame
+ * @param data - pointer to read data received by DMA.
+                 Length data for read equal to value returned by get_tdm_voice_block_size()
+ *
+ * Context: can sleep
+ * @return 0 on success; negative errno on failure
+ */
+static int kirkwood_recv(struct tdm_voice_channel *ch, u8 *data)
+{
+	struct kirkwood_tdm_voice *onchip_ch = ch->private_data;
+
+	wait_event_interruptible(ch->rx_queue,
+	                         get_rx_latency(onchip_ch) > 1);

If you are waiting interruptably, you need to be able to handle an interruption.

+
+	memcpy(data, onchip_ch->rx_buf[atomic_read(&onchip_ch->read_rx_buf_num)],
+	       ch->buffer_len);
+
+	atomic_set(&onchip_ch->read_rx_buf_num,
+	           inc_next_dma_buf_num(atomic_read(&onchip_ch->read_rx_buf_num)));
+
+	return 0;
+}

Re: TDM bus support in Linux Kernel [PATCH]

[Greg Kroah-Hartman]

On Wed, Jan 30, 2013 at 12:37:25PM +0000, Kurachkin Michail wrote:
> Hi Greg,
> 
> I followed your recommendations and created a diff using Linux 3.8-rc5
> sources. Please review it and give your comments.

As-is, it looks good enough to add to the staging tree, so can you
resend it, not as a base64 attachment, with a Signed-off-by: line (as
described in the kernel file, Documentation/SubmittingPatches) and we
can work to clean up the code in the kernel based on Oliver's comments.

thanks,

greg k-h

Re[2]: TDM bus support in Linux Kernel [PATCH]

[Michail Kurachkin]

Hi Oliver,

Thank you for the code review. I am working on the sources and soon
will send you the update.

By the way, I did not find suitable implementation of software circular buffer management. src/include/linux/circ_buf.h seems to be very limited solution. 
What do you think about adding the following functions/macros to the global namespace?

int cb_init(struct circ_buf *cb, int item_size, int count);
void cb_free(struct circ_buf *cb);
int cb_push(struct circ_buf *cb, void *item);
int cb_pop(struct circ_buf *cb, void *item);
int cb_is_full(struct circ_buf *cb);
int cb_is_empty(struct circ_buf *cb);

> On Wednesday 30 January 2013 12:37:25 Kurachkin Michail wrote:
>> Hi Greg,
>> 
>> I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.


--
Kurochkin Michail
Software engineer
Promwad Innovation Company
22, Olshevskogo St.,
220073, Minsk, BELARUS
phone: +375 17 312-1246 ext. 801
mobile: +375 29 609-1024
mail: Michail.Kurachkin@promwad.com
www: www.promwad.com

Re: TDM bus support in Linux Kernel [PATCH]

[Greg Kroah-Hartman]

On Tue, Feb 05, 2013 at 10:31:21AM +1100, Ryan Mallon wrote:
> On 30/01/13 23:37, Kurachkin Michail wrote:
> > Hi Greg,
> >
> > I followed your recommendations and created a diff using Linux 3.8-rc5 sources. Please review it and give your comments.
> >
> >
> 
> Hi Kurachkin,
> 
> Below is a quick skim-through review (of about 70% of the code). I've
> not really commented on the design, mostly just on coding style and
> small bugs. For patch-sets of this size, can you please break them down
> into multiple small patches, and please post patches inline which make
> reviewing easier.

Those types of changes can all be done after the code is in the
drivers/staging/ tree, and are not a barrier for acceptance at this
point in time.

thanks,

greg k-h

Re: TDM bus support in Linux Kernel [PATCH]

[Oliver Neukum]

On Monday 04 February 2013 16:08:58 Michail Kurachkin wrote:
> Hi Oliver,
> 
> Thank you for the code review. I am working on the sources and soon
> will send you the update.
> 
> By the way, I did not find suitable implementation of software circular buffer management. src/include/linux/circ_buf.h seems to be very limited solution. 
> What do you think about adding the following functions/macros to the global namespace?

Additions to the global namespace needs to be done very carefully.

> int cb_init(struct circ_buf *cb, int item_size, int count);
> void cb_free(struct circ_buf *cb);
> int cb_push(struct circ_buf *cb, void *item);
> int cb_pop(struct circ_buf *cb, void *item);

void pointers here may be suboptimal

> int cb_is_full(struct circ_buf *cb);
> int cb_is_empty(struct circ_buf *cb);

I suggest you implement them in private and if they serve you well,
we can discuss making them global. The important point is that you
don't duplicate code.

	Regards
		Oliver

Re[2]: TDM bus support in Linux Kernel [PATCH]

[Michail Kurachkin]

Hi,

I just sent reworked version of the code. I am looking forward for comments.
Sorry, I have no time to fix your next requests:

1) It's a bit clunky having two device types on the same character device. Is there a better way to do this?

2) + mutex_lock(&slic_chr_dev_lock);
This locking is very heavy handed. You are holding it across the entire open, close, read, write, ioctl, and it is protecting a bunch of different things. Can you make the locking a bit more fine grained?

3) + rc = add_to_slic_devices_list(&tdm_dev->dev, TDM_DEVICE);
This function is the same as probe_spi_slic, except for the device type. A single function would prevent the code duplication.

Will do it later.


Thanks,
Michail


> On Monday 04 February 2013 16:08:58 Michail Kurachkin wrote:
>> Hi Oliver,
>> 
>> Thank you for the code review. I am working on the sources and soon
>> will send you the update.
>> 
>> By the way, I did not find suitable implementation of software circular buffer management. src/include/linux/circ_buf.h seems to be very limited solution. 
>> What do you think about adding the following functions/macros to the global namespace?

> Additions to the global namespace needs to be done very carefully.

>> int cb_init(struct circ_buf *cb, int item_size, int count);
>> void cb_free(struct circ_buf *cb);
>> int cb_push(struct circ_buf *cb, void *item);
>> int cb_pop(struct circ_buf *cb, void *item);

> void pointers here may be suboptimal

>> int cb_is_full(struct circ_buf *cb);
>> int cb_is_empty(struct circ_buf *cb);

> I suggest you implement them in private and if they serve you well,
> we can discuss making them global. The important point is that you
> don't duplicate code.

>         Regards
>                 Oliver



--
Kurochkin Michail
Software engineer
Promwad Innovation Company
22, Olshevskogo St.,
220073, Minsk, BELARUS
phone: +375 17 312-1246 ext. 801
mobile: +375 29 609-1024
mail: Michail.Kurachkin@promwad.com
www: www.promwad.com

Re: TDM bus support in Linux Kernel [PATCH]

[Ivan Kuten]

On 13.02.2013 20:08, Michail Kurachkin wrote:
> Hi,
>
> I just sent reworked version of the code. I am looking forward for comments.
> Sorry, I have no time to fix your next requests:

Michail,
Where is your patch?

Regards,
Ivan