Hi,
Couple of comments:
On 1/2/07, Roberson, Jeremy <[EMAIL PROTECTED]> wrote:
> +
> +#include <linux/version.h>
> +#if (LINUX_VERSION_CODE > KERNEL_VERSION(2,6,14))
> +/** This define is for kernels 2.6.15 and up where the input driver
> + * interface has changed
> + */
> +#define USE_INPUT_ALLOCATE
> +#include <linux/usb_input.h>
> +#endif
The code submitted for inclusion in the mainline does not need
compatibility stuff.
> +#ifdef USE_INPUT_ALLOCATE
> +#define GET_INPUT_DEV_PTR(x) (x)->pInputDev
> +#define FREE_INPUT_DEV(x) input_free_device((x)->pInputDev);
> +#else
> +#define GET_INPUT_DEV_PTR(x) &((x)->inputDev)
> +#define FREE_INPUT_DEV(x)
> +#endif
These shoudl go away too.
> +
> +/* Structure to hold all of our device specific stuff */
> +typedef struct gtco_dev {
We prefer not to use typedefs in the kernel.
> +/**********************************************************************
> *******/
> +// Code for parsing the HID REPORT DESCRIPTOR
> +/**********************************************************************
> *******/
> +
> +/* From HID1.11 spec */
> +struct hid_descriptor
> +{
> + struct usb_descriptor_header header;
> + __le16 bcdHID;
> + u8 bCountryCode;
> + u8 bNumDescriptors;
> + u8 bDescriptorType;
> + __le16 wDescriptorLength;
> +} __attribute__ ((packed));
> +
Hmm, is there a reason why the standard HID driver is not suitable for
this device?
> +
> +/**
> + Called when opening the input device. This will submit the URB to
> + the usb system so we start getting reports
> +*/
> +static int GTCO_InputOpen(struct input_dev *inputdev)
> +{
> + struct gtco_dev *pDevice;
> + pDevice = inputdev->private;
> +
> + // Prevent us from submitting the one Urb more than once
> + if (pDevice->openCount++)
> + return 0;
Input core ensures serialization of open/close methods and guarantees
that they only called once for the first/last user.
> +
> +/**
> + Setup input device capabilities. Tell the input system what this
> + device is capable of generating.
> +
> + This information is based on what is read from the HID report and
> + placed in the GTCO_DEVICE_T structure
> +
> [EMAIL PROTECTED] apInputDev pointer to input device
> +*/
> +static void GTCO_SetupInputCaps(struct input_dev *apInputDev)
> +{
We prefer not to use mixed-case names (Re: Documentation/CodingStyle,
Chapter 4). Also please make sure that identation is done with hard
tabs, not spaces.
> +
> +#ifdef USE_BUTTONS
> +
> + // Buttons
> + // Set for the 16 digitizer buttons starting at TOOL_PEN
> + for (i=0;i<16;i++){
> + apInputDev->keybit[LONG(BTN_DIGI)] |= BIT((BTN_TOOL_PEN+i))
> ;
> + }
> +#endif
Why is this contitional?
> +
> + // Absolute values based on HID report info
> + input_set_abs_params(apInputDev, ABS_X, pDevice->min_X,
> pDevice->max_X, 0, 0);
> + input_set_abs_params(apInputDev, ABS_Y, pDevice->min_Y,
> pDevice->max_Y, 0, 0);
> +
> + // Proximity
> + input_set_abs_params(apInputDev, ABS_DISTANCE, 0, 1, 0, 0);
> +
> + // Tilt & pressure
> + input_set_abs_params(apInputDev, ABS_TILT_X, pDevice->minTilt_X,
> pDevice->maxTilt_X, 0, 0);
> + input_set_abs_params(apInputDev, ABS_TILT_Y, pDevice->minTilt_Y,
> pDevice->maxTilt_Y, 0, 0);
> + input_set_abs_params(apInputDev, ABS_PRESSURE, pDevice->minPressure,
> pDevice->maxPressure, 0, 0);
> +
> +
> + // Transducer
> + input_set_abs_params(apInputDev, ABS_MISC, 0,0xFF, 0, 0);
> +
> +}
> +
> +
> +/**********************************************************************
> *******/
> +// USB Routines
> +/**********************************************************************
> *******/
> +
> +/**
> + URB callback routine. Called when we get IRQ reports from the
> + digitizer.
> +
> + This bridges the USB and input device worlds. It generates events
> + on the input device based on the USB reports.
> +
> +*/
> +static void gtcoUrbCallback(struct urb *apUrb, struct pt_regs *apRegs)
> +{
> +
> +
> + GTCO_DEVICE_T *pDevice = apUrb->context;
> + struct input_dev *pInput;
> + int rc,i;
> + u32 val = 0;
> + s8 valSigned = 0;
> + char le_buffer[2];
> +
> + pInput = GET_INPUT_DEV_PTR(pDevice);
> +
> +
> + // Was callback OK?
> + if ((apUrb->status == -ECONNRESET ) ||
> + (apUrb->status == -ENOENT ) ||
> + (apUrb->status == -ESHUTDOWN )){
> +
> + // Shutdown is occurring. Return and don't queue up any more
> + return;
> + }
> +
> + if (apUrb->status != 0 ) {
> + // Some unknown error. Hopefully temporary. Just go and
> + // requeue an URB
> + goto resubmit;
> + }
> +
> + //
> + // Good URB, now process
> + //
> + input_regs(pInput, apRegs);
There is no input_regs() anymore.
> +
> + // All done, now register the input device
> + input_register_device(pInput);
Error handling please.
--
Dmitry
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
