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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Antonio,

On Tue, Aug 04, 2009 at 10:52:47PM +0200, Antonio Ospite wrote:
> On Mon, 3 Aug 2009 10:21:19 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> 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()?
>

It probably should be changed to "enable IRQ" instead of "request IRQ".

> > > +
> > > +	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?
> 

I think so, since you are not playing with enable/disable IRQ in this
driver.

> > > +	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?

I think I like Mark's 2nd suggestion the best, just fetch reference to
the chip from the parent device.

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

*nod*

-- 
Dmitry