On Thu, Sep 22, 2005 at 04:07:37PM -0700, Brian Pugh wrote:
> Here is a patch for USB IrDA dongles made with the MosChip mcs7780
> chipset. I and Judy Fischback have taken Lukasz Stelmach's shiny
> 0.2alpha.3 release and added in the code for the MIR and FIR speeds.
> The patch has been tested on the 2.6.13 kernel.
>
> Please CC any comments directly back to me as I am not subscribed to this
> mailing list.
Next time you might want to CC: the netdev mailing list, as they are
more capable of looking at the irda portions of this patch. And they
are the ones that can add it to the kernel tree, I can't as it's in the
net/ directory :)
First off, this looks very good. I only have some minor comments on it:
> +/*****************************************************************************
> +*
> +* Filename: mcs7780.c
The filename is obvious :)
> +* Version: 0.3-alpha
Is this needed? Driver versions usually track kernel versions.
> +* 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.
Do you really mean "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.
These two paragraphs are not needed.
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/config.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/kref.h>
> +#include <linux/usb.h>
> +#include <linux/device.h>
> +#include <asm/byteorder.h>
> +#include <asm/uaccess.h>
Put the <asm/*> files at the end of the list. Do you really need them
to build this driver? And do you really need all of the ones above too?
> +#include <net/irda/irda.h>
> +#include <net/irda/wrapper.h>
> +#include <net/irda/crc.h>
> +#include <linux/crc32.h>
> +#include <asm/unaligned.h>
> +
> +#include "mcs7780.h"
Why have a header file for a single driver .c file? Is it needed?
> +static int qos_mtt_bits = 0x07 /* > 1ms */ ;
> +module_param(qos_mtt_bits, int, 0);
> +MODULE_PARM_DESC(qos_mtt_bits, "Minimum Turn Time");
> +
> +static int receive_mode = 0x1;
> +module_param(receive_mode, int, 0);
> +MODULE_PARM_DESC(receive_mode,
> + "Receive mode of the device (1:fast, 0:slow, default:1)");
> +
> +static int sensitivity = MCS_ENABLE;
> +module_param(sensitivity, int, 0444);
> +MODULE_PARM_DESC(sensitivity,
> + "Receiver extra sensitivity (1:on, 0:off, default:1).");
> +
> +static int sir_tweak = MCS_ENABLE;
> +module_param(sir_tweak, int, 0444);
> +MODULE_PARM_DESC(sir_tweak,
> + "Default pulse width (1:1.6us, 0:3/16 bit, default:1).");
> +
> +static int transceiver_type = MCS_TSC_VISHAY;
> +module_param(transceiver_type, int, 0444);
> +MODULE_PARM_DESC(transceiver_type, "IR transceiver type, see mcs7780.h.");
> +
> +static int xbofs = 3;
> +module_param(xbofs, int, 0444);
> +MODULE_PARM_DESC(xbofs, "Number of extra BOFs.(default:3)");
These are all nice options, but are they really needed? And should they
be device specific options, and not driver wide (which will apply to all
devices of this type plugged in at the same time?) If so, just add them
as device specific sysfs values instead.
> +/* checks if t is enabled or disabled, if so return true */
> +static int check_onoff(int t)
> +{
> + switch (t) {
> + case MCS_ENABLE:
> + case MCS_DISABLE:
> + return 1;
> + default:
> + return 0;
> + }
> +}
The "true" statement above can get confusing to kernel developers.
Traditionally all functions return "0" on success, and -SOMETHING on
error. "1" doesn't play into the picture at all. Just be careful here.
> +/* 0: OK 1:ERROR */
> +static inline int mcs_setup_transceiver_vishay(struct mcs_cb *mcs)
Oops, here that's not ok. -ERR on error, 0 on success please.
> +{
> + int ret = 0;
> + __u16 rval;
> +
> + /* mcs_get_reg should read exactly two bytes from the dongle */
> + ret = mcs_get_reg(mcs, MCS_XCVR_REG, &rval);
> + if (unlikely(ret != 2)) {
Drivers, especially USB drivers, should not use the unlikely() and
likely() macros, as it's just noise and doesn't help out any. It
clutters up the code and provides no speed benifit in the end. Can you
remove all of them from this driver?
Generally those macros are only used in core kernel code that really
needs, and notices, the speedups.
> +/* Setup a communication between mcs7780 and agilent chip. */
> +static inline int mcs_setup_transceiver_agilent(struct mcs_cb *mcs)
> +{
> + warn("This transceiver type is not supported yet.");
> + return 1;
> +}
> +
> +/* Setup a communication between mcs7780 and sharp chip. */
> +static inline int mcs_setup_transceiver_sharp(struct mcs_cb *mcs)
> +{
> + warn("This transceiver type is not supported yet.");
> + return 1;
> +}
return 1 means what? Error? Please change this, and all other places
you return 1...
Also, why even have these functions for now, if you don't support this
device type?
> +/* Common setup for all transceivers */
> +static inline int mcs_setup_transceiver(struct mcs_cb *mcs)
> +{
> + int ret = 0;
> + __u16 rval;
__ versions of variables are only needed where the variable crosses the
kernelspace/userspace boundry. For in-kernel stuff only (including data
on the wire of the device), drop the "__" please.
> + if(unlikely(ret != 2))
> + goto error;
Space after "if" and before the first "(" please. Oh and drop the
unlikely too :)
> +static int mcs_set_reg(struct mcs_cb *mcs, __u16 reg, __u16 val);
> +static int mcs_get_reg(struct mcs_cb *mcs, __u16 reg, __u16 * val);
> +
> +static inline int mcs_setup_transceiver_vishay(struct mcs_cb *mcs);
> +static inline int mcs_setup_transceiver_agilent(struct mcs_cb *mcs);
> +static inline int mcs_setup_transceiver_sharp(struct mcs_cb *mcs);
> +static inline int mcs_setup_transceiver(struct mcs_cb *mcs);
> +static inline int mcs_wrap_sir_skb(struct sk_buff *skb, __u8 * buf);
> +static unsigned mcs_wrap_fir_skb(const struct sk_buff *skb, __u8 *buf);
> +static unsigned mcs_wrap_mir_skb(const struct sk_buff *skb, __u8 *buf);
> +static void mcs_unwrap_mir(struct mcs_cb *mcs, __u8 *buf, int len);
> +static void mcs_unwrap_fir(struct mcs_cb *mcs, __u8 *buf, int len);
> +static inline void mcs_sysfs_start(struct usb_interface *intf);
> +static inline void mcs_set_default_parms(struct mcs_cb *mcs);
> +static inline int mcs_setup_urbs(struct mcs_cb *mcs);
> +static inline int mcs_receive_start(struct mcs_cb *mcs);
> +static inline int mcs_find_endpoints(struct mcs_cb *mcs,
> + struct usb_host_endpoint *ep, int epnum);
Inline functions don't need prototypes :)
Also, I bet you can get rid of a lot of these other prototypes, based on
how you order the code around (if not already).
thanks,
greg k-h
-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
