Hi,

On Mon, Apr 27, 2009 at 11:03:20PM +0800, Wan ZongShun wrote:

Dear sirs,

The patch adds this  touch screen  driver to w90p910 ARM  platform.
It is based on version of 2.6.30-rc3.
would you please give me some advice about this patch?

Thank you very much for your patch. Some comments below.

All of this does not belong in a header file.

No camel-case in the kernel, please.

+{
+       pre_x = ((ADC_READ(REG_ADC_XDATA)*TSDEV_SCREEN_X)/ADC_RESOULTION);
+       pre_y = ((ADC_READ(REG_ADC_YDATA)*TSDEV_SCREEN_Y)/ADC_RESOULTION);

Why do we need to scale?

+
+       input_report_key(w90p910_dev, BTN_TOUCH, 1);
+       input_report_abs(w90p910_dev, ABS_X, pre_x);
+       input_report_abs(w90p910_dev, ABS_Y, pre_y);
+       input_report_abs(w90p910_dev, ABS_PRESSURE, 1000);

If your device does not provide true pressure readings to not fake them.

+       input_sync(w90p910_dev);
+       return 0;
+}
+
+static void wb_sensitivity(unsigned long  sensi)
+{
+       unsigned long flags;
+       down_interruptible(&sem);
+       spin_lock_irqsave(&w90x900_touch_lock, flags);
+
+       if ((ADC_READ(REG_ADC_TSC)&0x1) == 0x1) {
+
+               ADC_WRITE(REG_ADC_TSC, 0x0);
+               ADC_WRITE(REG_ADC_CON, (ADC_READ(REG_ADC_CON)|
+                                               0x00206000)&0xFF7F7FFF);
+               STATE = 1;
+       } else{
+               input_sync(w90p910_dev);

You should report events, then issue EV_SYN, not the other way around.

+               input_report_key(w90p910_dev, BTN_TOUCH, 0);
+               input_report_abs(w90p910_dev, ABS_PRESSURE, 0);
+               ADC_WRITE(REG_ADC_CON, (ADC_READ(REG_ADC_CON)|
+                                               0x0080C000)&0xFFCBFFFF);
+               STATE = 0;
+               point.status = 0;
+       }
+       up(&sem);
+       spin_unlock_irqrestore(&w90x900_touch_lock, flags);
+}
+
+static irqreturn_t wb_adc_irq(int irq, void *dev_id)
+{
+       unsigned long flags;
+
+       down_interruptible(&sem);

Down_interruptible in an IRQ context? That is... interesting...

+       spin_lock_irqsave(&w90x900_touch_lock, flags);
+       adc_get = 1;
+
+       if (((ADC_READ(REG_ADC_CON)&WT_INT)>>20) == 1 && STATE == 0) {
+               STATE = 1;
+               ADC_WRITE(REG_ADC_TSC, 0x0);
+               ADC_WRITE(REG_ADC_CON, (ADC_READ(REG_ADC_CON)|
+                                               0x00206000)&0xFF6F7FFF);

Magic values... Can we have proper #defines here?

+       }
+
+       if (((ADC_READ(REG_ADC_CON)&ADC_INT)>>18) == 1 && STATE == 1) {
+               STATE = 2;
+               ADC_WRITE(REG_ADC_TSC, 0x100);
+               ADC_WRITE(REG_ADC_CON, (ADC_READ(REG_ADC_CON)|
+                                               0x00206000)&0xFF7B7FFF);
+       }
+
+       if (((ADC_READ(REG_ADC_CON)&ADC_INT)>>18) == 1 && STATE == 2) {
+               ADCDataIn();
+               point.status = 1;
+               ADC_WRITE(REG_ADC_CON, (ADC_READ(REG_ADC_CON)|
+                                               0x0000C000)&0xFF5BFFFF);
+
+               del_timer(&ts_timer1);
+               ts_timer1.expires = jiffies+INTERVAL_TIME;
+               ts_timer1.data = 0UL;
+               add_timer(&ts_timer1);

This can be exprerssed as mod_timer().

+
+       }
+       up(&sem);
+       spin_unlock_irqrestore(&w90x900_touch_lock, flags);
+       return IRQ_HANDLED;
+}
+
+static int wb_adc_open(struct input_dev *dev)
+{
+
+       adc_block = 1;
+       adc_get = 1;
+       init_timer(&ts_timer1);
+       ts_timer1.function = wb_sensitivity;
+       /* reset */
+       ADC_WRITE(REG_ADC_CON, 0x00010000);
+       udelay(1000);
+       ADC_WRITE(REG_ADC_CON, 0x00000000);
+       udelay(1000);
+       /* ADC setting */
+       /* set delay and screen type */
+       ADC_WRITE(REG_ADC_TSC, ADC_READ(REG_CLKSEL) & 0xFFFFFFF9);
+       ADC_WRITE(REG_ADC_DLY, 0xF00);
+       /* waitting for trigger mode */
+       ADC_WRITE(REG_ADC_CON, (ADC_READ(REG_ADC_CON)|
+                                       0x0082C008) & 0xFFEBFF09);
+       STATE = 0;
+       pre_x = pre_y = 0;
+       return 0;
+}
+
+static void  wb_adc_close(struct input_dev *dev)
+{
+       free_irq(IRQ_ADC, NULL);

Uh-oh... What will happen if we try to open the device again? Don't free
IRQ here, do that in w90x900ts_remove().

+}
+
+static int __init w90x900ts_probe(struct platform_device *pdev)

__devinit.

+{
+       int irq, result, err;
+       unsigned int tmp32;
+       struct resource *res;

Blank line after declarations.

+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0) {
+               dev_err(&pdev->dev, "no irq for device\n");
+               return -ENOENT;
+       }
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (!res)
+               return -ENXIO;
+
+       if (!request_mem_region(res->start, res->end - res->start + 1,
+                               pdev->name)) {
+               dev_err(&pdev->dev, "Memory region busy\n");
+               err = -EBUSY;
+               goto fail;
+       }
+
+       /* enable the ADC clock */
+       tmp32 = ADC_READ(REG_CLKEN);
+       tmp32 = tmp32 | 0x10000000;
+       ADC_WRITE(REG_CLKEN, tmp32);
+
+       w90p910_dev = input_allocate_device();
+
+       if (!w90p910_dev) {
+               printk(KERN_ERR "w90p910_dev: not enough memory\n");
+               err = -ENOMEM;
+               goto fail;

'fail' does not free the memory region you requested though...

+       }
+
+       w90p910_dev->name = "W90P910 TouchScreen";
+       w90p910_dev->phys = "input/event0";

Umm, not specific enough for 'phys'... "w90p910_ts/input0" maybe?

+       w90p910_dev->id.bustype = BUS_HOST;
+       w90p910_dev->id.vendor  = 0x0005;
+       w90p910_dev->id.product = 0x0001;
+       w90p910_dev->id.version = 0x0100;

w90p910_dev->dev.parent = &pdev->dev;

+
+       w90p910_dev->open  = wb_adc_open;
+       w90p910_dev->close = wb_adc_close;
+
+       w90p910_dev->evbit[0] = BIT_MASK(EV_KEY)|
+                               BIT_MASK(EV_ABS) | BIT_MASK(EV_SYN);
+       w90p910_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+       input_set_abs_params(w90p910_dev, ABS_X, 0, 0x400, 0, 0);
+       input_set_abs_params(w90p910_dev, ABS_Y, 0, 0x400, 0, 0);
+       input_set_abs_params(w90p910_dev, ABS_PRESSURE, 0, 1000, 0, 0);
+
+       result = request_irq(irq, wb_adc_irq, IRQF_DISABLED, DEV_NAME, NULL);
+       if (result != 0)
+               printk(KERN_ERR"register the wb_adc_irq failed!\n");

And..? Do we really want to continue?

+
+       input_register_device(w90p910_dev);

Error handling please.

+       return 0;
+
+fail:
+       input_free_device(w90p910_dev);
+       return err;
+}
+
+static int w90x900ts_remove(struct platform_device *pdev)

__devexit

+{
+

Extra blank line

+       input_unregister_device(w90p910_dev);
+       return 0;
+}
+
+static struct platform_driver w90x900ts_driver = {
+       .probe          = w90x900ts_probe,
+       .remove         = w90x900ts_remove,

__devexit_p()

+       .driver         = {
+               .name   = "w90x900-ts",
+               .owner  = THIS_MODULE,
+       },
+};
+
+int __init w90x900ts_init(void)

static

+{
+       return platform_driver_register(&w90x900ts_driver);
+}
+
+static void __exit w90x900ts_exit(void)
+{
+       platform_driver_unregister(&w90x900ts_driver);
+}
+
+module_init(w90x900ts_init);
+module_exit(w90x900ts_exit);
+
+MODULE_AUTHOR("PT50 zswan [off-list ref]");
+MODULE_DESCRIPTION("w90p910 touch screen driver!");
+MODULE_LICENSE("GPL");

Also, please run the patch through scripts/checkpatch.pl and make sure
it does not complain. Thanks!

-- 
Dmitry