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@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()? > 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 -- 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
