Hi Samuel, Am Dienstag, 25. September 2012, 10:55:59 schrieb Samuel Ortiz: > Hi Lars, > > On Mon, Sep 24, 2012 at 06:46:19PM +0200, Lars Poeschel wrote: > > Hi Samuel, > > > > Thanks for your review. > > > > Am 19.09.2012 17:29, schrieb Samuel Ortiz: > > >Hi Lars, > > > > > >On Mon, Aug 27, 2012 at 03:08:38PM +0200, la...@wh2.tu-dresden.de > > > > > >wrote: > > >>From: Lars Poeschel <poesc...@lemonage.de> > > >> > > >>First version of the driver for Nano River Tech's > > >>viperboard added. > > >>It supports i2c, adc, gpio a and gpio b. spi and pwm on > > >>the first gpio a pins is not supported yet. > > >> > > >>Signed-off-by: Lars Poeschel <poesc...@lemonage.de> > > >>--- > > >> > > >> drivers/mfd/Kconfig | 17 + > > >> drivers/mfd/Makefile | 1 + > > >> drivers/mfd/viperboard.c | 1084 > > >> > > >>++++++++++++++++++++++++++++++++++++++++++++++ > > >> > > >> 3 files changed, 1102 insertions(+) > > >> create mode 100644 drivers/mfd/viperboard.c > > > > > >So the MFD driver contains a GPIO one, and an i2c bus driver. > > >You should really split this code into at least 3 pieces: 1 for > > >the GPIO > > >(drivers/gpio), another one for your i2c bus algorithm > > >(drivers/i2c/busses) > > >and a last one for the actual MFD driver. Your Kconfig entries > > >will define the > > >dependencies between them. > > >Then you can define your SoC subdevices as MFD cells and use the > > >MFD APIs to > > >add them as platform devices. > > > > I am not really sure, but maybe you got mislead by the term > > "viperboard". It is NOT an embedded evaluation board or starterkit. > > It is an USB to SPI, I2C, GPIO and ADC interface. You can get a > > quick overview here: http://nanorivertech.com/viperboard.html > > The I2C, GPIO or ADC parts of this driver will never be part of a > > "platform device" in terms of the linux kernel definining the > > configuration of different embedded evaluation boards (a platform). > > Well, that's something none of us know for sure :) > We've seen various IPs re-used across several devices in the past, which is > why we're keen on separating the various drivers. > Also, from an architectural point of view, it makes more sense and is > cleaner imho. > > > Do you still think I should split up the different parts into their > > respective subsystems in the kernel and have one "core" combining > > those parts in MFD ? > > Yes, I do. > > > If so, I will do this. As there would be multiple different > > maintainers involved, against which branch has my patch to be ? > > Most of your sub devices (GPIO, I2C, ADC) will be depending on the MFD one > (as in Kconfig dependency), so it's safe to send each driver to the > specific sub system maintainer and expect him/her to take it. We usually > figure that out once the patchset is ready for upstream inclusion. > > > Should I simply CC the patch to all involved maintainers ? > > It's better yes. Even though a maintainer typically cares about 1 or 2 > patches out of the whole patchset, they usually prefer to be able to get > the whole picture and understand where you want to go with this > submission.
I have done it, and it works! :-) And it looks indeed much cleaner now. But I have on problem left: I can not rmmod the "core" module. I get an kernel oops stacktrace origination from mfd_remove_devices_fn. I think the problem is as follows: Since my viperboard is an usb device, I write a struct usb_driver, that's probe function is called with struct usb_interface containing the struct device that I pass as parent device to mfd_add_devices. During disconnect function I pass the same parent device to mfd_remove_devices for removal of mfd_cells. In mfd_remove_devices_fn the pointer is then containter_of 'd to struct platform_device and to struct usb_interface (struct usb_interface is not containing a mfd_cell pointer either), which then obviously accesses wrong memory in platform_device_unregister. I am a bit confused now. Is the mfd_cell mechanism only working with platform_devices ? What should I do ? I think I have to options: 1. Extending mfd-core to have functions to work with usb_interface and add an mfd_cell to it. 2. Constructing some dummy platform_device in my core driver and passing this to mfd_add_devices and mfd_remove_devices to satisfy them. But this seems a bit ugly to me. Or am I doing something completly wrong ? Thanks for your help, Lars -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
