Re: [PATCH] drivers/mfd: Remove obsolete cleanup for clientdata
Hi Wolfram, On Sun, Oct 13, 2013 at 06:06:12PM +0200, Wolfram Sang wrote: > A few new i2c-drivers came into the kernel which clear the clientdata-pointer > on exit or error. This is obsolete meanwhile, the core will do it. > > Signed-off-by: Wolfram Sang > --- > drivers/mfd/twl6040.c | 8 ++-- > 1 file changed, 2 insertions(+), 6 deletions(-) Applied to mfd-next, thanks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/9] mfd: wm8994: convert to use I2C core runtime PM
On Thu, Sep 12, 2013 at 12:24:47PM +0300, Mika Westerberg wrote: > On Wed, Sep 11, 2013 at 06:12:43PM +0200, Samuel Ortiz wrote: > > I think it would make more sense for you to merge that one together with > > the related i2c changes. If you prefer that I take it through MFD, > > please let me know. > > I'm hoping that Wolfram takes all the I2C related patches to his tree. If Wolfram prepares an immutable branch for that, I can pull it in mfd-next and then merge the MFD patch on top of it. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/9] mfd: wm8994: convert to use I2C core runtime PM
Hi Mika, On Wed, Sep 11, 2013 at 06:32:37PM +0300, Mika Westerberg wrote: > The I2C core now prepares runtime PM on behalf of the I2C client device, so > only thing the driver needs to do is to call pm_runtime_put() at the end of > its ->probe(). > > This patch converts wm8994 driver to use this model. > > Signed-off-by: Mika Westerberg Acked-by: Samuel Ortiz I think it would make more sense for you to merge that one together with the related i2c changes. If you prefer that I take it through MFD, please let me know. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] Kontron PLD drivers
Hi Wim, On Thu, Jun 27, 2013 at 10:34:36PM +0200, Wim Van Sebroeck wrote: > Hi All, > > > > Changes since v2: > > > -Change Michael's "Signed-off-by" to "Originally-From" in all patches > > > -Add "From: Guenter Roeck " to gpio patch > > > > > > Guenter Roeck (1): > > > gpio: Kontron PLD gpio driver > > > > > > Kevin Strasser (3): > > > mfd: Kontron PLD mfd driver > > > i2c: Kontron PLD i2c bus driver > > > watchdog: Kontron PLD watchdog timer driver > > I just applied (after a warning fix) and pushed the MFD driver, thanks. > > > > I am happy to take the sub devices driver as well, although the > > respective maintainers can safely carry them as they all have a Kconfig > > dependency on MFD_KEMPLD. So it's up to them. > > If I have to take them, I'd need Wolfram's ACK for the i2c one and Wim's > > for the watchdog one. Linus already ACKed the GPIO one. > > Acked-by: Wim Van Sebroeck > > I'm OK that itgoes via the MFD tree. I took it, thanks for your ACK. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] Kontron PLD drivers
Hi Kevin, On Sun, Jun 23, 2013 at 09:00:02PM -0700, Kevin Strasser wrote: > Changes since v2: > -Change Michael's "Signed-off-by" to "Originally-From" in all patches > -Add "From: Guenter Roeck " to gpio patch > > Guenter Roeck (1): > gpio: Kontron PLD gpio driver > > Kevin Strasser (3): > mfd: Kontron PLD mfd driver > i2c: Kontron PLD i2c bus driver > watchdog: Kontron PLD watchdog timer driver I just applied (after a warning fix) and pushed the MFD driver, thanks. I am happy to take the sub devices driver as well, although the respective maintainers can safely carry them as they all have a Kconfig dependency on MFD_KEMPLD. So it's up to them. If I have to take them, I'd need Wolfram's ACK for the i2c one and Wim's for the watchdog one. Linus already ACKed the GPIO one. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
Hi Wolfram, On Wed, Jun 19, 2013 at 12:00:20PM +0200, Wolfram Sang wrote: > > > Applied with Stephen's Reviewed-by, thanks. > > According to Wolfgang, this is not needed as he will revert the i2c > > commits that are causing those crashes. > > I would not take it for now. > > "Wolfram", please ;) Otherwise correct. Apologies, it seems I have issues with first names today... Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
Hi Lee, On Wed, Jun 19, 2013 at 09:18:59AM +0100, Lee Jones wrote: > On Tue, 18 Jun 2013, Tuomas Tynkkynen wrote: > > > Commit "i2c: core: make it possible to match a pure device tree driver" > > changed semantics of the i2c probing for device tree devices. > > Device tree probed devices now get a NULL i2c_device_id pointer. > > This caused kernel panics due to NULL dereference. > > > > Moves the of_match_device call from tps65910_parse_dt to .probe to > > allow the chip type to be detected from device tree but with the > > device parameters coming from platform data. > > > > Signed-off-by: Tuomas Tynkkynen > > Applied with Stephen's Reviewed-by, thanks. According to Wolfgang, this is not needed as he will revert the i2c commits that are causing those crashes. I would not take it for now. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe
On Mon, Jun 17, 2013 at 12:16:33PM -0600, Stephen Warren wrote: > On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote: > > Commit "i2c: core: make it possible to match a pure device tree driver" > > changed semantics of the i2c probing for device tree devices. > > Device tree probed devices now get a NULL i2c_device_id pointer. > > This caused kernel panics due to NULL dereference. > > Tested-by: Stephen Warren > > (although I imagine I'd need to retest if there was a v2 to address my > previous comments) I would prefer seeing a v2, especially to address the pmic_plat_data overwriting issue. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] twl4030: Fix chained irq handling on resume from suspend
Hi Kalle, On Tue, Oct 16, 2012 at 05:59:35PM +0300, Kalle Jokiniemi wrote: > The irqs are enabled one-by-one in pm core resume_noirq phase. > This leads to situation where the twl4030 primary interrupt > handler (PIH) is enabled before the chained secondary handlers > (SIH). As the PIH cannot clear the pending interrupt, and > SIHs have not been enabled yet, a flood of interrupts hangs > the device. > > Fixed the issue by setting the SIH irqs with IRQF_EARLY_RESUME > flags, so they get enabled before the PIH. > > Signed-off-by: Kalle Jokiniemi > --- > drivers/mfd/twl4030-irq.c |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) Thanks, patch applied to my for-linus branch. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 0/4] add 88pm80x mfd driver
Hi Zhou, On Tue, Jul 10, 2012 at 01:07:29PM +0800, Qiao Zhou wrote: > change log [v6->v5]: > export_symbol_gpl for pm80x_regmap_config to fix build issue for module. I did already apply a patch from Axel Lin that fixes that build failure. See commit 78a73e59db21b465fe60e795a0b7eadb0451370b from my for-next branch. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4 V5] add 88pm80x mfd driver
Hi Mark, On Mon, Jul 09, 2012 at 11:55:33AM +0100, Mark Brown wrote: > On Mon, Jul 09, 2012 at 12:55:51PM +0200, Samuel Ortiz wrote: > > > I applied the first 2 patches, I'd like to get an ACK from the respective > > maintainers for the rtc and the input ones. They can even take it as both > > drivers are protected by the MFD symbol. > > Alessandro's been mostly offline recently so it might be as well to > apply the RTC patch anyway. Fair enough, applied now. Neither Alessandro nor the rtc ml wer not cc'ed on this thread, but that's another problem. Zhou, could you please send at least the rtc patch to Alessandro so that he could have a look at it if time permits ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4 V5] add 88pm80x mfd driver
Hi Arnd, On Mon, Jul 09, 2012 at 11:46:03AM +, Arnd Bergmann wrote: > On Monday 09 July 2012, Samuel Ortiz wrote: > > On Mon, Jul 09, 2012 at 02:37:31PM +0800, Qiao Zhou wrote: > > > change log [v5->v4]: > > > 1, change the file name, from 88pm800-core.c, 88pm805-core.c, > > > 88pm80x-i2c.c > > > to 88pm800.c, 88pm805.c, 88pm80x.c, and modified Makefile accordingly. > > > 2, replace the spinlock used to protect wakeup flag, with using set_bit/ > > > clear_bit, which is suitable adn enough in SMP env. > > > 3, add the version number in each patch. > > I applied the first 2 patches, I'd like to get an ACK from the respective > > maintainers for the rtc and the input ones. They can even take it as both > > drivers are protected by the MFD symbol. > > > > Please add my > > Reviewed-by: Arnd Bergmann > > comment to these two patches as well. Sure. Thanks a lot for your code review. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4 V5] add 88pm80x mfd driver
Hi Zhou, On Mon, Jul 09, 2012 at 02:37:31PM +0800, Qiao Zhou wrote: > change log [v5->v4]: > 1, change the file name, from 88pm800-core.c, 88pm805-core.c, 88pm80x-i2c.c > to 88pm800.c, 88pm805.c, 88pm80x.c, and modified Makefile accordingly. > 2, replace the spinlock used to protect wakeup flag, with using set_bit/ > clear_bit, which is suitable adn enough in SMP env. > 3, add the version number in each patch. I applied the first 2 patches, I'd like to get an ACK from the respective maintainers for the rtc and the input ones. They can even take it as both drivers are protected by the MFD symbol. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver
Hi Lee, On Fri, May 04, 2012 at 07:23:21PM +0100, Lee Jones wrote: > This patch will allow the ab8500-core driver to be probed and set up > when booting when Device Tree is enabled. This includes platform ID > look-up which identifies the machine it is currently running on. If > we are undergoing a DT enabled boot, we will refuse to setup each of > the other ab8500-* devices, as they will be probed individually by DT. This one doesn't apply to my for-next branch. Which tree/branch did you use to generate it ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/15] drivers/mfd: db8500-prcmu: Add support for regulator supply for nmk-i2c.4
Hi Lee, On Fri, May 04, 2012 at 07:23:20PM +0100, Lee Jones wrote: > This applies a supply alias for the db8500's fifth Nomadik i2c port. > > Signed-off-by: Lee Jones > --- > drivers/mfd/db8500-prcmu.c |1 + > 1 file changed, 1 insertion(+) Applied as well, thanks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/15] drivers/mfd: Enable Device Tree support for the db8500-prcmu
Hi Lee, On Fri, May 04, 2012 at 07:23:19PM +0100, Lee Jones wrote: > This patch will enable probing to occur during a Device Tree enabled > boot. The IRQ base is expected to be located in and will be fetched > from the DT itself. We also prevent any of the db8500 regulators > from being registered here, as they will be enabled via DT instead. > > Signed-off-by: Lee Jones > --- > drivers/mfd/db8500-prcmu.c | 29 +++-- > 1 file changed, 19 insertions(+), 10 deletions(-) Patch applied, thanks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mc13xxx: add I2C support, V5
Hi Marc, On Tue, Apr 17, 2012 at 09:21:41AM +1000, Marc Reilly wrote: > Hi Samuel, > > > On Sunday, April 01, 2012 04:41:35 PM Marc Reilly wrote: > > This series (against mfd-2.6/for-next) changes the mc13xxx driver to use > > regmap and adds I2C support. > > It has a compile dependency on regmap/for-next, as the spi driver uses the > > recently added pad_bits config field. > > (bump). > > Are these changes OK to go in? All 4 patches applied to my for-next branch. I had to manually apply patch #1, please verify that it's all good. I added an Acked-by: Oskar Schirmer to patch #4. Please make sure that you add the right tags to the corresponding patches in the future. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mfd: Add basic support for the Congatec CGEB BIOS interface
Hi Sascha, On Mon, Feb 20, 2012 at 07:07:52PM +0100, Sascha Hauer wrote: > Hi Samuel, > > On Mon, Feb 20, 2012 at 05:39:27PM +0100, Samuel Ortiz wrote: > > Hi Sascha, > > > > On Wed, Feb 01, 2012 at 02:26:31PM +0100, Sascha Hauer wrote: > > > The Congatec CGEB is a BIOS interface found on some Congatec x86 > > > modules. It provides access to on board peripherals like I2C busses > > > and watchdogs. This driver contains the basic support for accessing > > > the CGEB interface and registers the child devices. > > After looking at the code, I'm not entirely sure this one belongs to > > drivers/mfd/. Have you thought about putting it under arch/x86/platform/ ? > > There are similar examples there, like uv or olpc. > > I don't mind putting it in arch/x86/platform instead. I'm an ARM guy and > on ARM there currently is a rush to move everything looking remotely > like a driver out of arch/arm/ to drivers/, so putting it under > drivers/mfd/ seemed logical to me. Yes, this seems to be the trend apparently. Could you try pushing that into the x86/platform, please ? If they put you under fire, I guess I could take this patchset. But they have several "BIOS interfaces" there already... Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mc13xxx: add I2C support
Hi Marc, On Fri, Jan 20, 2012 at 06:55:35PM +1100, Marc Reilly wrote: > Hi, > > Thankyou all for your feedback and comments. I'll use them for a V2 but > Samuel, I'd like to know if you'd like me to base them on a specific branch > before I do. Please rebase it against my for-next branch. > Shawn, thanks for testing! > > On Thursday, January 19, 2012 10:29:41 PM Mark Brown wrote: > > On Thu, Jan 19, 2012 at 12:12:31PM +0100, Arnaud Patard wrote: > > > I've never looked at regmap deeply but can't it be done with regmap or is > > > it just a bad idea ? > > > > Glancing quickly at the existing code it should map on reasonably well, > > though a new format definition may be required for the 25 bit shift that > > would be trivial. > > I'm sadly unfamiliar with regmap, is it a far superior solution? does it need > to used now? (ie. I'm relucant to totally rework this now. Please convince me > I need to if required.) I think Mark exposed the advantages of using regmap. It's not a mandatory API usage at that point, but since we still have about 4 weeks before the next merge window opens, and since the changes wouldn't be a huge piece of work, I'd recommend switching to it now. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mfd: Add basic support for the Congatec CGEB BIOS interface
Hi Sascha, On Wed, Feb 01, 2012 at 02:26:31PM +0100, Sascha Hauer wrote: > The Congatec CGEB is a BIOS interface found on some Congatec x86 > modules. It provides access to on board peripherals like I2C busses > and watchdogs. This driver contains the basic support for accessing > the CGEB interface and registers the child devices. After looking at the code, I'm not entirely sure this one belongs to drivers/mfd/. Have you thought about putting it under arch/x86/platform/ ? There are similar examples there, like uv or olpc. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 03/12] mfd: twl-core: Add initial DT support for twl4030/twl6030
Hi Benoit, On Tue, Jan 03, 2012 at 03:09:49PM +0100, Cousson, Benoit wrote: > On 1/2/2012 10:04 AM, Grant Likely wrote: > > On Thu, Dec 22, 2011 at 03:56:37PM +0100, Benoit Cousson wrote: > >> Add initial device-tree support for twl familly chips. > >> The current version is missing the regulator entries due > >> to the lack of DT regulator bindings for the moment. > >> Only the simple sub-modules that do not depend on > >> platform_data information can be initialized properly. > >> > >> Add irqdomain support. > >> > >> Add documentation for the Texas Instruments TWL Integrated Chip. > >> > >> Signed-off-by: Benoit Cousson > >> Cc: Balaji T K > >> Cc: Graeme Gregory > >> Cc: Samuel Ortiz > >> --- > >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >> index f1391c2..f0de088 100644 > >> --- a/drivers/mfd/Kconfig > >> +++ b/drivers/mfd/Kconfig > >> @@ -200,6 +200,7 @@ config MENELAUS > >> config TWL4030_CORE > >>bool "Texas Instruments TWL4030/TWL5030/TWL6030/TPS659x0 Support" > >>depends on I2C=y&& GENERIC_HARDIRQS > >> + select IRQ_DOMAIN > > > > As discussed on linux-next, this breaks powerpc. Drivers cannot select > > IRQ_DOMAIN, it can only be selected by architectures. Drivers must depend > > on it instead. > > OK, good to know. The previous version was already breaking non-ARM platform > because the IRQ_DOMAIN was not defined, hence this update. > I was able to check that this patch was fixing at least x86... but did not > have any way to check the PPC build. > > To be honest, I did not fully understand that this flag was there because > some arch does not support IRQ domain yet and thus it should be selected at > arch level. > > Samuel, > Here is the updated version, and hopefully the last one, including Grant's > fix. > Thanks, I applied it now. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 03/12] mfd: twl-core: Add initial DT support for twl4030/twl6030
Hi Benoit, On Thu, Dec 22, 2011 at 03:56:37PM +0100, Benoit Cousson wrote: > Add initial device-tree support for twl familly chips. > The current version is missing the regulator entries due > to the lack of DT regulator bindings for the moment. > Only the simple sub-modules that do not depend on > platform_data information can be initialized properly. > > Add irqdomain support. > > Add documentation for the Texas Instruments TWL Integrated Chip. > > Signed-off-by: Benoit Cousson > Cc: Balaji T K > Cc: Graeme Gregory > Cc: Samuel Ortiz > --- > .../devicetree/bindings/mfd/twl-familly.txt| 47 ++ > drivers/mfd/Kconfig|1 + > drivers/mfd/twl-core.c | 51 > +++- > 3 files changed, 98 insertions(+), 1 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mfd/twl-familly.txt Patch applied instead of the previous one, thanks for the quick fix. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
Hi Pavan, On Mon, Jun 06, 2011 at 03:22:01PM -0500, Pavan Savoy wrote: > On Thu, Mar 31, 2011 at 9:30 AM, Samuel Ortiz wrote: > > On Thu, Mar 31, 2011 at 06:24:50PM +0300, waldemar.rymarkiew...@tieto.com > > wrote: > >> Hi Samuel, > >> > >> >That's the Bluetooth approach, and it works just fine as well. > >> >However, the netlink option for sending commands and receiving > >> >answers to/from a subsystem sounds more appropriate, at least to me. > >> > >> I haven't used netlink in practice, so just wondering why netlink. I > >> assume, then, it's more suitable :) > >> > > Have a look at the wireless nl80211.c implementation. It's a good example > > for > > that sort of usage. > > So, why was the network interface 'ala eth0 or the netlink interface dropped? The netlink interface was not dropped. See: http://marc.info/?l=linux-wireless&m=130705125228073&w=2 > I saw these patches recently "NFC: add nfc subsystem core" - which > seem to add just the device /dev/nfc - am I correct ? No, the /dev/nfc interface is gone with this subsystem. Instead you will talk to an AF_NFC socket, or to the NFC generic netlink socket for some NFC high level commands and events. See: http://marc.info/?l=linux-wireless&m=130705125728090&w=2 for more details. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
On Thu, Apr 07, 2011 at 07:35:15AM -0700, Grant Likely wrote:
> > Below is a patch for the Xilinx SPI example. Although this would fix the
> > issue, we'd still have to do that on device per device basis. I had a
> > similar
> > solution where MFD drivers would set a flag for sub drivers that don't need
> > any of the MFD bits. In that case the MFD core code would just forward the
> > platform data, instead of embedding it through an MFD cell.
>
> platform_data is already a fiddly bit where you don't know what
> structure type platform_data points at; it is implicitly known and
> easy to get wrong. This solution makes me *very* nervous
> because it would become even easier to get a mismatch on the
> platform_data pointer type.
How would that be more error prone than say a board file instantiating a
platform device after having set the platform_data pointer to point to an
implicitely know structure reference ?
Cheers,
Samuel.
P.S.: Would you be ok with something like the patch below ?
> > ---
> > drivers/mfd/timberdale.c |8
> > drivers/spi/xilinx_spi.c | 19 ++-
> > include/linux/mfd/core.h |3 +++
> > 3 files changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
> > index 94c6c8a..c9220ce 100644
> > --- a/drivers/mfd/timberdale.c
> > +++ b/drivers/mfd/timberdale.c
> > @@ -416,7 +416,7 @@ static __devinitdata struct mfd_cell
> > timberdale_cells_bar0_cfg0[] = {
> > .mfd_data = &timberdale_radio_platform_data,
> > },
> > {
> > - .name = "xilinx_spi",
> > + .name = "mfd_xilinx_spi",
> > .num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > .resources = timberdale_spi_resources,
> > .mfd_data = &timberdale_xspi_platform_data,
> > @@ -476,7 +476,7 @@ static __devinitdata struct mfd_cell
> > timberdale_cells_bar0_cfg1[] = {
> > .mfd_data = &timberdale_radio_platform_data,
> > },
> > {
> > - .name = "xilinx_spi",
> > + .name = "mfd_xilinx_spi",
> > .num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > .resources = timberdale_spi_resources,
> > .mfd_data = &timberdale_xspi_platform_data,
> > @@ -526,7 +526,7 @@ static __devinitdata struct mfd_cell
> > timberdale_cells_bar0_cfg2[] = {
> > .mfd_data = &timberdale_radio_platform_data,
> > },
> > {
> > - .name = "xilinx_spi",
> > + .name = "mfd_xilinx_spi",
> > .num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > .resources = timberdale_spi_resources,
> > .mfd_data = &timberdale_xspi_platform_data,
> > @@ -570,7 +570,7 @@ static __devinitdata struct mfd_cell
> > timberdale_cells_bar0_cfg3[] = {
> > .mfd_data = &timberdale_radio_platform_data,
> > },
> > {
> > - .name = "xilinx_spi",
> > + .name = "mfd_xilinx_spi",
> > .num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > .resources = timberdale_spi_resources,
> > .mfd_data = &timberdale_xspi_platform_data,
> > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> > index c69c6f2..3287b84 100644
> > --- a/drivers/spi/xilinx_spi.c
> > +++ b/drivers/spi/xilinx_spi.c
> > @@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct
> > platform_device *dev)
> > struct spi_master *master;
> > u8 i;
> >
> > - pdata = mfd_get_data(dev);
> > + if (platform_get_device_id(dev) &&
> > + platform_get_device_id(dev)->driver_data & MFD_PLATFORM_DEVICE)
> > + pdata = mfd_get_data(dev);
> > + else
> > + pdata = dev->dev.platform_data;
> > if (pdata) {
> > num_cs = pdata->num_chipselect;
> > little_endian = pdata->little_endian;
> > @@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct
> > platform_device *dev)
> > /* work with hotplug and coldplug */
> > MODULE_ALIAS("platform:" XILINX_SPI_NAME);
> >
> > +static const struct platform_device_id xilinx_spi_id_table[] = {
> > + {
> > + .name = XILINX_SPI_NAME,
> > + },
> > + {
> > + .name = "mfd_xilinx_spi",
> > + .driver_data = MFD_PLATFORM_DEVICE,
> > + },
> > + { }, /* Terminating Entry */
> > +};
> > +MODULE_DEVICE_TABLE(platform, xilinx_spi_id_table);
> > +
> > static struct platform_driver xilinx_spi_driver = {
> > .probe = xilinx_spi_probe,
> > .remove = __devexit_p(xilinx_spi_remove),
> > @@ -538,6 +554,7 @@ static struct platform_driver xilinx_spi_driver = {
> > .owner = THIS_MODULE,
> > .of_match_table = xilinx_spi_of_match,
> > },
> > + .id_table = xilinx_spi_id_table,
> > };
> >
> > static int __init xilinx_spi_pltfm_init(void)
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index ad1b19a..13f31f4 100644
> > --- a/include/linu
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
Hi Felipe,
On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
> Hi,
>
> On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
> > > > > What is a "MFD cell pointer" and why is it needed in struct device?
> > > > An MFD cell is an MFD instantiated device.
> > > > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > > > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > > > specific pointer, and sometimes both. Also, some of those drivers have
> > > > been
> > > > implemented as MFD sub drivers, while others know nothing about MFD and
> > > > just
> > > > expect a plain platform_data pointer.
> > >
> > > That sounds like a bug in those drivers, why not fix them to properly
> > > pass in the correct pointer?
> > Because they're drivers for generic IPs, not MFD ones. By forcing them to
> > use
> > MFD specific structure and APIs, we make it more difficult for platform code
> > to instantiate them.
>
> I agree. What I do on those cases is to have a simple platform_device
> for the core IP driver and use platform_device_id tables to do runtime
> checks of the small differences. If one platform X doesn't use a
> platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
> allocates a platform_device with the correct name and adds that to the
> driver model.
I see, thanks.
Below is a patch for the Xilinx SPI example. Although this would fix the
issue, we'd still have to do that on device per device basis. I had a similar
solution where MFD drivers would set a flag for sub drivers that don't need
any of the MFD bits. In that case the MFD core code would just forward the
platform data, instead of embedding it through an MFD cell.
Cheers,
Samuel.
---
drivers/mfd/timberdale.c |8
drivers/spi/xilinx_spi.c | 19 ++-
include/linux/mfd/core.h |3 +++
3 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
index 94c6c8a..c9220ce 100644
--- a/drivers/mfd/timberdale.c
+++ b/drivers/mfd/timberdale.c
@@ -416,7 +416,7 @@ static __devinitdata struct mfd_cell
timberdale_cells_bar0_cfg0[] = {
.mfd_data = &timberdale_radio_platform_data,
},
{
- .name = "xilinx_spi",
+ .name = "mfd_xilinx_spi",
.num_resources = ARRAY_SIZE(timberdale_spi_resources),
.resources = timberdale_spi_resources,
.mfd_data = &timberdale_xspi_platform_data,
@@ -476,7 +476,7 @@ static __devinitdata struct mfd_cell
timberdale_cells_bar0_cfg1[] = {
.mfd_data = &timberdale_radio_platform_data,
},
{
- .name = "xilinx_spi",
+ .name = "mfd_xilinx_spi",
.num_resources = ARRAY_SIZE(timberdale_spi_resources),
.resources = timberdale_spi_resources,
.mfd_data = &timberdale_xspi_platform_data,
@@ -526,7 +526,7 @@ static __devinitdata struct mfd_cell
timberdale_cells_bar0_cfg2[] = {
.mfd_data = &timberdale_radio_platform_data,
},
{
- .name = "xilinx_spi",
+ .name = "mfd_xilinx_spi",
.num_resources = ARRAY_SIZE(timberdale_spi_resources),
.resources = timberdale_spi_resources,
.mfd_data = &timberdale_xspi_platform_data,
@@ -570,7 +570,7 @@ static __devinitdata struct mfd_cell
timberdale_cells_bar0_cfg3[] = {
.mfd_data = &timberdale_radio_platform_data,
},
{
- .name = "xilinx_spi",
+ .name = "mfd_xilinx_spi",
.num_resources = ARRAY_SIZE(timberdale_spi_resources),
.resources = timberdale_spi_resources,
.mfd_data = &timberdale_xspi_platform_data,
diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
index c69c6f2..3287b84 100644
--- a/drivers/spi/xilinx_spi.c
+++ b/drivers/spi/xilinx_spi.c
@@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct
platform_device *dev)
struct spi_master *master;
u8 i;
- pdata = mfd_get_data(dev);
+ if (platform_get_device_id(dev) &&
+ platform_get_device_id(dev)->driver_data & MFD_PLATFORM_DEVICE)
+ pdata = mfd_get_data(dev);
+ else
+ pdata = dev->dev.platform_data;
if (pdata) {
num_cs = pdata->num_chipselect;
little_endian = pdata->little_endian;
@@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct
platform_device *dev)
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
On Wed, Apr 06, 2011 at 10:56:47AM -0700, Greg KH wrote:
> On Wed, Apr 06, 2011 at 07:05:38PM +0200, Samuel Ortiz wrote:
> > Hi Greg,
> >
> > On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
> > > On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -33,6 +33,7 @@ struct class;
> > > > struct subsys_private;
> > > > struct bus_type;
> > > > struct device_node;
> > > > +struct mfd_cell;
> > > >
> > > > struct bus_attribute {
> > > > struct attributeattr;
> > > > @@ -444,6 +445,8 @@ struct device {
> > > > struct device_node *of_node; /* associated device tree
> > > > node */
> > > > const struct of_device_id *of_match; /* matching of_device_id
> > > > from driver */
> > > >
> > > > + struct mfd_cell *mfd_cell; /* MFD cell pointer */
> > > > +
> > >
> > > What is a "MFD cell pointer" and why is it needed in struct device?
> > An MFD cell is an MFD instantiated device.
> > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > specific pointer, and sometimes both. Also, some of those drivers have been
> > implemented as MFD sub drivers, while others know nothing about MFD and just
> > expect a plain platform_data pointer.
>
> That sounds like a bug in those drivers, why not fix them to properly
> pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
The timberdale MFD for example is built with a Xilinx SPI controller, and a
Micrel ks8842 ethernet switch IP. Forcing those devices into being MFD devices
would mean any platform willing to instantiate them would have to use the MFD
APIs. That sounds a bit artificial to me.
Although there is currently no drivers instantiated by both an MFD driver
and some platform code, Grant complaint about the Xilinx SPI driver moving
from a platform driver to an MFD one makes sense to me.
> > So, adding an MFD cell pointer to the device structure allows us to cleanly
> > pass both pieces of information, while keeping all the MFD sub drivers
> > independant from the MFD core if they want/can.
>
> They shouldn't be "independant",
Excuse my poor spelling.
> make them "dependant" and go from there.
That's what the code currently does.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
Hi Ben, On Wed, Apr 06, 2011 at 06:16:49PM +0100, Ben Hutchings wrote: > > So, adding an MFD cell pointer to the device structure allows us to cleanly > > pass both pieces of information, while keeping all the MFD sub drivers > > independant from the MFD core if they want/can. > > Why isn't an MFD the parent of its component devices? It actually is. How would that help here ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
Hi Greg,
On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
> On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -33,6 +33,7 @@ struct class;
> > struct subsys_private;
> > struct bus_type;
> > struct device_node;
> > +struct mfd_cell;
> >
> > struct bus_attribute {
> > struct attributeattr;
> > @@ -444,6 +445,8 @@ struct device {
> > struct device_node *of_node; /* associated device tree node */
> > const struct of_device_id *of_match; /* matching of_device_id from
> > driver */
> >
> > + struct mfd_cell *mfd_cell; /* MFD cell pointer */
> > +
>
> What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
We've been faced with the problem of being able to pass both MFD related data
and a platform_data pointer to some of those drivers. Squeezing the MFD bits
in the sub driver platform_data pointer doesn't work for drivers that know
nothing about MFDs. It also adds an additional dependency on the MFD API to
all MFD sub drivers. That prevents any of those drivers to eventually be used
as plain platform device drivers.
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
On Mon, Apr 04, 2011 at 09:04:29PM -0600, Grant Likely wrote:
> > The second step would be to get rid of mfd_get_data() and have all
> > subdrivers
> > going back to the regular platform_data way. They would no longer be
> > dependant
> > on the MFD code except for those who really need it. In that case they could
> > just call mfd_get_cell() and get full access to their MFD cell.
>
> The revert to platform_data needs to happen ASAP though. If this
> second step isn't ready really quickly, then the current patches
> should be reverted to give some breathing room for creating the
> replacement patches. However, it's not such a rush if the below
> patch really does eliminate all of the nastiness of the original
> series. (I haven't looked and a rolled up diff of the first series and
> this change, so I don't know for sure).
I am done reverting these changes, with a final patch getting rid of
mfd_get_data. See
git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git for-linus
I still need to give it a second review before pushing it to lkml for
comments. It's 20 patches long, so I'm not entirely sure Linus would take that
at that point.
Pushing patch #1 would be enough for fixing the issues introduced by the
original patchset, so I'm leaning toward pushing it and leaving the 19 other
patches for the next merge window.
> In principle I agree with this patch. Some comments below.
Thanks for the comments. I think I addressed all of them in patch #1:
---
drivers/base/platform.c |1 +
drivers/mfd/mfd-core.c | 15 +--
include/linux/device.h |3 +++
include/linux/mfd/core.h |7 +--
4 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f051cff..bde6b97 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -149,6 +149,7 @@ static void platform_device_release(struct device *dev)
of_device_node_put(&pa->pdev.dev);
kfree(pa->pdev.dev.platform_data);
+ kfree(pa->pdev.dev.mfd_cell);
kfree(pa->pdev.resource);
kfree(pa);
}
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..99b0d6d 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -18,6 +18,18 @@
#include
#include
+static int mfd_platform_add_cell(struct platform_device *pdev, const struct
mfd_cell *cell)
+{
+ if (!cell)
+ return 0;
+
+ pdev->dev.mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
+ if (!pdev->dev.mfd_cell)
+ return -ENOMEM;
+
+ return 0;
+}
+
int mfd_cell_enable(struct platform_device *pdev)
{
const struct mfd_cell *cell = mfd_get_cell(pdev);
@@ -75,7 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
pdev->dev.parent = parent;
- ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+ ret = mfd_platform_add_cell(pdev, cell);
if (ret)
goto fail_res;
@@ -123,7 +135,6 @@ static int mfd_add_device(struct device *parent, int id,
return 0;
-/* platform_device_del(pdev); */
fail_res:
kfree(res);
fail_device:
diff --git a/include/linux/device.h b/include/linux/device.h
index ab8dfc0..cf353cf 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -33,6 +33,7 @@ struct class;
struct subsys_private;
struct bus_type;
struct device_node;
+struct mfd_cell;
struct bus_attribute {
struct attributeattr;
@@ -444,6 +445,8 @@ struct device {
struct device_node *of_node; /* associated device tree node */
const struct of_device_id *of_match; /* matching of_device_id from
driver */
+ struct mfd_cell *mfd_cell; /* MFD cell pointer */
+
dev_t devt; /* dev_t, creates the sysfs "dev" */
spinlock_t devres_lock;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index ad1b19a..28f81cf 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -86,7 +86,7 @@ extern int mfd_clone_cell(const char *cell, const char
**clones,
*/
static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
{
- return pdev->dev.platform_data;
+ return pdev->dev.mfd_cell;
}
/*
@@ -95,7 +95,10 @@ static inline const struct mfd_cell *mfd_get_cell(struct
platform_device *pdev)
*/
static inline void *mfd_get_data(struct platform_device *pdev)
{
- return mfd_get_cell(pdev)->mfd_data;
+ if (pdev->dev.mfd_cell)
+ return mfd_get_cell(pdev)->mfd_data;
+ else
+ return pdev->dev.platform_data;
}
extern int mfd_add_devices(struct device *parent, int id,
--
1.7.2.3
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
On Fri, Apr 01, 2011 at 05:58:44PM -0600, Grant Likely wrote:
> On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz wrote:
> > On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
> >> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon
> >> wrote:
> >> > On Fri, 1 Apr 2011 13:20:31 +0200
> >> > Samuel Ortiz wrote:
> >> >
> >> >> Hi Grant,
> >> >>
> >> >> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> >> > [...]
> >> >> > Gah. Not all devices instantiated via mfd will be an mfd device,
> >> >> > which means that the driver may very well expect an *entirely
> >> >> > different* platform_device pointer; which further means a very high
> >> >> > potential of incorrectly dereferenced structures (as evidenced by a
> >> >> > patch series that is not bisectable). For instance, the xilinx ip
> >> >> > cores are used by more than just mfd.
> >> >> I agree. Since the vast majority of the MFD subdevices are MFD
> >> >> specific IPs, I overlooked that part. The impacted drivers are the
> >> >> timberdale and the DaVinci voice codec ones.
> >>
> >> Another option is you could do this for MFD devices:
> >>
> >> struct mfd_device {
> >> struct platform_devce pdev;
> >> struct mfd_cell *cell;
> >> };
> >>
> >> However, that requires that drivers using the mfd_cell will *never*
> >> get instantiated outside of the mfd infrastructure, and there is no
> >> way to protect against this so it is probably a bad idea.
> >>
> >> Or, mfd_cell could be added to platform_device directly which would
> >> *by far* be the safest option at the cost of every platform_device
> >> having a mostly unused mfd_cell pointer. Not a significant cost in my
> >> opinion.
> > I thought about this one, but I had the impression people would want to kill
> > me for adding an MFD specific pointer to platform_device. I guess it's worth
> > giving it a try since it would be a simple and safe solution.
> > I'll look at it later this weekend.
> >
> > Thanks for the input.
>
> [cc'ing gregkh because we're talking about modifying struct platform_device]
>
> I'll back you up on this one. It is a far better solution than the
> alternatives. At least with mfd, it covers a large set of devices. I
> think there is a strong argument for doing this. Or alternatively,
> the particular interesting fields from mfd_cell could be added to
> platform_device. What information do child devices need access to?
In some cases, they need the whole cell to clone it. So I'm up for adding an
mfd_cell pointer to the platform_device structure.
Below is a tentative patch. This is a first step and would fix all
regressions. I tried to keep the MFD dependencies as small as possible, which
is why I placed the pdev->mfd_cell building code in mfd-core.c
The second step would be to get rid of mfd_get_data() and have all subdrivers
going back to the regular platform_data way. They would no longer be dependant
on the MFD code except for those who really need it. In that case they could
just call mfd_get_cell() and get full access to their MFD cell.
---
drivers/mfd/mfd-core.c | 27 ++-
include/linux/mfd/core.h|7 +--
include/linux/platform_device.h |5 +
3 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..c0fc1c0 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -18,6 +18,21 @@
#include
#include
+static int mfd_platform_add_cell(struct platform_device *pdev, const struct
mfd_cell *cell)
+{
+ struct mfd_cell *c;
+
+ if (cell == NULL)
+ return 0;
+
+ c = kmemdup(cell, sizeof(struct mfd_cell), GFP_KERNEL);
+ if (c == NULL)
+ return -ENOMEM;
+
+ pdev->mfd_cell = c;
+ return 0;
+}
+
int mfd_cell_enable(struct platform_device *pdev)
{
const struct mfd_cell *cell = mfd_get_cell(pdev);
@@ -75,7 +90,7 @@ static int mfd_add_device(struct device *parent, int id,
pdev->dev.parent = parent;
- ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+ ret = mfd_platform_add_cell(pdev, cell);
if (ret)
goto fail_res;
@@ -104,17 +119,17 @@ static int mfd_add_device(struct device *parent, int id,
if (!cell->ignore_resource_conflicts) {
ret = acpi_check_resource_conflict(res);
if (ret)
-
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon wrote:
> > On Fri, 1 Apr 2011 13:20:31 +0200
> > Samuel Ortiz wrote:
> >
> >> Hi Grant,
> >>
> >> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> > [...]
> >> > Gah. Not all devices instantiated via mfd will be an mfd device,
> >> > which means that the driver may very well expect an *entirely
> >> > different* platform_device pointer; which further means a very high
> >> > potential of incorrectly dereferenced structures (as evidenced by a
> >> > patch series that is not bisectable). For instance, the xilinx ip
> >> > cores are used by more than just mfd.
> >> I agree. Since the vast majority of the MFD subdevices are MFD
> >> specific IPs, I overlooked that part. The impacted drivers are the
> >> timberdale and the DaVinci voice codec ones.
>
> Another option is you could do this for MFD devices:
>
> struct mfd_device {
> struct platform_devce pdev;
> struct mfd_cell *cell;
> };
>
> However, that requires that drivers using the mfd_cell will *never*
> get instantiated outside of the mfd infrastructure, and there is no
> way to protect against this so it is probably a bad idea.
>
> Or, mfd_cell could be added to platform_device directly which would
> *by far* be the safest option at the cost of every platform_device
> having a mostly unused mfd_cell pointer. Not a significant cost in my
> opinion.
I thought about this one, but I had the impression people would want to kill
me for adding an MFD specific pointer to platform_device. I guess it's worth
giving it a try since it would be a simple and safe solution.
I'll look at it later this weekend.
Thanks for the input.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
On Fri, Apr 01, 2011 at 10:47:56AM -0700, Andres Salomon wrote:
> On Fri, 1 Apr 2011 13:20:31 +0200
> Samuel Ortiz wrote:
>
> > Hi Grant,
> >
> > On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> [...]
> > > Gah. Not all devices instantiated via mfd will be an mfd device,
> > > which means that the driver may very well expect an *entirely
> > > different* platform_device pointer; which further means a very high
> > > potential of incorrectly dereferenced structures (as evidenced by a
> > > patch series that is not bisectable). For instance, the xilinx ip
> > > cores are used by more than just mfd.
> > I agree. Since the vast majority of the MFD subdevices are MFD
> > specific IPs, I overlooked that part. The impacted drivers are the
> > timberdale and the DaVinci voice codec ones.
>
> Can you please provide pointers to what you're referring to? The only
> code that I could find that created platform devices prefixed with
> 'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.
The xilinx-spi, ocores-i2c, i2c-xiic drivers and to some extend the
ks8842 ethernet driver are generic IPs that the timberdale SOC happens to
use. So I agree it's extremely unlikely that anyone could come up with a
platform that would be re-using e.g. the timb-radio IP, but I think it's less
unikely for more generic IPs such as the xilinx-spi one.
> > To fix that problem I propose 2 alternatives:
> >
> > 1) When declaring the sub devices cells, the MFD driver should
> > specify an mfd_data_size value for sub devices that are not MFD
> > specific. It's the MFD driver responsibility to set the cell
> > properly, and the non MFD specific drivers are kept MFD agnostic.
> > See my patch below for the timberdale case.
> >
> > 2) Revert the mfd_get_data() call for getting sub devices platform
> > data pointers. That was introduced to ease the MFD cell sharing work,
> > so if we take this route we'll need the cs5535 MFD driver to pass its
> > cells as platform_data pointer. Andres, can you confirm that this
> > would be fine for the mfd_clone_cell() routine to keep working ?
>
> It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
> to clone. We could change it to accept the cell as an argument. It
> would also break mfd_cell_enable/disable, of course.
I'm talking about reverting the default behaviour of passing the MFD cell as
the platform data, and going back to the cell definitions setting their
platform_data pointer explicitely. In that case, the cs5535 driver would have
to do something like:
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 155fa04..3e3841d 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -106,6 +106,7 @@ static __devinitdata struct mfd_cell cs5535_mfd_cells[] =
{
.name = "cs5535-acpi",
.num_resources = 1,
.resources = &cs5535_mfd_resources[ACPI_BAR],
+ .platform_data = &cs5535_mfd_cells[ACPI_BAR],
.enable = cs5535_mfd_res_enable,
.disable = cs5535_mfd_res_disable,
mfd_get_cell would then return &cs5535_mfd_cells[ACPI_BAR].
This fix would put all sub devices drivers back to an MFD agnostic state,
although the vast majority of them will certainly never be found anywhere else
than in their current MFD SoC. That's why I'm still not sure which way to go
to fix that problem.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> On Wed, Feb 02, 2011 at 08:08:12PM -0800, Andres Salomon wrote:
> >
> > No need to explicitly set the cell's platform_data/data_size.
> >
> > In this case, move the various platform_data pointers
> > to driver_data. All of the clients which make use of it
> > are also changed.
> >
> > Signed-off-by: Andres Salomon
> > ---
> > drivers/dma/timb_dma.c |2 +-
> > drivers/gpio/timbgpio.c |5 +-
> > drivers/i2c/busses/i2c-ocores.c |2 +-
> > drivers/i2c/busses/i2c-xiic.c|2 +-
> > drivers/media/radio/radio-timb.c |2 +-
> > drivers/media/video/timblogiw.c |2 +-
> > drivers/mfd/timberdale.c | 81
> > +-
> > drivers/net/ks8842.c |2 +-
> > drivers/spi/xilinx_spi.c |2 +-
> > 9 files changed, 36 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
> > index 3b88a4e..aa06ca4 100644
> > --- a/drivers/dma/timb_dma.c
> > +++ b/drivers/dma/timb_dma.c
> > @@ -684,7 +684,7 @@ static irqreturn_t td_irq(int irq, void *devid)
> >
> > static int __devinit td_probe(struct platform_device *pdev)
> > {
> > - struct timb_dma_platform_data *pdata = pdev->dev.platform_data;
> > + struct timb_dma_platform_data *pdata = platform_get_drvdata(pdev);
>
> Hold the phone. I know this has already been merged, but this isn't correct.
>
> drvdata is under the ownership of the driver, which means it should
> *not* be set when .probe() gets called. It is for driver private data
> to do with as it needs, usually for internal state.
We didn't merge that version of the patchset, but one getting the
platform_data pointer from mfd_get_data(). That introduces the issue you're
talking about below.
> Gah. Not all devices instantiated via mfd will be an mfd device,
> which means that the driver may very well expect an *entirely
> different* platform_device pointer; which further means a very high
> potential of incorrectly dereferenced structures (as evidenced by a
> patch series that is not bisectable). For instance, the xilinx ip
> cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD specific IPs, I
overlooked that part. The impacted drivers are the timberdale and the DaVinci
voice codec ones.
To fix that problem I propose 2 alternatives:
1) When declaring the sub devices cells, the MFD driver should specify an
mfd_data_size value for sub devices that are not MFD specific. It's the MFD
driver responsibility to set the cell properly, and the non MFD specific
drivers are kept MFD agnostic.
See my patch below for the timberdale case.
2) Revert the mfd_get_data() call for getting sub devices platform data
pointers. That was introduced to ease the MFD cell sharing work, so if we take
this route we'll need the cs5535 MFD driver to pass its cells as platform_data
pointer. Andres, can you confirm that this would be fine for the
mfd_clone_cell() routine to keep working ?
Patch for solution 1:
drivers/mfd/mfd-core.c | 13 ++---
drivers/mfd/timberdale.c| 11 +++
include/linux/mfd/core.h|1 +
drivers/i2c/busses/i2c-ocores.c |3 +--
drivers/i2c/busses/i2c-xiic.c |3 +--
drivers/net/ks8842.c|3 +--
drivers/spi/xilinx_spi.c|3 +--
7 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..8abe510 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -75,9 +75,16 @@ static int mfd_add_device(struct device *parent, int id,
pdev->dev.parent = parent;
- ret = platform_device_add_data(pdev, cell, sizeof(*cell));
- if (ret)
- goto fail_res;
+ if (cell->mfd_data_size > 0) {
+ ret = platform_device_add_data(pdev,
+ cell->mfd_data, cell->mfd_data_size);
+ if (ret)
+ goto fail_res;
+ } else {
+ ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+ if (ret)
+ goto fail_res;
+ }
for (r = 0; r < cell->num_resources; r++) {
res[r].name = cell->resources[r].name;
diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
index 94c6c8a..b4d2d09 100644
--- a/drivers/mfd/timberdale.c
+++ b/drivers/mfd/timberdale.c
@@ -396,6 +396,7 @@ static __devinitdata struct mfd_cell
timberdale_cells_bar0_cfg0[] = {
.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
.resources = timberdale_xiic_resources,
.mfd_data = &timberdale_xiic_platform_data,
+ .mfd_data_size = sizeof(timberdale_xiic_platform_data)
},
{
.name = "timb-gpio",
@@ -420,12 +421,14 @@ static __devinitdata struct mfd_ce
Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
On Thu, Mar 31, 2011 at 06:24:50PM +0300, waldemar.rymarkiew...@tieto.com wrote: > Hi Samuel, > > >That's the Bluetooth approach, and it works just fine as well. > >However, the netlink option for sending commands and receiving > >answers to/from a subsystem sounds more appropriate, at least to me. > > I haven't used netlink in practice, so just wondering why netlink. I assume, > then, it's more suitable :) > Have a look at the wireless nl80211.c implementation. It's a good example for that sort of usage. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
Hi Waldemar, On Thu, Mar 31, 2011 at 05:42:21PM +0300, waldemar.rymarkiew...@tieto.com wrote: > >My idea of an initial NFC subsystem architecture was actually > >the following > >one: > >- A core NFC layer against which NFC drivers would register. > >- A netlink socket for handling the HCI commands. That would > >put a big part of the NFC HCI layer in kernel land and could > >potentially simplify the existing NFC stacks. > > Shouldn't be better to add a new AF_NFC sock family and then register new > LLCP and HCI protocols? > That's the Bluetooth approach, and it works just fine as well. However, the netlink option for sending commands and receiving answers to/from a subsystem sounds more appropriate, at least to me. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
On Tue, Mar 29, 2011 at 01:05:02PM +0200, Arnd Bergmann wrote: > On Tuesday 29 March 2011, waldemar.rymarkiew...@tieto.com wrote: > > >Yes, NFC seems to be a good fit for a new socket family. > > >Especially if we ever want to have a proper NFC p2p support > > >from the kernel. > > >Sending HCI commands should be done through a dedicated > > >netlink socket too. > > > > > >I am currently strting to work on such solution, and I hope to > > >be able to come up with a basic prototype for it in a few weeks. > > > > What about common drivers interface in this case. > > Should we go for common /dev/nfcX interface as well? > > I fear there can only be one. A good implementation of a socket > interface would mean that there is no need for a character device. My idea of an initial NFC subsystem architecture was actually the following one: - A core NFC layer against which NFC drivers would register. - A netlink socket for handling the HCI commands. That would put a big part of the NFC HCI layer in kernel land and could potentially simplify the existing NFC stacks. - A socket family for the LLCP abstraction, a.k.a. NFC peer to peer mode. We can start working on the first 2 items and leave the last one as a future enhancement, since what NFC is currently mostly used for is tag reading/writing and smartcard emulation. Basically, we'd replace the current character device option with a netlink one, allowing for a single kernel entry point for multiple applications willing to do NFC. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
On Fri, Mar 18, 2011 at 01:19:53PM +0100, Arnd Bergmann wrote: > On Friday 18 March 2011, Waldemar Rymarkiewicz wrote: > > Add new driver for MicroRead NFC chip connected to i2c bus. > > > > See Documentation/nfc/nfc-microread.txt. > > As I said in my first review and Alan also pointed out now, the > most important change will be to add a common NFC core layer, > before adding more hardware drivers. > > Also, regarding the user interface, we need to be really sure > that this is the best way of talking to NFC devices. The interface > you have today is a simple character device read/write kind, > which may be the best thing if the protocol stack on top is > really simple and there is never the need to have multiple > applications talking to different endpoints on the wireless > interface, and if there are no protocol headers being > send over the character device interface. > > Otherwise, a better interface is probably to add a new network > socket family and abstract the protocol layers in the kernel. Yes, NFC seems to be a good fit for a new socket family. Especially if we ever want to have a proper NFC p2p support from the kernel. Sending HCI commands should be done through a dedicated netlink socket too. I am currently strting to work on such solution, and I hope to be able to come up with a basic prototype for it in a few weeks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
Hi Waldemar, On Tue, Mar 15, 2011 at 02:59:09PM +0200, waldemar.rymarkiew...@tieto.com wrote: > Hi Samuel, > > >The few currently available NFC stacks are all implementing > >the NFC HCI layer in > >userspace, on top of a a more or less big HAL. By at least > >moving the HCI layer > >down to the kernel, and having userspace talk to it through > >netlink, we could > >eventually get rid of those HALs. > > Is HCI already standarised? I can't find the spec anywhere. ETSI has a final spec for it, TS 102 622 (latest version is 10.2.0). I know the NFC forum is trying to come up with a competing standard: NCI. As far as I know they haven't released any spec for it yet. Cheers, Samuel. P.S.: All ETSI specs are freely available. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
Hi Arnd, Arnd Bergmann writes: > > On Thursday 10 March 2011, Waldemar Rymarkiewicz wrote: > > Add new driver for MicroRead NFC chip connected to i2c bus. > > > > See Documentation/nfc/nfc-microread.txt. > > The imlpementation looks fairly clean, no objections on that. > > However, this is the second NFC driver (at least), and that means > it's time to come up with a generic way of talking to NFC devices > from user space. We cannot allow every NFC controller to have > another set of arbitrary ioctl numbers. I definitely agree we need to come up with some sort of NFC subsystem. Ideally though, it should be more than ioctls over character devices. NFC is not that far from Bluetooth or IrDA and having an NFC socket family would be quite nice. Especially for the p2p parts of the spec. And instead of passing commands through ioctls, we might want to look at a netlink based interface. The few currently available NFC stacks are all implementing the NFC HCI layer in userspace, on top of a a more or less big HAL. By at least moving the HCI layer down to the kernel, and having userspace talk to it through netlink, we could eventually get rid of those HALs. Cheers, Samuel. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/2] MAX8997/8966 MFD (includig PMIC) Initial Release
Hi Myung, On Fri, Mar 04, 2011 at 03:50:25PM +0900, MyungJoo Ham wrote: > MAX8997/8966 has > - PMIC > - RTC > - MUIC (usb switch) > - Flash control > - Haptic control > - Fuel Gauge (MAX17042 compatible) > - Battery charger control > > This patch adds an initial driver for Maxim Semiconductor 8997/8966's > PMIC function. I applied patch #1 from thsi serie, thanks a lot. Please address Mark's comments for patch #2 and I'll take it as well. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mc13xxx-core, support for i2c, V4
Hi Mark, On Sat, Feb 12, 2011 at 09:11:30PM +1100, Marc Reilly wrote: > Hi Grant, > > Thanks for your comments, > > On Saturday, February 12, 2011 07:40:35 pm Grant Likely wrote: > > On Tue, Jan 04, 2011 at 04:34:55PM +1100, Marc Reilly wrote: > > > Hi, > > > > > > These patches add i2c support for the mc13xxx-core drive. For v4 I've > > > tried to take in all previous comments, hopefully making it better to > > > follow, and bisectable. > > > > This series looks okay to me. Since this is audio drivers, I expect > > that it would best be taken via the ASoC tree? Have you sent it to > > Mark Brown and the ALSA list for review? > > > > g. > > Actually only the mc13873 has audio support. I think these IC's really fit > better under the MFD tree, but silly me didn't add Samuel Ortiz (MFD > maintainer) to this series... so included him now. > > Samuel, are you the most appropriate to take this series in and would you > like > me to resend these patches to you? The thread archive is at [1]. Yes, I would be the one taking those patches. Please resend the patch set to me, and I'll have a look. Has Uwe commented on them yet ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mfd: Remove obsolete cleanup for clientdata
Hi _Wolfram_, On Mon, Jan 24, 2011 at 11:44:27AM +0100, Wolfram Sang wrote: > A few new i2c-drivers came into the kernel which clear the clientdata-pointer > on exit or error. This is obsolete meanwhile, the core will do it. Thanks a lot, patch applied now. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] MAX8997/8966 MFD Driver Initial Release (PMIC+RTC+MUIC+Haptic+...)
Hi,
On Fri, Jan 14, 2011 at 03:53:37PM +0900, MyungJoo Ham wrote:
> MAX8997/MAX8966 chip is a multi-function device with I2C bussses. The
> chip includes PMIC, RTC, Fuel Gauge, MUIC, Haptic, Flash control, and
> Battery (charging) control.
>
> This patch is an initial release of a MAX8997/8966 driver that supports
> to enable the chip with its primary I2C bus that connects every device
> mentioned above except for Fuel Gauge, which uses another I2C bus. The
> fuel gauge is not supported by this mfd driver and is supported by a
> seperated driver of MAX17042 Fuel Gauge (yes, the fuel gauge part is
> compatible with MAX17042).
The patch looks good, I just have one comment:
> Signed-off-by: MyungJoo Ham
> Signed-off-by: Kyungmin Park
> ---
> drivers/mfd/Kconfig | 12 ++
> drivers/mfd/Makefile|1 +
> drivers/mfd/max8997.c | 211 +++
> include/linux/mfd/max8997-private.h | 238
> +++
> include/linux/mfd/max8997.h | 88 +
> 5 files changed, 550 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mfd/max8997.c
> create mode 100644 include/linux/mfd/max8997-private.h
> create mode 100644 include/linux/mfd/max8997.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index da9d297..486bf38 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -293,6 +293,18 @@ config MFD_MAX8925
> accessing the device, additional drivers must be enabled in order
> to use the functionality of the device.
>
> +config MFD_MAX8997
> + bool "Maxim Semiconductor MAX8997/8966 PMIC Support"
> + depends on I2C=y && GENERIC_HARDIRQS
> + select MFD_CORE
> + help
> + Say yes here to support for Maxim Semiconductor MAX8998/8966.
> + This is a Power Management IC with RTC, Flash, Fuel Gauge, Haptic,
> + MUIC controls on chip.
> + This driver provies common support for accessing the device,
> + additional drivers must be enabled in order to use the functionality
> + of the device.
> +
> config MFD_MAX8998
> bool "Maxim Semiconductor MAX8998/National LP3974 PMIC Support"
> depends on I2C=y && GENERIC_HARDIRQS
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 848e7ea..0356b85 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_UCB1400_CORE) += ucb1400_core.o
> obj-$(CONFIG_PMIC_DA903X)+= da903x.o
> max8925-objs := max8925-core.o max8925-i2c.o
> obj-$(CONFIG_MFD_MAX8925)+= max8925.o
> +obj-$(CONFIG_MFD_MAX8997)+= max8997.o
> obj-$(CONFIG_MFD_MAX8998)+= max8998.o max8998-irq.o
>
> pcf50633-objs:= pcf50633-core.o pcf50633-irq.o
> diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
> new file mode 100644
> index 000..ac70501
> --- /dev/null
> +++ b/drivers/mfd/max8997.c
> @@ -0,0 +1,211 @@
> +/*
> + * max8997.c - mfd core driver for the Maxim 8966 and 8997
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * This driver is based on max8998.c
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define I2C_ADDR_PMIC(0xCC >> 1)
> +#define I2C_ADDR_MUIC(0x4A >> 1)
> +#define I2C_ADDR_BATTERY (0x6C >> 1)
> +#define I2C_ADDR_RTC (0x0C >> 1)
> +#define I2C_ADDR_HAPTIC (0x90 >> 1)
> +
> +static struct mfd_cell max8997_devs[] = {
> +#ifdef CONFIG_REGULATOR_MAX8997
I think you can get rid of all the ifdefs below.
If the driver is not selected, you'll be safe anyway.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mfd: i2c: convert SCx200 driver from using raw PCI to platform device
Hi Andres, On Thu, Dec 30, 2010 at 08:27:33PM -0800, Andres Salomon wrote: > Note: this relies on the cs5535-mfd patches that are currently in the > MFD next tree. It's been tested on CS5536 hardware, but I don't have > access to any old SCx200 hardware to verify that I didn't break the ISA > stuff. > Patch applied, thanks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers/mfd: remove unneeded and dangerous clearing of clientdata
Hi Wolfgang, On Mon, May 17, 2010 at 02:10:29PM +0200, Wolfram Sang wrote: > Unlike real i2c-devices which get detached from the driver, dummy-devices get > truly unregistered. So, there has never been a need to clear the clientdata > because the device will go away anyhow. For the occasions fixed here, clearing > clientdata was even dangerous as the structure was freed already. Patch applied, many thanks. Cheers, Samuel. > Signed-off-by: Wolfram Sang > Cc: Jean Delvare > Cc: Samuel Ortiz > Cc: sta...@kernel.org > --- > > Note: While most of the other calls clearing clientdata became superfluous > meanwhile and will be fixed later to remove redundancy, this is a seperate > issue. It was wrong from the beginning and needs to be fixed as it can cause > crashes. Hopefully, during this release-cycle, all other clearings of > clientdata will be removed and we will never see the related confusion again. > > drivers/mfd/88pm860x-i2c.c |1 - > drivers/mfd/max8925-i2c.c |2 -- > 2 files changed, 0 insertions(+), 3 deletions(-) > > diff --git a/drivers/mfd/88pm860x-i2c.c b/drivers/mfd/88pm860x-i2c.c > index 4a6e718..e55f3d2 100644 > --- a/drivers/mfd/88pm860x-i2c.c > +++ b/drivers/mfd/88pm860x-i2c.c > @@ -200,7 +200,6 @@ static int __devexit pm860x_remove(struct i2c_client > *client) > > pm860x_device_exit(chip); > i2c_unregister_device(chip->companion); > - i2c_set_clientdata(chip->companion, NULL); > i2c_set_clientdata(chip->client, NULL); > kfree(chip); > return 0; > diff --git a/drivers/mfd/max8925-i2c.c b/drivers/mfd/max8925-i2c.c > index d9fd878..e73f3f5 100644 > --- a/drivers/mfd/max8925-i2c.c > +++ b/drivers/mfd/max8925-i2c.c > @@ -173,8 +173,6 @@ static int __devexit max8925_remove(struct i2c_client > *client) > max8925_device_exit(chip); > i2c_unregister_device(chip->adc); > i2c_unregister_device(chip->rtc); > - i2c_set_clientdata(chip->adc, NULL); > - i2c_set_clientdata(chip->rtc, NULL); > i2c_set_clientdata(chip->i2c, NULL); > kfree(chip); > return 0; > -- > 1.7.0 > -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] mfd: fix dangling pointers
Hi Wolfram, On Sat, Mar 20, 2010 at 03:12:54PM +0100, Wolfram Sang wrote: > Fix I2C-drivers which missed setting clientdata to NULL before freeing the > structure it points to. Also fix drivers which do this _after_ the structure > was freed already. Patch applied, many thanks. Cheers, Samuel. > Signed-off-by: Wolfram Sang > Cc: Samuel Ortiz > Cc: Mark Brown > --- > > Found using coccinelle, then reviewed. Full patchset is available via > kernel-janitors, linux-i2c, and LKML. > --- > drivers/mfd/88pm860x-i2c.c |1 + > drivers/mfd/ab3100-core.c |2 ++ > drivers/mfd/da903x.c|1 + > drivers/mfd/menelaus.c |3 ++- > drivers/mfd/pcf50633-core.c |1 + > drivers/mfd/tps65010.c |2 +- > drivers/mfd/wm8350-i2c.c|2 ++ > 7 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/88pm860x-i2c.c b/drivers/mfd/88pm860x-i2c.c > index c37e12b..aa81448 100644 > --- a/drivers/mfd/88pm860x-i2c.c > +++ b/drivers/mfd/88pm860x-i2c.c > @@ -201,6 +201,7 @@ static int __devexit pm860x_remove(struct i2c_client > *client) > i2c_unregister_device(chip->companion); > i2c_set_clientdata(chip->companion, NULL); > i2c_set_clientdata(chip->client, NULL); > + i2c_set_clientdata(client, NULL); > kfree(chip); > return 0; > } > diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c > index a2ce3b6..e6fc43f 100644 > --- a/drivers/mfd/ab3100-core.c > +++ b/drivers/mfd/ab3100-core.c > @@ -919,6 +919,7 @@ static int __init ab3100_probe(struct i2c_client *client, > i2c_unregister_device(ab3100->testreg_client); > exit_no_testreg_client: > exit_no_detect: > + i2c_set_clientdata(client, NULL); > kfree(ab3100); > return err; > } > @@ -940,6 +941,7 @@ static int __exit ab3100_remove(struct i2c_client *client) >* their notifiers so deactivate IRQ >*/ > free_irq(client->irq, ab3100); > + i2c_set_clientdata(client, NULL); > kfree(ab3100); > return 0; > } > diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c > index e5ffe56..ec8178c 100644 > --- a/drivers/mfd/da903x.c > +++ b/drivers/mfd/da903x.c > @@ -543,6 +543,7 @@ static int __devexit da903x_remove(struct i2c_client > *client) > struct da903x_chip *chip = i2c_get_clientdata(client); > > da903x_remove_subdevs(chip); > + i2c_set_clientdata(client, NULL); > kfree(chip); > return 0; > } > diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c > index 970afa1..d9e60ba 100644 > --- a/drivers/mfd/menelaus.c > +++ b/drivers/mfd/menelaus.c > @@ -1227,6 +1227,7 @@ fail2: > free_irq(client->irq, menelaus); > flush_scheduled_work(); > fail1: > + i2c_set_clientdata(client, NULL); > kfree(menelaus); > return err; > } > @@ -1236,8 +1237,8 @@ static int __exit menelaus_remove(struct i2c_client > *client) > struct menelaus_chip*menelaus = i2c_get_clientdata(client); > > free_irq(client->irq, menelaus); > - kfree(menelaus); > i2c_set_clientdata(client, NULL); > + kfree(menelaus); > the_menelaus = NULL; > return 0; > } > diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c > index 03dcc92..ab7b4dd 100644 > --- a/drivers/mfd/pcf50633-core.c > +++ b/drivers/mfd/pcf50633-core.c > @@ -675,6 +675,7 @@ static int __devexit pcf50633_remove(struct i2c_client > *client) > for (i = 0; i < PCF50633_NUM_REGULATORS; i++) > platform_device_unregister(pcf->regulator_pdev[i]); > > + i2c_set_clientdata(client, NULL); > kfree(pcf); > > return 0; > diff --git a/drivers/mfd/tps65010.c b/drivers/mfd/tps65010.c > index e595530..9b22a77 100644 > --- a/drivers/mfd/tps65010.c > +++ b/drivers/mfd/tps65010.c > @@ -530,8 +530,8 @@ static int __exit tps65010_remove(struct i2c_client > *client) > cancel_delayed_work(&tps->work); > flush_scheduled_work(); > debugfs_remove(tps->file); > - kfree(tps); > i2c_set_clientdata(client, NULL); > + kfree(tps); > the_tps = NULL; > return 0; > } > diff --git a/drivers/mfd/wm8350-i2c.c b/drivers/mfd/wm8350-i2c.c > index 8d8c932..2dd3e8a 100644 > --- a/drivers/mfd/wm8350-i2c.c > +++ b/drivers/mfd/wm8350-i2c.c > @@ -81,6 +81,7 @@ static int wm8350_i2c_probe(struct i2c_client *i2c, > return ret; > > err: > + i2c_set_clientdata(i2c, NULL); > kfree(wm8350); > return ret; > } > @@ -90,6 +91,7 @@ static int wm8350_i2c_remove(struct i2c_client *i2c) >
Re: [PATCH v4 2/3] i2c: convert i2c-isch to platform_device
Hi Denis,
On Mon, Mar 01, 2010 at 06:59:55PM +0200, Denis Turischev wrote:
> v2: there is no acpi_check_region, it will be implemented in mfd-core
> v3: patch refreshed against the latest Linus tree
> v4: refreshed again
Patch applied, many thanks.
Cheers,
Samuel.
> Signed-off-by: Denis Turischev
>
> ---
> drivers/i2c/busses/Kconfig|2 +-
> drivers/i2c/busses/i2c-isch.c | 68
> 2 files changed, 28 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 5f318ce..d15b6d3 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -104,7 +104,7 @@ config I2C_I801
>
> config I2C_ISCH
> tristate "Intel SCH SMBus 1.0"
> - depends on PCI
> + select LPC_SCH
> help
> Say Y here if you want to use SMBus controller on the Intel SCH
> based systems.
> diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
> index dba6eb0..ddc258e 100644
> --- a/drivers/i2c/busses/i2c-isch.c
> +++ b/drivers/i2c/busses/i2c-isch.c
> @@ -27,7 +27,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -46,12 +46,6 @@
> #define SMBHSTDAT1 (7 + sch_smba)
> #define SMBBLKDAT(0x20 + sch_smba)
>
> -/* count for request_region */
> -#define SMBIOSIZE64
> -
> -/* PCI Address Constants */
> -#define SMBBA_SCH0x40
> -
> /* Other settings */
> #define MAX_TIMEOUT 500
>
> @@ -63,7 +57,6 @@
> #define SCH_BLOCK_DATA 0x05
>
> static unsigned short sch_smba;
> -static struct pci_driver sch_driver;
> static struct i2c_adapter sch_adapter;
>
> /*
> @@ -256,37 +249,23 @@ static struct i2c_adapter sch_adapter = {
> .algo = &smbus_algorithm,
> };
>
> -static struct pci_device_id sch_ids[] = {
> - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
> - { 0, }
> -};
> -
> -MODULE_DEVICE_TABLE(pci, sch_ids);
> -
> -static int __devinit sch_probe(struct pci_dev *dev,
> - const struct pci_device_id *id)
> +static int __devinit smbus_sch_probe(struct platform_device *dev)
> {
> + struct resource *res;
> int retval;
> - unsigned int smba;
>
> - pci_read_config_dword(dev, SMBBA_SCH, &smba);
> - if (!(smba & (1 << 31))) {
> - dev_err(&dev->dev, "SMBus I/O space disabled!\n");
> - return -ENODEV;
> - }
> + res = platform_get_resource(dev, IORESOURCE_IO, 0);
> + if (!res)
> + return -EBUSY;
>
> - sch_smba = (unsigned short)smba;
> - if (sch_smba == 0) {
> - dev_err(&dev->dev, "SMBus base address uninitialized!\n");
> - return -ENODEV;
> - }
> - if (acpi_check_region(sch_smba, SMBIOSIZE, sch_driver.name))
> - return -ENODEV;
> - if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
> + if (!request_region(res->start, resource_size(res), dev->name)) {
> dev_err(&dev->dev, "SMBus region 0x%x already in use!\n",
> sch_smba);
> return -EBUSY;
> }
> +
> + sch_smba = res->start;
> +
> dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
>
> /* set up the sysfs linkage to our parent device */
> @@ -298,37 +277,43 @@ static int __devinit sch_probe(struct pci_dev *dev,
> retval = i2c_add_adapter(&sch_adapter);
> if (retval) {
> dev_err(&dev->dev, "Couldn't register adapter!\n");
> - release_region(sch_smba, SMBIOSIZE);
> + release_region(res->start, resource_size(res));
> sch_smba = 0;
> }
>
> return retval;
> }
>
> -static void __devexit sch_remove(struct pci_dev *dev)
> +static int __devexit smbus_sch_remove(struct platform_device *pdev)
> {
> + struct resource *res;
> if (sch_smba) {
> i2c_del_adapter(&sch_adapter);
> - release_region(sch_smba, SMBIOSIZE);
> + res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + release_region(res->start, resource_size(res));
> sch_smba = 0;
> }
> +
> + return 0;
> }
>
> -static struct pci_driver sch_driver = {
> - .name = "isch_smbus",
> - .id_table = sch_ids,
> - .probe = sch_probe,
> - .remove = __devexit_p(sch_remove),
> +static struct platform_driver smbus_sch_driver = {
> + .driver = {
> + .name = "isch_smbus",
> + .owner = THIS_MODULE,
> + },
> + .probe = smbus_sch_probe,
> + .remove = __devexit_p(smbus_sch_remove),
> };
>
> static int __init i2c_sch_init(void)
> {
> - return pci_register_driver(&sch_driver);
> + return platform_driver_register(&smbus_sch_driver);
> }
>
> static void __exit i2c_sch_exit(void)
> {
> - pci_unregister_driver(&sch_driver);
> + platform_driver_unregister(&smbus_sch_driver
Re: [PATCH v3 2/3] i2c: convert i2c-isch to platform_device
Hi Denis,
On Sun, Feb 21, 2010 at 02:46:58PM +0200, Denis Turischev wrote:
> v2: there is no acpi_check_region, it will be implemented in mfd-core
> v3: patch refreshed against the latest Linus tree
Still failing to apply against Linus' latest tree:
patching file drivers/i2c/busses/Kconfig
Hunk #1 FAILED at 104.
1 out of 1 hunk FAILED -- saving rejects to file
drivers/i2c/busses/Kconfig.rej
patching file drivers/i2c/busses/i2c-isch.c
Hunk #1 FAILED at 27.
Hunk #2 FAILED at 46.
Hunk #3 FAILED at 57.
Hunk #4 FAILED at 249.
Hunk #5 FAILED at 277.
Hunk #6 FAILED at 322.
6 out of 6 hunks FAILED -- saving rejects to file
drivers/i2c/busses/i2c-isch.c.rej
Cheers,
Samuel.
> Signed-off-by: Denis Turischev
> ---
> drivers/i2c/busses/Kconfig|2 +-
> drivers/i2c/busses/i2c-isch.c | 68
> 2 files changed, 28 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 5f318ce..d15b6d3 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -104,7 +104,7 @@ config I2C_I801
>
> config I2C_ISCH
> tristate "Intel SCH SMBus 1.0"
> - depends on PCI
> + select LPC_SCH
> help
> Say Y here if you want to use SMBus controller on the Intel SCH
> based systems.
> diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
> index dba6eb0..ddc258e 100644
> --- a/drivers/i2c/busses/i2c-isch.c
> +++ b/drivers/i2c/busses/i2c-isch.c
> @@ -27,7 +27,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -46,12 +46,6 @@
> #define SMBHSTDAT1 (7 + sch_smba)
> #define SMBBLKDAT(0x20 + sch_smba)
>
> -/* count for request_region */
> -#define SMBIOSIZE64
> -
> -/* PCI Address Constants */
> -#define SMBBA_SCH0x40
> -
> /* Other settings */
> #define MAX_TIMEOUT 500
>
> @@ -63,7 +57,6 @@
> #define SCH_BLOCK_DATA 0x05
>
> static unsigned short sch_smba;
> -static struct pci_driver sch_driver;
> static struct i2c_adapter sch_adapter;
>
> /*
> @@ -256,37 +249,23 @@ static struct i2c_adapter sch_adapter = {
> .algo = &smbus_algorithm,
> };
>
> -static struct pci_device_id sch_ids[] = {
> - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
> - { 0, }
> -};
> -
> -MODULE_DEVICE_TABLE(pci, sch_ids);
> -
> -static int __devinit sch_probe(struct pci_dev *dev,
> - const struct pci_device_id *id)
> +static int __devinit smbus_sch_probe(struct platform_device *dev)
> {
> + struct resource *res;
> int retval;
> - unsigned int smba;
>
> - pci_read_config_dword(dev, SMBBA_SCH, &smba);
> - if (!(smba & (1 << 31))) {
> - dev_err(&dev->dev, "SMBus I/O space disabled!\n");
> - return -ENODEV;
> - }
> + res = platform_get_resource(dev, IORESOURCE_IO, 0);
> + if (!res)
> + return -EBUSY;
>
> - sch_smba = (unsigned short)smba;
> - if (sch_smba == 0) {
> - dev_err(&dev->dev, "SMBus base address uninitialized!\n");
> - return -ENODEV;
> - }
> - if (acpi_check_region(sch_smba, SMBIOSIZE, sch_driver.name))
> - return -ENODEV;
> - if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
> + if (!request_region(res->start, resource_size(res), dev->name)) {
> dev_err(&dev->dev, "SMBus region 0x%x already in use!\n",
> sch_smba);
> return -EBUSY;
> }
> +
> + sch_smba = res->start;
> +
> dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
>
> /* set up the sysfs linkage to our parent device */
> @@ -298,37 +277,43 @@ static int __devinit sch_probe(struct pci_dev *dev,
> retval = i2c_add_adapter(&sch_adapter);
> if (retval) {
> dev_err(&dev->dev, "Couldn't register adapter!\n");
> - release_region(sch_smba, SMBIOSIZE);
> + release_region(res->start, resource_size(res));
> sch_smba = 0;
> }
>
> return retval;
> }
>
> -static void __devexit sch_remove(struct pci_dev *dev)
> +static int __devexit smbus_sch_remove(struct platform_device *pdev)
> {
> + struct resource *res;
> if (sch_smba) {
> i2c_del_adapter(&sch_adapter);
> - release_region(sch_smba, SMBIOSIZE);
> + res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + release_region(res->start, resource_size(res));
> sch_smba = 0;
> }
> +
> + return 0;
> }
>
> -static struct pci_driver sch_driver = {
> - .name = "isch_smbus",
> - .id_table = sch_ids,
> - .probe = sch_probe,
> - .remove = __devexit_p(sch_remove),
> +static struct platform_driver smbus_sch_driver = {
> + .driver = {
> + .name = "isch_smbus",
> + .owner = THIS_MODULE,
> + },
> +
Re: [PATCH v3 1/3] MFD: introduce lpc_sch for Intel SCH LPC bridge
Hi Denis,
On Tue, Feb 23, 2010 at 11:25:59AM +0200, Denis Turischev wrote:
> Hi Samuel,
> Regarding renaming of sch* to isch* do you want incremental patch, or fresh
> version?
>
I'll fix that myself, no worries.
Cheers,
Samuel.
> Denis
>
> Jean Delvare wrote:
> >>+static struct mfd_cell lpc_sch_cells[] = {
> >>+ {
> >>+ .name = "isch_smbus",
> >>+ .num_resources = 1,
> >>+ .resources = &smbus_sch_resource,
> >>+ },
> >>+ {
> >>+ .name = "sch_gpio",
> >>+ .num_resources = 1,
> >>+ .resources = &gpio_sch_resource,
> >>+ },
> >>+};
> >
> >These names are nicely inconsistent. What about "isch_gpio"?
> >
>
> >>+obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> >
> >I don't like this name either. There is another vendor (SMSC) shipping
> >LPC devices with "SCH" in their names, so there is room for confusion.
> >"isch" makes it clearer that we are talking about the Intel ones.
> >
>
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] i2c: convert i2c-isch to platform_device
Hi Jean, On Tue, Feb 23, 2010 at 09:12:21AM +0100, Jean Delvare wrote: > Hi Mike, > > On Tue, 23 Feb 2010 09:00:28 +0200, Mike Rapoport wrote: > > Hi Jean, > > > > Denis Turischev wrote: > > > v2: there is no acpi_check_region, it will be implemented in mfd-core > > > v3: patch refreshed against the latest Linus tree > > > > > > Signed-off-by: Denis Turischev > > > > Any chance this can go to 2.6.34? > > I can add my > > Acked-by: Jean Delvare > > but the patch itself would rather go through Samuel's mfd tree. The > different patches depend on each other so pushing them through > different trees would cause trouble. Exactly. I asked Denis to rebase them against Linus' latest because I was planning to merge them through my tree. I'll take patches 2 and 3 from this patchset. Cheers, Samuel. > -- > Jean Delvare -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] i2c: convert i2c-isch to platform_device
Hi Denis,
On Wed, Feb 17, 2010 at 05:42:21PM +0200, Denis Turischev wrote:
> acpi_check_region will be implemented in mfd-core, therefore v2 version avoids
> this check
>
> Signed-off-by: Denis Turischev
This patch doesnt apply properly against neither my mfd tree nor Linus tree.
Could you refresh it against the latest Linus tree, please ?
Same applies to the GPIO patch, btw.
Cheers,
Samuel.
> --- linux-2.6.33-rc7.orig/drivers/i2c/busses/i2c-isch.c 2010-02-07
> 00:17:12.0 +0200
> +++ linux-2.6.33-rc7/drivers/i2c/busses/i2c-isch.c2010-02-17
> 17:08:53.0 +0200
> @@ -27,7 +27,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -46,12 +46,6 @@
> #define SMBHSTDAT1 (7 + sch_smba)
> #define SMBBLKDAT(0x20 + sch_smba)
>
> -/* count for request_region */
> -#define SMBIOSIZE64
> -
> -/* PCI Address Constants */
> -#define SMBBA_SCH0x40
> -
> /* Other settings */
> #define MAX_TIMEOUT 500
>
> @@ -63,7 +57,6 @@
> #define SCH_BLOCK_DATA 0x05
>
> static unsigned short sch_smba;
> -static struct pci_driver sch_driver;
> static struct i2c_adapter sch_adapter;
>
> /*
> @@ -256,37 +249,23 @@
> .algo = &smbus_algorithm,
> };
>
> -static struct pci_device_id sch_ids[] = {
> - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
> - { 0, }
> -};
> -
> -MODULE_DEVICE_TABLE(pci, sch_ids);
> -
> -static int __devinit sch_probe(struct pci_dev *dev,
> - const struct pci_device_id *id)
> +static int __devinit smbus_sch_probe(struct platform_device *dev)
> {
> + struct resource *res;
> int retval;
> - unsigned int smba;
>
> - pci_read_config_dword(dev, SMBBA_SCH, &smba);
> - if (!(smba & (1 << 31))) {
> - dev_err(&dev->dev, "SMBus I/O space disabled!\n");
> - return -ENODEV;
> - }
> + res = platform_get_resource(dev, IORESOURCE_IO, 0);
> + if (!res)
> + return -EBUSY;
>
> - sch_smba = (unsigned short)smba;
> - if (sch_smba == 0) {
> - dev_err(&dev->dev, "SMBus base address uninitialized!\n");
> - return -ENODEV;
> - }
> - if (acpi_check_region(sch_smba, SMBIOSIZE, sch_driver.name))
> - return -ENODEV;
> - if (!request_region(sch_smba, SMBIOSIZE, sch_driver.name)) {
> + if (!request_region(res->start, resource_size(res), dev->name)) {
> dev_err(&dev->dev, "SMBus region 0x%x already in use!\n",
> sch_smba);
> return -EBUSY;
> }
> +
> + sch_smba = res->start;
> +
> dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
>
> /* set up the sysfs linkage to our parent device */
> @@ -298,37 +277,43 @@
> retval = i2c_add_adapter(&sch_adapter);
> if (retval) {
> dev_err(&dev->dev, "Couldn't register adapter!\n");
> - release_region(sch_smba, SMBIOSIZE);
> + release_region(res->start, resource_size(res));
> sch_smba = 0;
> }
>
> return retval;
> }
>
> -static void __devexit sch_remove(struct pci_dev *dev)
> +static int __devexit smbus_sch_remove(struct platform_device *pdev)
> {
> + struct resource *res;
> if (sch_smba) {
> i2c_del_adapter(&sch_adapter);
> - release_region(sch_smba, SMBIOSIZE);
> + res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + release_region(res->start, resource_size(res));
> sch_smba = 0;
> }
> +
> + return 0;
> }
>
> -static struct pci_driver sch_driver = {
> - .name = "isch_smbus",
> - .id_table = sch_ids,
> - .probe = sch_probe,
> - .remove = __devexit_p(sch_remove),
> +static struct platform_driver smbus_sch_driver = {
> + .driver = {
> + .name = "isch_smbus",
> + .owner = THIS_MODULE,
> + },
> + .probe = smbus_sch_probe,
> + .remove = __devexit_p(smbus_sch_remove),
> };
>
> static int __init i2c_sch_init(void)
> {
> - return pci_register_driver(&sch_driver);
> + return platform_driver_register(&smbus_sch_driver);
> }
>
> static void __exit i2c_sch_exit(void)
> {
> - pci_unregister_driver(&sch_driver);
> + platform_driver_unregister(&smbus_sch_driver);
> }
>
> MODULE_AUTHOR("Jacob Pan ");
> @@ -337,3 +322,4 @@
>
> module_init(i2c_sch_init);
> module_exit(i2c_sch_exit);
> +MODULE_ALIAS("platform:isch_smbus");
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] MFD: introduce lpc_sch for Intel SCH LPC bridge
Hi Denis,
On Thu, Feb 18, 2010 at 08:01:33PM +0200, Denis Turischev wrote:
> Intel Poulsbo (SCH) chipset LPC bridge controller contains several
> functions. Creating and MFD driver for the LPC bridge controller allows
> simultaneous use of SMBus and GPIO interfaces on the SCH.
>
> v2: added "select MFD_CORE"
> v3: removed "default m"
>
> Signed-off-by: Denis Turischev
patch applied, many thanks.
Cheers,
Samuel.
> diff -Nru linux-2.6.33-rc7.orig/drivers/mfd/Kconfig
> linux-2.6.33-rc7/drivers/mfd/Kconfig
> --- linux-2.6.33-rc7.orig/drivers/mfd/Kconfig 2010-02-07 00:17:12.0
> +0200
> +++ linux-2.6.33-rc7/drivers/mfd/Kconfig 2010-02-18 19:56:19.0
> +0200
> @@ -348,6 +348,14 @@
> read/write functions for the devices to get access to this chip.
> This chip embeds various other multimedia funtionalities as well.
>
> +config LPC_SCH
> + tristate "Intel SCH LPC"
> + depends on PCI
> + select MFD_CORE
> + help
> + LPC bridge function of the Intel SCH provides support for
> + System Management Bus and General Purpose I/O.
> +
> endmenu
>
> menu "Multimedia Capabilities Port drivers"
> diff -Nru linux-2.6.33-rc7.orig/drivers/mfd/lpc_sch.c
> linux-2.6.33-rc7/drivers/mfd/lpc_sch.c
> --- linux-2.6.33-rc7.orig/drivers/mfd/lpc_sch.c 1970-01-01
> 02:00:00.0 +0200
> +++ linux-2.6.33-rc7/drivers/mfd/lpc_sch.c2010-02-11 10:31:54.0
> +0200
> @@ -0,0 +1,133 @@
> +/*
> + * lpc_sch.c - LPC interface for Intel Poulsbo SCH
> + *
> + * LPC bridge function of the Intel SCH contains many other
> + * functional units, such as Interrupt controllers, Timers,
> + * Power Management, System Management, GPIO, RTC, and LPC
> + * Configuration Registers.
> + *
> + * Copyright (c) 2010 CompuLab Ltd
> + * Author: Denis Turischev
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; see the file COPYING. If not, write to
> + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SMBASE 0x40
> +#define SMBUS_IO_SIZE64
> +
> +#define GPIOBASE 0x44
> +#define GPIO_IO_SIZE 64
> +
> +static struct resource smbus_sch_resource = {
> + .flags = IORESOURCE_IO,
> +};
> +
> +
> +static struct resource gpio_sch_resource = {
> + .flags = IORESOURCE_IO,
> +};
> +
> +static struct mfd_cell lpc_sch_cells[] = {
> + {
> + .name = "isch_smbus",
> + .num_resources = 1,
> + .resources = &smbus_sch_resource,
> + },
> + {
> + .name = "sch_gpio",
> + .num_resources = 1,
> + .resources = &gpio_sch_resource,
> + },
> +};
> +
> +static struct pci_device_id lpc_sch_ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
> + { 0, }
> +};
> +MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> +
> +static int __devinit lpc_sch_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> + unsigned int base_addr_cfg;
> + unsigned short base_addr;
> +
> + pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> + if (!(base_addr_cfg & (1 << 31))) {
> + dev_err(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> + return -ENODEV;
> + }
> + base_addr = (unsigned short)base_addr_cfg;
> + if (base_addr == 0) {
> + dev_err(&dev->dev, "I/O space for SMBus uninitialized\n");
> + return -ENODEV;
> + }
> +
> + smbus_sch_resource.start = base_addr;
> + smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> +
> + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> + if (!(base_addr_cfg & (1 << 31))) {
> + dev_err(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> + return -ENODEV;
> + }
> + base_addr = (unsigned short)base_addr_cfg;
> + if (base_addr == 0) {
> + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> + return -ENODEV;
> + }
> +
> + gpio_sch_resource.start = base_addr;
> + gpio_sch_resource.end = base_addr + GPIO_IO_SIZE - 1;
> +
> + return mfd_add_devices(&dev->dev, -1,
> + lpc_sch_cells, ARRAY_SIZE(lpc_sch_cells), NULL, 0);
> +}
> +
> +static void __devexit lpc_sch_remove(struct pci_dev *dev)
> +{
> + mfd_rem
Re: [PATCH 1/3] MFD: introduce lpc_sch for Intel SCH LPC bridge
Hi Mike, On Wed, Feb 17, 2010 at 02:35:30PM +0200, Mike Rapoport wrote: > Samuel, > > Jean Delvare wrote: > > On Wed, 17 Feb 2010 12:03:17 +0200, Denis Turischev wrote: > >> Jean Delvare wrote: > >>> Might be a good idea to use acpi_check_resource_conflict() or similar > >>> before instantiating the platform devices. > >> May be it worth to add such resource check directly to mfd_add_device > >> function? > > > > I'm not sure. I suspect that many MFD devices are never used on > > ACPI-aware systems, so it might be considered overkill. OTOH the calls > > resolve to empty stubs when ACPI is disabled so... I have no objection, > > but I'll leave the decision to somebody else ;) > > > > What do you think? Shall we add something like mfd_verify_resources that will > call > acpi_check_region or something similar? Yes, that sounds like a reasonable idea. We should probably call acpi_check_resource_conflict() straight from mfd_add_device(). I'll do that, no need for Denis to add that patch for its code to be merged. Cheers, Samuel. > > -- > Sincerely yours, > Mike. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] MFD: introduce lpc_sch for Intel SCH LPC bridge
Hi Denis,
On Thu, Feb 11, 2010 at 12:26:19PM +0200, Denis Turischev wrote:
> Intel Poulsbo (SCH) chipset LPC bridge controller contains several
> functions. Creating and MFD driver for the LPC bridge controller allows
> simultaneous use of SMBus and GPIO interfaces on the SCH.
That looks like an nice patch to me. Before merging it, I'd like to get
Jacob's view on it though. Jacob, does moving the SCH SMBus driver to a
platform one look fine to you ?
Cheers,
Samuel.
> Signed-off-by: Denis Turischev
>
> diff -Nru linux-2.6.33-rc7.orig/drivers/mfd/Kconfig
> linux-2.6.33-rc7/drivers/mfd/Kconfig
> --- linux-2.6.33-rc7.orig/drivers/mfd/Kconfig 2010-02-07 00:17:12.0
> +0200
> +++ linux-2.6.33-rc7/drivers/mfd/Kconfig 2010-02-10 15:15:40.0
> +0200
> @@ -348,6 +348,14 @@
> read/write functions for the devices to get access to this chip.
> This chip embeds various other multimedia funtionalities as well.
>
> +config LPC_SCH
> + tristate "Intel SCH LPC"
> + default m
> + depends on PCI
> + help
> + LPC bridge function of the Intel SCH provides support for
> + System Management Bus and General Purpose I/O.
> +
> endmenu
>
> menu "Multimedia Capabilities Port drivers"
> diff -Nru linux-2.6.33-rc7.orig/drivers/mfd/lpc_sch.c
> linux-2.6.33-rc7/drivers/mfd/lpc_sch.c
> --- linux-2.6.33-rc7.orig/drivers/mfd/lpc_sch.c 1970-01-01
> 02:00:00.0 +0200
> +++ linux-2.6.33-rc7/drivers/mfd/lpc_sch.c2010-02-11 10:31:54.0
> +0200
> @@ -0,0 +1,133 @@
> +/*
> + * lpc_sch.c - LPC interface for Intel Poulsbo SCH
> + *
> + * LPC bridge function of the Intel SCH contains many other
> + * functional units, such as Interrupt controllers, Timers,
> + * Power Management, System Management, GPIO, RTC, and LPC
> + * Configuration Registers.
> + *
> + * Copyright (c) 2010 CompuLab Ltd
> + * Author: Denis Turischev
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; see the file COPYING. If not, write to
> + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SMBASE 0x40
> +#define SMBUS_IO_SIZE64
> +
> +#define GPIOBASE 0x44
> +#define GPIO_IO_SIZE 64
> +
> +static struct resource smbus_sch_resource = {
> + .flags = IORESOURCE_IO,
> +};
> +
> +
> +static struct resource gpio_sch_resource = {
> + .flags = IORESOURCE_IO,
> +};
> +
> +static struct mfd_cell lpc_sch_cells[] = {
> + {
> + .name = "isch_smbus",
> + .num_resources = 1,
> + .resources = &smbus_sch_resource,
> + },
> + {
> + .name = "sch_gpio",
> + .num_resources = 1,
> + .resources = &gpio_sch_resource,
> + },
> +};
> +
> +static struct pci_device_id lpc_sch_ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
> + { 0, }
> +};
> +MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> +
> +static int __devinit lpc_sch_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> + unsigned int base_addr_cfg;
> + unsigned short base_addr;
> +
> + pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> + if (!(base_addr_cfg & (1 << 31))) {
> + dev_err(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> + return -ENODEV;
> + }
> + base_addr = (unsigned short)base_addr_cfg;
> + if (base_addr == 0) {
> + dev_err(&dev->dev, "I/O space for SMBus uninitialized\n");
> + return -ENODEV;
> + }
> +
> + smbus_sch_resource.start = base_addr;
> + smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> +
> + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> + if (!(base_addr_cfg & (1 << 31))) {
> + dev_err(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> + return -ENODEV;
> + }
> + base_addr = (unsigned short)base_addr_cfg;
> + if (base_addr == 0) {
> + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> + return -ENODEV;
> + }
> +
> + gpio_sch_resource.start = base_addr;
> + gpio_sch_resource.end = base_addr + GPIO_IO_SIZE - 1;
> +
> + return mfd_add_devices(&dev->dev, -1,
> + lpc_sch_cells, ARRAY_SIZE(lpc_sch_cells), NULL, 0);
> +}
>
Re: [PATCH v3] [MFD] i2c-htcpld: Add the HTCPLD driver
Hi Jean, On Tue, Jan 19, 2010 at 12:48:12PM +0100, Jean Delvare wrote: > Hi Samuel, > > On Tue, 19 Jan 2010 12:09:17 +0100, Samuel Ortiz wrote: > > 2) Kconfig: we need to depend on I2C=y, to avoid the problematic case of > > having I2C_CORE=m and HTC_I2CPLD=y > > ?? > > There is no config option named I2C_CORE. The main option for I2C is, > well, I2C, Sorry, I meant I2C, not I2C_CORE. > and if you depend on I2C, then you can't be built into the > kernel if I2C is built as a module. I have: drivers/mfd/Kconfig: config HTC_I2CPLD bool "HTC I2C PLD chip support" depends on I2C Then make menuconfig allows me to build a .config with CONFIG_I2C=m and CONFIG_HTC_I2CPLD=y and that obviously fails to build. If I'm a bool depending on I2C, it has to be a built-in I2C. Cheers, Samuel. > What exact problem are you trying to solve? > > -- > Jean Delvare -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] [MFD] i2c-htcpld: Add the HTCPLD driver
Hi Cory, On Mon, Jan 18, 2010 at 01:23:25PM -0800, Cory Maccarrone wrote: > On Mon, Jan 18, 2010 at 7:36 AM, Samuel Ortiz wrote: > > On Mon, Jan 18, 2010 at 01:48:55PM +0100, Samuel Ortiz wrote: > >> Hi Cory, > >> > >> I applied this patch, thanks a lot. > >> In the future, I'd like to see the GPIO handling from this patch moved to > >> drivers/gpio. Could you please look at that ? > > Absolutely, thanks for applying it! I'll send out a follow-on patch > as soon as I've got that change. > > > I forgot to add that I also made it build on non ARM patforms. This line: > > set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); > > became: > > #ifdef CONFIG_ARM > > set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); > > #else > > set_irq_probe(irq); > > #endif > > > > Ah, cool, thanks! After the build failure reports from linux-next, I also had to fix a couple more things: 1) htcpld_set_type(): You're not supposed to access irq_desc directly, you should use irq_to_desc() instead 2) Kconfig: we need to depend on I2C=y, to avoid the problematic case of having I2C_CORE=m and HTC_I2CPLD=y Please let me know if you're not ok with those changes. You can grab them from the for-next branch of my kernel.org tree, at git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git for-next Cheers, Samuel. > - Cory -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] [MFD] i2c-htcpld: Add the HTCPLD driver
On Mon, Jan 18, 2010 at 01:48:55PM +0100, Samuel Ortiz wrote: > Hi Cory, > > On Sat, Jan 09, 2010 at 08:55:54AM -0800, Cory Maccarrone wrote: > > This change introduces a driver for the HTC PLD chip found > > on some smartphones, such as the HTC Wizard and HTC Herald. > > It works through the I2C bus and acts as a GPIO extender. > > Specifically: > > > > * it can have several sub-devices, each with its own I2C > >address > > * Each sub-device provides 8 output and 8 input pins > > * The chip attaches to one GPIO to signal when any of the > >input GPIOs change -- at which point all chips must be > >scanned for changes > > > > This driver implements the GPIOs throught the kernel's > > GPIO and IRQ framework. This allows any GPIO-servicing > > drivers to operate on htcpld pins, such as the gpio-keys > > and gpio-leds drivers. > I applied this patch, thanks a lot. > In the future, I'd like to see the GPIO handling from this patch moved to > drivers/gpio. Could you please look at that ? I forgot to add that I also made it build on non ARM patforms. This line: set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); became: #ifdef CONFIG_ARM set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); #else set_irq_probe(irq); #endif Cheers, Samuel. > Cheers, > Samuel. > > > > Signed-off-by: Cory Maccarrone > > --- > > drivers/mfd/Kconfig |9 + > > drivers/mfd/Makefile |1 + > > drivers/mfd/htc-i2cpld.c | 698 > > ++ > > include/linux/htcpld.h | 24 ++ > > 4 files changed, 732 insertions(+), 0 deletions(-) > > create mode 100644 drivers/mfd/htc-i2cpld.c > > create mode 100644 include/linux/htcpld.h > > > > Differences from v2 to v3 include various cleanups, including > > splitting out the various parts of the probe() function into > > subfunctions for readability and fixing checkpatch errors. > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 8782978..0dec97c 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -68,6 +68,15 @@ config HTC_PASIC3 > > HTC Magician devices, respectively. Actual functionality is > > handled by the leds-pasic3 and ds1wm drivers. > > > > +config HTC_I2CPLD > > + bool "HTC I2C PLD chip support" > > + depends on I2C > > + help > > + If you say yes here you get support for the supposed CPLD > > + found on omap850 HTC devices like the HTC Wizard and HTC Herald. > > + This device provides input and output GPIOs through an I2C > > + interface to one or more sub-chips. > > + > > config UCB1400_CORE > > tristate "Philips UCB1400 Core driver" > > depends on AC97_BUS > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index ca2f2c4..c7a295a 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_SH_MOBILE_SDHI)+= > > sh_mobile_sdhi.o > > > > obj-$(CONFIG_HTC_EGPIO)+= htc-egpio.o > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o > > +obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o > > > > obj-$(CONFIG_MFD_DM355EVM_MSP) += dm355evm_msp.o > > > > diff --git a/drivers/mfd/htc-i2cpld.c b/drivers/mfd/htc-i2cpld.c > > new file mode 100644 > > index 000..7a92e54 > > --- /dev/null > > +++ b/drivers/mfd/htc-i2cpld.c > > @@ -0,0 +1,698 @@ > > +/* > > + * htc-i2cpld.c > > + * Chip driver for an unknown CPLD chip found on omap850 HTC devices like > > + * the HTC Wizard and HTC Herald. > > + * The cpld is located on the i2c bus and acts as an input/output GPIO > > + * extender. > > + * > > + * Copyright (C) 2009 Cory Maccarrone > > + * > > + * Based on work done in the linwizard project > > + * Copyright (C) 2008-2009 Angelo Arrifano > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * Y
Re: [PATCH v3] [MFD] i2c-htcpld: Add the HTCPLD driver
Hi Cory,
On Sat, Jan 09, 2010 at 08:55:54AM -0800, Cory Maccarrone wrote:
> This change introduces a driver for the HTC PLD chip found
> on some smartphones, such as the HTC Wizard and HTC Herald.
> It works through the I2C bus and acts as a GPIO extender.
> Specifically:
>
> * it can have several sub-devices, each with its own I2C
>address
> * Each sub-device provides 8 output and 8 input pins
> * The chip attaches to one GPIO to signal when any of the
>input GPIOs change -- at which point all chips must be
>scanned for changes
>
> This driver implements the GPIOs throught the kernel's
> GPIO and IRQ framework. This allows any GPIO-servicing
> drivers to operate on htcpld pins, such as the gpio-keys
> and gpio-leds drivers.
I applied this patch, thanks a lot.
In the future, I'd like to see the GPIO handling from this patch moved to
drivers/gpio. Could you please look at that ?
Cheers,
Samuel.
> Signed-off-by: Cory Maccarrone
> ---
> drivers/mfd/Kconfig |9 +
> drivers/mfd/Makefile |1 +
> drivers/mfd/htc-i2cpld.c | 698
> ++
> include/linux/htcpld.h | 24 ++
> 4 files changed, 732 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mfd/htc-i2cpld.c
> create mode 100644 include/linux/htcpld.h
>
> Differences from v2 to v3 include various cleanups, including
> splitting out the various parts of the probe() function into
> subfunctions for readability and fixing checkpatch errors.
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8782978..0dec97c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -68,6 +68,15 @@ config HTC_PASIC3
> HTC Magician devices, respectively. Actual functionality is
> handled by the leds-pasic3 and ds1wm drivers.
>
> +config HTC_I2CPLD
> + bool "HTC I2C PLD chip support"
> + depends on I2C
> + help
> + If you say yes here you get support for the supposed CPLD
> + found on omap850 HTC devices like the HTC Wizard and HTC Herald.
> + This device provides input and output GPIOs through an I2C
> + interface to one or more sub-chips.
> +
> config UCB1400_CORE
> tristate "Philips UCB1400 Core driver"
> depends on AC97_BUS
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index ca2f2c4..c7a295a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_SH_MOBILE_SDHI) +=
> sh_mobile_sdhi.o
>
> obj-$(CONFIG_HTC_EGPIO) += htc-egpio.o
> obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
> +obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o
>
> obj-$(CONFIG_MFD_DM355EVM_MSP) += dm355evm_msp.o
>
> diff --git a/drivers/mfd/htc-i2cpld.c b/drivers/mfd/htc-i2cpld.c
> new file mode 100644
> index 000..7a92e54
> --- /dev/null
> +++ b/drivers/mfd/htc-i2cpld.c
> @@ -0,0 +1,698 @@
> +/*
> + * htc-i2cpld.c
> + * Chip driver for an unknown CPLD chip found on omap850 HTC devices like
> + * the HTC Wizard and HTC Herald.
> + * The cpld is located on the i2c bus and acts as an input/output GPIO
> + * extender.
> + *
> + * Copyright (C) 2009 Cory Maccarrone
> + *
> + * Based on work done in the linwizard project
> + * Copyright (C) 2008-2009 Angelo Arrifano
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct htcpld_chip {
> + spinlock_t lock;
> +
> + /* chip info */
> + u8 reset;
> + u8 addr;
> + struct device *dev;
> + struct i2c_client *client;
> +
> + /* Output details */
> + u8 cache_out;
> + struct gpio_chipchip_out;
> +
> + /* Input details */
> + u8 cache_in;
> + struct gpio_chipchip_in;
> +
> + u16 irqs_enabled;
> + uintirq_start;
> + int nirqs;
> +
> + /*
> + * Work structure to allow for setting values outside of any
> + * possible interrupt context
> + */
> + struct work_struct set_val_work;
> +};
> +
> +struct
Re: [PATCH v3] [MFD] i2c-htcpld: Add the HTCPLD driver
Hi Cory,
late review, sorry about that...
On Mon, Dec 14, 2009 at 05:38:55PM -0800, Cory Maccarrone wrote:
> This change introduces a driver for the HTC PLD chip found
> on some smartphones, such as the HTC Wizard and HTC Herald.
> It works through the I2C bus and acts as a GPIO extender.
> Specifically:
>
> * it can have several sub-devices, each with its own I2C
>address
> * Each sub-device provides 8 output and 8 input pins
> * The chip attaches to one GPIO to signal when any of the
>input GPIOs change -- at which point all chips must be
>scanned for changes
>
> This driver implements the GPIOs throught the kernel's
> GPIO and IRQ framework. This allows any GPIO-servicing
> drivers to operate on htcpld pins, such as the gpio-keys
> and gpio-leds drivers.
The driver looks fine to me, but I get 23 checkpatch warnings and 5 errors for
it.
Could you please fix them, keeping in mind that I'm fine with printk/dev_*
strings being more than 80 chars.
A couple of additional comments:
> diff --git a/drivers/mfd/htc-i2cpld.c b/drivers/mfd/htc-i2cpld.c
> new file mode 100644
> index 000..23338ff
> --- /dev/null
> +++ b/drivers/mfd/htc-i2cpld.c
> @@ -0,0 +1,591 @@
> +/*
> + * htc-i2cpld.c
> + * Chip driver for a ?cpld? found on omap850 HTC devices like the
?cpld? ??
> +static irqreturn_t htcpld_handler(int irq, void *dev)
> +{
> + struct htcpld_data *htcpld = dev;
> + unsigned int i;
> + unsigned long flags;
> + int irqpin;
> + struct irq_desc *desc;
> +
> + if (!htcpld) {
> + printk(KERN_INFO "htcpld is null\n");
Please use pr_* instead. It seems you're using it already, so let's be
consistent.
> +static int __devinit htcpld_core_probe(struct platform_device *pdev)
> +{
This routine is fairly big, and could be more readable by calling some
subroutines. The chips initialisation part could be extracted out for example.
> + struct htcpld_data *htcpld;
> + struct device *dev = &pdev->dev;
> + struct htcpld_core_platform_data *pdata;
> + struct resource *res;
> + int i, ret = 0;
> + unsigned int irq, irq_end;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + pdata = dev->platform_data;
> + if (!pdata) {
> + printk(KERN_ERR "Platform data not found for htcpld core!\n");
> + return -ENXIO;
> + }
> +
> + htcpld = kzalloc(sizeof(struct htcpld_data), GFP_KERNEL);
> + if (!htcpld)
> + return -ENOMEM;
> +
> + /* Find chained irq */
> + ret = -EINVAL;
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (res)
> + htcpld->chained_irq = res->start;
> +
> + platform_set_drvdata(pdev, htcpld);
> +
> + htcpld->int_reset_gpio_hi = pdata->int_reset_gpio_hi;
> + htcpld->int_reset_gpio_lo = pdata->int_reset_gpio_lo;
> +
> + /* Setup each chip's output GPIOs */
> + htcpld->nchips = pdata->num_chip;
> + htcpld->chip = kzalloc(sizeof(struct htcpld_chip) * htcpld->nchips,
> GFP_KERNEL);
> + if (!htcpld->chip) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + for (i = 0; i < htcpld->nchips; i++) {
> + struct i2c_adapter *adapter;
> + struct i2c_client *client;
> + struct i2c_board_info info;
> + struct gpio_chip *chip;
> + int ret;
> +
> + /* Setup the HTCPLD chips */
> + htcpld->chip[i].reset = pdata->chip[i].reset;
> + htcpld->chip[i].cache_out = pdata->chip[i].reset;
> + htcpld->chip[1].cache_in = 0;
Shouldnt it be: htcpld->chip[i].cache_in = 0; instead ?
> + htcpld->chip[i].dev = dev;
> + htcpld->chip[i].irq_start = pdata->chip[i].irq_base;
> + htcpld->chip[i].nirqs = pdata->chip[i].num_irqs;
> +
> + INIT_WORK(&(htcpld->chip[i].set_val_work), &htcpld_chip_set_ni);
> + spin_lock_init(&(htcpld->chip[i].lock));
> +
> + /* Setup the IRQs */
> + if (htcpld->chained_irq) {
> + int error = 0, flags;
> +
> + /* Setup irq handlers */
> + irq_end = htcpld->chip[i].irq_start +
> htcpld->chip[i].nirqs;
> + for (irq = htcpld->chip[i].irq_start; irq < irq_end;
> irq++) {
> + set_irq_chip(irq, &htcpld_muxed_chip);
> + set_irq_chip_data(irq, &htcpld->chip[i]);
> + set_irq_handler(irq, handle_simple_irq);
> + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> + }
> +
> + flags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING |
> IRQF_SAMPLE_RANDOM;
> + error = request_threaded_irq(
> + htcpld->chained_irq,
> + NULL,
> + htcpld_handler,
> +
Re: [PATCH 1/1] MFD: Add U300 AB3100 core support v5
Hi Linus,
On Thu, May 21, 2009 at 11:17:06PM +0200, Linus Walleij wrote:
> This adds a core driver for the AB3100 mixed-signal circuit
> found in the ST-Ericsson U300 series platforms. This driver
> is a singleton proxy for all accesses to the AB3100
> sub-drivers which will be merged on top of this one, RTC,
> regulators, battery and system power control, vibrator,
> LEDs, and an ALSA codec.
I'm happy with this one, thanks a lot.
Applied to my for-next branch.
Cheers,
Samuel.
> Signed-off-by: Linus Walleij
> Reviewed-by: Mike Rapoport
> Reviewed-by: Samuel Ortiz
> Reviewed-by: Ben Dooks
> ---
> drivers/mfd/Kconfig| 14 +
> drivers/mfd/Makefile |3 +-
> drivers/mfd/ab3100-core.c | 991
>
> include/linux/mfd/ab3100.h | 103 +
> 4 files changed, 1110 insertions(+), 1 deletions(-)
> create mode 100755 drivers/mfd/ab3100-core.c
> create mode 100644 include/linux/mfd/ab3100.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ee3927a..61f0346 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -241,6 +241,20 @@ config PCF50633_GPIO
>Say yes here if you want to include support GPIO for pins on
>the PCF50633 chip.
>
> +config AB3100_CORE
> + tristate "ST-Ericsson AB3100 Mixed Signal Circuit core functions"
> + depends on I2C
> + default y if ARCH_U300
> + help
> + Select this to enable the AB3100 Mixed Signal IC core
> + functionality. This connects to a AB3100 on the I2C bus
> + and expose a number of symbols needed for dependent devices
> + to read and write registers and subscribe to events from
> + this multi-functional IC. This is needed to use other features
> + of the AB3100 such as battery-backed RTC, charging control,
> + LEDs, vibrator, system power and temperature, power management
> + and ALSA sound.
> +
> endmenu
>
> menu "Multimedia Capabilities Port drivers"
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 3afb519..f5f3371 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -40,4 +40,5 @@ obj-$(CONFIG_PMIC_DA903X) += da903x.o
>
> obj-$(CONFIG_MFD_PCF50633) += pcf50633-core.o
> obj-$(CONFIG_PCF50633_ADC) += pcf50633-adc.o
> -obj-$(CONFIG_PCF50633_GPIO) += pcf50633-gpio.o
> \ No newline at end of file
> +obj-$(CONFIG_PCF50633_GPIO) += pcf50633-gpio.o
> +obj-$(CONFIG_AB3100_CORE)+= ab3100-core.o
> diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c
> new file mode 100755
> index 000..13e7d7b
> --- /dev/null
> +++ b/drivers/mfd/ab3100-core.c
> @@ -0,0 +1,991 @@
> +/*
> + * Copyright (C) 2007-2009 ST-Ericsson
> + * License terms: GNU General Public License (GPL) version 2
> + * Low-level core for exclusive access to the AB3100 IC on the I2C bus
> + * and some basic chip-configuration.
> + * Author: Linus Walleij
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* These are the only registers inside AB3100 used in this main file */
> +
> +/* Interrupt event registers */
> +#define AB3100_EVENTA1 0x21
> +#define AB3100_EVENTA2 0x22
> +#define AB3100_EVENTA3 0x23
> +
> +/* AB3100 DAC converter registers */
> +#define AB3100_DIS 0x00
> +#define AB3100_D0C 0x01
> +#define AB3100_D1C 0x02
> +#define AB3100_D2C 0x03
> +#define AB3100_D3C 0x04
> +
> +/* Chip ID register */
> +#define AB3100_CID 0x20
> +
> +/* AB3100 interrupt registers */
> +#define AB3100_IMRA1 0x24
> +#define AB3100_IMRA2 0x25
> +#define AB3100_IMRA3 0x26
> +#define AB3100_IMRB1 0x2B
> +#define AB3100_IMRB2 0x2C
> +#define AB3100_IMRB3 0x2D
> +
> +/* System Power Monitoring and control registers */
> +#define AB3100_MCA 0x2E
> +#define AB3100_MCB 0x2F
> +
> +/* SIM power up */
> +#define AB3100_SUP 0x50
> +
> +/*
> + * I2C communication
> + *
> + * The AB3100 is usually assigned address 0x48 (7-bit)
> + * The chip is defined in the platform i2c_board_data section.
> + */
> +static unsigned short normal_i2c[] = { 0x48, I2C_CLIENT_END };
> +I2C_CLIENT_INSMOD_1(ab3100);
> +
> +u8 ab3100_get_chip_type(struct ab3100 *ab3100)
> +{
> + u8 chip = ABUNKNOWN;
> +
> + switch (ab3100->chip_id & 0xf0) {
> + case 0xa0:
> + chi
Re: [PATCH 1/1] MFD: Add U300 AB3100 core support v4
Hi Linus,
On Wed, May 20, 2009 at 11:17:18AM +0200, Linus Walleij wrote:
> This adds a core driver for the AB3100 mixed-signal circuit
> found in the ST-Ericsson U300 series platforms. This driver
> is a singleton proxy for all accesses to the AB3100
> sub-drivers which will be merged on top of this one, RTC,
> regulators, battery and system power control, vibrator,
> LEDs, and an ALSA codec.
This one looks much better, I still have some comments though:
> +
> +/* A local all-containing singleton */
> +static struct ab3100 *ab3100_local;
I dont really like that, but if you insist on having a unique ab3100 instance
for your driver (instead of allocating one with every probe call and passing
it as an i2c client data pointer), I could still ACK it. However:
> +static int ab3100_set_test_register(u8 reg, u8 regval)
> +{
> + u8 regandval[2] = {reg, regval};
> + int err;
> +
> + err = mutex_lock_interruptible(&ab3100_local->access_mutex);
...that I wouldnt accept.
ab3100_set_test_register should be generic enough and have an ab3100 pointer
as its first parameter. It's called from ab3100_setup(), to which you can
passe the newly allocated ab3100.
There are several routines in your driver that rely on the existence of your
ab3100_local pointer. Let's go through them:
> +/* Interrupt handling worker */
> +static void ab3100_work(struct work_struct *work)
> +{
> + u8 event_regs[3];
> + u32 fatevent;
> + int err;
struct ab3100 *ab3100 = container_of(work, struct ab3100, work);
and you dont have to reference your ab3100_local pointer anymore.
> +static irqreturn_t ab3100_irq_handler(int irq, void *data)
> +{
struct ab3100 *ab3100 = (struct ab3100 *)data;
you're even passing the ab3100 pointer to request_irq, so it's all set
already.
> + */
> +static int ab3100_registers_print(struct seq_file *s, void *p)
> +{
> + u8 value;
> + u8 reg;
> +
> + seq_printf(s, "AB3100 registers:\n");
> +
> + for (reg = 0; reg < 0xff; reg++) {
> + ab3100_get_register(ab3100_local, reg, &value);
You could pass the probe() time allocated pointer to your debugfs_create_*()
calls, and then fetch it back here.
Same applies to all your debugfs code below.
> +static void ab3100_setup_debugfs(void)
> +{
This one needs to take a struct ab3100 * as an input.
> +static int __init ab3100_setup(void)
> +{
Ditto.
> +static int __init ab3100_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + int err;
> + int i;
> +
> + ab3100_local = kzalloc(sizeof(struct ab3100), GFP_KERNEL);
> + if (!ab3100_local) {
> + dev_err(&client->dev, "could not allocate AB3100 device\n");
> + return -ENOMEM;
> + }
> +
> + /* Initialize data structure */
> + mutex_init(&ab3100_local->access_mutex);
> + BLOCKING_INIT_NOTIFIER_HEAD(&ab3100_local->event_subscribers);
i2c_set_clientdata(client, ab3100); and you can get rid of your static
ab3100_local declaration.
> + ab3100_local->i2c_client = client;
> + ab3100_local->dev = &ab3100_local->i2c_client->dev;
> +
> + /* Read chip ID register */
> + err = ab3100_get_register(ab3100_local, AB3100_CID,
> + &ab3100_local->chip_id);
> + if (err) {
> + dev_err(&client->dev,
> + "could not communicate with the AB3100 analog "
> + "baseband chip\n");
> + goto exit_no_detect;
> + }
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] MFD: Add U300 AB3100 core support v1
Hi Linus,
Some code comments here:
On Thu, May 14, 2009 at 10:29:02AM +0200, Linus Walleij wrote:
> +/*
> + * I2C communication
> + *
> + * The AB3100 is assigned address 0x48 (7-bit)
> + * Since the driver does not require SMBus support, we
> + * cannot be sure to probe for the device address here,
> + * so the boot loader or modprobe may need to pass in a parameter
> + * like ab3100-core.force=0,0x48.
> + *
> + */
> +static unsigned short normal_i2c[] = { 0x48, I2C_CLIENT_END };
> +I2C_CLIENT_INSMOD_1(ab3100);
> +
> +/* Whether the chip has been initialized */
> +static bool ab3100_initialized;
> +/* Lock out concurrent accesses to the AB3100 registers */
> +static DEFINE_MUTEX(ab3100_access_mutex);
> +/* Self describing stuff */
> +static struct i2c_client *ab3100_i2c_client;
> +static char ab3100_chip_name[32];
> +static u8 ab3100_chip_id;
All those static definitions dont look good to me.
Typically, one would define a struct ab3100 structure containing all of those.
Then, at device probe time you dynamically allocate one of those structure,
and linke your i2c client to it through i2c_set_clientdata().
This would make your driver look much better.
> +/* Event handling */
> +struct event {
> + struct list_head node;
> + struct device *dev;
> + struct ab3100_event_mask event_mask;
> + void *client_data;
> + void (*cb_handler)(struct ab3100_event_mask, void *client_data);
> +};
> +/* The event list */
> +static DEFINE_MUTEX(event_list_mutex);
> +static LIST_HEAD(subscribers);
> +/* Save the first reading of the event registers */
> +static struct ab3100_event_mask startup_event_mask;
> +static bool startup_events_read;
> +
> +u8 ab3100_get_chip_type()
> +{
> + u8 chip = ABUNKNOWN;
> +
> + switch (ab3100_chip_id & 0xf0) {
> + case 0xa0:
> + chip = AB3000;
> + break;
> + case 0xc0:
> + chip = AB3100;
> + break;
> + }
> + return chip;
> +}
> +EXPORT_SYMBOL(ab3100_get_chip_type);
> +
> +int ab3100_set_register(u8 reg, u8 regval)
> +{
> + u8 regandval[2] = {reg, regval};
> + struct i2c_msg msgs[1];
> + int err = 0;
> +
> + if (!ab3100_initialized)
> + return -ENODEV;
> +
> + msgs[0].addr = ab3100_i2c_client->addr;
> + msgs[0].flags = 0;
> + msgs[0].len = 2;
> + msgs[0].buf = regandval;
> + err = mutex_lock_interruptible(&ab3100_access_mutex);
> + if (err)
> + return err;
> +
> + /*
> + * A two-byte write message with the first byte containing the register
> + * number and the second byte containing the value to be written
> + * effectively sets a register in the AB3100.
> + */
> + if ((i2c_transfer(ab3100_i2c_client->adapter,
> + &msgs[0], 1)) != 1) {
> + dev_err(&ab3100_i2c_client->dev,
> + "%s: write error (write register)\n",
> + __func__);
> + err = -EIO;
> + }
> + mutex_unlock(&ab3100_access_mutex);
> + return err;
> +}
> +EXPORT_SYMBOL(ab3100_set_register);
All those register access routines are accessible from anywhere. By
dynamically allocating your driver structure and registering its subdevices as
platform devices, you could restrict the usage of those routines to the
subdevices only.
> +static struct file_operations ab3100_registers_fops = {
> + .open = ab3100_registers_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> + .owner = THIS_MODULE,
> +};
This one should be const.
> +static int __init ab3100_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + int err;
> + int i;
> +
> + ab3100_i2c_client = client;
> + ab3100_initialized = true;
> +
> + /* Read chip ID register */
> + err = ab3100_get_register(AB3100_CID, &ab3100_chip_id);
> + if (err) {
> + dev_err(&client->dev,
> + "could not detect i2c bus for AB3100 analog"
> + "baseband chip\n");
> + goto exit_no_detect;
> + }
> +
> + for (i = 0; ids[i].id != 0x0; i++) {
> + if (ids[i].id == ab3100_chip_id) {
> + if (ids[i].name != NULL) {
> + snprintf(&ab3100_chip_name[0], 31, "AB3100 %s",
> + ids[i].name);
> + break;
> + } else {
> + dev_err(&client->dev,
> + "AB3000 is no longer supported\n");
> + goto exit_no_detect;
> + }
> + }
> + }
> +
> + if (ids[i].id == 0x0) {
> + dev_err(&client->dev, "Unknown chip id: 0x%x\n",
> + ab3100_chip_id);
> + goto exit_no_detect;
> + }
> +
> + dev_info(&client->dev, "Detected ch
