Hi Dmitry,

On Wed, Apr 2, 2008 at 3:08 PM, Dmitry Torokhov
[off-list ref] wrote:

 On Fri, Mar 28, 2008 at 06:51:01PM +0900, Magnus Damm wrote:
 > This is V2 of the MigoR touch screen driver. The chip we interface to
 > is unfortunately a custom designed microcontroller speaking some
 > undocumented protocol over i2c.

 Thank you for the patch, I just have a couple of comments below...

Your comments are much appreciated.

 > +     if ((event == EVENT_PENDOWN) || (event == EVENT_REPEAT)) {
 > +             input_report_key(priv->input, BTN_TOUCH, 1);
 > +             input_report_abs(priv->input, ABS_X, ypos); /*X-Y swap*/
 > +             input_report_abs(priv->input, ABS_Y, xpos);
 > +             input_report_abs(priv->input, ABS_PRESSURE, 120);
 > +             input_sync(priv->input);
 > +     } else if (event == EVENT_PENUP) {
 > +             input_report_abs(priv->input, ABS_PRESSURE, 0);

 Don't you need to signal BTN_TOUCH release here?

Yes, you are correct. It is missing.

 > +     input_set_abs_params(priv->input, ABS_X, 95, 955, 0, 0);
 > +     input_set_abs_params(priv->input, ABS_Y, 85, 935, 0, 0);
 > +     input_set_abs_params(priv->input, ABS_PRESSURE, 0, 0, 0, 0);

 The device does not support pressure reporting so don't pretend
 to send one. If this was done because of tslib please fix tslib
 instead.

Right, there is no need to pretend.

 > +     priv->input->name = client->driver_name;
 > +     priv->input->phys = "input/event0";

 Normally we encode bus slot or port in phys. If this data
 is unavailable it is better to omit phys altogether.

 What we can and should do is properly set up input device in
 sysfs hierarchy by parenting it to the i2c client:

        input->dev.parent = &client->dev;

Much better, thank you.

 > +     priv->input->id.bustype = BUS_ISA;

 Not BUS_i2C?

Yes please, BUS_I2C. =)

 > +     if (request_irq(priv->irq, migor_ts_isr, IRQF_TRIGGER_LOW,
 > +                     client->driver_name, priv)) {
 > +             dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
 > +             res = -EBUSY;
 > +             goto err2;
 > +     }
 > +
 > +     /* enable controller */
 > +     if (i2c_master_send(client, migor_ts_ena_seq, sizeof(migor_ts_ena_seq))
 > +         == sizeof(migor_ts_ena_seq))
 > +             return 0;
 > +
 > +     dev_err(&client->dev, "Unable to enable touchscreen.\n");
 > +

 Since you are not setting res here you will signal success the module
 loader and bad things will happen.

Yeah. Good catch.

 I tried implementing my suggestions in the patch below, please let me
 know if it still works for you and I will apply it.

I just tested using latest sh-2.6 git with evtest. Everything seems to
work well. The patch is much cleaner now. Please apply.

Thank you!

/ magnus