Return-Path: From: Arnd Bergmann To: "Par-Gunnar Hjalmdahl" Subject: Re: [PATCH 1/9] mfd: Add support for the ST-Ericsson CG2900. Date: Fri, 22 Oct 2010 17:36:57 +0200 Cc: linus.walleij@stericsson.com, linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org References: In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Message-Id: <201010221736.57671.arnd@arndb.de> Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Friday 22 October 2010 12:35:16 Par-Gunnar Hjalmdahl wrote: > This patch adds support for the ST-Ericsson CG2900 Connectivity > Combo controller. > This patch adds the central framework to be able to register CG2900 users, > transports, and chip handlers. > > Signed-off-by: Par-Gunnar Hjalmdahl Looks ok from a coding style perspective, but some important information is missing from the description: * What is a CG2900? * Why is it a MFD device rather than e.g. a bus or a subsystem? > +config MFD_CG2900 > + tristate "Support ST-Ericsson CG2900 main structure" > + depends on NET > + help > + Support for ST-Ericsson CG2900 Connectivity Combo controller main structure. > + Supports multiple functionalities muxed over a Bluetooth HCI H:4 interface. > + CG2900 support Bluetooth, FM radio, and GPS. > + Can you explain what it means to mux over a H:4 interface? Does this mean you use bluetooth infrastructure that is designed for wireless communication in order to connect on-board or on-chip devices? > @@ -0,0 +1,2401 @@ > +/* > + * drivers/mfd/cg2900/cg2900_core.c > + * > + * Copyright (C) ST-Ericsson SA 2010 > + * Authors: > + * Par-Gunnar Hjalmdahl (par-gunnar.p.hjalmdahl@stericsson.com) for > ST-Ericsson. Your email client rewraps lines. You need to fix so that other people can apply your patches. > +/* > + * chip_handlers - List of the register handlers for different chips. > + */ > +LIST_HEAD(chip_handlers); Should this be static? Don't you need a lock to access the list? > +/** > + * find_h4_user() - Get H4 user based on supplied H4 channel. > + * @h4_channel: H4 channel. > + * @dev: Stored CG2900 device. > + * @skb: (optional) skb with received packet. Set to NULL if NA. > + * > + * Returns: > + * 0 if there is no error. > + * -EINVAL if bad channel is supplied or no user was found. > + * -ENXIO if channel is audio channel but not registered with CG2900. > + */ > +static int find_h4_user(int h4_channel, struct cg2900_device **dev, > + const struct sk_buff * const skb) > +{ > + int err = 0; > + struct cg2900_users *users = &(core_info->users); > + struct cg2900_h4_channels *chan = &(core_info->h4_channels); > + > + if (h4_channel == chan->bt_cmd_channel) { > + *dev = users->bt_cmd; > + } else if (h4_channel == chan->bt_acl_channel) { > + *dev = users->bt_acl; > + } else if (h4_channel == chan->bt_evt_channel) { > + *dev = users->bt_evt; > + /* Check if it's event generated by previously sent audio user > + * command. If so then that event should be dispatched to audio > + * user*/ > + err = find_bt_audio_user(h4_channel, dev, skb); > + } else if (h4_channel == chan->gnss_channel) { > + *dev = users->gnss; > + } else if (h4_channel == chan->fm_radio_channel) { > + *dev = users->fm_radio; > + /* Check if it's an event generated by previously sent audio > + * user command. If so then that event should be dispatched to > + * audio user */ > + err = find_fm_audio_user(h4_channel, dev, skb); > + } else if (h4_channel == chan->debug_channel) { > + *dev = users->debug; > + } else if (h4_channel == chan->ste_tools_channel) { > + *dev = users->ste_tools; > + } else if (h4_channel == chan->hci_logger_channel) { > + *dev = users->hci_logger; > + } else if (h4_channel == chan->us_ctrl_channel) { > + *dev = users->us_ctrl; > + } else if (h4_channel == chan->core_channel) { > + *dev = users->core; > + } else { > + *dev = NULL; > + CG2900_ERR("Bad H:4 channel supplied: 0x%X", h4_channel); > + return -EINVAL; > + } > + > + return err; > +} You seem to have a number of functions that need to go through each possible user/channel/submodule. This looks like somethin is not abstracted well enough. > + > +/** > + * add_h4_user() - Add H4 user to user storage based on supplied H4 channel. > + * @dev: Stored CG2900 device. > + * @name: Device name to identify different devices that are using > + * the same H4 channel. > + * > + * Returns: > + * 0 if there is no error. > + * -EINVAL if NULL pointer or bad channel is supplied. > + * -EBUSY if there already is a user for supplied channel. > + */ > +static int add_h4_user(struct cg2900_device *dev, const char * const name) > +{ Where does that name come from? Why not just use an enum if the name is only use for strncmp? > +static ssize_t test_char_dev_read(struct file *filp, char __user *buf, > + size_t count, loff_t *f_pos) > +{ > + struct sk_buff *skb; > + int bytes_to_copy; > + int err; > + struct sk_buff_head *rx_queue = &core_info->test_char_dev->rx_queue; What is this read/write interface used for? The name suggests that this is only for testing and not an essential part of your driver. Could this be made a separate driver? It also looks like you do socket communication here, can't you use a proper AF_BLUETOOTH socket to do the same? > + CG2900_INFO("test_char_dev_read"); > + > + if (skb_queue_empty(rx_queue)) > + wait_event_interruptible(char_wait_queue, > + !(skb_queue_empty(rx_queue))); > + > + skb = skb_dequeue(rx_queue); > + if (!skb) { > + CG2900_INFO("skb queue is empty - return with zero bytes"); > + bytes_to_copy = 0; > + goto finished; > + } > + > + bytes_to_copy = min(count, skb->len); > + err = copy_to_user(buf, skb->data, bytes_to_copy); > + if (err) { > + skb_queue_head(rx_queue, skb); > + return -EFAULT; > + } > + > + skb_pull(skb, bytes_to_copy); > + > + if (skb->len > 0) > + skb_queue_head(rx_queue, skb); > + else > + kfree_skb(skb); > + > +finished: > + return bytes_to_copy; > +} This does not handle short/continued reads. > +EXPORT_SYMBOL(cg2900_reset); How about making your symbols EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL? > +module_param(cg2900_debug_level, int, S_IRUGO | S_IWUSR | S_IWGRP); > +MODULE_PARM_DESC(cg2900_debug_level, > + "Debug level. Default 1. Possible values:\n" > + "\t0 = No debug\n" > + "\t1 = Error prints\n" > + "\t10 = General info, e.g. function entries\n" > + "\t20 = Debug info, e.g. steps in a functionality\n" > + "\t25 = Data info, i.e. prints when data is transferred\n" > + "\t30 = Data content, i.e. contents of the transferred data"); Please don't introduce new debugging frameworks, we have enough of them already. Syslog handles different levels of output just fine, so just remove all your debugging stuff and use dev_dbg, dev_info, etc to print messages about the device if you must. Many messages can probably be removed entirely. > +#define CG2900_DEFAULT_DEBUG_LEVEL 1 > + > +/* module_param declared in cg2900_core.c */ > +extern int cg2900_debug_level; > + > +#if defined(NDEBUG) || CG2900_DEFAULT_DEBUG_LEVEL == 0 > + #define CG2900_DBG_DATA_CONTENT(__prefix, __buf, __len) > + #define CG2900_DBG_DATA(fmt, arg...) > + #define CG2900_DBG(fmt, arg...) > + #define CG2900_INFO(fmt, arg...) > + #define CG2900_ERR(fmt, arg...) > +#else > + > + #define CG2900_DBG_DATA_CONTENT(__prefix, __buf, __len) \ > + do { \ > + if (cg2900_debug_level >= 30) \ > + print_hex_dump_bytes("CG2900 " __prefix ": " , \ > + DUMP_PREFIX_NONE, __buf, __len); \ > + } while (0) > + > + #define CG2900_DBG_DATA(fmt, arg...) \ > + do { \ > + if (cg2900_debug_level >= 25) \ > + pr_debug("CG2900 %s: " fmt "\n" , __func__ , ## arg); \ > + } while (0) > + > + #define CG2900_DBG(fmt, arg...) \ > + do { \ > + if (cg2900_debug_level >= 20) \ > + pr_debug("CG2900 %s: " fmt "\n" , __func__ , ## arg); \ > + } while (0) > + > + #define CG2900_INFO(fmt, arg...) \ > + do { \ > + if (cg2900_debug_level >= 10) \ > + pr_info("CG2900: " fmt "\n" , ## arg); \ > + } while (0) > + > + #define CG2900_ERR(fmt, arg...) \ > + do { \ > + if (cg2900_debug_level >= 1) \ > + pr_err("CG2900 %s: " fmt "\n" , __func__ , ## arg); \ > + } while (0) > + > +#endif /* NDEBUG */ You'll feel relieved when all this is gone... > + > +#ifndef _BLUETOOTH_DEFINES_H_ > +#define _BLUETOOTH_DEFINES_H_ > + > +#include > + > +/* H:4 offset in an HCI packet */ > +#define HCI_H4_POS 0 > +#define HCI_H4_SIZE 1 > + > +/* Standardized Bluetooth H:4 channels */ > +#define HCI_BT_CMD_H4_CHANNEL 0x01 > +#define HCI_BT_ACL_H4_CHANNEL 0x02 > +#define HCI_BT_SCO_H4_CHANNEL 0x03 > +#define HCI_BT_EVT_H4_CHANNEL 0x04 > + > +/* Bluetooth Opcode Group Field (OGF) */ > +#define HCI_BT_OGF_LINK_CTRL 0x01 > +#define HCI_BT_OGF_LINK_POLICY 0x02 > +#define HCI_BT_OGF_CTRL_BB 0x03 > +#define HCI_BT_OGF_LINK_INFO 0x04 > +#define HCI_BT_OGF_LINK_STATUS 0x05 > +#define HCI_BT_OGF_LINK_TESTING 0x06 > +#define HCI_BT_OGF_VS 0x3F These all look like generic bluetooth definitions, not cg2900 specific. Should they be part of global headers? Are they perhaps already? Arnd