[2/3][PATCH][upstream] TDM Framework
Tabi Timur-B04825
B04825 at freescale.com
Tue Jul 24 02:33:27 EST 2012
On Mon, Jul 23, 2012 at 5:49 AM, <b37400 at freescale.com> wrote:
> From: Sandeep Singh <Sandeep at freescale.com>
Please fix your git configuration so that the From: line in your
emails contains your full name. This patch was sent with this From:
line:
From: <b37400 at freescale.com>
It should say:
From: Sandeep Singh <sandeep at freescale.com>
Three more things:
1) You don't need to add [upstream] to patches that are posted upstream.
2) This patch needs to be CC'd to linux-kernel at vger.kernel.org and
devel at driverdev.osuosl.org.
3) I'm concerned that there is no "struct device" anywhere in this
framework. I think that's a serious omission, and you need to figure
out where you can put this.
> diff --git a/drivers/tdm/tdm-core.c b/drivers/tdm/tdm-core.c
> new file mode 100644
> index 0000000..9973b6b
> --- /dev/null
> +++ b/drivers/tdm/tdm-core.c
> @@ -0,0 +1,1082 @@
> +/* driver/tdm/tdm-core.c
> + *
> + * Copyright (C) 2012 Freescale Semiconductor, Inc, All rights reserved.
I've been seeing this copyright messages a lot recently, and I don't
understand why. This message is incorrectly formatted. The "(C)" is
redundant, and the phrase "All rights reserved." is wrong. This patch
is licensed under the GPL, so we are NOT reserving all rights, we are
actually giving up some rights.
Please change this to:
Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * TDM core is the interface between TDM clients and TDM devices.
> + * It is also intended to serve as an interface for line controlled
> + * devices later on.
> + *
> + * Author:Hemant Agrawal <hemant at freescale.com>
> + * Rajesh Gumasta <rajesh.gumasta at freescale.com>
If these two are the authors, why are they not on the signed-off-by lines?
> + *
> + * Modified by Sandeep Kr Singh <sandeep at freescale.com>
> + * Poonam Aggarwal <poonam.aggarwal at freescale.com>
Do not add "modified" by lines to the code. That's why we have a git history.
> + * 1. Added framework based initialization of device.
> + * 2. All the init/run time configuration is now done by framework.
> + * 3. Added channel level operations.
> + * 4. Added sysfs knob to configure use_latest_tdm_data at runtime.
Again, this is not information that belongs in the source file.
> + *
> + * Note that some parts of this code may have been derived from i2c subsystem.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/tdm.h>
> +#include <linux/init.h>
> +#include <linux/idr.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/hardirq.h>
> +#include <linux/irqflags.h>
> +#include <linux/list.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
Are you sure you need all of these header files?
> +
> +
> +static DEFINE_MUTEX(tdm_core_lock);
> +static DEFINE_IDR(tdm_adapter_idr);
> +/* List of TDM adapters registered with TDM framework */
> +LIST_HEAD(adapter_list);
> +
> +/* List of TDM clients registered with TDM framework */
> +LIST_HEAD(driver_list);
> +
> +/* In case the previous data is not fetched by the client driver, the
> + * de-interleaving function will discard the old data and rewrite the
> + * new data */
Proper multi-line comment format is like this:
/*
* In case the previous data is not fetched by the client driver, the
* de-interleaving function will discard the old data and rewrite the
* new data.
*/
> +static int use_latest_tdm_data = 1;
> +
> +/* Data structures required for sysfs */
> +static struct tdm_sysfs attr = {
> + .attr.name = "use_latest_data",
> + .attr.mode = 0664,
> + .cmd_type = TDM_LATEST_DATA,
> +};
> +
> +static struct attribute *tdm_attr[] = {
> + &attr.attr,
> + NULL
> +};
> +
> +const struct sysfs_ops tdm_ops = {
> + .show = tdm_show_sysfs,
> + .store = tdm_store_sysfs,
> +};
> +
> +static struct kobj_type tdm_type = {
> + .sysfs_ops = &tdm_ops,
> + .default_attrs = tdm_attr,
> +};
Why are some of these 'const' and some aren't? They should all be 'const'
> +
> +/* tries to match client driver with the adapter */
> +static int tdm_device_match(struct tdm_driver *driver, struct tdm_adapter *adap)
> +{
> + /* match on an id table if there is one */
> + if (driver->id_table && driver->id_table->name[0]) {
> + if (!(strcmp(driver->id_table->name, adap->name)))
> + return (int)driver->id_table;
> + }
> + return TDM_E_OK;
Get rid of TDM_E_OK. It's the only error code that you've defined,
and it's set to 0. That doesn't mean anything.
> +}
> +
> +static int tdm_attach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + int ret = TDM_E_OK;
> + /* if driver is already attached to any other adapter, return*/
> + if (driver->adapter && (driver->adapter != adap))
> + return ret;
> +
> + driver->adapter = adap;
> +
> + if (driver->attach_adapter) {
> + ret = driver->attach_adapter(adap);
> + if (ret < 0) {
> + pr_err("attach_adapter failed for driver [%s] err:%d\n"
> + , driver->name, ret);
> + return ret;
> + }
> + }
> + adap->drv_count++;
> +
> + if (!adap->tasklet_conf) {
> + tdm_sysfs_init();
> + tasklet_init(&adap->tdm_data_tasklet, tdm_data_tasklet_fn,
> + (unsigned long)adap);
> + adap->tasklet_conf = 1;
> + }
> +
> + return ret;
If 'ret' can only be 0, then just return 0.
return 0;
> +/* tdm_adap_send - issue a TDM write
> + * @adap: Handle to TDM device
> + * @buf: Data that will be written to the TDM device
> + * @count: How many bytes to write
> + *
> + * Returns negative errno, or else the number of bytes written.
"Returns negative error number, or else the number of bytes written."
You're typing this on a full keyboard, not a smart phone. Please
don't use abbreviations unnecessarily.
> + */
> +int tdm_adap_send(struct tdm_adapter *adap, void **buf, int count)
> +{
> + int res;
> +
> + if (adap->algo->tdm_write)
> + res = adap->algo->tdm_write(adap, buf, count);
Why does tdm_write() return a u32? And shouldn't 'res' also be a u32,
to make tdm_write()?
> +/* Port Level APIs of TDM Framework */
> +int tdm_port_open(struct tdm_driver *driver, struct tdm_port **h_port)
> +{
> + struct tdm_port *port;
> + struct tdm_adapter *adap;
> + unsigned long flags;
> + int res = TDM_E_OK;
> +
> + adap = tdm_get_adapter(driver->adapter->id);
> + if (!adap)
> + return -ENODEV;
> +
> + /* This creates an anonymous tdm_port, which may later be
> + * pointed to some slot.
> + *
> + */
> + port = kzalloc(sizeof(*port), GFP_KERNEL);
> + if (!port) {
> + res = -ENOMEM;
> + goto out;
> + }
Just return here. You don't need to "goto out". And I think you
forgot to call tdm_put_adapter(), although it might be easier if you
call kzalloc() first, and then tdm_get_adapter().
> +
> + port->rx_max_frames = NUM_SAMPLES_PER_FRAME;
> + port->port_cfg.port_mode = TDM_PORT_CHANNELIZED;
> +
> + snprintf(driver->name, TDM_NAME_SIZE, "tdm-dev");
> + port->driver = driver;
> + port->adapter = adap;
> +
> + spin_lock_irqsave(&adap->portlist_lock, flags);
> + list_add_tail(&port->list, &adap->myports);
> + spin_unlock_irqrestore(&adap->portlist_lock, flags);
Are you sure you need to disable interrupts? That seems excessive.
> +int tdm_channel_read(struct tdm_port *h_port, struct tdm_channel *h_channel,
> + void *p_data, u16 *size)
> +{
> + struct tdm_channel *channel;
> + struct tdm_bd *rx_bd;
> + unsigned long flags;
> + int i, res = TDM_E_OK;
> + unsigned short *buf, *buf1;
> + channel = h_channel;
> +
> + if (!channel->p_ch_data || !channel->in_use)
> + return -EIO;
> +
> + spin_lock_irqsave(&channel->p_ch_data->rx_channel_lock, flags);
> + rx_bd = channel->p_ch_data->rx_out_data;
> +
> + if (rx_bd->flag) {
> + *size = rx_bd->length;
> + buf = (u16 *) p_data;
> + buf1 = (u16 *)rx_bd->p_data;
> + for (i = 0; i < NUM_SAMPLES_PER_FRAME; i++)
> + buf[i] = buf1[i];
Use memcpy() instead.
> +int tdm_channel_write(struct tdm_port *h_port, struct tdm_channel *h_channel,
> + void *p_data, u16 size)
> +{
> + struct tdm_port *port;
> + struct tdm_channel *channel;
> + struct tdm_bd *tx_bd;
> + unsigned long flags;
> + int err = TDM_E_OK;
> + port = h_port;
> + channel = h_channel;
> +#ifdef DEBUG
> + bool data_flag = 0;
> +#endif
> +
> + if (p_data == NULL) { /* invalid data*/
> + pr_err("Invalid Data");
> + return -EFAULT;
> + }
EFAULT is for invalid memory access. I think you mean EINVAL here.
> +
> + if (!channel->p_ch_data || !channel->in_use)
> + return -EIO;
> +
> + spin_lock_irqsave(&channel->p_ch_data->tx_channel_lock, flags);
> + tx_bd = channel->p_ch_data->tx_in_data;
> +
> + if (!tx_bd->flag) {
> + tx_bd->length = size;
> + memcpy(tx_bd->p_data, p_data,
> + size * port->adapter->adapt_cfg.slot_width);
> + tx_bd->flag = 1;
> + tx_bd->offset = 0;
> + channel->p_ch_data->tx_in_data = (tx_bd->wrap) ?
> + channel->p_ch_data->tx_data_fifo : tx_bd+1;
> + port->port_stat.tx_pkt_count++;
> +#ifdef DEBUG
> + data_flag = 1;
> +#endif
> + } else {
> + spin_unlock_irqrestore(&channel->p_ch_data->tx_channel_lock,
> + flags);
> + port->port_stat.tx_pkt_drop_count++;
> + pr_err("E_NO_MEMORY -Failed Transmit");
Please review ALL of your error messages, and make sure that have a
proper prefix ("tdm: ") and all look the same. Don't use __func__ in
any of them, and make sure the error messages are clear, English text.
> + return -ENOMEM;
I'm not sure ENOMEM is appropriate here.
> + }
> + spin_unlock_irqrestore(&channel->p_ch_data->tx_channel_lock, flags);
> +
> +#ifdef DEBUG
> + if (data_flag) {
> + int k;
> + pr_info("\nTX port:%d - Write - Port TX-%d\n",
> + port->port_id, size);
> + for (k = 0; k < size; k++)
> + pr_info("%x", p_data[k]);
> + pr_info("\n");
> + }
> +#endif
> + return err;
> +}
> +EXPORT_SYMBOL(tdm_channel_write);
> +
> +/* Driver Function for select and poll. Based on Channel, it sleeps on
> + * waitqueue */
> +int tdm_ch_poll(struct tdm_channel *h_channel, unsigned int wait_time)
> +{
> + struct tdm_channel *channel;
> + unsigned long timeout = msecs_to_jiffies(wait_time);
> + channel = h_channel;
> +
> + if (!channel->p_ch_data || !channel->in_use)
> + return -EIO;
> +
> + if (channel->p_ch_data->rx_out_data->flag) {
> + pr_debug("Data Available");
> + return TDM_E_OK;
> + }
> + if (timeout) {
> + wait_event_interruptible_timeout(channel->ch_wait_queue,
It'd be more efficient if you did this:
if (wait_time) {
unsigned long timeout = msecs_to_jiffies(wait_time);
wait_event_interruptible_timeout(channel->ch_wait_queue,
> + channel->p_ch_data->rx_out_data->flag,
> + timeout);
> +
> + if (channel->p_ch_data->rx_out_data->flag) {
> + pr_debug("Data Available");
> + return TDM_E_OK;
> + }
> + }
> + return -EAGAIN;
> +}
> +EXPORT_SYMBOL(tdm_ch_poll);
> +
> +unsigned int tdm_port_get_stats(struct tdm_port *h_port,
> + struct tdm_port_stats *portStat)
Do not use CamelCase in variable names.
> diff --git a/include/linux/tdm.h b/include/linux/tdm.h
> new file mode 100644
> index 0000000..a2f9628
> --- /dev/null
> +++ b/include/linux/tdm.h
> @@ -0,0 +1,338 @@
> +/* include/linux/tdm.h
> + *
> + * Copyright (C) 2012 Freescale Semiconductor, Inc, All rights reserved.
> + *
> + * tdm.h - definitions for the tdm-device framework interface
> + *
> + * Author:Hemant Agrawal <hemant at freescale.com>
> + * Rajesh Gumasta <rajesh.gumasta at freescale.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +
> +#ifndef _LINUX_TDM_H
> +#define _LINUX_TDM_H
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/device.h> /* for struct device */
> +#include <linux/sched.h> /* for completion */
> +#include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +#include <linux/sysfs.h>
> +
> +#define TDM_LATEST_DATA 1
> +#define CHANNEL_8BIT_LIN 0 /* 8 bit linear */
> +#define CHANNEL_8BIT_ULAW 1 /* 8 bit Mu-law */
> +#define CHANNEL_8BIT_ALAW 2 /* 8 bit A-law */
> +#define CHANNEL_16BIT_LIN 3 /* 16 bit Linear */
> +
> +/*
> + * Default adapter configuration. All the TDM adapters registered with
> + * framework will be configured with following default configuration.
> + */
> +#define NUM_CHANNELS 16
> +
> +/* Default configuration for typical voice data sample. These parameters
> + * will generally not be required to be changed for voice type applications.
> + */
> +#define NUM_SAMPLES_PER_MS 8 /* 8 samples per milli sec per
> + channel. Req for voice data */
> +#define NUM_MS 10
> +#define NUM_SAMPLES_PER_FRAME (NUM_MS * NUM_SAMPLES_PER_MS) /* Number of
> + samples for 1 client buffer */
> +#define NUM_OF_TDM_BUF 3
> +
> +/* General options */
> +
> +struct tdm_adapt_algorithm;
> +struct tdm_adapter;
> +struct tdm_port;
> +struct tdm_driver;
I don't think you need "struct tdm_driver;"
> +/* tdm_adapter_mode is to define in mode of the device */
> +enum tdm_adapter_mode {
> + TDM_ADAPTER_MODE_NONE = 0x00,
> + TDM_ADAPTER_MODE_T1 = 0x01,
> + TDM_ADAPTER_MODE_E1 = 0x02,
> + TDM_ADAPTER_MODE_T1_RAW = 0x10,
> + TDM_ADAPTER_MODE_E1_RAW = 0x20,
Where did these numbers come from?
--
Timur Tabi
Linux kernel developer at Freescale
More information about the Linuxppc-dev
mailing list