lkml.org 
[lkml]   [2012]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] add new NRP power meter USB device driver
    On Tue, May 29, 2012 at 09:14:18PM +0200, stefani@seibold.net wrote:
    > From: Stefani Seibold <stefani@seibold.net>
    >
    > This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
    > sensors are intelligent standalone instruments that communicate via USB.
    >
    > A power meter is a device for analyzing the RF power output level of an
    > electrical device, similar to an oscilloscope.

    Nice, but why is this a kernel driver? Can't you control this device
    from userspace using libusb/usbfs?

    > The Power Meter Sensors will be used in a wide range of environements, like
    >
    > - Manufacturing (e.g. air planes and smart phones)
    > - Radio and TV broadcasting
    > - Mobile communications
    > - Engeeniering
    > - Science Labs
    > - Education
    >
    > The NRP Power Meters support the following measurements:
    >
    > - Dynamic range: up to 90 dB (sensor dependent)
    > - Level range: -67 dBm to +45 dBm (sensor dependent)
    > - Frequency range: DC to 67 GHz (sensor dependent)
    > - Measurement speed: 1500 measurements per second in the buffered mode
    > - Linearity uncertainty: 0.007 dB
    > - Precise average power measurements irrespective of modulation and bandwidth
    > - Flexible measurements on up to 128 time slots per power sensor
    > - S parameter correction of components between sensor and test object

    These measurements are good, but if you want to be a kernel driver, you
    should tie into the IIO framework which should support a common
    userspace API for these sensor readings, and not create driver-custom
    ioctls for just this one device.

    > The device will be controlled by a SCPI like interface.

    What is scpi?

    > The patch is against linux 3.5.0
    > (commit 1e2aec873ad6d16538512dbb96853caa1fa076af)
    >
    > The source is checked with checkpatch.pl and has no errors. Only 11
    > "line over 80 characters" warnings are left. I see no reason to satisfy this
    > boring punch card limit for this lines, since it will make the source noisier.
    >
    > make C=1 is quiet.
    >
    > Please apply it to the kernel tree.
    > Thanks.
    >
    > Signed-off-by: Stefani Seibold <stefani@seibold.net>
    > ---
    > drivers/usb/misc/Kconfig | 12 +
    > drivers/usb/misc/Makefile | 1 +
    > drivers/usb/misc/nrpz.c | 1069 ++++++++++++++++++++++++++++++++++++++++
    > include/linux/usb/nrpzmodule.h | 47 ++
    > 4 files changed, 1129 insertions(+), 0 deletions(-)
    > create mode 100644 drivers/usb/misc/nrpz.c
    > create mode 100644 include/linux/usb/nrpzmodule.h
    >
    > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
    > index 1bfcd02..2c55d5f 100644
    > --- a/drivers/usb/misc/Kconfig
    > +++ b/drivers/usb/misc/Kconfig
    > @@ -244,3 +244,15 @@ config USB_YUREX
    > To compile this driver as a module, choose M here: the
    > module will be called yurex.
    >
    > +config USB_NRPZ
    > + tristate "USB NRPZ power sensor driver support"
    > + depends on USB
    > + help
    > + This driver supports the Rohde&Schwarz NRP RF Power Meter Sensors.
    > +
    > + These sensors are intelligent standalone instruments that
    > + communicate via USB and provide a wide range of measurements.
    > +
    > + To compile this driver as a module, choose M here: the
    > + module will be called nrpz.
    > +
    > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
    > index 796ce7e..9a0364c 100644
    > --- a/drivers/usb/misc/Makefile
    > +++ b/drivers/usb/misc/Makefile
    > @@ -25,5 +25,6 @@ obj-$(CONFIG_USB_TRANCEVIBRATOR) += trancevibrator.o
    > obj-$(CONFIG_USB_USS720) += uss720.o
    > obj-$(CONFIG_USB_SEVSEG) += usbsevseg.o
    > obj-$(CONFIG_USB_YUREX) += yurex.o
    > +obj-$(CONFIG_USB_NRPZ) += nrpz.o
    >
    > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
    > diff --git a/drivers/usb/misc/nrpz.c b/drivers/usb/misc/nrpz.c
    > new file mode 100644
    > index 0000000..e341703
    > --- /dev/null
    > +++ b/drivers/usb/misc/nrpz.c
    > @@ -0,0 +1,1069 @@
    > +/*
    > + * Rohde & Schwarz USB NRP Zxx power meter kernel module driver
    > + *
    > + * Version: 4.0
    > + *
    > + * Copyright (c) 2012 by Rohde & Scharz GmbH & Co. KG
    > + * written by Stefani Seibold <stefani@seibold.net>
    > + *
    > + * Based on USB Skeleton driver written by Greg Kroah-Hartman <greg@kroah.com>
    > + *
    > + * This driver supports the RF Power Meter R&S ® NRP Sensors. These
    > + * sensors are intelligent standalone instruments that communicate via USB
    > + * and support the following measurements:
    > + *
    > + * - Dynamic range: up to 90 dB (sensor dependent)
    > + * - Level range: -67 dBm to +45 dBm (sensor dependent)
    > + * - Frequency range: DC to 67 GHz (sensor dependent)
    > + * - Measurement speed: 1500 measurements per second in the buffered mode
    > + * - Linearity uncertainty: 0.007 dB
    > + * - Precise average power measurements irrespective of modulation and bandwidth
    > + * - Flexible measurements on up to 128 time slots per power sensor
    > + * - S parameter correction of components between sensor and test object
    > + *
    > + * History:
    > + *
    > + * 2012_05_18 - 4.0 - revamp for kernel inclusion
    > + * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
    > + * 2001_05_01 - 0.1 - first version
    > + *
    > + * 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.

    Are you sure "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.

    This paragraph is never needed, unless you want to track the movements
    of the FSF's office for the next 40 years.

    > + */
    > +
    > +/*
    > + * Internal format of the NRPZ USB messages:
    > + *
    > + * Byte 0: Signature
    > + * Byte 1: Error Code
    > + * Byte 2/3: Array Index
    > + * or State (Byte 2)
    > + * or Group Number (Byte 2) / Param Number (Byte 3)
    > + * Byte 4-15: Data depening on signature type (Byte 0):
    > + * floats, bit fields and integer are 32 bit
    > + *
    > + * Signature types:
    > + * 'E': single float value
    > + * 'e': single value
    > + * 'A': float array
    > + * 'P': auxiliary or peak float array
    > + * 'a': interim float array
    > + * 'p': interim auxiliary or peak float array
    > + * 'F': feature bit field
    > + * 'U': float parameter (32 bit)
    > + * 'V': bit field parameter (32 bit)
    > + * 'W': integer parameter (32 bit)
    > + * 'T': string data
    > + * 'R': receipt data (string or binary data)
    > + * 'X': internal error
    > + * 'Z': current state (Byte 2)
    > + * 'z': life sign package
    > + * 'L': float value (32 bit)
    > + * 'M': bit field value (32 bit)
    > + * 'N': long value (32 bit)
    > + * 'B': binary data
    > + *
    > + * State values:
    > + * 0: trigger idle
    > + * 1: trigger reserved
    > + * 2: wait for trigger
    > + * 3: trigger measuring
    > + *
    > + * Communication in direction from host to the device are mostly like SCPI.
    > + */
    > +
    > +#include <linux/kernel.h>
    > +#include <linux/sched.h>
    > +#include <linux/signal.h>
    > +#include <linux/errno.h>
    > +#include <linux/poll.h>
    > +#include <linux/init.h>
    > +#include <linux/slab.h>
    > +#include <linux/fcntl.h>
    > +#include <linux/module.h>
    > +#include <linux/spinlock.h>
    > +#include <linux/list.h>
    > +#include <linux/usb.h>
    > +#include <linux/version.h>
    > +#include <linux/kref.h>
    > +#include <linux/bitops.h>
    > +#include <linux/mutex.h>
    > +
    > +#include <linux/usb/nrpzmodule.h>
    > +
    > +#if 0
    > +#define CONFIG_NRP_DEBUG
    > +#endif
    > +#ifdef CONFIG_NRP_DEBUG
    > +static int debug = 1;
    > +#else
    > +static int debug;
    > +#endif
    > +
    > +module_param(debug, int, 0);
    > +MODULE_PARM_DESC(debug, "Debug enabled or not");
    > +
    > +#define nrpz_dbg(format, arg...) \
    > + do { if (debug) printk(KERN_DEBUG "nrpz: " format "\n", ##arg); } while (0)
    > +#define nrpz_err(format, arg...) \
    > + do { printk(KERN_ERR "nrpz: " format "\n", ##arg); } while (0)
    > +#define nrpz_info(format, arg...) \
    > + do { printk(KERN_INFO "nrpz: " format "\n", ##arg); } while (0)

    Ick, no, please use dev_dbg(), dev_err(), and dev_info() instead, don't
    create your own macros.

    > +/* Version Information */
    > +#define DRIVER_VERSION "v4.00"

    What happened to the first 3 versions of this code? :)

    > +#define DRIVER_AUTHOR "Stefani Seibold <stefani@seibold.net>"
    > +#define DRIVER_DESC "Rohde&Schwarz NRP-Zxx USB Powermeter"
    > +
    > +/* Get a minor range for your devices from the usb maintainer */
    > +#define NRPZ_MINOR_BASE 192

    You didn't ask me for a minor base, did you?

    > +#define to_nrpz_dev(d) container_of(d, struct usb_nrpz, kref)
    > +#define list_to_urb(d) container_of(d, struct urb, urb_list)

    Make these inline functions instead?

    > +
    > +/* Define these values to match your device */

    This comment can be fixed, right?

    > +#define USB_RS_VENDOR_ID 0x0aad
    > +#define USB_NRP_PRODUCT_ID 0x0002
    > +#define USB_NRPZ21_PRODUCT_ID 0x0003
    > +#define USB_NRPFU_PRODUCT_ID 0x0004
    > +#define USB_FSHZ1_PRODUCT_ID 0x000b
    > +#define USB_NRPZ11_PRODUCT_ID 0x000c
    > +#define USB_NRPZ22_PRODUCT_ID 0x0013
    > +#define USB_NRPZ23_PRODUCT_ID 0x0014
    > +#define USB_NRPZ24_PRODUCT_ID 0x0015
    > +#define USB_NRPZ51_PRODUCT_ID 0x0016
    > +#define USB_NRPZ52_PRODUCT_ID 0x0017
    > +#define USB_NRPZ55_PRODUCT_ID 0x0018
    > +#define USB_NRPZ56_PRODUCT_ID 0x0019
    > +#define USB_FSHZ18_PRODUCT_ID 0x001a
    > +#define USB_NRPZ91_PRODUCT_ID 0x0021
    > +#define USB_NRPZ81_PRODUCT_ID 0x0023
    > +#define USB_NRPZ31_PRODUCT_ID 0x002c
    > +#define USB_NRPZ37_PRODUCT_ID 0x002d
    > +#define USB_NRPZ96_PRODUCT_ID 0x002e
    > +#define USB_NRPZ27_PRODUCT_ID 0x002f
    > +#define USB_NRPZ28_PRODUCT_ID 0x0051
    > +#define USB_NRPZ98_PRODUCT_ID 0x0052
    > +#define USB_NRPZ92_PRODUCT_ID 0x0062
    > +#define USB_NRPZ57_PRODUCT_ID 0x0070
    > +#define USB_NRPZ85_PRODUCT_ID 0x0083
    > +#define USB_NRPC40_PRODUCT_ID 0x008F
    > +#define USB_NRPC50_PRODUCT_ID 0x0090
    > +#define USB_NRPZ86_PRODUCT_ID 0x0095
    > +#define USB_NRPZ41_PRODUCT_ID 0x0096
    > +#define USB_NRPZ61_PRODUCT_ID 0x0097
    > +#define USB_NRPZ71_PRODUCT_ID 0x0098
    > +#define USB_NRPZ32_PRODUCT_ID 0x009A
    > +#define USB_NRPZ211_PRODUCT_ID 0x00A6
    > +#define USB_NRPZ221_PRODUCT_ID 0x00A7
    > +#define USB_NRPZ58_PRODUCT_ID 0x00A8
    > +#define USB_NRPC33_PRODUCT_ID 0x00B6
    > +#define USB_NRPC18_PRODUCT_ID 0x00BF
    > +
    > +/* table of devices that work with this driver */
    > +static struct usb_device_id nrpz_table[] = {
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRP_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ21_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPFU_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ1_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ11_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ22_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ23_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ24_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ51_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ52_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ55_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ56_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ57_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ18_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ91_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ81_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ31_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ37_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ96_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ27_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ28_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ98_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ92_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ85_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC40_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC50_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ86_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ41_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ61_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ71_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ32_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ211_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ221_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ58_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC33_PRODUCT_ID)},
    > + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC18_PRODUCT_ID)},
    > + {} /* Terminating entry */
    > +};
    > +
    > +MODULE_DEVICE_TABLE(usb, nrpz_table);
    > +
    > +/* Structure to hold all of our device specific stuff */
    > +struct usb_nrpz {
    > + struct usb_device *udev;
    > + struct usb_interface *intf;

    Do you really need both?

    > + unsigned minor; /* minor number for this device */
    > + unsigned long in_use; /* in use flag */

    Why a long for this?

    > +
    > + size_t in_size; /* size of the receive buffer */
    > + size_t out_size; /* size of the send buffer */
    > + __u8 in_epAddr; /* address of the bulk in endpoint */
    > + __u8 out_epAddr; /* address of the bulk out endpoint */
    > +
    > + struct kref kref;
    > + wait_queue_head_t wq;
    > +
    > + struct usb_anchor out_running; /* list of in use output buffers */
    > + struct list_head out_avail; /* list of available output buffers */
    > + spinlock_t write_lock; /* spinlock for transmit list */
    > + struct mutex write_mutex; /* exclusive write data semaphore */
    > +
    > + struct usb_anchor in_running; /* list of in use input buffers */
    > + struct list_head in_avail; /* list of available input buffers */
    > + spinlock_t read_lock; /* spinlock for receive list */
    > + struct mutex read_mutex; /* exclusive read data semaphore */
    > +
    > + struct urb out_urbs[64]; /* array of urb's for output */
    > + struct urb in_urbs[64]; /* array of urb's for input */
    > +};
    > +
    > +/* forward declaration */
    > +static struct usb_driver nrpz_driver;
    > +
    > +static inline void urb_list_add(spinlock_t *lock, struct urb *urb,
    > + struct list_head *head)
    > +{
    > + spin_lock_irq(lock);
    > + list_add(&urb->urb_list, head);
    > + spin_unlock_irq(lock);
    > +}
    > +
    > +static inline void urb_list_add_tail(spinlock_t *lock, struct urb *urb,
    > + struct list_head *head)
    > +{
    > + spin_lock_irq(lock);
    > + list_add_tail(&urb->urb_list, head);
    > + spin_unlock_irq(lock);
    > +}

    Why the lists of urbs? Why not either just create them all dynamically
    all the time, or use urb anchors?

    > + case NRPZ_GETSENSORINFO:
    > + {
    > + struct nrpz_sensor_info __user *sensor_info =
    > + (struct nrpz_sensor_info __user *)arg;
    > +
    > + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
    > + return -EFAULT;
    > +
    > + __put_user(dev->udev->descriptor.bcdDevice,
    > + &sensor_info->bcdDevice);
    > + __put_user(dev->udev->descriptor.bcdUSB,
    > + &sensor_info->bcdUSB);
    > + __put_user(dev->udev->descriptor.bDescriptorType,
    > + &sensor_info->bDescriptorType);
    > + __put_user(dev->udev->descriptor.bDeviceClass,
    > + &sensor_info->bDeviceClass);
    > + __put_user(dev->udev->descriptor.bDeviceSubClass,
    > + &sensor_info->bDeviceSubClass);
    > + __put_user(dev->udev->descriptor.bDeviceProtocol,
    > + &sensor_info->bDeviceProtocol);
    > + __put_user(dev->udev->descriptor.bMaxPacketSize0,
    > + &sensor_info->bMaxPacketSize0);
    > + __put_user(dev->udev->descriptor.bNumConfigurations,
    > + &sensor_info->bNumConfigurations);
    > + __put_user(dev->udev->descriptor.iManufacturer,
    > + &sensor_info->iManufacturer);
    > + __put_user(dev->udev->descriptor.iProduct,
    > + &sensor_info->iProduct);
    > + __put_user(dev->udev->descriptor.iSerialNumber,
    > + &sensor_info->iSerialNumber);
    > + __put_user(dev->udev->descriptor.idVendor,
    > + &sensor_info->vendorId);
    > + __put_user(dev->udev->descriptor.idProduct,
    > + &sensor_info->productId);
    > + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
    > + (char __force *)sensor_info->manufacturer,
    > + sizeof(sensor_info->manufacturer));
    > + usb_string(dev->udev, dev->udev->descriptor.iProduct,
    > + (char __force *)sensor_info->productName,
    > + sizeof(sensor_info->productName));
    > + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
    > + (char __force *)sensor_info->serialNumber,
    > + sizeof(sensor_info->serialNumber));

    Why are you sending this information through an ioctl, when it is
    already exported in sysfs? That seems quite needless.

    Anyway, I think the main questions are, "Why is this a kernel driver",
    and "If this is a kernel driver, why not use the IIO interface"?

    thanks,

    greg k-h
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-05-30 01:21    [W:0.057 / U:125.172 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site