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:4.436 / U:0.232 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site