[PATCH][2/3][RFC] TDM Framework
Scott Wood
scottwood at freescale.com
Tue Apr 24 10:34:43 EST 2012
On 03/10/2012 06:57 AM, Poonam Aggrwal wrote:
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index ad6c1eb..25f7f5b 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -130,4 +130,5 @@ source "drivers/virt/Kconfig"
>
> source "drivers/net/dpa/NetCommSw/Kconfig"
>
> +source "drivers/tdm/Kconfig"
> endmenu
When posting patches to this list, please ensure that they are based on
an upstream Linux kernel and not a Freescale BSP kernel.
> +if TDM
> +
> +config TDM_DEBUG_CORE
> + bool "TDM Core debugging messages"
> + help
> + Say Y here if you want the TDM core to produce a bunch of debug
> + messages to the system log. Select this if you are having a
> + problem with TDM support and want to see more of what is going on.
> +
> +endif # TDM
Please use the normal kernel mechanisms for controlling debug messages.
> diff --git a/drivers/tdm/tdm-core.c b/drivers/tdm/tdm-core.c
> new file mode 100644
> index 0000000..cdda260
> --- /dev/null
> +++ b/drivers/tdm/tdm-core.c
> @@ -0,0 +1,1146 @@
> +/* driver/tdm/tdm-core.c
> + *
> + * Copyright (C) 2012 Freescale Semiconductor, Inc, All rights reserved.
> + *
> + * TDM core is the interface between TDM clients and TDM devices.
> + * It is also intended to serve as an interface for line controld
> + * devices later on.
> + *
> + * Author:Hemant Agrawal <hemant at freescale.com>
> + * Rajesh Gumasta <rajesh.gumasta at freescale.com>
> + *
> + * Modified by Sandeep Kr Singh <sandeep at freescale.com>
> + * Poonam Aggarwal <poonam.aggarwal at freescale.com>
> + * 1. Added framework based initialization of device.
> + * 2. All the init/run time configuration is now done by framework.
> + * 3. Added channel level operations.
> + *
> + * 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.
> + */
> +
> +/* if read write debug required */
> +#undef TDM_CORE_DEBUG
> +
> +#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>
> +#include "device/tdm_fsl.h"
What is a reference to tdm_fsl.h doing in the presumably
hardware-independent tdm-core.c?
> +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 */
> +static int use_latest_tdm_data = 1;
Describe when one would want to change this, and provide a better way to
change it (e.g. module parameter or sysfs knob).
/*
* Linux kernel
* multi-line comment style
* is like this.
*/
> +/* this tasklet is created for each adapter instance */
> +static void tdm_data_tasklet_fn(unsigned long);
> +
> +/* 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;
> +}
s/TDM_E_OK/0/g
> +static int tdm_attach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + /* if driver is already attached to any other adapter, return*/
> + if (driver->adapter && (driver->adapter != adap))
> + return TDM_E_OK;
Why can't one driver service multiple adapters? How would multiple
drivers service one adapter? Are there sub-devices that need individual
controlling?
> + driver->adapter = adap;
> +
> + if (driver->attach_adapter) {
> + if (driver->attach_adapter(adap) < 0)
> + /* We ignore the return code; if it fails, too bad */
> + pr_err("attach_adapter failed for driver [%s]\n",
> + driver->name);
Why ignore errors?
> +/* Detach client driver and adapter */
> +static int tdm_detach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + int res = TDM_E_OK;
> +
> + if (!driver->adapter || (driver->adapter != adap))
> + return TDM_E_OK;
Shouldn't this be an error?
> + if (!driver->detach_adapter)
> + return TDM_E_OK;
> +
> + adap->drv_count--;
If the driver doesn't have a detach_adapter method, you skip
decrementing the count and leave the tasklet lying around?
> +/* TDM adapter Registration/De-registration with TDM framework */
> +
> +static int tdm_register_adapter(struct tdm_adapter *adap)
> +{
> + int res = TDM_E_OK;
> + struct tdm_driver *driver, *next;
> +
> + if (!adap) {
> + pr_err("%s:Invalid handle\n", __func__);
> + return -EINVAL;
> + }
Pointers aren't handles. The caller should not be passing NULL, and it
would be more useful to crash and get a backtrace if it does. It's not
realistic to check every pointer for being NULL when there's no
legitimate reason it could be NULL, and it doesn't help you if you have
some other bad value besides NULL.
> + mutex_init(&adap->adap_lock);
> + INIT_LIST_HEAD(&adap->myports);
> + spin_lock_init(&adap->portlist_lock);
> +
> + adap->drv_count = 0;
> + adap->tasklet_conf = 0;
> +
> + list_add_tail(&adap->list, &adapter_list);
> +
> + /* initialization of driver by framework in default configuration */
> + init_config_adapter(adap);
> +
> + /* Notify drivers */
> + pr_info("adapter [%s] registered\n", adap->name);
This is too noisy. You haven't even gotten a match yet.
> +/*
> + * tdm_add_adapter - declare tdm adapter, use dynamic device number
Why are there device numbers at all?
I suspect there's a fair bit of copy and paste going on of another
subsystem's quirks (i2c?). I don't see any mention in the copyright
block of this code having been derived from anything else, though...
> +
> +
> +/**
> + * tdm_del_adapter - unregister TDM adapter
> + * @adap: the adapter being unregistered
> + *
> + * This unregisters an TDM adapter which was previously registered
> + * by @tdm_add_adapter.
> + */
> +int tdm_del_adapter(struct tdm_adapter *adap)
> +{
> + int res = TDM_E_OK;
> + struct tdm_adapter *found;
> + struct tdm_driver *driver, *next;
> +
> + if (!adap) {
> + pr_err("%s:Invalid handle\n", __func__);
> + return -EINVAL;
> + }
> +
> + /* First make sure that this adapter was ever added */
> + mutex_lock(&tdm_core_lock);
> + found = idr_find(&tdm_adapter_idr, adap->id);
> + mutex_unlock(&tdm_core_lock);
> + if (found != adap) {
> + pr_err("tdm-core: attempting to delete unregistered "
> + "adapter [%s]\n", adap->name);
> + return -EINVAL;
> + }
> +
> + /*disable and kill the data processing tasklet */
> + if (adap->tasklet_conf) {
> + tasklet_disable(&adap->tdm_data_tasklet);
> + tasklet_kill(&adap->tdm_data_tasklet);
> + adap->tasklet_conf = 0;
> + }
> +
> + /* Detach any active ports. This can't fail, thus we do not
> + checking the returned value. */
> + mutex_lock(&tdm_core_lock);
> + list_for_each_entry_safe(driver, next, &driver_list, list) {
> + if (tdm_device_match(driver, adap)) {
> + tdm_detach_driver_adap(driver, adap);
> + pr_info(
> + "Driver(ID=%d) is detached from Adapter %s(ID = %d)\n",
> + driver->id, adap->name, adap->id);
> + }
> + }
> + mutex_unlock(&tdm_core_lock);
> +
> + mutex_lock(&tdm_core_lock);
> + idr_remove(&tdm_adapter_idr, adap->id);
> + mutex_unlock(&tdm_core_lock);
Why are you dropping the lock then reacquiring it?
> +/* TDM Client Drivers Registration/De-registration Functions */
> +int tdm_register_driver(struct tdm_driver *driver)
> +{
> + int res = TDM_E_OK;
> + struct tdm_adapter *adap, *next;
> +
> + list_add_tail(&driver->list, &driver_list);
> +
> + mutex_lock(&tdm_core_lock);
> + /* Walk the adapters that are already present */
> + list_for_each_entry_safe(adap, next, &adapter_list, list) {
> + if (tdm_device_match(driver, adap)) {
> + res = tdm_attach_driver_adap(driver, adap);
> + pr_info("TDM Driver(ID=%d)is attached with Adapter"
> + "%s(ID = %d) drv_count=%d", driver->id,
> + adap->name, adap->id, adap->drv_count);
> + break;
> + }
> + }
Indentation.
> + mutex_unlock(&tdm_core_lock);
> +
> + return res;
> +}
> +EXPORT_SYMBOL(tdm_register_driver);
> +
> +/*
> + * tdm_unregister_driver - unregister TDM client driver from TDM framework
> + * @driver: the driver being unregistered
> + */
> +void tdm_unregister_driver(struct tdm_driver *driver)
> +{
> + if (!driver) {
> + pr_err("%s:Invalid handle\n", __func__);
> + return;
> + }
> + /* A driver can register to only one adapter,
> + * so no need to browse the list */
Whitespace.
> + mutex_lock(&tdm_core_lock);
> + tdm_detach_driver_adap(driver, driver->adapter);
> + mutex_unlock(&tdm_core_lock);
> +
> + list_del(&driver->list);
> +
> + pr_debug("tdm-core: driver [%s] unregistered\n", driver->name);
> +}
> +EXPORT_SYMBOL(tdm_unregister_driver);
> +
> +/* TDM Framework init and exit */
> +static int __init tdm_init(void)
> +{
> + pr_info("%s\n", __func__);
> + return TDM_E_OK;
> +}
> +
> +static void __exit tdm_exit(void)
> +{
> + pr_info("%s\n", __func__);
> + return;
> +}
Don't spam the console just because the driver got loaded or unloaded
(at this point you haven't even found the hardware).
> +/* We must initialize early, because some subsystems register tdm drivers
> + * in subsys_initcall() code, but are linked (and initialized) before tdm.
> + */
> +postcore_initcall(tdm_init);
> +module_exit(tdm_exit);
tdm_init doesn't do anything, so why does it need to run early?
> +/* Port Level APIs of TDM Framework */
> +unsigned int tdm_port_open(struct tdm_driver *driver, void **h_port)
Why is the return unsigned int? You're returning negative numbers.
Also consider having the return be a pointer, and use PTR_ERR/ERR_PTR --
or at least put a proper type on h_port (what is the "h_"?).
> +{
> + struct tdm_port *port;
> + struct tdm_adapter *adap;
> + unsigned long flags;
> + int res = TDM_E_OK;
> +
> + if (driver == NULL) {
> + pr_err("driver NULL\n");
> + return -ENODEV;
> + }
> + if (driver->adapter == NULL) {
> + pr_err("adapter NULL\n");
> + return -ENODEV;
> + }
Either make these pr_debug (or remove them), or make the message more
specific.
> + 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;
> + }
> +
> + init_waitqueue_head(&port->ch_wait_queue);
> +
> +
> + port->rx_max_frames = NUM_SAMPLES_PER_FRAME;
> + port->port_cfg.port_mode = e_TDM_PORT_CHANNELIZED;
> +
> + port->in_use = 1;
When would a port have in_use be false, other than this brief window
where nothing else should be looking at it?
> + list_for_each_entry_safe(channel, temp, &port->mychannels, list) {
> + if (channel)
> + if (channel->in_use) {
> + pr_err("%s: Cannot close port. Channel in use\n",
> + __func__);
> + res = -ENXIO;
> + goto out;
> + }
> + }
Broken indentation.
> +unsigned int tdm_channel_read(void *h_port, void *h_channel,
> + void *p_data, u16 *size)
> +{
> + struct tdm_port *port;
> + struct tdm_channel *channel;
> + struct tdm_bd *rx_bd;
> + unsigned long flags;
> + int i, res = TDM_E_OK;
> + unsigned short *buf, *buf1;
> + port = (struct tdm_port *)h_port;
> + channel = (struct tdm_channel *)h_channel;
Unnecessary casts.
> + if ((port && channel) == 0) { /* invalid handle*/
This is an odd construct -- how about "if (!port || !channel)"?
> + pr_err("%s:Invalid Handle\n", __func__);
> + return -ENXIO;
> + }
> +
> + if (!port->in_use)
> + return -EIO;
> + if (!channel->p_ch_data || !channel->in_use)
> + return -EIO;
> +
> + spin_lock_irqsave(&channel->p_ch_data->rx_channel_lock, flags);
Shouldn't you test whether it's in use after you get the lock?
> + 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];
> + rx_bd->flag = 0;
> + rx_bd->offset = 0;
> + channel->p_ch_data->rx_out_data = (rx_bd->wrap) ?
> + channel->p_ch_data->rx_data_fifo : rx_bd + 1;
> +
> + } else {
> + spin_unlock_irqrestore(&channel->p_ch_data->rx_channel_lock,
> + flags);
> + pr_info("No Data Available");
> + return -EAGAIN;
> + }
That pr_info() is inappropriate. This driver appears to be overly
chatty in general (and with quite vague messages) -- or is this debug
stuff that will be removed in a non-RFC patch?
> +unsigned int tdm_port_get_stats(void *h_port, struct tdm_port_stats *portStat)
> +{
> + struct tdm_port *port;
> + int port_num;
> + port = (struct tdm_port *)h_port;
> +
> + if (port == NULL || portStat == NULL) { /* invalid handle*/
> + pr_err("Invalid Handle");
> + return -ENXIO;
> + }
> + port_num = port->port_id;
> +
> + memcpy(portStat, &port->port_stat, sizeof(struct tdm_port_stats));
> +
> + pr_info("TDM Port %d Get Stats", port_num);
> +
> + return TDM_E_OK;
> +}
> +EXPORT_SYMBOL(tdm_port_get_stats);
> +
> +/* Data handling functions */
> +
> +static int tdm_data_rx_deinterleave(struct tdm_adapter *adap)
> +{
> + struct tdm_port *port, *next;
> + struct tdm_channel *channel, *temp;
> + struct tdm_bd *ch_bd;
> +
> + int i, buf_size, ch_data_len;
> + u16 *input_tdm_buffer;
> + u16 *pcm_buffer;
> + int slot_width;
> + int frame_ch_data_size;
> + bool ch_data;
> + int bytes_in_fifo_per_frame;
> + int bytes_slot_offset;
> +
> + ch_data_len = NUM_SAMPLES_PER_FRAME;
> + frame_ch_data_size = NUM_SAMPLES_PER_FRAME;
> + ch_data = 0;
> +
> + if (!adap) { /* invalid handle*/
> + pr_err("%s: Invalid Handle\n", __func__);
> + return -ENXIO;
> + }
> +
> + slot_width = adap->adapt_cfg.slot_width;
> + buf_size = tdm_adap_recv(adap, (void **)&input_tdm_buffer);
> + if (buf_size <= 0 || !input_tdm_buffer)
> + return -EINVAL;
> +
> + bytes_in_fifo_per_frame = buf_size/frame_ch_data_size;
> + bytes_slot_offset = bytes_in_fifo_per_frame/slot_width;
> +
> + /* de-interleaving for all ports*/
> + list_for_each_entry_safe(port, next, &adap->myports, list) {
> +
> + /* if the port is not open */
> + if (!port->in_use)
> + continue;
> +
> + list_for_each_entry_safe(channel, temp, &port->mychannels,
> + list) {
> + /* if the channel is not open */
> + if (!channel->in_use || !channel->p_ch_data)
> + continue;
> + ch_bd = channel->p_ch_data->rx_in_data;
> + spin_lock(&channel->p_ch_data->rx_channel_lock);
> + /*if old data is to be discarded */
> + if (use_latest_tdm_data)
> + if (ch_bd->flag) {
> + ch_bd->flag = 0;
> + ch_bd->offset = 0;
> + if (ch_bd == channel->p_ch_data->rx_out_data)
> + channel->p_ch_data->rx_out_data =
> + ch_bd->wrap ?
> + channel->p_ch_data->rx_data_fifo
> + : ch_bd+1;
> + port->port_stat.rx_pkt_drop_count++;
> + }
> + /* if the bd is empty */
> + if (!ch_bd->flag) {
> + if (ch_bd->offset == 0)
> + ch_bd->length = port->rx_max_frames;
> +
> + pcm_buffer = ch_bd->p_data + ch_bd->offset;
> + /* De-interleaving the data */
> + for (i = 0; i < ch_data_len; i++) {
> + pcm_buffer[i]
> + = input_tdm_buffer[i*bytes_slot_offset +
> + channel->ch_id];
> + }
> + ch_bd->offset += ch_data_len * slot_width;
> +
> + if (ch_bd->offset >=
> + (ch_bd->length - frame_ch_data_size)*
> + (adap->adapt_cfg.slot_width)) {
> + ch_bd->flag = 1;
> + ch_bd->offset = 0;
> + channel->p_ch_data->rx_in_data =
> + ch_bd->wrap ?
> + channel->p_ch_data->rx_data_fifo
> + : ch_bd+1;
> + ch_data = 1;
> + }
> + } else {
> + port->port_stat.rx_pkt_drop_count++;
> + }
> + spin_unlock(&channel->p_ch_data->rx_channel_lock);
> + }
Broken indentation. Spaces around operators.
> +int tdm_channel_close(u16 chanid, u16 ch_width, struct tdm_port *port,
> + struct tdm_channel *h_channel)
> +{
> + struct tdm_channel *channel;
> + unsigned long flags;
> + int res = TDM_E_OK;
> + channel = h_channel;
> +
> + if (!(port && channel)) {
> + pr_err("%s: Invalid handle\n", __func__);
> + res = -EINVAL;
> + goto out;
> + }
> +
> + if (ch_width != 1) {
> + pr_err("%s: Mode not supported\n", __func__);
> + res = -EINVAL;
> + goto out;
> + }
close() seems an odd time to be checking for supported channel width,
especially since you don't use that variable anywhere else in this function.
> +#ifndef _LINUX_TDM_H
> +#define _LINUX_TDM_H
> +
> +#ifdef __KERNEL__
Is this supposed to be a userspace header ever? If TDM exposes a
userspace interface, it needs to be documented.
> +#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>
> +
> +#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 */
> +
> +#define NUM_CHANNELS 16
> +#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
These need proper namespacing -- plus, should all of these really be
hardcoded like this? Is this standard stuff that will always be the same?
> +/* General options */
> +
> +struct tdm_adapt_algorithm;
> +struct tdm_adapter;
> +struct tdm_port;
> +struct tdm_driver;
> +
> +/* Align addr on a size boundary - adjust address up if needed */
> +/* returns min value greater than size which is multiple of alignment */
> +static inline int ALIGN_SIZE(u64 size, u32 alignment)
> +{
> + return (size + alignment - 1) & (~(alignment - 1));
> +}
Use the already existing ALIGN().
> +/**
> + * struct tdm_driver - represent an TDM device driver
> + * @class: What kind of tdm device we instantiate (for detect)
> + * @id:Driver id
> + * @name: Name of the driver
> + * @attach_adapter: Callback for device addition (for legacy drivers)
> + * @detach_adapter: Callback for device removal (for legacy drivers)
> + * @probe: Callback for device binding
> + * @remove: Callback for device unbinding
> + * @shutdown: Callback for device shutdown
> + * @suspend: Callback for device suspend
> + * @resume: Callback for device resume
> + * @command: Callback for sending commands to device
> + * @id_table: List of TDM devices supported by this driver
> + * @list: List of drivers created (for tdm-core use only)
> + */
> +struct tdm_driver {
> + unsigned int class;
> + unsigned int id;
> + char name[TDM_NAME_SIZE];
> +
> + int (*attach_adapter)(struct tdm_adapter *);
> + int (*detach_adapter)(struct tdm_adapter *);
> +
> + /* Standard driver model interfaces */
> + int (*probe)(const struct tdm_device_id *);
> + int (*remove)(void);
> +
> + /* driver model interfaces that don't relate to enumeration */
> + void (*shutdown)(void);
> + int (*suspend)(pm_message_t mesg);
> + int (*resume)(void);
> +
> + /* a ioctl like command that can be used to perform specific functions
> + * with the device.
> + */
> + int (*command)(unsigned int cmd, void *arg);
Please describe what semantics you expect for the non-standard functions.
Where are the "ioctl like commands" defined? When would they be used?
I don't see it used in this patchset.
> +/* struct tdm_channel- represent a TDM channel for a port */
> +struct tdm_channel {
> + u16 ch_id; /* logical channel number */
> + struct list_head list; /* list of channels in a port*/
> + struct tdm_port *port; /* port for this channel */
> + u16 in_use; /* channel is enabled? */
> + struct tdm_ch_cfg ch_cfg; /* channel configuration */
> + struct tdm_ch_data *p_ch_data; /* data storage space for channel */
> +};
Why are ch_id and especially in_use u16?
> +/* tdm_adapt_algorithm is for accessing the routines of device */
> +struct tdm_adapt_algorithm {
> + u32 (*tdm_read)(struct tdm_adapter *, u16 **);
> + u32 (*tdm_get_write_buf)(struct tdm_adapter *, u16 **);
> + u32 (*tdm_write)(struct tdm_adapter *, void * , unsigned int len);
> + int (*tdm_enable)(struct tdm_adapter *);
> + int (*tdm_disable)(struct tdm_adapter *);
> +};
>
Provide parameter names and document the semantics you're expecting.
> +/* tdm_adapter_mode is to define in mode of the device */
> +enum tdm_adapter_mode {
> + e_TDM_ADAPTER_MODE_NONE = 0x00,
> + e_TDM_ADAPTER_MODE_T1 = 0x01,
> + e_TDM_ADAPTER_MODE_E1 = 0x02,
> + e_TDM_ADAPTER_MODE_T1_RAW = 0x10,
> + e_TDM_ADAPTER_MODE_E1_RAW = 0x20,
> +};
> +
> +/* tdm_port_mode defines the mode in which the port is configured to operate
> + * It can be channelized/full/fractional.
> + */
> +enum tdm_port_mode {
> + e_TDM_PORT_CHANNELIZED = 0 /* Channelized mode */
> + , e_TDM_PORT_FULL = 1 /* Full mode */
> + , e_TDM_PORT_FRACTIONAL = 2 /* Fractional mode */
> +};
> +
> +/* tdm_process_mode used for testing the tdm device in normal mode or internal
> + * loopback or external loopback
> + */
> +enum tdm_process_mode {
> + e_TDM_PROCESS_NORMAL = 0 /* Normal mode */
> + , e_TDM_PROCESS_INT_LPB = 1 /* Internal loop mode */
> + , e_TDM_PROCESS_EXT_LPB = 2 /* External Loopback mode */
> +};
Commas go at the end of lines, not the beginning.
No hungarian notation -- drop the "e_".
> +/* TDM configuration parameters */
> +struct fsl_tdm_adapt_cfg {
> + u8 num_ch; /* Number of channels in this adpater */
> + u8 ch_size_type; /* reciever/transmit channel
> + size for all channels */
> + u8 slot_width; /* 1 or 2 Is defined by channel type */
> + u8 frame_len; /* Length of frame in samples */
Is u8 really appropriate here? Maybe int?
What does "type" mean in "ch_size_type"?
> + u8 adap_mode; /* 0=None, 1= T1, 2= T1-FULL, 3=E1,
> + 4 = E1-FULL */
Use #defines or an enum.
> + int max_num_ports; /* Not Used: Max Number of ports that
> + can be created on this adapter */
If it's not used, why is it here?
> +/*
> + * tdm_adapter is the structure used to identify a physical tdm device along
> + * with the access algorithms necessary to access it.
> + */
> +struct tdm_adapter {
> + struct module *owner; /* owner of the adapter module */
> + unsigned int id; /* Adapter Id */
What does the id mean? Why do we need an arbitrary numberspace?
> + unsigned int class; /* classes to allow probing for */
What sort of values would go here?
> + unsigned int drv_count; /* Number of drivers associated with the
> + adapter */
> +
> + const struct tdm_adapt_algorithm *algo; /* the algorithm to access the
> + adapter*/
> +
> + char name[TDM_NAME_SIZE]; /* Name of Adapter */
> + struct mutex adap_lock;
> + struct device *parent; /*Not Used*/
Why is the parent device not used?
> + struct tasklet_struct tdm_data_tasklet; /* tasklet handle to perform
> + data processing*/
> + int tasklet_conf; /* flag for tasklet configuration */
> + int tdm_rx_flag;
What does "tdm_rx_flag" indicate? What about "tasklet_conf"?
> + struct list_head myports; /* list of ports, created on this
> + adapter */
> + struct list_head list;
If you've got more than one list, it's probably a bad idea for any of
them to be simply called "list".
> + spinlock_t portlist_lock; /* Spin Lock for port_list */
Comments should add new information, not just restate what the code
already said.
> +static inline void *tdm_get_adapdata(const struct tdm_adapter *dev)
> +{
> + return dev->data;
> +}
> +
> +static inline void tdm_set_adapdata(struct tdm_adapter *dev, void *data)
> +{
> + dev->data = data;
> +}
Is this really needed?
> +
> +/* functions exported by tdm.o */
> +
> +extern int tdm_add_adapter(struct tdm_adapter *);
> +extern int tdm_del_adapter(struct tdm_adapter *);
> +extern int tdm_register_driver(struct tdm_driver *);
> +extern void tdm_del_driver(struct tdm_driver *);
> +extern void tdm_unregister_driver(struct tdm_driver *);
> +extern void init_config_adapter(struct tdm_adapter *);
> +
> +extern unsigned int tdm_port_open(struct tdm_driver *, void **);
> +extern unsigned int tdm_port_close(void *);
> +extern unsigned int tdm_port_ioctl(void *, unsigned int, unsigned long);
> +extern unsigned int tdm_channel_read(void *, void *, void *, u16 *);
> +extern unsigned int tdm_channel_write(void *, void * , void *, u16);
> +extern unsigned int tdm_port_poll(void *, unsigned int);
> +
> +extern int tdm_channel_open(u16, u16, struct tdm_port *, void **);
> +extern int tdm_channel_close(u16, u16, struct tdm_port *,
> + struct tdm_channel *);
Please provide parameter names with prototypes.
The "extern" is unnecessary.
> +static inline int tdm_add_driver(struct tdm_driver *driver)
> +{
> + return tdm_register_driver(driver);
> +}
What is the semantic difference between tdm_add_driver() and
tdm_register_driver()? Why do they both exist?
-Scott
More information about the Linuxppc-dev
mailing list