Hi Dmitry,
On Wed, Apr 2, 2008 at 3:08 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
