Hi Philipp, Thanks for the review! :) Em Seg, 2009-05-25 às 21:55 +0200, pHilipp Zabel escreveu: > > +static int pcap_add_subdev(struct spi_device *spi, struct pcap_subdev > > *subdev) > > +{ > > + struct platform_device *pdev; > > + > > + pdev = platform_device_alloc(subdev->name, subdev->id); > > + pdev->dev.parent = &spi->dev; > > + pdev->dev.platform_data = subdev->platform_data; > > + > > + return platform_device_add(pdev); > > +} > > This pcap_subdev, pcap_remove/add_subdev business looks a bit like > you're duplicating the MFD core just because it doesn't handle > resource-less mfd_cells. I wonder whether it'd be better to fix that > and then use > mfd_cell, mfd_add/remove_devices instead?
Due to the lack of comments on this driver, i started grepping LKML for previous MFD reviews. Now the driver allows multiple instances and no longer keeps pcap_chip as a global(as per Samuel comments on U300/AB3100 driver), result is that resource-less mfd_cells are no longer the only issue so I can use the mfd_ API: Now I also need a way to pass a pcap_chip pointer to the subdevs. > > + set_irq_type(pdata->irq, IRQ_TYPE_EDGE_RISING); > > + set_irq_chained_handler(pdata->irq, pcap_irq_handler); > > + set_irq_wake(pdata->irq, 1); > > Looking at the other MFD drivers that register a chained handler, > providing this IRQ via a resource seems to be the default. Ok, will do. I will send a new version of the patch tomorrow, after i finish converting all the subdevices to the new code. Thanks! -- Daniel Ribeiro