lkml.org 
[lkml]   [2012]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] mfd: viperboard driver added
Date
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, larsi@wh2.tu-dresden.de
> > >
> > >wrote:
> > >>From: Lars Poeschel <poeschel@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 <poeschel@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


\
 
 \ /
  Last update: 2012-09-28 17:01    [W:0.016 / U:0.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site