Re: [PATCH] cy8ctmg110: capacitive touchscreen support

[Alan Cox <alan@linux.intel.com>]

> > +/*
> > + * The touch position structure.
> > + */
> > +struct ts_event {
> > +	int x1;
> > +	int y1;
> > +	int x2;
> > +	int y2;
> > +};
> 
> Are there more changes to the driver? Currently I do not see the
> reason for having this structure.

With the other clean ups agreed.

> > +/*
> > + * cy8ctmg110_power is the routine that is called when touch
> > hardware
> > + * will powered off or on.
> > + */
> > +static void cy8ctmg110_power(int poweron)
> 
> bool poweron?

If you prefer - changed


> > +	input_report_key(input, BTN_TOUCH, 1);
> > +	x2 = (u16) (y * X_SCALE_FACTOR);
> > +	y2 = (u16) (x * Y_SCALE_FACTOR);
> 
> Why do we scale in kernel? We should be reportig raw coordinates and
> let userspace to scale if needed.

Ok


> > +	} else if (tsc->tc.x1 != x || tsc->tc.y1 != y) {
> > +		tsc->tc.y1 = y;
> > +		tsc->tc.x1 = x;
> > +		cy8ctmg110_send_event(tsc);
> 
> Send always, input core will filter out duplicates, if needed.

Done and cleaned up the surrounding bits

> > +	client->irq = CY8CTMG110_TOUCH_IRQ;
> 
> Eww...

Real hardware/firmware isn't always pretty - its a mix of GPIO and I²C
interfaces.

> Set up input_dev->dev.parent = client->...

Done

> Frankly, anything that polls with high frequency in a mobile device is
> plainly not useful. I'd just kill these polling bits altogether.

For production hardware yes. I'll split polling out as we can keep that
internally and then burn it when its not needed.

> > +		err = gpio_request(CY8CTMG110_IRQ_PIN_GPIO,
> > "touch_irq_key");
> 
> Always the same GPIO number?

Yes. 

> Please idnt properly, I do not care about 80 lines limit if the result
> causes bad identation. We can also drop "cy8ctmg110_ts: " prefixes -
> dev_xxx() shousl give enogh context.

Ok

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html