Re: [PATCH v2] input: PCAP2 based touchscreen driver

[Antonio Ospite]

On Mon, 3 Aug 2009 10:21:19 -0700
Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:

> Hi Daniel,
>

Hi Dmitry, I am handling this review round.

> > Please note that the driver depends on some changes from the for-next branch
> > in Samuel Ortiz's mfd tree:
> > http://git.kernel.org/?p=linux/kernel/git/sameo/mfd-2.6.git;a=shortlog;h=for-next
> > 
> > should this be queued by Samuel or the input people will you take care
> > to send this mainline only after Samuel's tree has been merged?
> > 
> 
> I don't mind it going through another tree once all the kinks are worked
> out.
>

Fine.

> > +static int pcap_ts_open(struct input_dev *dev)
> > +{
> > +	struct pcap_ts *pcap_ts = input_get_drvdata(dev);
> > +	int err;
> > +
> > +	err = request_irq(pcap_to_irq(pcap_ts->pcap, PCAP_IRQ_TS),
> > +			pcap_ts_event_touch, 0, "Touch Screen", pcap_ts);
> > +	if (err)
> > +		return err;
> 
> Normally we try to request IRQ in probe() methods instead of delaying it
> till open. Open() is supposed to kick-start the device, but not allocate
> resoirces.
>

Ok, will do.

I must have misunderstood the description of the .open() method in
linux/input.h:
 * @open: this method is called when the very first user calls
 *	input_open_device(). The driver must prepare the device
 *	to start generating events (start polling thread,
 *	request an IRQ, submit URB, etc.)

Does the second sentence here intends that the preparation must be done
in .probe()?

> > +
> > +	pcap_ts->read_state = PCAP_ADC_TS_M_STANDBY;
> > +	schedule_delayed_work(&pcap_ts->work, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static void pcap_ts_close(struct input_dev *dev)
> > +{
> > +	struct pcap_ts *pcap_ts = input_get_drvdata(dev);
> > +
> > +	cancel_delayed_work_sync(&pcap_ts->work);
> 
> So what happens if the device raises IRQ here?
>

Swapping the line with the free_irq() should be ok?

> > +	free_irq(pcap_to_irq(pcap_ts->pcap, PCAP_IRQ_TS), pcap_ts);
> > +
> > +	pcap_ts->read_state = PCAP_ADC_TS_M_NONTS;
> > +	pcap_set_ts_bits(pcap_ts->pcap,
> > +				pcap_ts->read_state << PCAP_ADC_TS_M_SHIFT);
> > +}
> > +
> > +static int __devinit pcap_ts_probe(struct platform_device *pdev)
> > +{
> > +	struct input_dev *input_dev;
> > +	struct pcap_ts *pcap_ts;
> > +	int err = -ENOMEM;
> > +
> > +	pcap_ts = kzalloc(sizeof(*pcap_ts), GFP_KERNEL);
> > +	if (!pcap_ts)
> > +		return err;
> > +
> > +	pcap_ts->pcap = platform_get_drvdata(pdev);
> > +	platform_set_drvdata(pdev, pcap_ts);
> 
> Ewww... Don't mess with data that does not belong to you. Also I don't
> see where you restore it so after unloading the driver reload with
> probably lead to "inetersting" results.
>

Dmitry can you suggest a better way to make the pcap_ts pointer get to
pcap_ts_remove()? We need it in order to remove the input device.
Or keeping this hack, restoring the original value on remove, can be
acceptable?

We will have to fix this also in all other pcap subdrivers.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

Attachment: pgp00000.pgp
Description: PGP signature