linux-kernel - Re: [PATCH] mfd: viperboard driver added

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 28 Sep 2012 15:59:07 +0200
From:	Lars Poeschel <poeschel@...onage.de>
To:	Samuel Ortiz <sameo@...ux.intel.com>
Cc:	Lars Poeschel <larsi@....tu-dresden.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mfd: viperboard driver added

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@....tu-dresden.de
> > >
> > >wrote:
> > >>From: Lars Poeschel <poeschel@...onage.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@...onage.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/