Re: [PATCH] macintosh: Switch i2c drivers back to use .probe()
On Tue, 23 May 2023 21:50:53 +0200, Uwe Kleine-König wrote:
> After commit b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new()
> call-back type"), all drivers being converted to .probe_new() and then
> 03c835f498b5 ("i2c: Switch .probe() to not take an id parameter") convert
> back to (the new) .probe() to be able to eventually drop .probe_new() from
> struct i2c_driver.
>
> Signed-off-by: Uwe Kleine-König
> ---
> Hello,
>
> this patch was generated using coccinelle, but I aligned the result to
> the per-file indention.
>
> I chose to convert all drivers below drivers/macintosh in a single
> patch, but if you prefer I can split by driver.
>
> v6.4-rc1 was taken as a base, as there are no commits in next touching
> drivers/macintosh I don't expect problems when applying this patch. If
> conflicts arise until this is applied, feel free to just drop the files
> with conflicts from this patch. I'll care about the fallout later then.
>
> Also note there is no coordination necessary with the i2c tree. Dropping
> .probe_new() will happen only when all (or most) drivers are converted,
> which will happen after v6.5-rc1 for sure.
>
> Best regards
> Uwe
>
> drivers/macintosh/ams/ams-i2c.c | 2 +-
> drivers/macintosh/therm_adt746x.c | 2 +-
> drivers/macintosh/therm_windtunnel.c| 2 +-
> drivers/macintosh/windfarm_ad7417_sensor.c | 2 +-
> drivers/macintosh/windfarm_fcu_controls.c | 2 +-
> drivers/macintosh/windfarm_lm75_sensor.c| 2 +-
> drivers/macintosh/windfarm_lm87_sensor.c| 2 +-
> drivers/macintosh/windfarm_max6690_sensor.c | 2 +-
> drivers/macintosh/windfarm_smu_sat.c| 2 +-
> 9 files changed, 9 insertions(+), 9 deletions(-)
> (...)
Reviewed-by: Jean Delvare
--
Jean Delvare
SUSE L3 Support
Re: [PATCH 6/6] i2c: Make remove callback return void
On Tue, 28 Jun 2022 16:03:12 +0200, Uwe Kleine-König wrote: > From: Uwe Kleine-König > > The value returned by an i2c driver's remove function is mostly ignored. > (Only an error message is printed if the value is non-zero that the > error is ignored.) > > So change the prototype of the remove function to return no value. This > way driver authors are not tempted to assume that passing an error to > the upper layer is a good idea. All drivers are adapted accordingly. > There is no intended change of behaviour, all callbacks were prepared to > return 0 before. > > Signed-off-by: Uwe Kleine-König > --- That's a huge change for a relatively small benefit, but if this is approved by the I2C core maintainer then fine with me. For: > drivers/hwmon/adc128d818.c| 4 +--- > drivers/hwmon/adt7470.c | 3 +-- > drivers/hwmon/asb100.c| 6 ++ > drivers/hwmon/asc7621.c | 4 +--- > drivers/hwmon/dme1737.c | 4 +--- > drivers/hwmon/f75375s.c | 5 ++--- > drivers/hwmon/fschmd.c| 6 ++ > drivers/hwmon/ftsteutates.c | 3 +-- > drivers/hwmon/ina209.c| 4 +--- > drivers/hwmon/ina3221.c | 4 +--- > drivers/hwmon/jc42.c | 3 +-- > drivers/hwmon/mcp3021.c | 4 +--- > drivers/hwmon/occ/p8_i2c.c| 4 +--- > drivers/hwmon/pcf8591.c | 3 +-- > drivers/hwmon/smm665.c| 3 +-- > drivers/hwmon/tps23861.c | 4 +--- > drivers/hwmon/w83781d.c | 4 +--- > drivers/hwmon/w83791d.c | 6 ++ > drivers/hwmon/w83792d.c | 6 ++ > drivers/hwmon/w83793.c| 6 ++ > drivers/hwmon/w83795.c| 4 +--- > drivers/hwmon/w83l785ts.c | 6 ++ > drivers/i2c/i2c-core-base.c | 6 +- > drivers/i2c/i2c-slave-eeprom.c| 4 +--- > drivers/i2c/i2c-slave-testunit.c | 3 +-- > drivers/i2c/i2c-smbus.c | 3 +-- > drivers/i2c/muxes/i2c-mux-ltc4306.c | 4 +--- > drivers/i2c/muxes/i2c-mux-pca9541.c | 3 +-- > drivers/i2c/muxes/i2c-mux-pca954x.c | 3 +-- Reviewed-by: Jean Delvare -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c: don't print error when adding adapter fails
On Tue, 9 Aug 2016 13:36:17 +0200, Wolfram Sang wrote: > The core will do this for us now. > > Signed-off-by: Wolfram Sang <wsa-...@sang-engineering.com> > --- > drivers/i2c/busses/i2c-amd756.c | 5 + > drivers/i2c/busses/i2c-at91.c | 2 -- > drivers/i2c/busses/i2c-axxia.c | 8 +--- > drivers/i2c/busses/i2c-bcm-iproc.c | 8 +--- > drivers/i2c/busses/i2c-bcm-kona.c | 4 +--- > drivers/i2c/busses/i2c-bfin-twi.c | 4 +--- > drivers/i2c/busses/i2c-brcmstb.c| 4 +--- > drivers/i2c/busses/i2c-cadence.c| 4 +--- > drivers/i2c/busses/i2c-cpm.c| 4 +--- > drivers/i2c/busses/i2c-cros-ec-tunnel.c | 4 +--- > drivers/i2c/busses/i2c-davinci.c| 4 +--- > drivers/i2c/busses/i2c-diolan-u2c.c | 4 +--- > drivers/i2c/busses/i2c-dln2.c | 4 +--- > drivers/i2c/busses/i2c-efm32.c | 1 - > drivers/i2c/busses/i2c-exynos5.c| 4 +--- > drivers/i2c/busses/i2c-hix5hd2.c| 4 +--- > drivers/i2c/busses/i2c-i801.c | 1 - > drivers/i2c/busses/i2c-ibm_iic.c| 4 +--- > drivers/i2c/busses/i2c-img-scb.c| 4 +--- > drivers/i2c/busses/i2c-imx.c| 4 +--- > drivers/i2c/busses/i2c-isch.c | 4 +--- > drivers/i2c/busses/i2c-ismt.c | 4 +--- > drivers/i2c/busses/i2c-jz4780.c | 4 +--- > drivers/i2c/busses/i2c-lpc2k.c | 4 +--- > drivers/i2c/busses/i2c-meson.c | 1 - > drivers/i2c/busses/i2c-mpc.c| 4 +--- > drivers/i2c/busses/i2c-mt65xx.c | 4 +--- > drivers/i2c/busses/i2c-mxs.c| 1 - > drivers/i2c/busses/i2c-nforce2.c| 1 - > drivers/i2c/busses/i2c-nomadik.c| 4 +--- > drivers/i2c/busses/i2c-ocores.c | 4 +--- > drivers/i2c/busses/i2c-octeon.c | 4 +--- > drivers/i2c/busses/i2c-omap.c | 4 +--- > drivers/i2c/busses/i2c-piix4.c | 1 - > drivers/i2c/busses/i2c-pmcmsp.c | 4 +--- > drivers/i2c/busses/i2c-pnx.c| 4 +--- > drivers/i2c/busses/i2c-puv3.c | 5 + > drivers/i2c/busses/i2c-pxa.c| 4 +--- > drivers/i2c/busses/i2c-rcar.c | 4 +--- > drivers/i2c/busses/i2c-riic.c | 4 +--- > drivers/i2c/busses/i2c-rk3x.c | 4 +--- > drivers/i2c/busses/i2c-s3c2410.c| 1 - > drivers/i2c/busses/i2c-sh7760.c | 4 +--- > drivers/i2c/busses/i2c-sh_mobile.c | 1 - > drivers/i2c/busses/i2c-sirf.c | 4 +--- > drivers/i2c/busses/i2c-st.c | 4 +--- > drivers/i2c/busses/i2c-stu300.c | 5 + > drivers/i2c/busses/i2c-tegra.c | 4 +--- > drivers/i2c/busses/i2c-uniphier-f.c | 7 +-- > drivers/i2c/busses/i2c-uniphier.c | 7 +-- > drivers/i2c/busses/i2c-wmt.c| 4 +--- > drivers/i2c/busses/i2c-xgene-slimpro.c | 1 - > drivers/i2c/busses/i2c-xiic.c | 1 - > drivers/i2c/busses/i2c-xlp9xx.c | 4 +--- > drivers/i2c/busses/i2c-xlr.c| 4 +--- > 55 files changed, 44 insertions(+), 161 deletions(-) > (...) I like the idea. For i2c-amd756, i2c-diolan-u2c, i2c-i801, i2c-isch, i2c-ismt, i2c-nforce and i2c-piix4: Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support
Re: therm_adt746x: -3 invalid for parameter limit_adjust
Hi Christian, On Thu, 26 Sep 2013 02:16:16 -0700 (PDT), Christian Kujau wrote: Hi, after upgrading from 3.11 to 3.12-rc2, the therm_adt746x module could not be loaded any more: therm_adt746x: `-2' invalid for parameter `limit_adjust' I've alwasy passed limit_adjust=-3 (negative 3) to the module via modprobe.conf, to lower the max temperature. Up until 3.11, loading the module would print: adt746x: Lowering max temperatures from 73, 80, 109 to 67, 47, 67 I can load the module without limit_adjust or with positive values, but really wanted to lower the temperature, so that the fan would kick in earlier. For reference, this is what happens in 3.12-rc2: v--- limit_adjust | 0: adt746x: Lowering max temperatures from 73, 80, 109 to 70, 50, 70 1: adt746x: Lowering max temperatures from 73, 80, 109 to 71, 51, 71 2: adt746x: Lowering max temperatures from 73, 80, 109 to 72, 52, 72 3: adt746x: Lowering max temperatures from 73, 80, 109 to 73, 53, 73 4: adt746x: Lowering max temperatures from 73, 80, 109 to 74, 54, 74 10: adt746x: Lowering max temperatures from 73, 80, 109 to 80, 60, 80 Was passing negative values to therm_adt746x ever supported? As far as I can see, yes. drivers/macintosh/therm_adt746x.c hasn't been touched in a while, this means some other change did it. Before I attempt a full git bisect, any hints what could have caused this? I think it is a bug in: commit 6072ddc8520b86adfac6939ca32fb6e6c4de017a Author: Jingoo Han jg1@samsung.com Date: Thu Sep 12 15:14:07 2013 -0700 kernel: replace strict_strto*() with kstrto*() The change was a good idea but the code itself is not, it has kstrtoul in many places where kstrtol should be used. Please try the following patch, hopefully that should fix your problem: From: Jean Delvare kh...@linux-fr.org Subject: kernel/params: Fix handling of signed integer types Commit 6072ddc8520b86adfac6939ca32fb6e6c4de017a broke the handling of signed integer types, fix it. Reported-by: Christian Kujau li...@nerdbynature.de Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Jingoo Han jg1@samsung.com Cc: Andrew Morton a...@linux-foundation.org --- kernel/params.c |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- linux-3.12-rc2.orig/kernel/params.c 2013-09-24 00:41:09.0 +0200 +++ linux-3.12-rc2/kernel/params.c 2013-09-26 11:32:43.434586197 +0200 @@ -254,11 +254,11 @@ int parse_args(const char *doing, STANDARD_PARAM_DEF(byte, unsigned char, %hhu, unsigned long, kstrtoul); -STANDARD_PARAM_DEF(short, short, %hi, long, kstrtoul); +STANDARD_PARAM_DEF(short, short, %hi, long, kstrtol); STANDARD_PARAM_DEF(ushort, unsigned short, %hu, unsigned long, kstrtoul); -STANDARD_PARAM_DEF(int, int, %i, long, kstrtoul); +STANDARD_PARAM_DEF(int, int, %i, long, kstrtol); STANDARD_PARAM_DEF(uint, unsigned int, %u, unsigned long, kstrtoul); -STANDARD_PARAM_DEF(long, long, %li, long, kstrtoul); +STANDARD_PARAM_DEF(long, long, %li, long, kstrtol); STANDARD_PARAM_DEF(ulong, unsigned long, %lu, unsigned long, kstrtoul); int param_set_charp(const char *val, const struct kernel_param *kp) -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/7] rapidio: add modular build option for the subsystem core
Hi Alexandre, Le Friday 28 June 2013 à 15:18 -0400, Alexandre Bounine a écrit : Add a configuration option to build RapidIO subsystem core code as a loadable kernel module. Currently this option is available only for x86-based platforms, with the additional patch for PowerPC planned to be provided later. This patch replaces kernel command line parameter riohdid= with its module-specific analog rapidio.hdid=. Signed-off-by: Alexandre Bounine alexandre.boun...@idt.com Cc: Matt Porter mpor...@kernel.crashing.org Cc: Li Yang le...@freescale.com Cc: Kumar Gala ga...@kernel.crashing.org Cc: Andre van Herk andre.van.h...@prodrive.nl Cc: Micha Nelissen micha.nelis...@prodrive.nl Cc: Stef van Os stef.van...@prodrive.nl Cc: Jean Delvare jdelv...@suse.de --- arch/x86/Kconfig |4 ++-- drivers/rapidio/Makefile |4 +++- drivers/rapidio/rio.c| 27 ++- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index fe120da..583ac42 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2246,11 +2246,11 @@ source drivers/pcmcia/Kconfig source drivers/pci/hotplug/Kconfig config RAPIDIO - bool RapidIO support + tristate RapidIO support Is there a reason why this was only done for x86 and not for mips and powerpc? depends on PCI default n help - If you say Y here, the kernel will include drivers and + If enabled this option will include drivers and the core infrastructure code to support RapidIO interconnect devices. -- Jean Delvare Suse L3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/7] rapidio: modularize rapidio subsystem
Hi Alexandre, Le Friday 28 June 2013 à 15:18 -0400, Alexandre Bounine a écrit : The following set of patches modifies kernel RapidIO subsystem to enable build and use of its components as loadable kernel modules. Combinations of statically linked and modular components are also supported. (...) Thanks to this patch set, I was finally able to make all RapidIO support modular in openSUSE kernels. This is very appreciated, thanks a lot! -- Jean Delvare Suse L3 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] i2c: Remove unneeded xxx_set_drvdata(..., NULL) calls
On Fri, 15 Feb 2013 15:18:35 -0800, Doug Anderson wrote:
There is simply no reason to be manually setting the private driver
data to NULL in the remove/fail to probe cases. This is just extra
cruft code that can be removed.
A few notes:
* Nothing relies on drvdata being set to NULL.
* The __device_release_driver() function eventually calls
dev_set_drvdata(dev, NULL) anyway, so there's no need to do it
twice.
I had not noticed this change. Very good news!
* I verified that there were no cases where xxx_get_drvdata() was
being called in these drivers and checking for / relying on the NULL
return value.
This could be cleaned up kernel-wide but for now just take the baby
step and remove from the i2c subsystem.
Reported-by: Wolfram Sang w...@the-dreams.de
Reported-by: Stephen Warren swar...@wwwdotorg.org
Signed-off-by: Doug Anderson diand...@chromium.org
---
(...)
For i2c-taos-evm:
Reviewed-by: Jean Delvare kh...@linux-fr.org
Also a note:
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -595,7 +595,7 @@ static int octeon_i2c_probe(struct platform_device *pdev)
result = i2c_add_adapter(i2c-adap);
if (result 0) {
dev_err(i2c-dev, failed to add adapter\n);
- goto fail_add;
+ goto out;
}
dev_info(i2c-dev, version %s\n, DRV_VERSION);
@@ -603,8 +603,6 @@ static int octeon_i2c_probe(struct platform_device *pdev)
return 0;
-fail_add:
- platform_set_drvdata(pdev, NULL);
out:
return result;
};
There no longer is any point in this error path, all gotos in this
function could be changed to returns (in a separate patch, obviously.)
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 207/493] i2c: remove use of __devinit
Hi Bill, On Mon, 19 Nov 2012 13:22:36 -0500, Bill Pemberton wrote: CONFIG_HOTPLUG is going away as an option so __devinit is no longer needed. Can you please point me/us to the discussion explaining the rationale behind this move, and the explanation of what will be done exactly? While I can easily understand that we want to drop CONFIG_HOTPLUG and always enable hot-plug support, I don't see where we are going with removing __devinit annotations and the like. Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 207/493] i2c: remove use of __devinit
On Tue, 20 Nov 2012 13:23:42 +, Russell King - ARM Linux wrote: On Tue, Nov 20, 2012 at 09:20:46AM +0100, Jean Delvare wrote: Hi Bill, On Mon, 19 Nov 2012 13:22:36 -0500, Bill Pemberton wrote: CONFIG_HOTPLUG is going away as an option so __devinit is no longer needed. Can you please point me/us to the discussion explaining the rationale behind this move, and the explanation of what will be done exactly? While I can easily understand that we want to drop CONFIG_HOTPLUG and always enable hot-plug support, I don't see where we are going with removing __devinit annotations and the like. It's actually very simple to understand. 1. CONFIG_HOTPLUG is going away; it's already defined to always be 'Y'. 2. This means that the the devinit sections will not be discarded anymore. 3. As a result, there's no point the devinit sections existing anymore. 4. As there's no devinit sections, the __devinit marker is entirely redundant and useless. Ah, yes, very simple indeed. Not sure how I managed to not understand it earlier today. Thanks for explaining. The reason this is being done is because the benefit to cost ratio of this is far too high; it's well proven that people constantly get these markings wrong, and with most kernels having had hotplug enabled anyway, it's not providing much in the way of space saving benefit over the number of section conflicts it causes. So, it's been decided a few years ago that this is Yes, I completely agree. going to happen, with that justification, and it's been accepted by 300 odd kernel developers in at least one kernel summit when it was talked Probably that was one I didn't attend to, as I can't remember this discussion. about... and it's been mentioned on mailing lists several times. Maybe LKML, which I don't read. So I wasn't aware of the plan before seeing Bill's patches. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 065/493] i2c: remove use of __devexit_p
On Mon, 19 Nov 2012 13:20:14 -0500, Bill Pemberton wrote: CONFIG_HOTPLUG is going away as an option so __devexit_p is no longer needed. As mentioned on the lm-sensors list for hwmon patches already, I think it would be much clearer to not split __devexit, __devexit_p, __devinit etc. removal into separate patches. One patch per subsystem would be easier to review and apply. If patches grow too large then you'd rather split in a different direction, for example drivers/i2c/muxes vs. drivers/i2c/busses or even grouped by related bus drivers (see entries I2C OVER PARALLEL PORT and I2C/SMBUS CONTROLLER DRIVERS FOR PC in MAINTAINERS for examples of meaningful groups.) -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] i2c: let the core register devices from devicetree
Hi Wolfram, On Wed, 13 Jun 2012 23:12:10 +0200, Wolfram Sang wrote: Currently, every driver has to do it on its own, but it should be done in the core, like we already do with board_info structs. Signed-off-by: Wolfram Sang w.s...@pengutronix.de --- Based on v3.5-rc2. Only build tested, I don't have a OF based device around at the moment. (...) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index a6ad32b..4791833 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -39,6 +39,7 @@ #include linux/irqflags.h #include linux/rwsem.h #include linux/pm_runtime.h +#include linux/of_i2c.h #include asm/uaccess.h #include i2c-core.h @@ -880,6 +881,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) #endif /* create pre-declared device nodes */ + of_i2c_register_devices(adap); + if (adap-nr __i2c_first_dynamic_bus_num) i2c_scan_static_board_info(adap); This was proposed in the past, and rejected because of dependency issues. I don't think the situation changed. of_i2c needs i2c-core for i2c_new_device(), and with the change above, i2c-core needs of_i2c for of_i2c_register_devices(). If either is built as a module, it will fail. This might be the reason why of_spi ended up being merged into drivers/spi according to Grant? I have no objection to the same being done for of_i2c if it makes everybody happy, as long as it does not create additional dependencies (i.e. I2C should not depend on OF.) -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 00/15] PowerMac i2c API conversions windfarm updates
On Thu, 19 Apr 2012 18:16:41 +1000, Benjamin Herrenschmidt wrote: The goal of this series is to convert a bulk of PowerMac i2c drivers to the new proper i2c driver registration model. This series is not complete in that there are still a few drivers to do but it goes through the bulk of thermal control and some of the nastiest. The biggest change is that windfarm is ported to generally use the new model, and I've written a new set of windfarm modules to take over from the old therm_pm72 (which was mostly unfixable) on the PowerMac G5 AGP and Xserve G5 machines. This had a bit of testing on a couple of PowerMac G5 AGP variants but none at all on Xserve G5 since the one I have is unfortunately dead. If you want to help me test this, don't forget to disable therm_pm72, and instead load windfarm_pm72 or windfarm_rm31 depending on your machine (you can load or build-in the whole lot as well). As usual windfarm don't properly auto-load yet, I still need to fix that. So please test ! Especially if you have an XServe G5 Benjamin, thanks a lot for doing this! These drivers were the last ones blocking the removal of the legacy binding model in i2c-core. Testers are very welcome. The earliest we can get these changes upstream, the better. Christian, can you please test this series? I'm sure Benjamin can send the patches to you directly if needed. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 00/15] PowerMac i2c API conversions windfarm updates
Hallo Andreas, On Thu, 19 Apr 2012 12:11:25 +0200, Andreas Schwab wrote: Jean Delvare kh...@linux-fr.org writes: These drivers were the last ones blocking the removal of the legacy binding model in i2c-core. There are also still some uses in snd-aoa. You're right, there are still 3 sound drivers which need to be converted (aoa/onyx, aoa/tas and ppc/keywest.) I believe Benjamin will take care of the latter when he is done with the thermal management drivers. Not sure who can work on and/or test the aoa sound drivers. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3] i2c: move of_i2c_register_devices call into core
Hi Rob, Grant, On Fri, 05 Aug 2011 18:22:36 -0500, Rob Herring wrote: Grant, On 08/05/2011 05:54 PM, Grant Likely wrote: On Fri, Aug 05, 2011 at 04:24:26PM -0500, Rob Herring wrote: From: Rob Herring rob.herr...@calxeda.com All callers of of_i2c_register_devices are immediately preceded by a call to i2c_add_adapter or i2c_add_numbered_adapter. Add call to of_i2c_register_devices and remove all other callers. This causes a module dependency loop that is resolved by the next commit. Wrong way around. Don't break bisectability. I'll leave it up to Ben and Jean on weather or not they want to move this code. I intend to do the same thing for spi/gpio, but I maintain those subsystems so I get to choose. i2c is not up to me. Well, I know, but it's only broken for bisect in the case of both being modules. So it's only run-time brokenness. That's better than the 3 months it was broken before. I couldn't come up with a better way. The choices I thought of were: -temporarily exporting and adding of_i2c_register_devices to i2c.h and then removing it. I'm not a fan of adding that churn. -just combining it all into 1 patch. That would have been the right thing to do, yes. The two patches are not so large, so the resulting merged patch would still be reasonable. There's no point in splitting patches if they can't be applied in sequence, bisected, and rejected, reverted or backported individually; which is clearly not the case here. But anyway, I don't buy the whole thing. We currently have a nice separation between OF and the underlying bus types. Breaking it all just for the sake of saving one call in 4 drivers (out of ~70 in the current kernel tree) doesn't seem worth it, at all. It would make things harder to maintain, too (I definitely do not want to maintain OF-specific code). If you are really worried about the extra call to of_i2c_register_devices(), then maybe the of_i2c implementation is incorrect, and you should rework it to build on top of i2c_register_board_info() rather than i2c_new_device(). -reverse the order and leave i2c device registration broken for 1 commit. Thinking some more about it, perhaps that is a bit better than broken module loading. Guess it depends if you are doing modules or built-in. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Move ams driver to macintosh
Hi Benjamin, On Tue, 16 Nov 2010 15:33:25 +1100, Benjamin Herrenschmidt wrote: On Tue, 2010-10-05 at 12:10 +0200, Jean Delvare wrote: The ams driver isn't a hardware monitoring driver, so it shouldn't live under driver/hwmon. drivers/macintosh seems much more appropriate, as the driver is only useful on PowerBooks and iBooks. Going through backlog... Do you want me to carry this in powerpc or you'll deal with it directly ? Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Guenter Roeck guenter.ro...@ericsson.com Cc: Stelian Pop stel...@popies.net Cc: Michael Hanselmann linux-ker...@hansmi.ch Acked-by: Benjamin Herrenschmidt b...@kernel.crashing.org I'm glad you like this patch, as it has already been applied to Linus' tree ;) -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] Move ams driver to macintosh
The ams driver isn't a hardware monitoring driver, so it shouldn't
live under driver/hwmon. drivers/macintosh seems much more
appropriate, as the driver is only useful on PowerBooks and iBooks.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Guenter Roeck guenter.ro...@ericsson.com
Cc: Stelian Pop stel...@popies.net
Cc: Michael Hanselmann linux-ker...@hansmi.ch
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Grant Likely grant.lik...@secretlab.ca
---
MAINTAINERS |2
drivers/hwmon/Kconfig | 26 ---
drivers/hwmon/Makefile|1
drivers/hwmon/ams/Makefile|8 -
drivers/hwmon/ams/ams-core.c | 250 -
drivers/hwmon/ams/ams-i2c.c | 277 -
drivers/hwmon/ams/ams-input.c | 157
drivers/hwmon/ams/ams-pmu.c | 201 --
drivers/hwmon/ams/ams.h | 70 -
drivers/macintosh/Kconfig | 26 +++
drivers/macintosh/Makefile|2
drivers/macintosh/ams/Makefile|8 +
drivers/macintosh/ams/ams-core.c | 250 +
drivers/macintosh/ams/ams-i2c.c | 277 +
drivers/macintosh/ams/ams-input.c | 157
drivers/macintosh/ams/ams-pmu.c | 201 ++
drivers/macintosh/ams/ams.h | 70 +
17 files changed, 992 insertions(+), 991 deletions(-)
--- linux-2.6.36-rc6.orig/drivers/hwmon/ams/Makefile2010-08-02
00:11:14.0 +0200
+++ /dev/null 1970-01-01 00:00:00.0 +
@@ -1,8 +0,0 @@
-#
-# Makefile for Apple Motion Sensor driver
-#
-
-ams-y := ams-core.o ams-input.o
-ams-$(CONFIG_SENSORS_AMS_PMU) += ams-pmu.o
-ams-$(CONFIG_SENSORS_AMS_I2C) += ams-i2c.o
-obj-$(CONFIG_SENSORS_AMS) += ams.o
--- linux-2.6.36-rc6.orig/drivers/hwmon/ams/ams-core.c 2010-08-02
00:11:14.0 +0200
+++ /dev/null 1970-01-01 00:00:00.0 +
@@ -1,250 +0,0 @@
-/*
- * Apple Motion Sensor driver
- *
- * Copyright (C) 2005 Stelian Pop (stel...@popies.net)
- * Copyright (C) 2006 Michael Hanselmann (linux-ker...@hansmi.ch)
- *
- * 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
USA.
- */
-
-#include linux/module.h
-#include linux/types.h
-#include linux/errno.h
-#include linux/init.h
-#include linux/of_platform.h
-#include asm/pmac_pfunc.h
-
-#include ams.h
-
-/* There is only one motion sensor per machine */
-struct ams ams_info;
-
-static unsigned int verbose;
-module_param(verbose, bool, 0644);
-MODULE_PARM_DESC(verbose, Show free falls and shocks in kernel output);
-
-/* Call with ams_info.lock held! */
-void ams_sensors(s8 *x, s8 *y, s8 *z)
-{
- u32 orient = ams_info.vflag? ams_info.orient1 : ams_info.orient2;
-
- if (orient 0x80)
- /* X and Y swapped */
- ams_info.get_xyz(y, x, z);
- else
- ams_info.get_xyz(x, y, z);
-
- if (orient 0x04)
- *z = ~(*z);
- if (orient 0x02)
- *y = ~(*y);
- if (orient 0x01)
- *x = ~(*x);
-}
-
-static ssize_t ams_show_current(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- s8 x, y, z;
-
- mutex_lock(ams_info.lock);
- ams_sensors(x, y, z);
- mutex_unlock(ams_info.lock);
-
- return snprintf(buf, PAGE_SIZE, %d %d %d\n, x, y, z);
-}
-
-static DEVICE_ATTR(current, S_IRUGO, ams_show_current, NULL);
-
-static void ams_handle_irq(void *data)
-{
- enum ams_irq irq = *((enum ams_irq *)data);
-
- spin_lock(ams_info.irq_lock);
-
- ams_info.worker_irqs |= irq;
- schedule_work(ams_info.worker);
-
- spin_unlock(ams_info.irq_lock);
-}
-
-static enum ams_irq ams_freefall_irq_data = AMS_IRQ_FREEFALL;
-static struct pmf_irq_client ams_freefall_client = {
- .owner = THIS_MODULE,
- .handler = ams_handle_irq,
- .data = ams_freefall_irq_data,
-};
-
-static enum ams_irq ams_shock_irq_data = AMS_IRQ_SHOCK;
-static struct pmf_irq_client ams_shock_client = {
- .owner = THIS_MODULE,
- .handler = ams_handle_irq,
- .data = ams_shock_irq_data,
-};
-
-/* Once hard disk parking is implemented
Re: [PATCH (Option 1)] of/i2c: fix module load order issue caused by of_i2c.c
On Wed, 29 Sep 2010 00:20:54 +0100, Ben Dooks wrote: On Fri, Sep 24, 2010 at 04:14:53PM -0600, Grant Likely wrote: Commit 959e85f7, i2c: add OF-style registration and binding caused a module dependency loop where of_i2c.c calls functions in i2c-core, and i2c-core calls of_i2c_register_devices() in of_i2c. This means that when i2c support is built as a module when CONFIG_OF is set, then neither i2c_core nor of_i2c are able to be loaded. This patch fixes the problem by moving the of_i2c_register_devices() function into the body of i2c_core and renaming it to i2c_scan_of_devices (of_i2c_register_devices is analogous to the existing i2c_scan_static_board_info function and so should be named similarly). This function isn't called by any code outside of i2c_core, and it must always be present when CONFIG_OF is selected, so it makes sense to locate it there. When CONFIG_OF is not selected, of_i2c_register_devices() becomes a no-op. I sort of go with this one. Actually I would prefer option #2, even though I understand it won't make Grant too happy. Having a large chunk of OF-specific code in i2c-core, leaving of_i2c.c almost empty, doesn't seem right. I took a look at what other relevant subsystems do. SPI is boolean so it doesn't have the issue. MDIO is tristate, the registration function is in of_mdio.c and individual drivers call it. And there are a lot more of these (9) than i2c drivers (3). So I would let individual drivers call of_i2c_register_devices(), as it used to be until 2.6.35. 2 extra functions calls doesn't seem a high price to pay to keep the code logically separated. This also make things consistent, with all OF registration functions living under drivers/of. Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH (Option 2)] of/i2c: fix module load order issue caused by of_i2c.c
On Fri, 24 Sep 2010 16:15:18 -0600, Grant Likely wrote: Commit 959e85f7, i2c: add OF-style registration and binding caused a module dependency loop where of_i2c.c calls functions in i2c-core, and i2c-core calls of_i2c_register_devices() in of_i2c. This means that when i2c support is built as a module when CONFIG_OF is set, then neither i2c_core nor of_i2c are able to be loaded. This patch fixes the problem by moving the of_i2c_register_devices() calls back into the device drivers. Device drivers already specifically request the core code to parse the device tree for devices anyway by setting the of_node pointer, so it isn't a big deal to also call the registration function. The drivers just become slightly more verbose. Signed-off-by: Grant Likely grant.lik...@secretlab.ca --- drivers/i2c/busses/i2c-cpm.c |5 + drivers/i2c/busses/i2c-ibm_iic.c |3 +++ drivers/i2c/busses/i2c-mpc.c |1 + drivers/i2c/i2c-core.c |4 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c index f7bd261..f2de3be 100644 --- a/drivers/i2c/busses/i2c-cpm.c +++ b/drivers/i2c/busses/i2c-cpm.c @@ -677,6 +677,11 @@ static int __devinit cpm_i2c_probe(struct platform_device *ofdev, dev_dbg(ofdev-dev, hw routines for %s registered.\n, cpm-adap.name); + /* + * register OF I2C devices + */ + of_i2c_register_devices(cpm-adap); + return 0; out_shut: cpm_i2c_shutdown(cpm); diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index 43ca32f..89eedf4 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -761,6 +761,9 @@ static int __devinit iic_probe(struct platform_device *ofdev, dev_info(ofdev-dev, using %s mode\n, dev-fast_mode ? fast (400 kHz) : standard (100 kHz)); + /* Now register all the child nodes */ + of_i2c_register_devices(adap); + return 0; error_cleanup: diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index a1c419a..b74e6dc 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -632,6 +632,7 @@ static int __devinit fsl_i2c_probe(struct platform_device *op, dev_err(i2c-dev, failed to add adapter\n); goto fail_add; } + of_i2c_register_devices(i2c-adap); return result; diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 6649176..a9589f5 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -32,7 +32,6 @@ #include linux/init.h #include linux/idr.h #include linux/mutex.h -#include linux/of_i2c.h #include linux/of_device.h #include linux/completion.h #include linux/hardirq.h @@ -876,9 +875,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap) if (adap-nr __i2c_first_dynamic_bus_num) i2c_scan_static_board_info(adap); - /* Register devices from the device tree */ - of_i2c_register_devices(adap); - /* Notify drivers */ mutex_lock(core_lock); bus_for_each_drv(i2c_bus_type, NULL, adap, __process_new_adapter); I've applied this variant, thanks. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [BUG 2.6.36-rc5] of_i2c.ko - i2c-core.ko dependency loop
On Thu, 23 Sep 2010 15:05:59 -0700, Randy Dunlap wrote: On Thu, 23 Sep 2010 22:16:32 +0200 Mikael Pettersson wrote: Randy Dunlap writes: No kconfig warnings? Not that I recall. I can check tomorrow if necessary. No kconfig warnings. I checked with your .config file. Please post your full .config file. Just a matter of module i2c-core calls of_ functions and module of_i2c calls i2c_ functions. Hmph. Something for Grant, Jean, and Ben to work out. As far as I can see this is caused by this commit from Grant: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=959e85f7751c33d1a2dabc5cc3fe2ed0db7052e5 Mikael, can you please try reverting this patch and see if it solves your problem? -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [BUG 2.6.36-rc5] of_i2c.ko - i2c-core.ko dependency loop
Hi Mikael,
On Fri, 24 Sep 2010 12:50:01 +0200, Mikael Pettersson wrote:
Jean Delvare writes:
As far as I can see this is caused by this commit from Grant:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=959e85f7751c33d1a2dabc5cc3fe2ed0db7052e5
Mikael, can you please try reverting this patch and see if it solves
your problem?
Yes, reverting the above commit from 2.6.36-rc5 eliminated the warnings,
and I was able to insmod the i2c-{core,dev,powermac}.ko modules.
Thanks for testing and reporting. Grant, unless you come up with a fix
very quickly, I'll have to revert
959e85f7751c33d1a2dabc5cc3fe2ed0db7052e5 for 2.6.36.
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [lm-sensors] [PATCH] hwmon: (tmp421) Add nfactor support (2nd attempt)
Hi Andre, Guenter, Jeff, On Wed, 21 Jul 2010 21:46:50 +0200, Andre Prendel wrote: On Tue, Jul 20, 2010 at 08:59:52AM -0700, Guenter Roeck wrote: On Tue, Jul 20, 2010 at 11:09:53AM -0400, Andre Prendel wrote: On Thu, May 20, 2010 at 09:35:56PM +0200, Andre Prendel wrote: On Thu, May 20, 2010 at 03:07:05PM -0400, Jeff Angielski wrote: In any event, here it is again: Acked-by: Andre Prendel andre.pren...@gmx.de Hi Jeff, I'de suggest to resend the patch with my acked-by to the lm-sensors list and Andrew Morton. It looks like Jean is too busy at the moment. Regards, Andre From 9acd29ff48c64e58a7f5cdb888c86e737c56281c Mon Sep 17 00:00:00 2001 From: Jeff Angielski j...@theptrgroup.com Date: Mon, 10 May 2010 10:26:34 -0400 Subject: [PATCH] hwmon: (tmp421) Add nfactor support Add support for reading and writing the n-factor correction registers. This is needed to compensate for the characteristics of a particular sensor hanging off of the remote channels. My concerns with this approach are 1) It changes the sysfs abi. libsensors won't support it. It opens up a can of worms with everyone starting to put chip-specific extensions into the ABI. If such an extension has to be made, it should be a) really necessary and b) a generic extension which applies to all chips. A chip-specific extension can't be also generic. So we have to decide whether weaccept chip-specific extensions or not. libsensors 3 will never support chip-specific extensions. We've been there with libsensors 2, it was a nightmare, never again please. This doesn't mean we can't have chip-specific extensions, and actually some drivers have such extensions already. But as they are often undocumented and bound to be replaced by standard implementations in the future, nobody should rely on them. Which makes their usefulness dubious. This is the reason why I always insist on trying to define a standard suitable for all chips before you think of adding a not-yet-supported feature to a given driver. 2) My understanding is that value adjustments are supposed to be made via sensors.conf, and that values reported by the chip should always be 'raw', ie unadjusted values. Instead of exporting n_adjust to the user via sysfs, it might make more sense to reset the correction factor to its default (if it was changed), and handle required adjustments in sensors.conf. Jeff, what do you think? I'm not Jeff, but resetting the factors is a bad idea. The BIOS/firmware might have set them up properly, so the default should be to leave them untouched (as we do with every other setting.) Even if Jean doesn't have time to handle the patch, you should at least get his Ack for the ABI changes. That was the intention of resending the patch to the lm-sensors list. It would be a pity to lose Jeff's effort. I'm not giving my ack for any non-standard feature, sorry. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/6] Remove owner field from sysfs attribute structure
On Mon, 2 Aug 2010 16:31:28 -0700, Greg KH wrote: On Wed, Jul 28, 2010 at 11:16:35PM -0700, Eric Biederman wrote: On Wed, Jul 28, 2010 at 10:09 PM, Guenter Roeck guenter.ro...@ericsson.com wrote: The following comment is found in include/linux/sysfs.h: /* FIXME * The *owner field is no longer used. * x86 tree has been cleaned up. The owner * attribute is still left for other arches. */ As it turns out, the *owner field is (again?) initialized in several modules, suggesting that such initialization may be creeping back into the code. This patch set removes the above comment, the *owner field, and each instance in the code where it was found to be initialized. Compiled with x86 allmodconfig as well as with all alpha, arm, mips, powerpc, and sparc defconfig builds. This seems reasonable to me. Can we get this in linux-next? It will show up in linux-next tomorrow. Related bug? https://bugzilla.kernel.org/show_bug.cgi?id=16544 -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] rtc: add support for DS3232 RTC
Hi Roy, On Mon, 5 Jul 2010 14:45:26 +0800, Roy Zang wrote: This patch adds the driver for RTC chip DS3232 via I2C bus Signed-off-by: Mingkai Hu mingkai...@freescale.com Signed-off-by: Jingchang Lu b22...@freescale.com Signed-off-by: Srikanth Srinivasan srikanth.sriniva...@freescale.com Signed-off-by: Roy Zang tie-fei.z...@freescale.com --- Tested on MPC8536DS and P4080DS board drivers/rtc/Kconfig | 11 + drivers/rtc/Makefile |1 + drivers/rtc/rtc-ds3232.c | 466 ++ 3 files changed, 478 insertions(+), 0 deletions(-) create mode 100644 drivers/rtc/rtc-ds3232.c You're sending this patch to the wrong list. Please read MAINTAINERS again. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] drivers: remove all i2c_set_clientdata(client, NULL)
On Mon, 31 May 2010 14:55:48 +0200, Wolfram Sang wrote: I2C-drivers can use the clientdata-pointer to point to private data. As I2C devices are not really unregistered, but merely detached from their driver, it used to be the drivers obligation to clear this pointer during remove() or a failed probe(). As a couple of drivers forgot to do this, it was agreed that it was cleaner if the i2c-core does this clearance when appropriate, as there is no guarantee for the lifetime of the clientdata-pointer after remove() anyhow. This feature was added to the core with commit e4a7b9b04de15f6b63da5ccdd373ffa3057a3681 to fix the faulty drivers. As there is no need anymore to clear the clientdata-pointer, remove all current occurrences in the drivers to simplify the code and prevent confusion. Signed-off-by: Wolfram Sang w.s...@pengutronix.de Cc: Jean Delvare kh...@linux-fr.org (...) Applied, thanks. Will go to Linus in the next few days. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] drivers: remove all i2c_set_clientdata(client, NULL)
Hi Dmitry, On Mon, 31 May 2010 12:09:12 -0700, Dmitry Torokhov wrote: Frankly I'd prefer taking input stuff through my tree with the goal of .36 merge window just to minimize potential merge issues. This is a simple cleanup patch that has no dependencies, so there is little gain from doing it all in one go. If I take the patch in my i2c tree, the aim is to merge it upstream immediately, so merge issues won't exist. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [lm-sensors] [PATCH] hwmon: (tmp421) Add nfactor support (2nd attempt)
Hi Andre, On Wed, 19 May 2010 09:26:59 +0200, Andre Prendel wrote: Jean, should we describe this interface in Documentation/hwmon/sysfs-interface too? For the moment tmp421 is the only driver implementing it. If the values are expressed in a standard unit (i.e. not raw register values) then yes, it would be nice to have a standard interface described for these files. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] hwmon: (tmp421) Add nfactor support.
Hi Jeff, On Tue, 11 May 2010 15:34:29 -0400, Jeff Angielski wrote: On 05/11/2010 03:03 PM, Andre Prendel wrote: I'de prefer implementing the sysfs access methods in a consistent way (see other functions). That means adding the nfactor register to the tmp421_data structure and using tmp421_update_device() to update the structure. I did this on purpose since the nfactor typically only changes once at runtime when you program it for your sensor. It seemed like a waste of processing power and i2c bandwidth to read a pseudo static register over and over again. It can easily be changed if that's what will help the community the best. I get your point and it makes sense. However, we also have to ensure that no regular user can saturate the I2C link by repeatedly polling for the same register value. So, any sysfs attribute which triggers an immediate I2C transaction shouldn't be accessible by regular users (i.e. change the mode from 0644 to 0640). An alternative is to slit the cache into short-lived (~ 1 or 2 seconds) and long-lived (1 to 5 minutes.) A number of drivers do this. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] hwmon: (tmp421) Add nfactor support.
On Tue, 11 May 2010 21:03:27 +0200, Andre Prendel wrote:
On Mon, May 10, 2010 at 10:43:07AM -0400, Jeff Angielski wrote:
Hi Jeff,
A few comments below.
Add support for reading and writing the n-factor correction
registers. This is needed to compensate for the characteristics
of a particular sensor hanging off of the remote channels.
Signed-off-by: Jeff Angielski j...@theptrgroup.com
---
drivers/hwmon/tmp421.c | 42 ++
1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 738c472..c9e9855 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
+static const u8 TMP421_NFACTOR[3] = { 0x21, 0x22, 0x23 };
/* Flags */
#define TMP421_CONFIG_SHUTDOWN 0x40
@@ -157,6 +158,38 @@ static ssize_t show_fault(struct device *dev,
return sprintf(buf, 0\n);
}
+static ssize_t show_nfactor(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp421_data *data = i2c_get_clientdata(client);
+ int index = to_sensor_dev_attr(devattr)-index;
+ s8 nfactor;
+
+ mutex_lock(data-update_lock);
+ nfactor = i2c_smbus_read_byte_data(client, TMP421_NFACTOR[index-1]);
There should be spaces within the array index, [index - 1].
+ mutex_unlock(data-update_lock);
+
+ return sprintf(buf, %d\n, nfactor);
+}
I'de prefer implementing the sysfs access methods in a consistent way (see
other functions). That means adding the nfactor register to the tmp421_data
structure and using tmp421_update_device() to update the structure.
+static ssize_t set_nfactor(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp421_data *data = i2c_get_clientdata(client);
+ int index = to_sensor_dev_attr(devattr)-index;
+ int nfactor = simple_strtol(buf, NULL, 10);
+
+ mutex_lock(data-update_lock);
+ i2c_smbus_write_byte_data(client, TMP421_NFACTOR[index-1],
Missing spaces in array index again.
+ SENSORS_LIMIT(nfactor, -128, 127));
+ mutex_unlock(data-update_lock);
+
+ return count;
+}
+
static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
int n)
{
@@ -177,19 +210,28 @@ static mode_t tmp421_is_visible(struct kobject
*kobj, struct attribute *a,
static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_fault, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp2_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 1);
static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2);
static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_fault, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp3_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 2);
static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3);
static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_fault, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp4_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 3);
static struct attribute *tmp421_attr[] = {
sensor_dev_attr_temp1_input.dev_attr.attr,
sensor_dev_attr_temp2_input.dev_attr.attr,
sensor_dev_attr_temp2_fault.dev_attr.attr,
+ sensor_dev_attr_temp2_nfactor.dev_attr.attr,
sensor_dev_attr_temp3_input.dev_attr.attr,
sensor_dev_attr_temp3_fault.dev_attr.attr,
+ sensor_dev_attr_temp3_nfactor.dev_attr.attr,
sensor_dev_attr_temp4_input.dev_attr.attr,
sensor_dev_attr_temp4_fault.dev_attr.attr,
+ sensor_dev_attr_temp4_nfactor.dev_attr.attr,
NULL
};
Any hope to standardize on the sysfs attribute files and units? So that
other drivers can implement the same interface.
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 33/37] sound/soc: use .dev.of_node instead of .node in struct of_device
On Thu, 11 Mar 2010 14:22:37 -0700, Grant Likely wrote: On Thu, Mar 11, 2010 at 12:34 PM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Thu, Mar 11, 2010 at 11:06:50AM -0700, Grant Likely wrote: .node is being removed Signed-off-by: Grant Likely grant.lik...@secretlab.ca Acked-by: Mark Brown broo...@opensource.wolfsonmicro.com but please ensure that Liam and especially Timur also check this (both CCed). For enormous patch serieses like this it's really nice if you can ensure that each person is only CCed on the patches that they need to review. Much less stuff in the inbox. Yeah, sorry about that (and to everyone receiving this thread, I'm really sorry. I won't do it again). I've already been yelled at for that. What happened is that on a previous series I was yelled at for not sending all patches to everyone (so that the patches could be reviewed in context). So, naturally, I made sure to include everyone on the whole series this time doh. Next time I post I'll constrain it to small chunks. A good compromise IMHO is to send only the pieces they really have to see and ack to each person, and provide a pointer to somewhere the full series can be seen and downloaded for the interested. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c_powermac: Kernel access of bad area
On Sat, 30 Jan 2010 22:05:26 -0800 (PST), Christian Kujau wrote: OK, I've now applied both the drivers/hwmon/ams/ patches and your earlier one for drivers/macintosh/therm_adt746x.c [0] to a current -git checkout and now I can unload i2c_powermac, list and read the remaining files in /sys/devices/temperatures, load i2c_powermac again, and unload, and load, and ... - it's wonderful! :-) No oopses so far. Feel free to add a Tested-by: Christian Kujau li...@nerdbynature.de so that the patches can be pushed into mainline. Thanks, I'll resend both patches with proper comments and headers now. Thanks for the fixes, Jean! You're welcome. -- Jean Delvare http://khali.linux-fr.org/wishlist.html ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c_powermac: Kernel access of bad area
On Sat, 30 Jan 2010 08:34:55 +1100, Benjamin Herrenschmidt wrote: Ben, what about applying this patch of mine, as Christian reported it fixed his oops? Sure. I never quite know with i2c which ones you will apply directly and which ones you want to go through my tree :-) Hopefully they should still be referened on patchwork, I'll dig there and pick them up. Well, basically I pick patches that touch drivers/i2c/*, and I don't pick patches that touch drivers/macintosh/*. When I can't build the drivers, I don't really feel like pushing the patches myself. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c_powermac: Kernel access of bad area
Ben, Christian,
On Thu, 7 Jan 2010 17:17:38 +0100, Jean Delvare wrote:
On Wed, 6 Jan 2010 19:41:05 -0800 (PST), Christian Kujau wrote:
Hi Jean,
On Wed, 6 Jan 2010 at 17:37, Jean Delvare wrote:
I think that sysfs files creation should be moved to the end of
probe_thermostat() and sysfs files removal should be moved to the
beginning of remove_thermostat(). Something like the totally untested
patch below (no ppc machine at hand):
I applied your patch to 2.6.33-rc3, and was able to unload i2c-powermac and
then reading the files left in /sys/devices/temperatures. I even tried to
read the non-existant files (e.g. sensor1_fan_speed, etc...), but the
kernel just wouldn't oops :)
So the initial oops is gone - yeah!
Ben, what about applying this patch of mine, as Christian reported it
fixed his oops?
However, the Badness remains when I try to modprobe i2c-powermac again:
[ 442.148222] PowerMac i2c bus pmu 2 registered
[ 442.148792] PowerMac i2c bus pmu 1 registered
[ 442.149299] PowerMac i2c bus mac-io 0 registered
[ 442.163573] adt746x: ADT7467 initializing
[ 442.170072] adt746x: Lowering max temperatures from 73, 80, 109 to 67,
47, 67
[ 442.176559] PowerMac i2c bus uni-n 1 registered
[ 442.227115] sysfs: cannot create duplicate filename '/devices/ams'
[ 442.227697] [ cut here ]
[ 442.228176] Badness at fs/sysfs/dir.c:487
[ 442.228642] NIP: c00eb71c LR: c00eb71c CTR:
[ 442.229117] REGS: eea0fa50 TRAP: 0700 Not tainted (2.6.33-rc3)
[ 442.229592] MSR: 00029032 EE,ME,CE,IR,DR CR: 42008444 XER:
[ 442.230151] TASK = eea1[2821] 'modprobe' THREAD: eea0e000
[ 442.230191] GPR00: c00eb71c eea0fb00 eea1 004c 64c6
[ 442.230758] GPR08: efa71740 c03e 64c6 44008428 10020390
100e 100df49c
[ 442.231326] GPR16: 100b54c0 100df49c 100ddd20 1018fb08 100b5340 c03e674c
c03e6720 c03ea044
[ 442.231902] GPR24: 24008422 ffea eea0fb58 ef0e9000 ef0e9000
ef0a9ea0 ffef
[ 442.233187] NIP [c00eb71c] sysfs_add_one+0x94/0xc0
[ 442.233695] LR [c00eb71c] sysfs_add_one+0x94/0xc0
[ 442.234363] Call Trace:
I've put the whole dmesg on:
http://nerdbynature.de/bits/2.6.33-rc2/i2c_powermac/r1/
Hmm. Looks like a different but somewhat similar problem in the ams
driver: some code that is in ams_exit() (the module exit code) should
instead be called when the device (not module) is removed. It probably
doesn't make much of a difference in the PMU case, but in the I2C case
it does matter.
The following, totally untested patch may fix it. I make no guarantee
that my code isn't racy though, I'm not familiar enough with the ams
driver code to tell for sure.
---
drivers/hwmon/ams/ams-core.c | 11 +++
drivers/hwmon/ams/ams-i2c.c |2 ++
drivers/hwmon/ams/ams-pmu.c |2 ++
drivers/hwmon/ams/ams.h |1 +
4 files changed, 12 insertions(+), 4 deletions(-)
--- linux-2.6.33-rc3.orig/drivers/hwmon/ams/ams-core.c2009-06-10
05:05:27.0 +0200
+++ linux-2.6.33-rc3/drivers/hwmon/ams/ams-core.c 2010-01-07
17:14:25.0 +0100
@@ -213,7 +213,7 @@ int __init ams_init(void)
return -ENODEV;
}
-void ams_exit(void)
+void ams_sensor_detach(void)
{
/* Remove input device */
ams_input_exit();
@@ -221,9 +221,6 @@ void ams_exit(void)
/* Remove attributes */
device_remove_file(ams_info.of_dev-dev, dev_attr_current);
- /* Shut down implementation */
- ams_info.exit();
-
/* Flush interrupt worker
*
* We do this after ams_info.exit(), because an interrupt might
@@ -239,6 +236,12 @@ void ams_exit(void)
pmf_unregister_irq_client(ams_freefall_client);
}
+static void __exit ams_exit(void)
+{
+ /* Shut down implementation */
+ ams_info.exit();
+}
+
MODULE_AUTHOR(Stelian Pop, Michael Hanselmann);
MODULE_DESCRIPTION(Apple Motion Sensor driver);
MODULE_LICENSE(GPL);
--- linux-2.6.33-rc3.orig/drivers/hwmon/ams/ams-i2c.c 2009-06-10
05:05:27.0 +0200
+++ linux-2.6.33-rc3/drivers/hwmon/ams/ams-i2c.c 2010-01-07
17:12:46.0 +0100
@@ -238,6 +238,8 @@ static int ams_i2c_probe(struct i2c_clie
static int ams_i2c_remove(struct i2c_client *client)
{
if (ams_info.has_device) {
+ ams_sensor_detach();
+
/* Disable interrupts */
ams_i2c_set_irq(AMS_IRQ_ALL, 0);
--- linux-2.6.33-rc3.orig/drivers/hwmon/ams/ams-pmu.c 2009-06-10
05:05:27.0 +0200
+++ linux-2.6.33-rc3/drivers/hwmon/ams/ams-pmu.c 2010-01-07
17:13:47.0 +0100
@@ -133,6 +133,8 @@ static void ams_pmu_get_xyz(s8 *x, s8 *y
static void ams_pmu_exit(void)
{
+ ams_sensor_detach();
+
/* Disable interrupts */
ams_pmu_set_irq(AMS_IRQ_ALL, 0);
--- linux-2.6.33-rc3.orig/drivers
Re: i2c_powermac: Kernel access of bad area
Hi Christian, hi Ben,
On Tue, 29 Dec 2009 20:34:15 +1100, Benjamin Herrenschmidt wrote:
On Mon, 2009-12-28 at 20:40 -0800, Christian Kujau wrote:
Hi there,
I was trying to find out if I still need i2c_powermac to get details off
/sys/devices/temperatures, since lm-sensors isn't working for me anyway.
This is because the macintosh/therm_adt746x driver doesn't follow the
standard hwmon sysfs interface. I would love to see that change.
After unloading this module, I noticed that reading certain objects would
fail with a segfault on the prompt and an Oops in the kernel log:
Definitely looks like we fail to clean things up on remove.
Indeed.
I don't have much time to look into this right now, so best is you file
a bug report on bugzilla.kernel.org, make sure I'm CCed.
Jean (added on CC), in case you already know what's up, please share :-)
I don't know anything about this for now. But I'll help if I can.
Christian, did you happen to try removing the i2c-powermac driver
before? I am curious if this is a recent problem, possibly caused by
changes to the i2c stack, or if older kernels already had this problem
and nobody ever noticed. I suspect the latter.
Looking at drivers/macintosh/therm_adt746x.c, the sysfs files are
created in thermostat_init() and removed in thermostat_exit(), which
are the driver's init and exit functions. These files are backed-up by
a per-device structure, so it looks like the wrong thing to do. I think
this is the problem: the sysfs files have a lifetime longer than the
data structure that is backing it up. This is somewhat different from
we fail to clean things up on remove: I suspect that even if
i2c-powermac was never ever loaded, the same problem would happen.
I think that sysfs files creation should be moved to the end of
probe_thermostat() and sysfs files removal should be moved to the
beginning of remove_thermostat(). Something like the totally untested
patch below (no ppc machine at hand):
---
drivers/macintosh/therm_adt746x.c | 34 --
1 file changed, 24 insertions(+), 10 deletions(-)
--- linux-2.6.33-rc3.orig/drivers/macintosh/therm_adt746x.c 2010-01-06
17:30:48.0 +0100
+++ linux-2.6.33-rc3/drivers/macintosh/therm_adt746x.c 2010-01-06
17:36:35.0 +0100
@@ -90,6 +90,8 @@ static struct task_struct *thread_therm
static void write_both_fan_speed(struct thermostat *th, int speed);
static void write_fan_speed(struct thermostat *th, int speed, int fan);
+static void thermostat_create_files(void);
+static void thermostat_remove_files(void);
static int
write_reg(struct thermostat* th, int reg, u8 data)
@@ -161,6 +163,8 @@ remove_thermostat(struct i2c_client *cli
struct thermostat *th = i2c_get_clientdata(client);
int i;
+ thermostat_remove_files();
+
if (thread_therm != NULL) {
kthread_stop(thread_therm);
}
@@ -449,6 +453,8 @@ static int probe_thermostat(struct i2c_c
return -ENOMEM;
}
+ thermostat_create_files();
+
return 0;
}
@@ -566,7 +572,6 @@ thermostat_init(void)
struct device_node* np;
const u32 *prop;
int i = 0, offset = 0;
- int err;
np = of_find_node_by_name(NULL, fan);
if (!np)
@@ -633,6 +638,17 @@ thermostat_init(void)
return -ENODEV;
}
+#ifndef CONFIG_I2C_POWERMAC
+ request_module(i2c-powermac);
+#endif
+
+ return i2c_add_driver(thermostat_driver);
+}
+
+static void thermostat_create_files(void)
+{
+ int err;
+
err = device_create_file(of_dev-dev, dev_attr_sensor1_temperature);
err |= device_create_file(of_dev-dev, dev_attr_sensor2_temperature);
err |= device_create_file(of_dev-dev, dev_attr_sensor1_limit);
@@ -647,16 +663,9 @@ thermostat_init(void)
if (err)
printk(KERN_WARNING
Failed to create tempertaure attribute file(s).\n);
-
-#ifndef CONFIG_I2C_POWERMAC
- request_module(i2c-powermac);
-#endif
-
- return i2c_add_driver(thermostat_driver);
}
-static void __exit
-thermostat_exit(void)
+static void thermostat_remove_files(void)
{
if (of_dev) {
device_remove_file(of_dev-dev, dev_attr_sensor1_temperature);
@@ -673,9 +682,14 @@ thermostat_exit(void)
device_remove_file(of_dev-dev,
dev_attr_sensor2_fan_speed);
- of_device_unregister(of_dev);
}
+}
+
+static void __exit
+thermostat_exit(void)
+{
i2c_del_driver(thermostat_driver);
+ of_device_unregister(of_dev);
}
module_init(thermostat_init);
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c-powermac fails
On Thu, 15 Oct 2009 16:05:13 +0200, Jean Delvare wrote: On Thu, 15 Oct 2009 22:19:19 +1100, Benjamin Herrenschmidt wrote: On Thu, 2009-10-15 at 12:49 +0200, Jean Delvare wrote: Oh. Well, if that was the case, we would see errors all the time, not just during initialization, right? Or does the I2C clock frequency change over time somehow? No but maybe we are a bit on the limit of the device and some registers take long to respond than others ? Unlikely. The ADT7460 can run at I2C clock rates up to 400 kHz while the Keywest I2C runs at 25, 50 or 100 kHz if I read the code properly. I don't know what exact speed is used on Tim's system, apparently it is read from the hardware in the device tree directly? We could have low_i2c.c log the I2C clock frequency and/or try to force the lowest speed (25 kHz) and see if it helps, but I very much doubt it. And I'd rather wait for Tim to report the result with my last patch first. Ben, wouldn't this recent patch of yours be worth testing too? http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=11a50873ef2b3c1c3fe99a661c22c08f35d93553 If it solves problems at resume time, I guess it might also solve problems at boot time? -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c-powermac fails
On Thu, 15 Oct 2009 08:26:15 +1100, Benjamin Herrenschmidt wrote: On Wed, 2009-10-14 at 23:02 +0200, Jean Delvare wrote: Hi all, On Tue, 13 Oct 2009 11:49:48 +0200, Jean Delvare wrote: I2C bus being setup too fast sounds more likely. It might be worth adding an arbitrary delay after initialization, just to see if it helps. Not sure where though, as I'm not familiar with the Powermac initialization steps. Maybe right before i2c_add_adapter() in i2c_powermac_probe? Tim, can you please give a try to this patch? Obviously your machine will take 5 additional seconds to boot, and this isn't meant as a real fix, but if it helps, this will be an interesting hint for further debugging attempts. Oh, I was actually thinking about the frequency of the I2C clock :-) Oh. Well, if that was the case, we would see errors all the time, not just during initialization, right? Or does the I2C clock frequency change over time somehow? -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c-powermac fails
On Thu, 15 Oct 2009 22:19:19 +1100, Benjamin Herrenschmidt wrote: On Thu, 2009-10-15 at 12:49 +0200, Jean Delvare wrote: Oh. Well, if that was the case, we would see errors all the time, not just during initialization, right? Or does the I2C clock frequency change over time somehow? No but maybe we are a bit on the limit of the device and some registers take long to respond than others ? Unlikely. The ADT7460 can run at I2C clock rates up to 400 kHz while the Keywest I2C runs at 25, 50 or 100 kHz if I read the code properly. I don't know what exact speed is used on Tim's system, apparently it is read from the hardware in the device tree directly? We could have low_i2c.c log the I2C clock frequency and/or try to force the lowest speed (25 kHz) and see if it helps, but I very much doubt it. And I'd rather wait for Tim to report the result with my last patch first. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] therm_adt746x: Don't access non-existing register
The ADT746x don't have any register at sub-address 0, so better use an
existing register for the initial test read.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Colin Leroy co...@colino.net
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
---
Tim, I don't really expect this to solve your problem, but who knows...
Care to give it a try, just in case?
drivers/macintosh/therm_adt746x.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-2.6.32-rc4.orig/drivers/macintosh/therm_adt746x.c 2009-10-12
11:53:59.0 +0200
+++ linux-2.6.32-rc4/drivers/macintosh/therm_adt746x.c 2009-10-14
17:27:46.0 +0200
@@ -387,7 +387,7 @@ static int probe_thermostat(struct i2c_c
i2c_set_clientdata(client, th);
th-clt = client;
- rc = read_reg(th, 0);
+ rc = read_reg(th, CONFIG_REG);
if (rc 0) {
dev_err(client-dev, Thermostat failed to read config!\n);
kfree(th);
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c-powermac fails
Hi all, On Tue, 13 Oct 2009 11:49:48 +0200, Jean Delvare wrote: I2C bus being setup too fast sounds more likely. It might be worth adding an arbitrary delay after initialization, just to see if it helps. Not sure where though, as I'm not familiar with the Powermac initialization steps. Maybe right before i2c_add_adapter() in i2c_powermac_probe? Tim, can you please give a try to this patch? Obviously your machine will take 5 additional seconds to boot, and this isn't meant as a real fix, but if it helps, this will be an interesting hint for further debugging attempts. --- kernel32.orig/drivers/macintosh/therm_adt746x.c +++ kernel32/drivers/macintosh/therm_adt746x.c @@ -380,6 +380,7 @@ static int probe_thermostat(struct i2c_c if (thermostat) return 0; + msleep(5000); th = kzalloc(sizeof(struct thermostat), GFP_KERNEL); if (!th) return -ENOMEM; -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
i2c-powermac fails
Hi Ben, Paul, I had a report by Tim Shepard (Cc'd) that the therm_adt746x driver sometimes fails to initialize on his PowerBook G4 running kernel 2.6.31. The following error message can be seen in the logs when the failure happens: therm_adt746x 7-002e: Thermostat failed to read config! After enabling low-level i2c debugging, it turns out that the problem is caused by low-level errors at the I2C bus level: PowerMac i2c bus pmu 2 registered PowerMac i2c bus pmu 1 registered PowerMac i2c bus mac-io 0 registered low_i2c:xfer() chan=0, addrdir=0x5d, mode=4, subsize=1, subaddr=0x0, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 6) low_i2c:KW: NAK on address low_i2c:xfer error -6 i2c-adapter i2c-7: I2C transfer at 0x2e failed, size 2, err -6 therm_adt746x 7-002e: Thermostat failed to read config! PowerMac i2c bus uni-n 0 registered So apparently the I2C controller doesn't see the ack from the ADT7467. However the ADT7467 is a SMBus-compliant device, so it must always ack his address. It is worth noting that many other I2C errors happen and go unnoticed. Below is the log of a successful therm_adt746x registration: PowerMac i2c bus pmu 2 registered PowerMac i2c bus pmu 1 registered PowerMac i2c bus mac-io 0 registered low_i2c:xfer() chan=0, addrdir=0x5d, mode=4, subsize=1, subaddr=0x0, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 2) low_i2c:kw_handle_interrupt(state_read, isr: 5) adt746x: ADT7467 initializing low_i2c:xfer() chan=0, addrdir=0x5d, mode=4, subsize=1, subaddr=0x6b, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 2) low_i2c:kw_handle_interrupt(state_read, isr: 5) low_i2c:xfer() chan=0, addrdir=0x5c, mode=3, subsize=1, subaddr=0x6b, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 6) low_i2c:KW: NAK on address low_i2c:xfer error -6 i2c-adapter i2c-7: I2C transfer at 0x2e failed, size 2, err -6 low_i2c:xfer() chan=0, addrdir=0x5d, mode=4, subsize=1, subaddr=0x6a, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 2) low_i2c:kw_handle_interrupt(state_read, isr: 1) low_i2c:kw_handle_interrupt(state_stop, isr: 4) low_i2c:xfer() chan=0, addrdir=0x5c, mode=3, subsize=1, subaddr=0x6a, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 ieee1394: Host added: ID:BUS[0-00:1023] GUID[001124fffed61a88] low_i2c:kw_handle_interrupt(state_addr, isr: 6) low_i2c:KW: NAK on address low_i2c:xfer error -6 i2c-adapter i2c-7: I2C transfer at 0x2e failed, size 2, err -6 low_i2c:xfer() chan=0, addrdir=0x5d, mode=4, subsize=1, subaddr=0x6c, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 2) low_i2c:kw_handle_interrupt(state_read, isr: 5) low_i2c:xfer() chan=0, addrdir=0x5c, mode=3, subsize=1, subaddr=0x6c, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 2) low_i2c:kw_handle_interrupt(state_write, isr: 1) low_i2c:kw_handle_interrupt(state_stop, isr: 4) adt746x: Lowering max temperatures from 81, 80, 87 to 70, 50, 70 low_i2c:xfer() chan=0, addrdir=0x5d, mode=4, subsize=1, subaddr=0x5c, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 eth0: Link is up at 1000 Mbps, full-duplex. low_i2c:kw_handle_interrupt(state_addr, isr: 6) low_i2c:KW: NAK on address low_i2c:xfer error -6 i2c-adapter i2c-7: I2C transfer at 0x2e failed, size 2, err -6 low_i2c:xfer() chan=0, addrdir=0x5c, mode=3, subsize=1, subaddr=0x5c, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 6) low_i2c:KW: NAK on address low_i2c:xfer error -6 i2c-adapter i2c-7: I2C transfer at 0x2e failed, size 2, err -6 low_i2c:xfer() chan=0, addrdir=0x5c, mode=3, subsize=1, subaddr=0x30, 1 bytes, bus /un...@f800/i...@f8001000/i2c-...@0 low_i2c:kw_handle_interrupt(state_addr, isr: 2) low_i2c:kw_handle_interrupt(state_write, isr: 1) low_i2c:kw_handle_interrupt(state_stop, isr: 4) PowerMac i2c bus uni-n 0 registered As you can see there are 4 errors, but the config register read doesn't fail so this is considered a success. Ever heard of this problem? One very interesting thing I've noticed is that therm_adt746x register access _after_ the initialization works reliably. Errors only happen in probe_thermostat(). This makes me suspect that the problem is either a low level initialization happening too late, or another initialization step happening in parallel and interfering with probe_thermostat(). Tim found evidences in older boot logs that the problem isn't new and was already present back in kernel 2.6.24 at least. Any idea what the problem can be and/or how to debug it further? -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: i2c-powermac fails
On Tue, 13 Oct 2009 20:32:28 +1100, Benjamin Herrenschmidt wrote: On Tue, 2009-10-13 at 11:23 +0200, Jean Delvare wrote: Hi Ben, Paul, I had a report by Tim Shepard (Cc'd) that the therm_adt746x driver sometimes fails to initialize on his PowerBook G4 running kernel 2.6.31. The following error message can be seen in the logs when the failure happens: therm_adt746x 7-002e: Thermostat failed to read config! After enabling low-level i2c debugging, it turns out that the problem is caused by low-level errors at the I2C bus level: Nothing comes to mind immediately, but I'll have another look tomorrow. Maybe we are configuring the i2c bus too fast ? Another possibility would be that the device needs some retries ... I guess that retrying would work around the problem, yes. But I do not think this is the proper solution. If retries were needed, they would be needed all the time, not just at initialization time. And as I said, the SMBus specification says that devices have to always ack their slave address (they can always delay the transaction later if they need more time) so I am reasonably certain that the ADT7467 does ack his address always. If it seems otherwise, this suggests that either the message was not properly sent on the bus (so the ADT7467 did not have anything to ack), or the ADT7467's ack went on the bus but the I2C master didn't see it. I2C bus being setup too fast sounds more likely. It might be worth adding an arbitrary delay after initialization, just to see if it helps. Not sure where though, as I'm not familiar with the Powermac initialization steps. Maybe right before i2c_add_adapter() in i2c_powermac_probe? -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sound: Don't assume i2c device probing always succeeds
Hi Takashi,
On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
At Wed, 30 Sep 2009 18:55:05 +0200,
Jean Delvare wrote:
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
Yes, indeed I prefer NULL check because the user can know the error
at the right place. I share your concern about the code addition,
though :)
I already made a patch below, but it's totally untested.
It'd be helpful if someone can do review and build-test it.
thanks,
Takashi
---
diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
index f0ebc97..0f810c8 100644
--- a/sound/aoa/codecs/tas.c
+++ b/sound/aoa/codecs/tas.c
@@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
client = i2c_new_device(adapter, info);
if (!client)
return -ENODEV;
+ if (!client-driver) {
+ i2c_unregister_device(client);
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
index 835fa19..473c5a6 100644
--- a/sound/ppc/keywest.c
+++ b/sound/ppc/keywest.c
@@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter
*adapter)
strlcpy(info.type, keywest, I2C_NAME_SIZE);
info.addr = keywest_ctx-addr;
keywest_ctx-client = i2c_new_device(adapter, info);
+ if (!keywest_ctx-client)
+ return -ENODEV;
+ if (!keywest_ctx-client-driver) {
+ i2c_unregister_device(keywest_ctx-client);
+ keywest_ctx-client = NULL;
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
This looks good to me. Please add the following comment before the
client-driver check in both drivers:
/*
* We know the driver is already loaded, so the device should be
* already bound. If not it means binding failed, and then there
* is no point in keeping the device instantiated.
*/
Otherwise it's a little difficult to understand why the check is there.
Fair enough. I applied the patch with the comment now.
Thanks!
I see this is upstream now. While the keywest fix was essentially
theoretical, the tas one addresses a crash which really could happen,
so I think it would be worth sending to stable for 2.6.31. What do you
think? Will you take care, or do you want me to?
Thanks,
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] macintosh: Don't assume i2c device probing always succeeds
If i2c device probing fails, then there is no driver to dereference
after calling i2c_new_device(). Stop assuming that probing will always
succeed, to avoid NULL pointer dereferences. We have an easier access
to the driver anyway.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Tested-by: Tim Shepard s...@alum.mit.edu
Cc: Colin Leroy co...@colino.net
---
This fixes a real-world oops and should be pushed to Linus ASAP, thanks.
drivers/macintosh/therm_adt746x.c |4 +++-
drivers/macintosh/therm_pm72.c |4 +++-
drivers/macintosh/windfarm_lm75_sensor.c|4 +++-
drivers/macintosh/windfarm_max6690_sensor.c |4 +++-
drivers/macintosh/windfarm_smu_sat.c|4 +++-
5 files changed, 15 insertions(+), 5 deletions(-)
--- linux-2.6.32-rc1.orig/drivers/macintosh/therm_adt746x.c 2009-09-30
15:12:12.0 +0200
+++ linux-2.6.32-rc1/drivers/macintosh/therm_adt746x.c 2009-09-30
15:13:04.0 +0200
@@ -124,6 +124,8 @@ read_reg(struct thermostat* th, int reg)
return data;
}
+static struct i2c_driver thermostat_driver;
+
static int
attach_thermostat(struct i2c_adapter *adapter)
{
@@ -148,7 +150,7 @@ attach_thermostat(struct i2c_adapter *ad
* Let i2c-core delete that device on driver removal.
* This is safe because i2c-core holds the core_lock mutex for us.
*/
- list_add_tail(client-detected, client-driver-clients);
+ list_add_tail(client-detected, thermostat_driver.clients);
return 0;
}
--- linux-2.6.32-rc1.orig/drivers/macintosh/therm_pm72.c2009-09-30
15:12:12.0 +0200
+++ linux-2.6.32-rc1/drivers/macintosh/therm_pm72.c 2009-09-30
15:13:04.0 +0200
@@ -286,6 +286,8 @@ struct fcu_fan_tablefcu_fans[] = {
},
};
+static struct i2c_driver therm_pm72_driver;
+
/*
* Utility function to create an i2c_client structure and
* attach it to one of u3 adapters
@@ -318,7 +320,7 @@ static struct i2c_client *attach_i2c_chi
* Let i2c-core delete that device on driver removal.
* This is safe because i2c-core holds the core_lock mutex for us.
*/
- list_add_tail(clt-detected, clt-driver-clients);
+ list_add_tail(clt-detected, therm_pm72_driver.clients);
return clt;
}
--- linux-2.6.32-rc1.orig/drivers/macintosh/windfarm_lm75_sensor.c
2009-09-30 15:12:12.0 +0200
+++ linux-2.6.32-rc1/drivers/macintosh/windfarm_lm75_sensor.c 2009-09-30
15:13:04.0 +0200
@@ -115,6 +115,8 @@ static int wf_lm75_probe(struct i2c_clie
return rc;
}
+static struct i2c_driver wf_lm75_driver;
+
static struct i2c_client *wf_lm75_create(struct i2c_adapter *adapter,
u8 addr, int ds1775,
const char *loc)
@@ -157,7 +159,7 @@ static struct i2c_client *wf_lm75_create
* Let i2c-core delete that device on driver removal.
* This is safe because i2c-core holds the core_lock mutex for us.
*/
- list_add_tail(client-detected, client-driver-clients);
+ list_add_tail(client-detected, wf_lm75_driver.clients);
return client;
fail:
return NULL;
--- linux-2.6.32-rc1.orig/drivers/macintosh/windfarm_max6690_sensor.c
2009-09-30 15:12:12.0 +0200
+++ linux-2.6.32-rc1/drivers/macintosh/windfarm_max6690_sensor.c
2009-09-30 15:13:04.0 +0200
@@ -88,6 +88,8 @@ static int wf_max6690_probe(struct i2c_c
return rc;
}
+static struct i2c_driver wf_max6690_driver;
+
static struct i2c_client *wf_max6690_create(struct i2c_adapter *adapter,
u8 addr, const char *loc)
{
@@ -119,7 +121,7 @@ static struct i2c_client *wf_max6690_cre
* Let i2c-core delete that device on driver removal.
* This is safe because i2c-core holds the core_lock mutex for us.
*/
- list_add_tail(client-detected, client-driver-clients);
+ list_add_tail(client-detected, wf_max6690_driver.clients);
return client;
fail:
--- linux-2.6.32-rc1.orig/drivers/macintosh/windfarm_smu_sat.c 2009-09-30
15:12:12.0 +0200
+++ linux-2.6.32-rc1/drivers/macintosh/windfarm_smu_sat.c 2009-09-30
15:13:04.0 +0200
@@ -194,6 +194,8 @@ static struct wf_sensor_ops wf_sat_ops =
.owner = THIS_MODULE,
};
+static struct i2c_driver wf_sat_driver;
+
static void wf_sat_create(struct i2c_adapter *adapter, struct device_node *dev)
{
struct i2c_board_info info;
@@ -222,7 +224,7 @@ static void wf_sat_create(struct i2c_ada
* Let i2c-core delete that device on driver removal.
* This is safe because i2c-core holds the core_lock mutex for us.
*/
- list_add_tail(client-detected, client-driver-clients);
+ list_add_tail(client-detected, wf_sat_driver.clients);
}
static int wf_sat_probe(struct i2c_client *client,
--
Jean Delvare
[PATCH] sound: Don't assume i2c device probing always succeeds
If i2c device probing fails, then there is no driver to dereference after calling i2c_new_device(). Stop assuming that probing will always succeed, to avoid NULL pointer dereferences. We have an easier access to the driver anyway. Reported-by: Tim Shepard s...@alum.mit.edu Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Johannes Berg johan...@sipsolutions.net --- The code is similar to the one in therm_adt746x, for which Tim reported a real-world oops, so it should be fixed ASAP. sound/aoa/codecs/onyx.c |4 +++- sound/aoa/codecs/tas.c |4 +++- sound/ppc/keywest.c |4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) --- linux-2.6.32-rc1.orig/sound/aoa/codecs/onyx.c 2009-09-30 15:13:12.0 +0200 +++ linux-2.6.32-rc1/sound/aoa/codecs/onyx.c2009-09-30 15:13:58.0 +0200 @@ -996,6 +996,8 @@ static void onyx_exit_codec(struct aoa_c onyx-codec.soundbus_dev-detach_codec(onyx-codec.soundbus_dev, onyx); } +static struct i2c_driver onyx_driver; + static int onyx_create(struct i2c_adapter *adapter, struct device_node *node, int addr) @@ -1027,7 +1029,7 @@ static int onyx_create(struct i2c_adapte * Let i2c-core delete that device on driver removal. * This is safe because i2c-core holds the core_lock mutex for us. */ - list_add_tail(client-detected, client-driver-clients); + list_add_tail(client-detected, onyx_driver.clients); return 0; } --- linux-2.6.32-rc1.orig/sound/aoa/codecs/tas.c2009-09-30 15:13:12.0 +0200 +++ linux-2.6.32-rc1/sound/aoa/codecs/tas.c 2009-09-30 15:13:58.0 +0200 @@ -882,6 +882,8 @@ static void tas_exit_codec(struct aoa_co } +static struct i2c_driver tas_driver; + static int tas_create(struct i2c_adapter *adapter, struct device_node *node, int addr) @@ -902,7 +904,7 @@ static int tas_create(struct i2c_adapter * Let i2c-core delete that device on driver removal. * This is safe because i2c-core holds the core_lock mutex for us. */ - list_add_tail(client-detected, client-driver-clients); + list_add_tail(client-detected, tas_driver.clients); return 0; } --- linux-2.6.32-rc1.orig/sound/ppc/keywest.c 2009-09-30 15:13:12.0 +0200 +++ linux-2.6.32-rc1/sound/ppc/keywest.c2009-09-30 15:13:58.0 +0200 @@ -40,6 +40,8 @@ static int keywest_probe(struct i2c_clie return 0; } +struct i2c_driver keywest_driver; + /* * This is kind of a hack, best would be to turn powermac to fixed i2c * bus numbers and declare the sound device as part of platform @@ -65,7 +67,7 @@ static int keywest_attach_adapter(struct * This is safe because i2c-core holds the core_lock mutex for us. */ list_add_tail(keywest_ctx-client-detected, - keywest_ctx-client-driver-clients); + keywest_driver.clients); return 0; } -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sound: Don't assume i2c device probing always succeeds
Hi Takashi, Thanks for the swift reply. On Wed, 30 Sep 2009 16:13:49 +0200, Takashi Iwai wrote: At Wed, 30 Sep 2009 15:25:42 +0200, Jean Delvare wrote: If i2c device probing fails, then there is no driver to dereference after calling i2c_new_device(). Stop assuming that probing will always succeed, to avoid NULL pointer dereferences. We have an easier access to the driver anyway. Reported-by: Tim Shepard s...@alum.mit.edu Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Johannes Berg johan...@sipsolutions.net --- The code is similar to the one in therm_adt746x, for which Tim reported a real-world oops, so it should be fixed ASAP. Jean, thanks for the patch. I'm just wondering whether the additional NULL check of client-driver would be enough? If yes, sound/aoa/onyx.c has it, at least, and we can add the similar checks to the rest, too. The NULL check of client-driver, if followed by a call to i2c_unregister_device(), would indeed be enough. But unlike the onyx driver which we know we sometimes load erroneously, the other drivers should never fail. I am reluctant to add code to handle error cases which are not supposed to happen, which is why I prefer my proposed fix: it does not add code. That being said, I will be happy with any solution that fixes the problem, so if you prefer client-driver NULL checks, I can send a patch doing this. Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sound: Don't assume i2c device probing always succeeds
On Wed, 30 Sep 2009 17:05:11 +0200, Johannes Berg wrote: On Wed, 2009-09-30 at 17:00 +0200, Jean Delvare wrote: The NULL check of client-driver, if followed by a call to i2c_unregister_device(), would indeed be enough. But unlike the onyx driver which we know we sometimes load erroneously, the other drivers should never fail. All of these drivers can be loaded manually and then fail though, or am I misunderstanding something? I don't think so. At least tas and keywest have checks before they attempt to instantiate an i2c client, which I think are reliable. The onyx case is different because apparently some machines have it but are difficult to detect: /* if that didn't work, try desperate mode for older * machines that have stuff missing from the device tree */ And then we have to attempt to instantiate i2c devices at a not completely known address, and that may fail. I think this is the reason why onyx has this extra client-driver NULL check and the other two drivers do not. I would really love if someone with good knowledge of the device tree on mac would convert all these hacks to proper i2c device declarations. All the infrastructure is available already, but I don't know enough about open firmware and mac the device tree to do it myself. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sound: Don't assume i2c device probing always succeeds
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
Yes, indeed I prefer NULL check because the user can know the error
at the right place. I share your concern about the code addition,
though :)
I already made a patch below, but it's totally untested.
It'd be helpful if someone can do review and build-test it.
thanks,
Takashi
---
diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
index f0ebc97..0f810c8 100644
--- a/sound/aoa/codecs/tas.c
+++ b/sound/aoa/codecs/tas.c
@@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
client = i2c_new_device(adapter, info);
if (!client)
return -ENODEV;
+ if (!client-driver) {
+ i2c_unregister_device(client);
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
index 835fa19..473c5a6 100644
--- a/sound/ppc/keywest.c
+++ b/sound/ppc/keywest.c
@@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter
*adapter)
strlcpy(info.type, keywest, I2C_NAME_SIZE);
info.addr = keywest_ctx-addr;
keywest_ctx-client = i2c_new_device(adapter, info);
+ if (!keywest_ctx-client)
+ return -ENODEV;
+ if (!keywest_ctx-client-driver) {
+ i2c_unregister_device(keywest_ctx-client);
+ keywest_ctx-client = NULL;
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
This looks good to me. Please add the following comment before the
client-driver check in both drivers:
/*
* We know the driver is already loaded, so the device should be
* already bound. If not it means binding failed, and then there
* is no point in keeping the device instantiated.
*/
Otherwise it's a little difficult to understand why the check is there.
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] i2c-mpc: Do not generate STOP after read.
On Mon, 28 Sep 2009 09:30:32 +0200, Joakim Tjernlund wrote: Jean Delvare kh...@linux-fr.org wrote on 28/09/2009 09:28:09: On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote: Jean, I just noticed you pull request for i2c on LKML but I didn't see this patch nor have I got any feedback from you. What is your view? My view is that i2c-mpc is nor under my jurisdiction, so I did not, and will not, pay any attention to it. Ah, that explains it. Who then will look after i2c-mpc? Kumar? What does MAINTAINERS say? -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] i2c-mpc: Do not generate STOP after read.
On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote: Jean, I just noticed you pull request for i2c on LKML but I didn't see this patch nor have I got any feedback from you. What is your view? My view is that i2c-mpc is nor under my jurisdiction, so I did not, and will not, pay any attention to it. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Removing deprecated drivers from drivers/i2c/chips
Hi Wolfram, On Wed, 9 Sep 2009 23:22:47 +0200, Wolfram Sang wrote: continuing the quest to clean up and ultimately remove the drivers/i2c/chips directory, this patch series removes three drivers for GPIO-expanders which are obsoleted and marked as deprecated for more than a year. The newer (and better) drivers can be found in drivers/gpio. As it is ensured that the newer drivers cover the same i2c_device_ids, all platform_devices will still match. Some defconfig updates may be necessary though, but according to [1] this is left to the arch|platform-maintainers (also as most defconfigs are quite outdated). For that reason, I put the relevant arch-mailing-lists to Cc. Comments are welcome. Looks very good, I'll apply the 3 patches removing the legacy drivers. Not sure about the patch to drivers/gpio/pcf857x.c, as there is no gpio tree and no maintainer either AFAIK, I guess I shall pick it too? Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] keywest: Get rid of useless i2c_device_name() macro
The i2c_device_name() macro is used only once and doesn't have much
value, it hurts redability more than it helps. Get rid of it.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
---
sound/ppc/keywest.c |6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
--- linux-2.6.30-rc1.orig/sound/ppc/keywest.c 2009-04-10 16:22:08.0
+0200
+++ linux-2.6.30-rc1/sound/ppc/keywest.c2009-04-10 16:25:16.0
+0200
@@ -33,10 +33,6 @@
static struct pmac_keywest *keywest_ctx;
-#ifndef i2c_device_name
-#define i2c_device_name(x) ((x)-name)
-#endif
-
static int keywest_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -56,7 +52,7 @@ static int keywest_attach_adapter(struct
if (! keywest_ctx)
return -EINVAL;
- if (strncmp(i2c_device_name(adapter), mac-io, 6))
+ if (strncmp(adapter-name, mac-io, 6))
return 0; /* ignored */
memset(info, 0, sizeof(struct i2c_board_info));
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Need a patch tested on a windtunnel powermac
On Mon, 4 May 2009 23:02:16 +0200, Giuliano Pochini wrote: On Thu, 30 Apr 2009 09:30:59 +1000 Paul Mackerras pau...@samba.org wrote: Does anyone here still have a windtunnel powermac (PowerMac3,6) machine running Linux? We need someone to test Jean Delvare's patch to the therm_windtunnel driver. The patch is at: http://patchwork.ozlabs.org/patch/26095/ I don't have a windtunnel machine myself - I have a PowerMac3,5 but not a PowerMac3,6. I have a dual-G4 MDD (/proc/cpuinfo says PowerMac3,6). On: Linux Jay 2.6.30-rc4 #1 SMP Sun May 3 16:20:08 CEST 2009 ppc 7455, altivec supported PowerMac3,6 GNU/Linux plus the patch above seems working fine as usual: temperature reports are written in the logs and fan speed changes correctly. therm_windtunnel is not compiled as module. I tested it for a few hours only. Thanks for the report, Giuliano! I've added the information to the wiki. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Need a patch tested on a windtunnel powermac
Hi Josh, On Mon, 4 May 2009 10:52:32 -0400, Josh Boyer wrote: On Thu, Apr 30, 2009 at 03:17:08PM -0400, Josh Boyer wrote: On Wed, Apr 29, 2009 at 08:24:48PM -0400, Josh Boyer wrote: On Thu, Apr 30, 2009 at 09:30:59AM +1000, Paul Mackerras wrote: Does anyone here still have a windtunnel powermac (PowerMac3,6) machine running Linux? We need someone to test Jean Delvare's patch to the therm_windtunnel driver. The patch is at: http://patchwork.ozlabs.org/patch/26095/ I don't have a windtunnel machine myself - I have a PowerMac3,5 but not a PowerMac3,6. I might have one. I'll pull the G4 I have out of it's box and check tomorrow. Amazingly enough, I do. It's running some crusty-arse gentoo install from 2005, so I need to install something newer on it. Hopefully that goes smoothly today... Ok, installed 2.6.30-rc4 plus Jean's patch and it appears to do something correctly: [jwbo...@localhost macintosh]$ sudo modprobe therm_windtunnel [jwbo...@localhost macintosh]$ dmesg snip DS1775 digital thermometer [...@49] Temp: 52.5 C Hyst: 75.0 C OS: 80.0 C ADM1030 fan controller [...@2c] Reducing overheating limit to 65.0 C (Hyst: 60.0 C) CPU-temp: 52.9 C, Case: 31.3 C, Fan: 0 (tuned -11) [jwbo...@localhost macintosh]$ uname -a Linux localhost.localdomain 2.6.30-rc4 #1 SMP Mon May 4 10:20:39 EDT 2009 ppc ppc ppc GNU/Linux Thanks for testing and reporting. This completes the testing of all macintosh thermal drivers. For completeness, as I see you built the driver as a module, could you try unloading and reloading it? Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Need a patch tested on a windtunnel powermac
On Mon, 4 May 2009 11:42:08 -0400, Josh Boyer wrote: On Mon, May 04, 2009 at 05:40:01PM +0200, Jean Delvare wrote: For completeness, as I see you built the driver as a module, could you try unloading and reloading it? Sure: [jwbo...@localhost macintosh]$ sudo rmmod therm_windtunnel [jwbo...@localhost macintosh]$ sudo modprobe therm_windtunnel [jwbo...@localhost macintosh]$ dmesg snip DS1775 digital thermometer [...@49] Temp: 53.6 C Hyst: 60.0 C OS: 65.0 C ADM1030 fan controller [...@2c] CPU-temp: 53.6 C, Case: 31.8 C, Fan: 0 (tuned -11) Seems to work fine. Excellent. Thanks again! -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: Convert to a new-style i2c drivers
On Sat, 2 May 2009 15:07:12 +1000, Paul Mackerras wrote: Jean Delvare writes: The legacy i2c binding model is going away soon, so convert the macintosh windfarm drivers to the new model or they will break. All works OK on my quad G5 (PowerMac11,2). Tested-by: Paul Mackerras pau...@samba.org Great, thanks for the test results Paul! Only therm_windtunnel still needs to be tested. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] therm_windtunnel: Convert to a new-style i2c driver
On Thu, 16 Apr 2009 23:01:01 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc
therm_windtunnel driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
---
Can someone please test this patch for me? I could only build-test it.
Remember that you need this patch applied:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=935298696f469c0e07c73be687bd055878074ce0
Paul, any progress on this?
Thanks.
drivers/macintosh/therm_windtunnel.c | 126
--
1 file changed, 60 insertions(+), 66 deletions(-)
--- linux-2.6.30-rc2.orig/drivers/macintosh/therm_windtunnel.c
2009-03-24 13:40:47.0 +0100
+++ linux-2.6.30-rc2/drivers/macintosh/therm_windtunnel.c 2009-04-16
22:56:39.0 +0200
@@ -48,16 +48,6 @@
#define LOG_TEMP 0 /* continously log
temperature */
-static int do_probe( struct i2c_adapter *adapter, int
addr, int kind);
-
-/* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */
-static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
- 0x4c, 0x4d, 0x4e, 0x4f,
- 0x2c, 0x2d, 0x2e, 0x2f,
- I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
static struct {
volatile intrunning;
struct task_struct *poll_task;
@@ -315,53 +305,54 @@ static int control_loop(void *dummy)
static int
do_attach( struct i2c_adapter *adapter )
{
- int ret = 0;
+ /* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */
+ static const unsigned short scan_ds1775[] = {
+ 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f,
+ I2C_CLIENT_END
+ };
+ static const unsigned short scan_adm1030[] = {
+ 0x2c, 0x2d, 0x2e, 0x2f,
+ I2C_CLIENT_END
+ };
if( strncmp(adapter-name, uni-n, 5) )
return 0;
if( !x.running ) {
- ret = i2c_probe( adapter, addr_data, do_probe );
+ struct i2c_board_info info;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, therm_ds1775, I2C_NAME_SIZE);
+ i2c_new_probed_device(adapter, info, scan_ds1775);
+
+ strlcpy(info.type, therm_adm1030, I2C_NAME_SIZE);
+ i2c_new_probed_device(adapter, info, scan_adm1030);
+
if( x.thermostat x.fan ) {
x.running = 1;
x.poll_task = kthread_run(control_loop, NULL, g4fand);
}
}
- return ret;
+ return 0;
}
static int
-do_detach( struct i2c_client *client )
+do_remove(struct i2c_client *client)
{
- int err;
-
- if( (err=i2c_detach_client(client)) )
- printk(KERN_ERR failed to detach thermostat client\n);
- else {
- if( x.running ) {
- x.running = 0;
- kthread_stop(x.poll_task);
- x.poll_task = NULL;
- }
- if( client == x.thermostat )
- x.thermostat = NULL;
- else if( client == x.fan )
- x.fan = NULL;
- else {
- printk(KERN_ERR g4fan: bad client\n);
- }
- kfree( client );
+ if (x.running) {
+ x.running = 0;
+ kthread_stop(x.poll_task);
+ x.poll_task = NULL;
}
- return err;
-}
+ if (client == x.thermostat)
+ x.thermostat = NULL;
+ else if (client == x.fan)
+ x.fan = NULL;
+ else
+ printk(KERN_ERR g4fan: bad client\n);
-static struct i2c_driver g4fan_driver = {
- .driver = {
- .name = therm_windtunnel,
- },
- .attach_adapter = do_attach,
- .detach_client = do_detach,
-};
+ return 0;
+}
static int
attach_fan( struct i2c_client *cl )
@@ -374,13 +365,8 @@ attach_fan( struct i2c_client *cl )
goto out;
printk(ADM1030 fan controller [...@%02x]\n, cl-addr );
- strlcpy( cl-name, ADM1030 fan controller, sizeof(cl-name) );
-
- if( !i2c_attach_client(cl) )
- x.fan = cl;
+ x.fan = cl;
out:
- if( cl != x.fan )
- kfree( cl );
return 0;
}
@@ -412,39 +398,47 @@ attach_thermostat( struct i2c_client *cl
x.temp = temp;
x.overheat_temp = os_temp;
x.overheat_hyst = hyst_temp;
-
- strlcpy( cl-name, DS1775 thermostat, sizeof(cl-name) );
-
- if( !i2c_attach_client(cl) )
- x.thermostat = cl;
+ x.thermostat = cl;
out:
- if( cl
Re: [PATCH] therm_adt746x: Convert to a new-style i2c driver
On Thu, 16 Apr 2009 22:59:27 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc
therm_adt746x driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
---
Can someone please test this patch for me? I could only build-test it.
Remember that you need this patch applied:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=935298696f469c0e07c73be687bd055878074ce0
Any tester for this patch, please?
Thanks.
drivers/macintosh/therm_adt746x.c | 84
++---
1 file changed, 42 insertions(+), 42 deletions(-)
--- linux-2.6.30-rc2.orig/drivers/macintosh/therm_adt746x.c 2009-04-16
21:06:31.0 +0200
+++ linux-2.6.30-rc2/drivers/macintosh/therm_adt746x.c2009-04-16
21:37:05.0 +0200
@@ -71,7 +71,7 @@ MODULE_PARM_DESC(verbose,Verbose log op
(default 0));
struct thermostat {
- struct i2c_client clt;
+ struct i2c_client *clt;
u8 temps[3];
u8 cached_temp[3];
u8 initial_limits[3];
@@ -86,9 +86,6 @@ static struct of_device * of_dev;
static struct thermostat* thermostat;
static struct task_struct *thread_therm = NULL;
-static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
- int busno);
-
static void write_both_fan_speed(struct thermostat *th, int speed);
static void write_fan_speed(struct thermostat *th, int speed, int fan);
@@ -100,7 +97,7 @@ write_reg(struct thermostat* th, int reg
tmp[0] = reg;
tmp[1] = data;
- rc = i2c_master_send(th-clt, (const char *)tmp, 2);
+ rc = i2c_master_send(th-clt, (const char *)tmp, 2);
if (rc 0)
return rc;
if (rc != 2)
@@ -115,12 +112,12 @@ read_reg(struct thermostat* th, int reg)
int rc;
reg_addr = (u8)reg;
- rc = i2c_master_send(th-clt, reg_addr, 1);
+ rc = i2c_master_send(th-clt, reg_addr, 1);
if (rc 0)
return rc;
if (rc != 1)
return -ENODEV;
- rc = i2c_master_recv(th-clt, (char *)data, 1);
+ rc = i2c_master_recv(th-clt, (char *)data, 1);
if (rc 0)
return rc;
return data;
@@ -130,26 +127,36 @@ static int
attach_thermostat(struct i2c_adapter *adapter)
{
unsigned long bus_no;
+ struct i2c_board_info info;
+ struct i2c_client *client;
if (strncmp(adapter-name, uni-n, 5))
return -ENODEV;
bus_no = simple_strtoul(adapter-name + 6, NULL, 10);
if (bus_no != therm_bus)
return -ENODEV;
- return attach_one_thermostat(adapter, therm_address, bus_no);
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, therm_adt746x, I2C_NAME_SIZE);
+ info.addr = therm_address;
+ client = i2c_new_device(adapter, info);
+ if (!client)
+ return -ENODEV;
+
+ /*
+ * Let i2c-core delete that device on driver removal.
+ * This is safe because i2c-core holds the core_lock mutex for us.
+ */
+ list_add_tail(client-detected, client-driver-clients);
+ return 0;
}
static int
-detach_thermostat(struct i2c_adapter *adapter)
+remove_thermostat(struct i2c_client *client)
{
- struct thermostat* th;
+ struct thermostat *th = i2c_get_clientdata(client);
int i;
- if (thermostat == NULL)
- return 0;
-
- th = thermostat;
-
if (thread_therm != NULL) {
kthread_stop(thread_therm);
}
@@ -165,8 +172,6 @@ detach_thermostat(struct i2c_adapter *ad
write_both_fan_speed(th, -1);
- i2c_detach_client(th-clt);
-
thermostat = NULL;
kfree(th);
@@ -174,14 +179,6 @@ detach_thermostat(struct i2c_adapter *ad
return 0;
}
-static struct i2c_driver thermostat_driver = {
- .driver = {
- .name = therm_adt746x,
- },
- .attach_adapter = attach_thermostat,
- .detach_adapter = detach_thermostat,
-};
-
static int read_fan_speed(struct thermostat *th, u8 addr)
{
u8 tmp[2];
@@ -369,8 +366,8 @@ static void set_limit(struct thermostat
th-limits[i] = default_limits_local[i] + limit_adjust;
}
-static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
- int busno)
+static int probe_thermostat(struct i2c_client *client,
+ const struct i2c_device_id *id)
{
struct thermostat* th;
int rc;
@@ -383,16 +380,12 @@ static int attach_one_thermostat(struct
if (!th)
return -ENOMEM;
- th-clt.addr = addr;
- th-clt.adapter = adapter;
- th-clt.driver
Re: [PATCH] therm_windtunnel: Convert to a new-style i2c driver
On Thu, 16 Apr 2009 23:01:01 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc
therm_windtunnel driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
---
Can someone please test this patch for me? I could only build-test it.
Remember that you need this patch applied:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=935298696f469c0e07c73be687bd055878074ce0
Any tester for this patch, please?
Thanks.
drivers/macintosh/therm_windtunnel.c | 126
--
1 file changed, 60 insertions(+), 66 deletions(-)
--- linux-2.6.30-rc2.orig/drivers/macintosh/therm_windtunnel.c
2009-03-24 13:40:47.0 +0100
+++ linux-2.6.30-rc2/drivers/macintosh/therm_windtunnel.c 2009-04-16
22:56:39.0 +0200
@@ -48,16 +48,6 @@
#define LOG_TEMP 0 /* continously log
temperature */
-static int do_probe( struct i2c_adapter *adapter, int
addr, int kind);
-
-/* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */
-static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
- 0x4c, 0x4d, 0x4e, 0x4f,
- 0x2c, 0x2d, 0x2e, 0x2f,
- I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
static struct {
volatile intrunning;
struct task_struct *poll_task;
@@ -315,53 +305,54 @@ static int control_loop(void *dummy)
static int
do_attach( struct i2c_adapter *adapter )
{
- int ret = 0;
+ /* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */
+ static const unsigned short scan_ds1775[] = {
+ 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f,
+ I2C_CLIENT_END
+ };
+ static const unsigned short scan_adm1030[] = {
+ 0x2c, 0x2d, 0x2e, 0x2f,
+ I2C_CLIENT_END
+ };
if( strncmp(adapter-name, uni-n, 5) )
return 0;
if( !x.running ) {
- ret = i2c_probe( adapter, addr_data, do_probe );
+ struct i2c_board_info info;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, therm_ds1775, I2C_NAME_SIZE);
+ i2c_new_probed_device(adapter, info, scan_ds1775);
+
+ strlcpy(info.type, therm_adm1030, I2C_NAME_SIZE);
+ i2c_new_probed_device(adapter, info, scan_adm1030);
+
if( x.thermostat x.fan ) {
x.running = 1;
x.poll_task = kthread_run(control_loop, NULL, g4fand);
}
}
- return ret;
+ return 0;
}
static int
-do_detach( struct i2c_client *client )
+do_remove(struct i2c_client *client)
{
- int err;
-
- if( (err=i2c_detach_client(client)) )
- printk(KERN_ERR failed to detach thermostat client\n);
- else {
- if( x.running ) {
- x.running = 0;
- kthread_stop(x.poll_task);
- x.poll_task = NULL;
- }
- if( client == x.thermostat )
- x.thermostat = NULL;
- else if( client == x.fan )
- x.fan = NULL;
- else {
- printk(KERN_ERR g4fan: bad client\n);
- }
- kfree( client );
+ if (x.running) {
+ x.running = 0;
+ kthread_stop(x.poll_task);
+ x.poll_task = NULL;
}
- return err;
-}
+ if (client == x.thermostat)
+ x.thermostat = NULL;
+ else if (client == x.fan)
+ x.fan = NULL;
+ else
+ printk(KERN_ERR g4fan: bad client\n);
-static struct i2c_driver g4fan_driver = {
- .driver = {
- .name = therm_windtunnel,
- },
- .attach_adapter = do_attach,
- .detach_client = do_detach,
-};
+ return 0;
+}
static int
attach_fan( struct i2c_client *cl )
@@ -374,13 +365,8 @@ attach_fan( struct i2c_client *cl )
goto out;
printk(ADM1030 fan controller [...@%02x]\n, cl-addr );
- strlcpy( cl-name, ADM1030 fan controller, sizeof(cl-name) );
-
- if( !i2c_attach_client(cl) )
- x.fan = cl;
+ x.fan = cl;
out:
- if( cl != x.fan )
- kfree( cl );
return 0;
}
@@ -412,39 +398,47 @@ attach_thermostat( struct i2c_client *cl
x.temp = temp;
x.overheat_temp = os_temp;
x.overheat_hyst = hyst_temp;
-
- strlcpy( cl-name, DS1775 thermostat, sizeof(cl-name) );
-
- if( !i2c_attach_client(cl) )
- x.thermostat = cl;
+ x.thermostat = cl;
out:
- if( cl
Re: [PATCH] keywest: Convert to new-style i2c driver
On Tue, 21 Apr 2009 11:23:00 +0200, Jean Delvare wrote: On Tue, 21 Apr 2009 08:31:00 +0200, Takashi Iwai wrote: At Mon, 20 Apr 2009 22:56:59 +0200, Jean Delvare wrote: The legacy i2c binding model is going away soon, so convert the ppc keywest sound driver to the new model or it will break. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Takashi Iwai ti...@suse.de --- Takashi, please push this patch to Linus quickly, as this is blocking the removal of the legacy i2c binding model, which is scheduled for 2.6.30. I did not get any test report for this one, but it's relatively simple so I am confident I got it right. Applied this one, too. Thanks. Thanks Takashi. BTW, I forgot to mention again that the new i2c binding model is functional since kernel 2.6.25, so for the external alsa driver tree you will want to guard these changes with appropriate ifdef magic. Err, make that 2.6.30, sorry. While the infrastructure was there since 2.6.25, the way I converted the sound drivers doesn't fit in what earlier kernels considered acceptable (the checks on which driver methods were implemented was a little too strict IMHO). So the converted drivers can only be used with kernels = 2.6.30. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] keywest: Convert to new-style i2c driver
Hi Paul, Takashi, On Tue, 21 Apr 2009 08:33:43 +0200, Takashi Iwai wrote: At Tue, 21 Apr 2009 08:34:02 +1000, Paul Mackerras wrote: Jean Delvare writes: Takashi, please push this patch to Linus quickly, as this is blocking the removal of the legacy i2c binding model, which is scheduled for 2.6.30. I really don't think you can remove it from Linus' tree at this stage in the 2.6.30 cycle. If it was going to be removed it should have been removed in the merge window. It would have happened if the developers/maintainers who see deprecation warnings when they build their drivers had paid attention to them. And actually some did, but not all. The remaining drivers are the ones nobody cared about, and this is the reason why I have to take care of them myself, that late in the development cycle. Note that the removal had already been scheduled for 2.6.29, and it did not happen. The set of legacy drivers did shrink a bit between 2.6.29-rc1 and 2.6.30-rc1 (thanks to Hans Verkuil) but not as much as it should have. Basically the number of remaining driver was halved. If the number of remaining drivers is halved with each release cycle, the legacy model is never going to be removed ;) Removing it now has too much risk of introducing regressions in my opinion. Not removing it now has a high risk of developers continuing to ignore the deprecation warnings and adding new legacy drivers, which I then must convert to the new model. This never ends. I know my behavior may seem a bit rude, but apparently this is the only way to get things to actually happen. I've been waiting for over a year already! I don't think the risk is that high, at least not for sound drivers. The conversions are fairly easy and if something really went wrong, fixing it is a matter of minutes. Please note that the removal of the legacy model isn't my goal per se. The fact is that the legacy model needs to be removed for further developments of i2c-core to happen, in particular the support of i2C bus multiplexers. There are patches waiting for inclusion since early February, which I can't take as long as the legacy i2c model is in. This is why I am pushing. I presume you have a development tree where you queue up commits for the i2c subsystem for the next merge window. I suggest you do the removal there now (or whenever you like) and push it to Linus in the next merge window. At least, the conversion patch Jean posted can be in 2.6.30, I think. As the old API is marked deprecated, it should be fixed sooner or later. Whether to remove the whole old i2c-binding in 2.6.30 is a different question, although I myself feel it's feasible. I have converted all remaining drivers by now: http://i2c.wiki.kernel.org/index.php/Legacy_drivers_to_be_converted It's really only a matter of getting them tested in time now. Given that most drivers are powermac ones, what I really need here is powermac users/maintainers to test my patches and report success or failure. Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
Hi Johannes, On Mon, 20 Apr 2009 23:04:52 +0200, Johannes Berg wrote: On Mon, 2009-04-20 at 22:54 +0200, Jean Delvare wrote: The legacy i2c binding model is going away soon, so convert the AOA codec drivers to the new model or they'll break. Signed-off-by: Jean Delvare kh...@linux-fr.org Tested-by: Johannes Berg johan...@sipsolutions.net Tested-by: Andreas Schwab sch...@linux-m68k.org Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Takashi Iwai ti...@suse.de --- Takashi, please push this patch to Linus quickly, as this is blocking the removal of the legacy i2c binding model, which is scheduled for 2.6.30. Have you taken care of snd-powermac similarly? Yes I did, see my patch keywest: Convert to new-style i2c driver to the alsa-devel and linuxppc-dev mailing lists: http://ozlabs.org/pipermail/linuxppc-dev/2009-April/070884.html Would you be able to test this patch? This would be very welcome. I can resend it to you if it helps. Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] keywest: Convert to new-style i2c driver
On Tue, 21 Apr 2009 08:31:00 +0200, Takashi Iwai wrote: At Mon, 20 Apr 2009 22:56:59 +0200, Jean Delvare wrote: The legacy i2c binding model is going away soon, so convert the ppc keywest sound driver to the new model or it will break. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Takashi Iwai ti...@suse.de --- Takashi, please push this patch to Linus quickly, as this is blocking the removal of the legacy i2c binding model, which is scheduled for 2.6.30. I did not get any test report for this one, but it's relatively simple so I am confident I got it right. Applied this one, too. Thanks. Thanks Takashi. BTW, I forgot to mention again that the new i2c binding model is functional since kernel 2.6.25, so for the external alsa driver tree you will want to guard these changes with appropriate ifdef magic. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
The legacy i2c binding model is going away soon, so convert the AOA
codec drivers to the new model or they'll break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Tested-by: Johannes Berg johan...@sipsolutions.net
Tested-by: Andreas Schwab sch...@linux-m68k.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Takashi Iwai ti...@suse.de
---
Takashi, please push this patch to Linus quickly, as this is blocking
the removal of the legacy i2c binding model, which is scheduled for
2.6.30.
sound/aoa/codecs/onyx.c | 76 ---
sound/aoa/codecs/tas.c | 66 +---
2 files changed, 95 insertions(+), 47 deletions(-)
--- linux-2.6.30-rc2.orig/sound/aoa/codecs/onyx.c 2009-04-15
09:55:20.0 +0200
+++ linux-2.6.30-rc2/sound/aoa/codecs/onyx.c2009-04-15 14:07:50.0
+0200
@@ -47,7 +47,7 @@ MODULE_DESCRIPTION(pcm3052 (onyx) codec
struct onyx {
/* cache registers 65 to 80, they are write-only! */
u8 cache[16];
- struct i2c_client i2c;
+ struct i2c_client *i2c;
struct aoa_codeccodec;
u32 initialised:1,
spdif_locked:1,
@@ -72,7 +72,7 @@ static int onyx_read_register(struct ony
*value = onyx-cache[reg-FIRSTREGISTER];
return 0;
}
- v = i2c_smbus_read_byte_data(onyx-i2c, reg);
+ v = i2c_smbus_read_byte_data(onyx-i2c, reg);
if (v 0)
return -1;
*value = (u8)v;
@@ -84,7 +84,7 @@ static int onyx_write_register(struct on
{
int result;
- result = i2c_smbus_write_byte_data(onyx-i2c, reg, value);
+ result = i2c_smbus_write_byte_data(onyx-i2c, reg, value);
if (!result)
onyx-cache[reg-FIRSTREGISTER] = value;
return result;
@@ -996,12 +996,45 @@ static void onyx_exit_codec(struct aoa_c
onyx-codec.soundbus_dev-detach_codec(onyx-codec.soundbus_dev, onyx);
}
-static struct i2c_driver onyx_driver;
-
static int onyx_create(struct i2c_adapter *adapter,
struct device_node *node,
int addr)
{
+ struct i2c_board_info info;
+ struct i2c_client *client;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, aoa_codec_onyx, I2C_NAME_SIZE);
+ info.addr = addr;
+ info.platform_data = node;
+ client = i2c_new_device(adapter, info);
+ if (!client)
+ return -ENODEV;
+
+ /*
+* We know the driver is already loaded, so the device should be
+* already bound. If not it means binding failed, which suggests
+* the device doesn't really exist and should be deleted.
+* Ideally this would be replaced by better checks _before_
+* instantiating the device.
+*/
+ if (!client-driver) {
+ i2c_unregister_device(client);
+ return -ENODEV;
+ }
+
+ /*
+* Let i2c-core delete that device on driver removal.
+* This is safe because i2c-core holds the core_lock mutex for us.
+*/
+ list_add_tail(client-detected, client-driver-clients);
+ return 0;
+}
+
+static int onyx_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device_node *node = client-dev.platform_data;
struct onyx *onyx;
u8 dummy;
@@ -1011,20 +1044,12 @@ static int onyx_create(struct i2c_adapte
return -ENOMEM;
mutex_init(onyx-mutex);
- onyx-i2c.driver = onyx_driver;
- onyx-i2c.adapter = adapter;
- onyx-i2c.addr = addr 0x7f;
- strlcpy(onyx-i2c.name, onyx audio codec, I2C_NAME_SIZE);
-
- if (i2c_attach_client(onyx-i2c)) {
- printk(KERN_ERR PFX failed to attach to i2c\n);
- goto fail;
- }
+ onyx-i2c = client;
+ i2c_set_clientdata(client, onyx);
/* we try to read from register ONYX_REG_CONTROL
* to check if the codec is present */
if (onyx_read_register(onyx, ONYX_REG_CONTROL, dummy) != 0) {
- i2c_detach_client(onyx-i2c);
printk(KERN_ERR PFX failed to read control register\n);
goto fail;
}
@@ -1036,14 +1061,14 @@ static int onyx_create(struct i2c_adapte
onyx-codec.node = of_node_get(node);
if (aoa_codec_register(onyx-codec)) {
- i2c_detach_client(onyx-i2c);
goto fail;
}
printk(KERN_DEBUG PFX created and attached onyx instance\n);
return 0;
fail:
+ i2c_set_clientdata(client, NULL);
kfree(onyx);
- return -EINVAL;
+ return -ENODEV;
}
static int onyx_i2c_attach(struct i2c_adapter *adapter)
@@ -1080,28 +1105,33 @@ static int onyx_i2c_attach(struct i2c_ad
return onyx_create(adapter, NULL, 0x47
[PATCH] keywest: Convert to new-style i2c driver
The legacy i2c binding model is going away soon, so convert the ppc
keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Takashi Iwai ti...@suse.de
---
Takashi, please push this patch to Linus quickly, as this is blocking
the removal of the legacy i2c binding model, which is scheduled for
2.6.30.
I did not get any test report for this one, but it's relatively simple
so I am confident I got it right.
sound/ppc/keywest.c | 82 +--
1 file changed, 41 insertions(+), 41 deletions(-)
--- linux-2.6.30-rc2.orig/sound/ppc/keywest.c 2009-04-20 22:40:27.0
+0200
+++ linux-2.6.30-rc2/sound/ppc/keywest.c2009-04-20 22:47:34.0
+0200
@@ -33,26 +33,25 @@
static struct pmac_keywest *keywest_ctx;
-static int keywest_attach_adapter(struct i2c_adapter *adapter);
-static int keywest_detach_client(struct i2c_client *client);
-
-struct i2c_driver keywest_driver = {
- .driver = {
- .name = PMac Keywest Audio,
- },
- .attach_adapter = keywest_attach_adapter,
- .detach_client = keywest_detach_client,
-};
-
-
#ifndef i2c_device_name
#define i2c_device_name(x) ((x)-name)
#endif
+static int keywest_probe(struct i2c_client *client,
+const struct i2c_device_id *id)
+{
+ i2c_set_clientdata(client, keywest_ctx);
+ return 0;
+}
+
+/*
+ * This is kind of a hack, best would be to turn powermac to fixed i2c
+ * bus numbers and declare the sound device as part of platform
+ * initialization
+ */
static int keywest_attach_adapter(struct i2c_adapter *adapter)
{
- int err;
- struct i2c_client *new_client;
+ struct i2c_board_info info;
if (! keywest_ctx)
return -EINVAL;
@@ -60,46 +59,47 @@ static int keywest_attach_adapter(struct
if (strncmp(i2c_device_name(adapter), mac-io, 6))
return 0; /* ignored */
- new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
- if (! new_client)
- return -ENOMEM;
-
- new_client-addr = keywest_ctx-addr;
- i2c_set_clientdata(new_client, keywest_ctx);
- new_client-adapter = adapter;
- new_client-driver = keywest_driver;
- new_client-flags = 0;
-
- strcpy(i2c_device_name(new_client), keywest_ctx-name);
- keywest_ctx-client = new_client;
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, keywest, I2C_NAME_SIZE);
+ info.addr = keywest_ctx-addr;
+ keywest_ctx-client = i2c_new_device(adapter, info);
- /* Tell the i2c layer a new client has arrived */
- if (i2c_attach_client(new_client)) {
- snd_printk(KERN_ERR tumbler: cannot attach i2c client\n);
- err = -ENODEV;
- goto __err;
- }
-
+ /*
+* Let i2c-core delete that device on driver removal.
+* This is safe because i2c-core holds the core_lock mutex for us.
+*/
+ list_add_tail(keywest_ctx-client-detected,
+ keywest_ctx-client-driver-clients);
return 0;
-
- __err:
- kfree(new_client);
- keywest_ctx-client = NULL;
- return err;
}
-static int keywest_detach_client(struct i2c_client *client)
+static int keywest_remove(struct i2c_client *client)
{
+ i2c_set_clientdata(client, NULL);
if (! keywest_ctx)
return 0;
if (client == keywest_ctx-client)
keywest_ctx-client = NULL;
- i2c_detach_client(client);
- kfree(client);
return 0;
}
+
+static const struct i2c_device_id keywest_i2c_id[] = {
+ { keywest, 0 },
+ { }
+};
+
+struct i2c_driver keywest_driver = {
+ .driver = {
+ .name = PMac Keywest Audio,
+ },
+ .attach_adapter = keywest_attach_adapter,
+ .probe = keywest_probe,
+ .remove = keywest_remove,
+ .id_table = keywest_i2c_id,
+};
+
/* exported */
void snd_pmac_keywest_cleanup(struct pmac_keywest *i2c)
{
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
On Tue, 14 Apr 2009 16:45:38 +0200, Takashi Iwai wrote: Johannes, please let me know if the patch works. Then I'll merge them. Note if it matters: the new I2C binding model my patch uses is only available since kernel 2.6.26. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Question about windfarm drivers
Hi Ben, hi Paul, As I am converting the windfarm drivers to the new i2c device binding model, I need to understand how these drivers work currently. There's one thing I do not understand so I'd appreciate if you could explain it to me. I am looking at function wf_lm75_detach() in windfarm_lm75_sensor.c. This function is called by i2c-core if either i2c-powermac or windfarm_lm75_sensor is unloaded. It sets lm-i2c.adapter to NULL and then calls wf_unregister_sensor(), which in turn calls wf_put_sensor(), which calls wf_sensor_release() is the reference count drops to 0, which in turns call the driver's .release() method, that is, wf_lm75_release() in our case. In wf_lm75_release(), i2c_detach_client() is called if and only if lm-i2c.adapter is set, which is not the case, and then the data structure, including the i2c client, is freed from memory. This means that the freed i2c client is still registered with i2c-core, this looks wrong. Am I missing something? Or is this clean-up path broken and nobody ever noticed? I am also curious why wf_unregister_sensor() calls wf_put_sensor() while wf_register_sensor() doesn't call wf_get_sensor(). Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] windfarm: Convert to a new-style i2c drivers
The legacy i2c binding model is going away soon, so convert the
macintosh windfarm drivers to the new model or they will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
---
Can someone please test this patch for me? I could only build-test it.
Remember that you need this patch applied:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=935298696f469c0e07c73be687bd055878074ce0
Thanks.
drivers/macintosh/windfarm_lm75_sensor.c| 131 +++
drivers/macintosh/windfarm_max6690_sensor.c | 103 -
drivers/macintosh/windfarm_smu_sat.c| 109 --
3 files changed, 198 insertions(+), 145 deletions(-)
--- linux-2.6.30-rc2.orig/drivers/macintosh/windfarm_lm75_sensor.c
2009-04-16 11:15:00.0 +0200
+++ linux-2.6.30-rc2/drivers/macintosh/windfarm_lm75_sensor.c 2009-04-16
11:23:17.0 +0200
@@ -37,34 +37,22 @@
struct wf_lm75_sensor {
int ds1775 : 1;
int inited : 1;
- struct i2c_client i2c;
+ struct i2c_client *i2c;
struct wf_sensor sens;
};
#define wf_to_lm75(c) container_of(c, struct wf_lm75_sensor, sens)
-#define i2c_to_lm75(c) container_of(c, struct wf_lm75_sensor, i2c)
-
-static int wf_lm75_attach(struct i2c_adapter *adapter);
-static int wf_lm75_detach(struct i2c_client *client);
-
-static struct i2c_driver wf_lm75_driver = {
- .driver = {
- .name = wf_lm75,
- },
- .attach_adapter = wf_lm75_attach,
- .detach_client = wf_lm75_detach,
-};
static int wf_lm75_get(struct wf_sensor *sr, s32 *value)
{
struct wf_lm75_sensor *lm = wf_to_lm75(sr);
s32 data;
- if (lm-i2c.adapter == NULL)
+ if (lm-i2c == NULL)
return -ENODEV;
/* Init chip if necessary */
if (!lm-inited) {
- u8 cfg_new, cfg = (u8)i2c_smbus_read_byte_data(lm-i2c, 1);
+ u8 cfg_new, cfg = (u8)i2c_smbus_read_byte_data(lm-i2c, 1);
DBG(wf_lm75: Initializing %s, cfg was: %02x\n,
sr-name, cfg);
@@ -73,7 +61,7 @@ static int wf_lm75_get(struct wf_sensor
* the firmware for now
*/
cfg_new = cfg ~0x01;
- i2c_smbus_write_byte_data(lm-i2c, 1, cfg_new);
+ i2c_smbus_write_byte_data(lm-i2c, 1, cfg_new);
lm-inited = 1;
/* If we just powered it up, let's wait 200 ms */
@@ -81,7 +69,7 @@ static int wf_lm75_get(struct wf_sensor
}
/* Read temperature register */
- data = (s32)le16_to_cpu(i2c_smbus_read_word_data(lm-i2c, 0));
+ data = (s32)le16_to_cpu(i2c_smbus_read_word_data(lm-i2c, 0));
data = 8;
*value = data;
@@ -92,12 +80,6 @@ static void wf_lm75_release(struct wf_se
{
struct wf_lm75_sensor *lm = wf_to_lm75(sr);
- /* check if client is registered and detach from i2c */
- if (lm-i2c.adapter) {
- i2c_detach_client(lm-i2c);
- lm-i2c.adapter = NULL;
- }
-
kfree(lm);
}
@@ -107,59 +89,77 @@ static struct wf_sensor_ops wf_lm75_ops
.owner = THIS_MODULE,
};
-static struct wf_lm75_sensor *wf_lm75_create(struct i2c_adapter *adapter,
-u8 addr, int ds1775,
-const char *loc)
+static int wf_lm75_probe(struct i2c_client *client,
+const struct i2c_device_id *id)
{
struct wf_lm75_sensor *lm;
int rc;
- DBG(wf_lm75: creating %s device at address 0x%02x\n,
- ds1775 ? ds1775 : lm75, addr);
-
lm = kzalloc(sizeof(struct wf_lm75_sensor), GFP_KERNEL);
if (lm == NULL)
- return NULL;
+ return -ENODEV;
+
+ lm-inited = 0;
+ lm-ds1775 = id-driver_data;
+ lm-i2c = client;
+ lm-sens.name = client-dev.platform_data;
+ lm-sens.ops = wf_lm75_ops;
+ i2c_set_clientdata(client, lm);
+
+ rc = wf_register_sensor(lm-sens);
+ if (rc) {
+ i2c_set_clientdata(client, NULL);
+ kfree(lm);
+ }
+
+ return rc;
+}
+
+static struct i2c_client *wf_lm75_create(struct i2c_adapter *adapter,
+u8 addr, int ds1775,
+const char *loc)
+{
+ struct i2c_board_info info;
+ struct i2c_client *client;
+ char *name;
+
+ DBG(wf_lm75: creating %s device at address 0x%02x\n,
+ ds1775 ? ds1775 : lm75, addr);
/* Usual rant about sensor names not beeing very consistent in
* the device-tree, oh well ...
* Add more entries below as you deal with more setups
*/
if (!strcmp(loc, Hard drive
[PATCH] therm_adt746x: Convert to a new-style i2c driver
The legacy i2c binding model is going away soon, so convert the ppc
therm_adt746x driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
---
Can someone please test this patch for me? I could only build-test it.
Remember that you need this patch applied:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=935298696f469c0e07c73be687bd055878074ce0
Thanks.
drivers/macintosh/therm_adt746x.c | 84 ++---
1 file changed, 42 insertions(+), 42 deletions(-)
--- linux-2.6.30-rc2.orig/drivers/macintosh/therm_adt746x.c 2009-04-16
21:06:31.0 +0200
+++ linux-2.6.30-rc2/drivers/macintosh/therm_adt746x.c 2009-04-16
21:37:05.0 +0200
@@ -71,7 +71,7 @@ MODULE_PARM_DESC(verbose,Verbose log op
(default 0));
struct thermostat {
- struct i2c_client clt;
+ struct i2c_client *clt;
u8 temps[3];
u8 cached_temp[3];
u8 initial_limits[3];
@@ -86,9 +86,6 @@ static struct of_device * of_dev;
static struct thermostat* thermostat;
static struct task_struct *thread_therm = NULL;
-static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
-int busno);
-
static void write_both_fan_speed(struct thermostat *th, int speed);
static void write_fan_speed(struct thermostat *th, int speed, int fan);
@@ -100,7 +97,7 @@ write_reg(struct thermostat* th, int reg
tmp[0] = reg;
tmp[1] = data;
- rc = i2c_master_send(th-clt, (const char *)tmp, 2);
+ rc = i2c_master_send(th-clt, (const char *)tmp, 2);
if (rc 0)
return rc;
if (rc != 2)
@@ -115,12 +112,12 @@ read_reg(struct thermostat* th, int reg)
int rc;
reg_addr = (u8)reg;
- rc = i2c_master_send(th-clt, reg_addr, 1);
+ rc = i2c_master_send(th-clt, reg_addr, 1);
if (rc 0)
return rc;
if (rc != 1)
return -ENODEV;
- rc = i2c_master_recv(th-clt, (char *)data, 1);
+ rc = i2c_master_recv(th-clt, (char *)data, 1);
if (rc 0)
return rc;
return data;
@@ -130,26 +127,36 @@ static int
attach_thermostat(struct i2c_adapter *adapter)
{
unsigned long bus_no;
+ struct i2c_board_info info;
+ struct i2c_client *client;
if (strncmp(adapter-name, uni-n, 5))
return -ENODEV;
bus_no = simple_strtoul(adapter-name + 6, NULL, 10);
if (bus_no != therm_bus)
return -ENODEV;
- return attach_one_thermostat(adapter, therm_address, bus_no);
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, therm_adt746x, I2C_NAME_SIZE);
+ info.addr = therm_address;
+ client = i2c_new_device(adapter, info);
+ if (!client)
+ return -ENODEV;
+
+ /*
+* Let i2c-core delete that device on driver removal.
+* This is safe because i2c-core holds the core_lock mutex for us.
+*/
+ list_add_tail(client-detected, client-driver-clients);
+ return 0;
}
static int
-detach_thermostat(struct i2c_adapter *adapter)
+remove_thermostat(struct i2c_client *client)
{
- struct thermostat* th;
+ struct thermostat *th = i2c_get_clientdata(client);
int i;
- if (thermostat == NULL)
- return 0;
-
- th = thermostat;
-
if (thread_therm != NULL) {
kthread_stop(thread_therm);
}
@@ -165,8 +172,6 @@ detach_thermostat(struct i2c_adapter *ad
write_both_fan_speed(th, -1);
- i2c_detach_client(th-clt);
-
thermostat = NULL;
kfree(th);
@@ -174,14 +179,6 @@ detach_thermostat(struct i2c_adapter *ad
return 0;
}
-static struct i2c_driver thermostat_driver = {
- .driver = {
- .name = therm_adt746x,
- },
- .attach_adapter = attach_thermostat,
- .detach_adapter = detach_thermostat,
-};
-
static int read_fan_speed(struct thermostat *th, u8 addr)
{
u8 tmp[2];
@@ -369,8 +366,8 @@ static void set_limit(struct thermostat
th-limits[i] = default_limits_local[i] + limit_adjust;
}
-static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
-int busno)
+static int probe_thermostat(struct i2c_client *client,
+ const struct i2c_device_id *id)
{
struct thermostat* th;
int rc;
@@ -383,16 +380,12 @@ static int attach_one_thermostat(struct
if (!th)
return -ENOMEM;
- th-clt.addr = addr;
- th-clt.adapter = adapter;
- th-clt.driver = thermostat_driver;
- strcpy(th-clt.name, thermostat);
+ i2c_set_clientdata(client
[PATCH] therm_windtunnel: Convert to a new-style i2c driver
The legacy i2c binding model is going away soon, so convert the ppc
therm_windtunnel driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paul Mackerras pau...@samba.org
---
Can someone please test this patch for me? I could only build-test it.
Remember that you need this patch applied:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=935298696f469c0e07c73be687bd055878074ce0
Thanks.
drivers/macintosh/therm_windtunnel.c | 126 --
1 file changed, 60 insertions(+), 66 deletions(-)
--- linux-2.6.30-rc2.orig/drivers/macintosh/therm_windtunnel.c 2009-03-24
13:40:47.0 +0100
+++ linux-2.6.30-rc2/drivers/macintosh/therm_windtunnel.c 2009-04-16
22:56:39.0 +0200
@@ -48,16 +48,6 @@
#define LOG_TEMP 0 /* continously log
temperature */
-static int do_probe( struct i2c_adapter *adapter, int
addr, int kind);
-
-/* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */
-static const unsigned shortnormal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
-0x4c, 0x4d, 0x4e, 0x4f,
-0x2c, 0x2d, 0x2e, 0x2f,
-I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
static struct {
volatile intrunning;
struct task_struct *poll_task;
@@ -315,53 +305,54 @@ static int control_loop(void *dummy)
static int
do_attach( struct i2c_adapter *adapter )
{
- int ret = 0;
+ /* scan 0x48-0x4f (DS1775) and 0x2c-2x2f (ADM1030) */
+ static const unsigned short scan_ds1775[] = {
+ 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f,
+ I2C_CLIENT_END
+ };
+ static const unsigned short scan_adm1030[] = {
+ 0x2c, 0x2d, 0x2e, 0x2f,
+ I2C_CLIENT_END
+ };
if( strncmp(adapter-name, uni-n, 5) )
return 0;
if( !x.running ) {
- ret = i2c_probe( adapter, addr_data, do_probe );
+ struct i2c_board_info info;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, therm_ds1775, I2C_NAME_SIZE);
+ i2c_new_probed_device(adapter, info, scan_ds1775);
+
+ strlcpy(info.type, therm_adm1030, I2C_NAME_SIZE);
+ i2c_new_probed_device(adapter, info, scan_adm1030);
+
if( x.thermostat x.fan ) {
x.running = 1;
x.poll_task = kthread_run(control_loop, NULL, g4fand);
}
}
- return ret;
+ return 0;
}
static int
-do_detach( struct i2c_client *client )
+do_remove(struct i2c_client *client)
{
- int err;
-
- if( (err=i2c_detach_client(client)) )
- printk(KERN_ERR failed to detach thermostat client\n);
- else {
- if( x.running ) {
- x.running = 0;
- kthread_stop(x.poll_task);
- x.poll_task = NULL;
- }
- if( client == x.thermostat )
- x.thermostat = NULL;
- else if( client == x.fan )
- x.fan = NULL;
- else {
- printk(KERN_ERR g4fan: bad client\n);
- }
- kfree( client );
+ if (x.running) {
+ x.running = 0;
+ kthread_stop(x.poll_task);
+ x.poll_task = NULL;
}
- return err;
-}
+ if (client == x.thermostat)
+ x.thermostat = NULL;
+ else if (client == x.fan)
+ x.fan = NULL;
+ else
+ printk(KERN_ERR g4fan: bad client\n);
-static struct i2c_driver g4fan_driver = {
- .driver = {
- .name = therm_windtunnel,
- },
- .attach_adapter = do_attach,
- .detach_client = do_detach,
-};
+ return 0;
+}
static int
attach_fan( struct i2c_client *cl )
@@ -374,13 +365,8 @@ attach_fan( struct i2c_client *cl )
goto out;
printk(ADM1030 fan controller [...@%02x]\n, cl-addr );
- strlcpy( cl-name, ADM1030 fan controller, sizeof(cl-name) );
-
- if( !i2c_attach_client(cl) )
- x.fan = cl;
+ x.fan = cl;
out:
- if( cl != x.fan )
- kfree( cl );
return 0;
}
@@ -412,39 +398,47 @@ attach_thermostat( struct i2c_client *cl
x.temp = temp;
x.overheat_temp = os_temp;
x.overheat_hyst = hyst_temp;
-
- strlcpy( cl-name, DS1775 thermostat, sizeof(cl-name) );
-
- if( !i2c_attach_client(cl) )
- x.thermostat = cl;
+ x.thermostat = cl;
out:
- if( cl != x.thermostat )
- kfree( cl );
return 0;
}
+enum
Re: [PATCH 1/2] keywest: Convert to new-style i2c driver
Hi Paul, On Wed, 15 Apr 2009 14:57:30 +1000, Paul Mackerras wrote: Jean Delvare writes: The legacy i2c binding model is going away soon, But not before 2.6.30, right? Ideally, yes, before 2.6.30. This is what Documentation/feature-removal-schedule.txt says: --- What: i2c_attach_client(), i2c_detach_client(), i2c_driver-detach_client(), i2c_adapter-client_register(), i2c_adapter-client_unregister When: 2.6.30 Check: i2c_attach_client i2c_detach_client Why:Deprecated by the new (standard) device driver binding model. Use i2c_driver-probe() and -remove() instead. Who:Jean Delvare kh...@linux-fr.org --- There are only 10 legacy i2c drivers remaining (not counting staging), 9 of which are ppc drivers. My hope was that ppc developers would see the deprecation warning and convert the drivers themselves, but it did not happen. Which is why I am trying to do it myself now, admittedly quite late in the 2.6.30 development cycle. This seems doable to me if my patches get proper testing fast (which seems to be the case, thanks!) and positive results (not true for onyx, but tas and therm_pm72 are apparently OK). Now I know I have to be realistic, if problems arise and/or ppc people feel too bad about me pushing these patches at about rc3 time, I will refrain from doing so and wait for 2.6.31. But there are a number or i2c-core improvements other developers are asking for for quite some times now, which depend on the removal of the legacy model, so delaying this wound not make me happy at all. Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
On Wed, 15 Apr 2009 00:48:08 +0200, Andreas Schwab wrote: Jean Delvare kh...@linux-fr.org writes: Hi Johannes, On Tue, 14 Apr 2009 19:41:55 +0200, Johannes Berg wrote: Alright, with the patch Andreas pointed out it loads, but segfaults, as below. Works fine without your patch. Thanks for the quick test and sorry that it didn't work. I'll take a look at the trace below and try to figure out what went wrong. Did you remove the 2 MODULE_DEVICE_TABLE from my patch? If you didn't, please pick the latest version of my patch which doesn't have them: ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/sound-aoa-codecs-convert-to-new-style.patch I don't think they are the reason of the crash, but who knows... I tried with a tas-based iBook, and it works fine here. Good news, thanks Andreas! -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers (v3)
The legacy i2c binding model is going away soon, so convert the AOA
codec drivers to the new model or they'll break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Johannes Berg johan...@sipsolutions.net
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Tested-by: Andreas Schwab sch...@linux-m68k.org
---
Johannes, this is a reworked patch which assumes that the onyx codec
probing can fail. It's not exactly clean but that's the easiest
approach for now. Can you please give it a try? Thanks.
sound/aoa/codecs/onyx.c | 76 ---
sound/aoa/codecs/tas.c | 66 +---
2 files changed, 95 insertions(+), 47 deletions(-)
--- linux-2.6.30-rc2.orig/sound/aoa/codecs/onyx.c 2009-04-15
09:55:20.0 +0200
+++ linux-2.6.30-rc2/sound/aoa/codecs/onyx.c2009-04-15 14:07:50.0
+0200
@@ -47,7 +47,7 @@ MODULE_DESCRIPTION(pcm3052 (onyx) codec
struct onyx {
/* cache registers 65 to 80, they are write-only! */
u8 cache[16];
- struct i2c_client i2c;
+ struct i2c_client *i2c;
struct aoa_codeccodec;
u32 initialised:1,
spdif_locked:1,
@@ -72,7 +72,7 @@ static int onyx_read_register(struct ony
*value = onyx-cache[reg-FIRSTREGISTER];
return 0;
}
- v = i2c_smbus_read_byte_data(onyx-i2c, reg);
+ v = i2c_smbus_read_byte_data(onyx-i2c, reg);
if (v 0)
return -1;
*value = (u8)v;
@@ -84,7 +84,7 @@ static int onyx_write_register(struct on
{
int result;
- result = i2c_smbus_write_byte_data(onyx-i2c, reg, value);
+ result = i2c_smbus_write_byte_data(onyx-i2c, reg, value);
if (!result)
onyx-cache[reg-FIRSTREGISTER] = value;
return result;
@@ -996,12 +996,45 @@ static void onyx_exit_codec(struct aoa_c
onyx-codec.soundbus_dev-detach_codec(onyx-codec.soundbus_dev, onyx);
}
-static struct i2c_driver onyx_driver;
-
static int onyx_create(struct i2c_adapter *adapter,
struct device_node *node,
int addr)
{
+ struct i2c_board_info info;
+ struct i2c_client *client;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, aoa_codec_onyx, I2C_NAME_SIZE);
+ info.addr = addr;
+ info.platform_data = node;
+ client = i2c_new_device(adapter, info);
+ if (!client)
+ return -ENODEV;
+
+ /*
+* We know the driver is already loaded, so the device should be
+* already bound. If not it means binding failed, which suggests
+* the device doesn't really exist and should be deleted.
+* Ideally this would be replaced by better checks _before_
+* instantiating the device.
+*/
+ if (!client-driver) {
+ i2c_unregister_device(client);
+ return -ENODEV;
+ }
+
+ /*
+* Let i2c-core delete that device on driver removal.
+* This is safe because i2c-core holds the core_lock mutex for us.
+*/
+ list_add_tail(client-detected, client-driver-clients);
+ return 0;
+}
+
+static int onyx_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device_node *node = client-dev.platform_data;
struct onyx *onyx;
u8 dummy;
@@ -1011,20 +1044,12 @@ static int onyx_create(struct i2c_adapte
return -ENOMEM;
mutex_init(onyx-mutex);
- onyx-i2c.driver = onyx_driver;
- onyx-i2c.adapter = adapter;
- onyx-i2c.addr = addr 0x7f;
- strlcpy(onyx-i2c.name, onyx audio codec, I2C_NAME_SIZE);
-
- if (i2c_attach_client(onyx-i2c)) {
- printk(KERN_ERR PFX failed to attach to i2c\n);
- goto fail;
- }
+ onyx-i2c = client;
+ i2c_set_clientdata(client, onyx);
/* we try to read from register ONYX_REG_CONTROL
* to check if the codec is present */
if (onyx_read_register(onyx, ONYX_REG_CONTROL, dummy) != 0) {
- i2c_detach_client(onyx-i2c);
printk(KERN_ERR PFX failed to read control register\n);
goto fail;
}
@@ -1036,14 +1061,14 @@ static int onyx_create(struct i2c_adapte
onyx-codec.node = of_node_get(node);
if (aoa_codec_register(onyx-codec)) {
- i2c_detach_client(onyx-i2c);
goto fail;
}
printk(KERN_DEBUG PFX created and attached onyx instance\n);
return 0;
fail:
+ i2c_set_clientdata(client, NULL);
kfree(onyx);
- return -EINVAL;
+ return -ENODEV;
}
static int onyx_i2c_attach(struct i2c_adapter *adapter)
@@ -1080,28 +1105,33 @@ static int onyx_i2c_attach(struct i2c_ad
return onyx_create(adapter, NULL, 0x47
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
On Wed, 15 Apr 2009 14:52:14 +0200, Johannes Berg wrote: OK, I understand better what is going on now. I do not understand the crash at the end though, but I suspect it isn't a bug in my code but simply a faulty error path which had never been taken before. That would be weird -- the error path _has_ to be taken always in onyx. Unless you're talking about something in the i2c core or whatever? Yes, i2c core or even driver core. I'll see if I can reproduce it. (...) Well, there is a dirty workaround, which I will apply for now, but... ideally the layout factory should be revisited so that the codec check happens earlier. Is this something you could help with? That's not really possible unless the factory post-processes the entire device-tree -- very ugly. What I had in mind was not so complex. Simply, we could move the i2c_new_device() calls into layout_found_codec(). That way we can decide to instantiate the I2C device if and only if check_codec() is successful. This is more efficient that creating the device, letting the driver attach to it, with probing eventually failing, and then removing the device if it wasn't the right one. That is, the i2c client would be a mere helper on top of struct aoa_codec, rather than the other way around. There may be preliminary work needed, for example switching powermac to numbered I2C buses. That's something which isn't too clear to me: is there a physical device at 2-0046 and 3-0046? The onyx codec is accepted for the latter, however it seems that the test of a device presence at 2-0046 succeeds as well... It's the _same_ physical device. Wow. One I2C device which can be reached through 2 different I2C buses? First time I hear about something like this. Very odd. I can't see the point of doing this. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
On Wed, 15 Apr 2009 15:18:10 +0200, Johannes Berg wrote: On Wed, 2009-04-15 at 15:06 +0200, Jean Delvare wrote: Yes, i2c core or even driver core. I'll see if I can reproduce it. Alright. Hmm, couldn't reproduce it. Maybe it is fixed in rc2. I don't have too much time to spend on this, so if we don't hit it again I consider it solved. (...) What I had in mind was not so complex. Simply, we could move the i2c_new_device() calls into layout_found_codec(). That way we can decide to instantiate the I2C device if and only if check_codec() is successful. This is more efficient that creating the device, letting the driver attach to it, with probing eventually failing, and then removing the device if it wasn't the right one. That is, the i2c client would be a mere helper on top of struct aoa_codec, rather than the other way around. Ah. That would probably work, but right now I have little motivation to work on it -- I hardly use the G5 desktop machine any more. OK, no problem. I don't want to force anyone to spend time on this. But if anyone ever does and need my help for the i2c part, just drop me a line! Wow. One I2C device which can be reached through 2 different I2C buses? First time I hear about something like this. Very odd. I can't see the point of doing this. Hmm. How do you know it's on different buses? 2-0046 fails and 3-0046 succeeds. The second number is the hexadecimal I2C address, the first number is the I2C bus number. So, unless i2c-powermac was told to register the same I2C bus twice (which would be dangerous), the device can be accessed through 2 different buses. I don't really know anything -- all I know is that the DT is buggered and lists the codec twice. Since we can talk to both, then I'm sure it's just one chip, they wouldn't put the chip in twice when you can use only one of them :) There's probably little point in trying to guess anything further, the only thing that would help are the detailed schematics of that part of the board. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers (v3)
On Wed, 15 Apr 2009 15:00:44 +0200, Johannes Berg wrote: On Wed, 2009-04-15 at 14:22 +0200, Jean Delvare wrote: The legacy i2c binding model is going away soon, so convert the AOA codec drivers to the new model or they'll break. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Johannes Berg johan...@sipsolutions.net Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Tested-by: Andreas Schwab sch...@linux-m68k.org --- Johannes, this is a reworked patch which assumes that the onyx codec probing can fail. It's not exactly clean but that's the easiest approach for now. Can you please give it a try? Thanks. This works. Tested-by: Johannes Berg johan...@sipsolutions.net [quad G5] Excellent, thanks for the testing! -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] therm_pm72: Convert to a new-style i2c driver
Hi Paul, On Tue, 14 Apr 2009 09:07:35 +1000, Paul Mackerras wrote: Jean Delvare writes: The legacy i2c binding model is going away soon, so convert the macintosh therm_pm72 driver to the new model or it will break. This is really a quick and dirty conversion, that should do the trick for now, but no doubt that something cleaner can be done if anyone is interested. Signed-off-by: Jean Delvare kh...@linux-fr.org Seems to work fine on my powermac7,2 machine (dual 2.0GHz G5). Excellent, thanks a lot for the fast testing! I'll try to convert the remaining powermac drivers by the end of the week, but I wouldn't mind if somebody with more knowledge of these drivers than me wants to help. Basically the remaining 5 drivers must be converted the same way I did for the therm_pm72 driver. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] therm_pm72: Convert to a new-style i2c driver
On Mon, 13 Apr 2009 16:51:00 +0200, Jean Delvare wrote: On Mon, 13 Apr 2009 16:09:27 +0200, Jean Delvare wrote: The legacy i2c binding model is going away soon, so convert the macintosh therm_pm72 driver to the new model or it will break. This is really a quick and dirty conversion, that should do the trick for now, but no doubt that something cleaner can be done if anyone is interested. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Benjamin Herrenschmidt b...@kernel.crashing.org --- Can anyone please run-test this patch? I could only build-test it. Damn, I forgot once again. You need to apply the following patch beforehand: ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch I'll push this patch to Linus now. This patch is is Linus' tree now: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=935298696f469c0e07c73be687bd055878074ce0 -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] keywest: Convert to new-style i2c driver
On Fri, 10 Apr 2009 17:09:51 +0200, Jean Delvare wrote: Ah, I forgot. You need the following patch applied before testing: ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch This patch is is Linus' tree now: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=935298696f469c0e07c73be687bd055878074ce0 -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
On Fri, 10 Apr 2009 17:02:38 +0200, Jean Delvare wrote: On Thu, 9 Apr 2009 14:19:45 +0200, Jean Delvare wrote: From: Jean Delvare kh...@linux-fr.org Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers The legacy i2c binding model is going away soon, so convert the AOA codec drivers to the new model or they'll break. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Johannes Berg johan...@sipsolutions.net Cc: Benjamin Herrenschmidt b...@kernel.crashing.org --- sound/aoa/codecs/onyx.c | 71 +-- sound/aoa/codecs/tas.c | 63 + 2 files changed, 84 insertions(+), 50 deletions(-) Note to potential testers of this patch: you need to apply this i2c patch first: ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch This patch is is Linus' tree now: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=935298696f469c0e07c73be687bd055878074ce0 -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
Hi Johannes, On Tue, 14 Apr 2009 17:50:27 +0200, Johannes Berg wrote: On Tue, 2009-04-14 at 17:40 +0200, Johannes Berg wrote: So I went to test this, but for some reason aoa doesn't work for me at all, neither with nor without this patch. The reason seems to be that aoa_fabric_layout_probe() is never called, but my driver model fu is weak so I can't tell why -- I definitely see the i2sbus soundbus devices registered. soundbus_probe() isn't getting called either though, but we do of_device_register(). I'm confused, but don't have time to dig further right now. 2.6.29 works -- guess somebody could bisect, but I can't now. Thanks for your try. Note that you should be able to apply my AOA patch to 2.6.29 for testing purposes. All you need is to also apply the other patch first: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=935298696f469c0e07c73be687bd055878074ce0 Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
] .class_for_each_device+0xb4/0x10c [ 10.268308] [c001e19a3990] [c037ecec] .i2c_register_driver+0xf0/0x128 [ 10.268315] [c001e19a3a30] [d1291f20] .onyx_init+0x20/0x668 [snd_aoa_codec_onyx] [ 10.268319] [c001e19a3ab0] [c0007f68] .do_one_initcall+0x9c/0x1dc [ 10.268323] [c001e19a3d90] [c0099b0c] .SyS_init_module+0xd8/0x238 [ 10.268328] [c001e19a3e30] [c0007554] syscall_exit+0x0/0x40 [ 10.268331] ---[ end trace dbcf63aa775331e7 ]--- -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] therm_pm72: Convert to a new-style i2c driver
The legacy i2c binding model is going away soon, so convert the
macintosh therm_pm72 driver to the new model or it will break.
This is really a quick and dirty conversion, that should do the trick
for now, but no doubt that something cleaner can be done if anyone is
interested.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
---
Can anyone please run-test this patch? I could only build-test it.
drivers/macintosh/therm_pm72.c | 95 +++-
1 file changed, 47 insertions(+), 48 deletions(-)
--- linux-2.6.30-rc1.orig/drivers/macintosh/therm_pm72.c2009-04-12
08:49:54.0 +0200
+++ linux-2.6.30-rc1/drivers/macintosh/therm_pm72.c 2009-04-13
09:50:50.0 +0200
@@ -287,22 +287,6 @@ struct fcu_fan_table fcu_fans[] = {
};
/*
- * i2c_driver structure to attach to the host i2c controller
- */
-
-static int therm_pm72_attach(struct i2c_adapter *adapter);
-static int therm_pm72_detach(struct i2c_adapter *adapter);
-
-static struct i2c_driver therm_pm72_driver =
-{
- .driver = {
- .name = therm_pm72,
- },
- .attach_adapter = therm_pm72_attach,
- .detach_adapter = therm_pm72_detach,
-};
-
-/*
* Utility function to create an i2c_client structure and
* attach it to one of u3 adapters
*/
@@ -310,6 +294,7 @@ static struct i2c_client *attach_i2c_chi
{
struct i2c_client *clt;
struct i2c_adapter *adap;
+ struct i2c_board_info info;
if (id 0x200)
adap = k2;
@@ -320,31 +305,21 @@ static struct i2c_client *attach_i2c_chi
if (adap == NULL)
return NULL;
- clt = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
- if (clt == NULL)
- return NULL;
-
- clt-addr = (id 1) 0x7f;
- clt-adapter = adap;
- clt-driver = therm_pm72_driver;
- strncpy(clt-name, name, I2C_NAME_SIZE-1);
-
- if (i2c_attach_client(clt)) {
+ memset(info, 0, sizeof(struct i2c_board_info));
+ info.addr = (id 1) 0x7f;
+ strlcpy(info.type, therm_pm72, I2C_NAME_SIZE);
+ clt = i2c_new_device(adap, info);
+ if (!clt) {
printk(KERN_ERR therm_pm72: Failed to attach to i2c ID
0x%x\n, id);
- kfree(clt);
return NULL;
}
- return clt;
-}
-/*
- * Utility function to get rid of the i2c_client structure
- * (will also detach from the adapter hopepfully)
- */
-static void detach_i2c_chip(struct i2c_client *clt)
-{
- i2c_detach_client(clt);
- kfree(clt);
+ /*
+* Let i2c-core delete that device on driver removal.
+* This is safe because i2c-core holds the core_lock mutex for us.
+*/
+ list_add_tail(clt-detected, clt-driver-clients);
+ return clt;
}
/*
@@ -1203,8 +1178,6 @@ static int init_cpu_state(struct cpu_pid
return 0;
fail:
- if (state-monitor)
- detach_i2c_chip(state-monitor);
state-monitor = NULL;
return -ENODEV;
@@ -1232,7 +1205,6 @@ static void dispose_cpu_state(struct cpu
device_remove_file(of_dev-dev, dev_attr_cpu1_intake_fan_rpm);
}
- detach_i2c_chip(state-monitor);
state-monitor = NULL;
}
@@ -1407,7 +1379,6 @@ static void dispose_backside_state(struc
device_remove_file(of_dev-dev, dev_attr_backside_temperature);
device_remove_file(of_dev-dev, dev_attr_backside_fan_pwm);
- detach_i2c_chip(state-monitor);
state-monitor = NULL;
}
@@ -1532,7 +1503,6 @@ static void dispose_drives_state(struct
device_remove_file(of_dev-dev, dev_attr_drives_temperature);
device_remove_file(of_dev-dev, dev_attr_drives_fan_rpm);
- detach_i2c_chip(state-monitor);
state-monitor = NULL;
}
@@ -1654,7 +1624,6 @@ static void dispose_dimms_state(struct d
device_remove_file(of_dev-dev, dev_attr_dimms_temperature);
- detach_i2c_chip(state-monitor);
state-monitor = NULL;
}
@@ -1779,7 +1748,6 @@ static void dispose_slots_state(struct s
device_remove_file(of_dev-dev, dev_attr_slots_temperature);
device_remove_file(of_dev-dev, dev_attr_slots_fan_pwm);
- detach_i2c_chip(state-monitor);
state-monitor = NULL;
}
@@ -2008,8 +1976,6 @@ static int attach_fcu(void)
*/
static void detach_fcu(void)
{
- if (fcu)
- detach_i2c_chip(fcu);
fcu = NULL;
}
@@ -2060,12 +2026,21 @@ static int therm_pm72_attach(struct i2c_
return 0;
}
+static int therm_pm72_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ /* Always succeed, the real work was done in therm_pm72_attach() */
+ return 0;
+}
+
/*
- * Called on every adapter when the driver or the i2c controller
+ * Called when any of the devices which participates into thermal management
* is going away
Re: [PATCH] therm_pm72: Convert to a new-style i2c driver
On Mon, 13 Apr 2009 16:09:27 +0200, Jean Delvare wrote: The legacy i2c binding model is going away soon, so convert the macintosh therm_pm72 driver to the new model or it will break. This is really a quick and dirty conversion, that should do the trick for now, but no doubt that something cleaner can be done if anyone is interested. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Benjamin Herrenschmidt b...@kernel.crashing.org --- Can anyone please run-test this patch? I could only build-test it. Damn, I forgot once again. You need to apply the following patch beforehand: ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch I'll push this patch to Linus now. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
On Thu, 9 Apr 2009 14:19:45 +0200, Jean Delvare wrote:
From: Jean Delvare kh...@linux-fr.org
Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers
The legacy i2c binding model is going away soon, so convert the AOA
codec drivers to the new model or they'll break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Johannes Berg johan...@sipsolutions.net
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
---
sound/aoa/codecs/onyx.c | 71
+--
sound/aoa/codecs/tas.c | 63 +
2 files changed, 84 insertions(+), 50 deletions(-)
Note to potential testers of this patch: you need to apply this i2c
patch first:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch
--- linux-2.6.30-rc1.orig/sound/aoa/codecs/onyx.c 2009-04-09
11:53:11.0 +0200
+++ linux-2.6.30-rc1/sound/aoa/codecs/onyx.c 2009-04-09 13:58:44.0
+0200
@@ -47,7 +47,7 @@ MODULE_DESCRIPTION(pcm3052 (onyx) codec
struct onyx {
/* cache registers 65 to 80, they are write-only! */
u8 cache[16];
- struct i2c_client i2c;
+ struct i2c_client *i2c;
struct aoa_codeccodec;
u32 initialised:1,
spdif_locked:1,
@@ -72,7 +72,7 @@ static int onyx_read_register(struct ony
*value = onyx-cache[reg-FIRSTREGISTER];
return 0;
}
- v = i2c_smbus_read_byte_data(onyx-i2c, reg);
+ v = i2c_smbus_read_byte_data(onyx-i2c, reg);
if (v 0)
return -1;
*value = (u8)v;
@@ -84,7 +84,7 @@ static int onyx_write_register(struct on
{
int result;
- result = i2c_smbus_write_byte_data(onyx-i2c, reg, value);
+ result = i2c_smbus_write_byte_data(onyx-i2c, reg, value);
if (!result)
onyx-cache[reg-FIRSTREGISTER] = value;
return result;
@@ -996,12 +996,38 @@ static void onyx_exit_codec(struct aoa_c
onyx-codec.soundbus_dev-detach_codec(onyx-codec.soundbus_dev, onyx);
}
-static struct i2c_driver onyx_driver;
-
static int onyx_create(struct i2c_adapter *adapter,
struct device_node *node,
int addr)
{
+ struct i2c_board_info info;
+ struct i2c_client *client;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, aoa_codec_onyx, I2C_NAME_SIZE);
+ if (node) {
+ info.addr = addr;
+ info.platform_data = node;
+ client = i2c_new_device(adapter, info);
+ } else {
+ /* probe both possible addresses for the onyx chip */
+ unsigned short probe_onyx[] = {
+ 0x46, 0x47, I2C_CLIENT_END
+ };
+
+ client = i2c_new_probed_device(adapter, info, probe_onyx);
+ }
+ if (!client)
+ return -ENODEV;
+
+ list_add_tail(client-detected, client-driver-clients);
+ return 0;
+}
+
+static int onyx_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device_node *node = client-dev.platform_data;
struct onyx *onyx;
u8 dummy;
@@ -1011,20 +1037,12 @@ static int onyx_create(struct i2c_adapte
return -ENOMEM;
mutex_init(onyx-mutex);
- onyx-i2c.driver = onyx_driver;
- onyx-i2c.adapter = adapter;
- onyx-i2c.addr = addr 0x7f;
- strlcpy(onyx-i2c.name, onyx audio codec, I2C_NAME_SIZE);
-
- if (i2c_attach_client(onyx-i2c)) {
- printk(KERN_ERR PFX failed to attach to i2c\n);
- goto fail;
- }
+ onyx-i2c = client;
+ i2c_set_clientdata(client, onyx);
/* we try to read from register ONYX_REG_CONTROL
* to check if the codec is present */
if (onyx_read_register(onyx, ONYX_REG_CONTROL, dummy) != 0) {
- i2c_detach_client(onyx-i2c);
printk(KERN_ERR PFX failed to read control register\n);
goto fail;
}
@@ -1036,7 +1054,6 @@ static int onyx_create(struct i2c_adapte
onyx-codec.node = of_node_get(node);
if (aoa_codec_register(onyx-codec)) {
- i2c_detach_client(onyx-i2c);
goto fail;
}
printk(KERN_DEBUG PFX created and attached onyx instance\n);
@@ -1074,19 +1091,13 @@ static int onyx_i2c_attach(struct i2c_ad
return -ENODEV;
printk(KERN_DEBUG PFX found k2-i2c, checking if onyx chip is on it\n);
- /* probe both possible addresses for the onyx chip */
- if (onyx_create(adapter, NULL, 0x46) == 0)
- return 0;
- return onyx_create(adapter, NULL, 0x47);
+ return onyx_create(adapter, NULL, 0);
}
-static int onyx_i2c_detach(struct i2c_client *client)
+static int onyx_i2c_remove(struct i2c_client
[PATCH 1/2] keywest: Convert to new-style i2c driver
The legacy i2c binding model is going away soon, so convert the ppc
keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
---
sound/ppc/keywest.c | 81 +--
1 file changed, 40 insertions(+), 41 deletions(-)
--- linux-2.6.30-rc1.orig/sound/ppc/keywest.c 2009-04-10 15:22:18.0
+0200
+++ linux-2.6.30-rc1/sound/ppc/keywest.c2009-04-10 16:41:36.0
+0200
@@ -33,26 +33,25 @@
static struct pmac_keywest *keywest_ctx;
-static int keywest_attach_adapter(struct i2c_adapter *adapter);
-static int keywest_detach_client(struct i2c_client *client);
-
-struct i2c_driver keywest_driver = {
- .driver = {
- .name = PMac Keywest Audio,
- },
- .attach_adapter = keywest_attach_adapter,
- .detach_client = keywest_detach_client,
-};
-
-
#ifndef i2c_device_name
#define i2c_device_name(x) ((x)-name)
#endif
+static int keywest_probe(struct i2c_client *client,
+const struct i2c_device_id *id)
+{
+ i2c_set_clientdata(client, keywest_ctx);
+ return 0;
+}
+
+/*
+ * This is kind of a hack, best would be to turn powermac to fixed i2c
+ * bus numbers and declare the sound device as part of platform
+ * initialization
+ */
static int keywest_attach_adapter(struct i2c_adapter *adapter)
{
- int err;
- struct i2c_client *new_client;
+ struct i2c_board_info info;
if (! keywest_ctx)
return -EINVAL;
@@ -60,46 +59,46 @@ static int keywest_attach_adapter(struct
if (strncmp(i2c_device_name(adapter), mac-io, 6))
return 0; /* ignored */
- new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
- if (! new_client)
- return -ENOMEM;
-
- new_client-addr = keywest_ctx-addr;
- i2c_set_clientdata(new_client, keywest_ctx);
- new_client-adapter = adapter;
- new_client-driver = keywest_driver;
- new_client-flags = 0;
-
- strcpy(i2c_device_name(new_client), keywest_ctx-name);
- keywest_ctx-client = new_client;
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, keywest, I2C_NAME_SIZE);
+ info.addr = keywest_ctx-addr;
+ keywest_ctx-client = i2c_new_device(adapter, info);
- /* Tell the i2c layer a new client has arrived */
- if (i2c_attach_client(new_client)) {
- snd_printk(KERN_ERR tumbler: cannot attach i2c client\n);
- err = -ENODEV;
- goto __err;
- }
-
+ /*
+* Let i2c-core delete that device on driver removal.
+* This is safe because i2c-core holds the core_lock mutex for us.
+*/
+ list_add_tail(keywest_ctx-client-detected,
+ keywest_ctx-client-driver-clients);
return 0;
-
- __err:
- kfree(new_client);
- keywest_ctx-client = NULL;
- return err;
}
-static int keywest_detach_client(struct i2c_client *client)
+static int keywest_remove(struct i2c_client *client)
{
if (! keywest_ctx)
return 0;
if (client == keywest_ctx-client)
keywest_ctx-client = NULL;
- i2c_detach_client(client);
- kfree(client);
return 0;
}
+
+static const struct i2c_device_id keywest_i2c_id[] = {
+ { keywest, 0 },
+ { }
+};
+
+struct i2c_driver keywest_driver = {
+ .driver = {
+ .name = PMac Keywest Audio,
+ },
+ .attach_adapter = keywest_attach_adapter,
+ .probe = keywest_probe,
+ .remove = keywest_remove,
+ .id_table = keywest_i2c_id,
+};
+
/* exported */
void snd_pmac_keywest_cleanup(struct pmac_keywest *i2c)
{
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH 2/2] keywest: Get rid of useless i2c_device_name macro
The i2c_device_name macro doesn't have much value, get rid of it.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
---
sound/ppc/keywest.c |6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
--- linux-2.6.30-rc1.orig/sound/ppc/keywest.c 2009-04-10 16:22:08.0
+0200
+++ linux-2.6.30-rc1/sound/ppc/keywest.c2009-04-10 16:25:16.0
+0200
@@ -33,10 +33,6 @@
static struct pmac_keywest *keywest_ctx;
-#ifndef i2c_device_name
-#define i2c_device_name(x) ((x)-name)
-#endif
-
static int keywest_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -56,7 +52,7 @@ static int keywest_attach_adapter(struct
if (! keywest_ctx)
return -EINVAL;
- if (strncmp(i2c_device_name(adapter), mac-io, 6))
+ if (strncmp(adapter-name, mac-io, 6))
return 0; /* ignored */
memset(info, 0, sizeof(struct i2c_board_info));
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] keywest: Convert to new-style i2c driver
On Fri, 10 Apr 2009 17:07:26 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc
keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
---
sound/ppc/keywest.c | 81
+--
1 file changed, 40 insertions(+), 41 deletions(-)
Ah, I forgot. You need the following patch applied before testing:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch
--- linux-2.6.30-rc1.orig/sound/ppc/keywest.c 2009-04-10 15:22:18.0
+0200
+++ linux-2.6.30-rc1/sound/ppc/keywest.c 2009-04-10 16:41:36.0
+0200
@@ -33,26 +33,25 @@
static struct pmac_keywest *keywest_ctx;
-static int keywest_attach_adapter(struct i2c_adapter *adapter);
-static int keywest_detach_client(struct i2c_client *client);
-
-struct i2c_driver keywest_driver = {
- .driver = {
- .name = PMac Keywest Audio,
- },
- .attach_adapter = keywest_attach_adapter,
- .detach_client = keywest_detach_client,
-};
-
-
#ifndef i2c_device_name
#define i2c_device_name(x) ((x)-name)
#endif
+static int keywest_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ i2c_set_clientdata(client, keywest_ctx);
+ return 0;
+}
+
+/*
+ * This is kind of a hack, best would be to turn powermac to fixed i2c
+ * bus numbers and declare the sound device as part of platform
+ * initialization
+ */
static int keywest_attach_adapter(struct i2c_adapter *adapter)
{
- int err;
- struct i2c_client *new_client;
+ struct i2c_board_info info;
if (! keywest_ctx)
return -EINVAL;
@@ -60,46 +59,46 @@ static int keywest_attach_adapter(struct
if (strncmp(i2c_device_name(adapter), mac-io, 6))
return 0; /* ignored */
- new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
- if (! new_client)
- return -ENOMEM;
-
- new_client-addr = keywest_ctx-addr;
- i2c_set_clientdata(new_client, keywest_ctx);
- new_client-adapter = adapter;
- new_client-driver = keywest_driver;
- new_client-flags = 0;
-
- strcpy(i2c_device_name(new_client), keywest_ctx-name);
- keywest_ctx-client = new_client;
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, keywest, I2C_NAME_SIZE);
+ info.addr = keywest_ctx-addr;
+ keywest_ctx-client = i2c_new_device(adapter, info);
- /* Tell the i2c layer a new client has arrived */
- if (i2c_attach_client(new_client)) {
- snd_printk(KERN_ERR tumbler: cannot attach i2c client\n);
- err = -ENODEV;
- goto __err;
- }
-
+ /*
+ * Let i2c-core delete that device on driver removal.
+ * This is safe because i2c-core holds the core_lock mutex for us.
+ */
+ list_add_tail(keywest_ctx-client-detected,
+ keywest_ctx-client-driver-clients);
return 0;
-
- __err:
- kfree(new_client);
- keywest_ctx-client = NULL;
- return err;
}
-static int keywest_detach_client(struct i2c_client *client)
+static int keywest_remove(struct i2c_client *client)
{
if (! keywest_ctx)
return 0;
if (client == keywest_ctx-client)
keywest_ctx-client = NULL;
- i2c_detach_client(client);
- kfree(client);
return 0;
}
+
+static const struct i2c_device_id keywest_i2c_id[] = {
+ { keywest, 0 },
+ { }
+};
+
+struct i2c_driver keywest_driver = {
+ .driver = {
+ .name = PMac Keywest Audio,
+ },
+ .attach_adapter = keywest_attach_adapter,
+ .probe = keywest_probe,
+ .remove = keywest_remove,
+ .id_table = keywest_i2c_id,
+};
+
/* exported */
void snd_pmac_keywest_cleanup(struct pmac_keywest *i2c)
{
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
Hi Johannes, On Thu, 09 Apr 2009 09:44:49 +0200, Johannes Berg wrote: Can you please point me to the layout fabric / aoa fabric? I'd like to understand how it works. Then I may be able to rewrite my patch completely differently. That's in sound/aoa/fabric/layout.c OK, I understand how it works now, thanks for pointing me to the relevant piece of code. It's easier to modify the code when you understand the current logic ;) There are basically two ways to instantiate I2C devices in the new model. The first method is to declare the I2C devices as platform data and let i2c-core instantiate them. The second method is to have the i2c bus driver instantiate the clients. My patch uses the second method because I didn't know how to use the first method. However using the first method would solve the issues you raised. But I need some help from someone more familiar with the powermac platform initialization code to get it right. Interesting. Does it need to be _platform_ data, or could sound/aoa/fabric/layout.c declare the devices instead? It can be anywhere as long as it is early enough. Early enough means that the code must run before device drivers can loaded. As snd-aoa-fabric-layout can be built as a module, it doesn't seem right. The idea is to assign fixed i2c bus numbers to the relevant i2c buses, and declare the i2c devices connected to each bus by bus number and device address. The i2c-powermac buses are created in arch/powerpc/platforms/powermac/low_i2c.c, so you would have to instantiate the i2c devices there too. That would basically mean merging part of snd-aoa-fabric-layout into arch/powerpc/platforms/powermac/low_i2c.c as I understand it, I don't know if this sounds reasonable to you. More generally, in order to instantiate an i2c device (for the new binding model), you need at least two things: a reference to the underlying i2c bus, and the address of the i2c device. The address can be read from the device tree, but what is more difficult is to get the reference to the underlying i2c bus. That reference can be the i2c_adapter number (which only makes sense in a fixed-bus-number scheme), or a pointer to the i2c_adapter. For now the powermac systems do not have fixed i2c bus numbers, which is why I put the code instantiating the devices in i2c-powermac: there I have a pointer to the i2c_adapter. So I think we have two options: switch the powermac systems to fixed i2c bus numbers and instantiate the i2c sound devices from arch/powerpc/platforms/powermac/*, or find a way to obtain a reference to the relevant i2c_adapter. I think I have found a way to achieve the latter. This isn't exactly how the new model was supposed to work, but it has the advantage to be way less intrusive than my original proposal. It may require larger changes in the future as the i2c-core cleanups go on, but this should do for now. (...) I think I've found that one by now (snd_pmac_detect in sound/ppc/pmac.c, right?), thanks for the clarification. That's snd-powermac, yes. BTW, that code doesn't seem significantly different from what my patch is adding to i2c-powermac. That would be true, yes, with your patch I don't understand how to load snd-powermac _or_ snd-aoa based on the platform data (or in the case of snd-powermac, not load it automatically at all). My patch was really only about snd-aoa, it did not take care about snd-powermac at all, because I expected them to be essentially independent. Of course, if the code I added to i2c-powermac instantiates devices on systems which were previous handled by snd-powermac, it would break. More work would be needed to handle the systems supported by snd-powermac. (...) Oh ok, that kinda ties in with my very first question about what happens if the same chip is known in different contexts, on different buses etc. Makes sense in that case I guess. But I still think that you shouldn't auto-load the aoa codec modules based on this anyway. My new approach doesn't auto-load anything. Here we go, what you think? This time there is no logic change, I'm really only turning legacy code into new-style i2c code (basically calling i2c_new_device() instead of i2c_attach_client()) and that's about it. (Once again this is only build-tested and I would like people with the hardware to give it a try.) From: Jean Delvare kh...@linux-fr.org Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers The legacy i2c binding model is going away soon, so convert the AOA codec drivers to the new model or they'll break. Signed-off-by: Jean Delvare kh...@linux-fr.org Cc: Johannes Berg johan...@sipsolutions.net Cc: Benjamin Herrenschmidt b...@kernel.crashing.org --- sound/aoa/codecs/onyx.c | 71 +-- sound/aoa/codecs/tas.c | 63 + 2 files changed, 84 insertions(+), 50 deletions(-) --- linux-2.6.30-rc1.orig/sound/aoa/codecs/onyx.c 2009-04-09
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
Hi Johannes,
On Thu, 09 Apr 2009 14:34:44 +0200, Johannes Berg wrote:
My new approach doesn't auto-load anything. Here we go, what you think?
This time there is no logic change, I'm really only turning legacy code
into new-style i2c code (basically calling i2c_new_device() instead of
i2c_attach_client()) and that's about it.
(Once again this is only build-tested and I would like people with the
hardware to give it a try.)
Looks reasonable.
static int onyx_create(struct i2c_adapter *adapter,
struct device_node *node,
int addr)
{
+ struct i2c_board_info info;
+ struct i2c_client *client;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, aoa_codec_onyx, I2C_NAME_SIZE);
+ if (node) {
+ info.addr = addr;
+ info.platform_data = node;
+ client = i2c_new_device(adapter, info);
+ } else {
+ /* probe both possible addresses for the onyx chip */
+ unsigned short probe_onyx[] = {
+ 0x46, 0x47, I2C_CLIENT_END
+ };
+
+ client = i2c_new_probed_device(adapter, info, probe_onyx);
+ }
+ if (!client)
+ return -ENODEV;
+
+ list_add_tail(client-detected, client-driver-clients);
That list_add looks a little hackish, and wouldn't it be racy?
Yes it is a little hackish, the idea is to let i2c-core remove the
device if our i2c_driver is removed (which happens when you rmmod the
module). I'll add a comment to explain that. If there are more use
cases I might move that to a helper function in i2c-core.
No it's not racy, we're called with i2c-core's main mutex held.
+static const struct i2c_device_id onyx_i2c_id[] = {
+ { aoa_codec_onyx, 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, onyx_i2c_id);
Should it really export the device table?
(same comments for tas)
No, that's a leftover, I intended to remove it but forgot. It's gone
now. That being said, it's useless, but I don't think it would hurt.
It'll be until mid next week that I can test anything.
OK, thanks.
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
Hi all,
The legacy i2c model is going away soon, the remaining drivers that
still use it need to be converted very quickly. There are 3 sound
drivers remaining:
sound/aoa/codecs/onyx.c
sound/aoa/codecs/tas.c
sound/ppc/keywest.c
I've given a try to the former two, patch below. I could only
build-test it, so I would appreciate if anyone with supported hardware
could test the patch. I also welcome reviews and comments, of course. I
am not familiar with PowerPC so I might as well have done it wrong.
Basically the idea is to move the I2C device instantiation from the
codec drivers (onyx, tas) to the I2C adapter driver (i2c-powermac.)
This follows the Linux device driver model, requires slightly less
code, runs faster and and lets the required chip drivers be loaded by
udev or similar automatically.
The last driver to convert, keywest, is a mystery to me. I have a hard
time understanding how it interacts with tumbler and daca. I have the
feeling that it is essentially a glue module to workaround the legacy
i2c model limitations, so probably it could go away entirely now, but
I'm not sure how to do that in practice. Maybe tumbler and daca would
be made separate i2c drivers, I'm not sure.
One thing in particular which I find strange is that tumbler has
references to the TAS3004 device, much like the tas codec driver. It is
unclear to me whether tas is a replacement for tumbler, or if both
drivers work together, or if they are for separate hardware. I would
appreciate clarifications about this.
Thanks.
* * * * *
The legacy i2c binding model is going away soon, so convert the AOA
codec drivers to the new model or they'll break.
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Johannes Berg johan...@sipsolutions.net
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
---
drivers/i2c/busses/i2c-powermac.c | 76 +++
sound/aoa/codecs/onyx.c | 77 +++-
sound/aoa/codecs/tas.c| 89 +
3 files changed, 116 insertions(+), 126 deletions(-)
--- linux-2.6.30-rc1.orig/drivers/i2c/busses/i2c-powermac.c 2009-04-08
08:52:48.0 +0200
+++ linux-2.6.30-rc1/drivers/i2c/busses/i2c-powermac.c 2009-04-08
13:48:31.0 +0200
@@ -204,7 +204,7 @@ static int __devexit i2c_powermac_remove
static int __devinit i2c_powermac_probe(struct platform_device *dev)
{
struct pmac_i2c_bus *bus = dev-dev.platform_data;
- struct device_node *parent = NULL;
+ struct device_node *parent = NULL, *busnode, *devnode;
struct i2c_adapter *adapter;
char name[32];
const char *basename;
@@ -212,6 +212,7 @@ static int __devinit i2c_powermac_probe(
if (bus == NULL)
return -EINVAL;
+ busnode = pmac_i2c_get_bus_node(bus);
/* Ok, now we need to make up a name for the interface that will
* match what we used to do in the past, that is basically the
@@ -289,6 +290,79 @@ static int __devinit i2c_powermac_probe(
}
}
+ devnode = NULL;
+ while ((devnode = of_get_next_child(busnode, devnode)) != NULL) {
+ struct i2c_board_info info;
+ const u32 *addr;
+
+ /* Instantiate I2C sound device if present */
+ if (of_device_is_compatible(devnode, pcm3052)) {
+ printk(KERN_DEBUG i2c-powermac: found pcm3052\n);
+ addr = of_get_property(devnode, reg, NULL);
+ if (!addr)
+ continue;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ info.addr = (*addr) 1;
+ strlcpy(info.type, aoa_codec_onyx, I2C_NAME_SIZE);
+ info.platform_data = devnode;
+ i2c_new_device(adapter, info);
+ continue;
+ }
+
+ if (of_device_is_compatible(devnode, tas3004)) {
+ printk(KERN_DEBUG i2c-powermac: found tas3004\n);
+ addr = of_get_property(devnode, reg, NULL);
+ if (!addr)
+ continue;
+
+ memset(info, 0, sizeof(struct i2c_board_info));
+ info.addr = ((*addr) 1) 0x7f;
+ strlcpy(info.type, aoa_codec_tas, I2C_NAME_SIZE);
+ info.platform_data = devnode;
+ i2c_new_device(adapter, info);
+ continue;
+ }
+
+ /* older machines have no 'codec' node with a 'compatible'
+* property that says 'tas3004', they just have a 'deq'
+* node without any such property... */
+ if (strcmp(devnode-name, deq) == 0) {
+ printk(KERN_DEBUG i2c-powermac: found 'deq' node\n);
+ addr = of_get_property(devnode
Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers
. (Incidentally, snd-powermac cannot be auto-loaded at all currently, while aoa can via the fabric driver's device-tree binding) Hmm, OK, I expected the code I moved from the aoa drivers to i2c-powermac to only match the subset of machines actually supported by aoa. If that's not the case then indeed it is incorrect. Therefore, probing the codecs in i2c-powermac and automatically loading the corresponding aoa module will break sound on old machines. Does this mean that manually loading snd_aoa_codec_tas today on an old system with a TAS would break sound too? I would think that if you removed the MODULE_DEVICE_TABLE from your patch, it may continue to work because the aoa fabric driver loads the modules as before, and on old machines nothing loads automatically and snd-powermac can be loaded manually. However, it will still be less efficient because you will be probing _all_ those codecs, notably the tas family, even on machines that are known to not have it (machines that have onyx). Putting that mutual exclusion information into i2c-powermac would be misplaced, imho. Note also that there's one more codec (topaz) which isn't currently supported. In conclusion, I think that the old/existing/legacy i2c binding model was much better suited to platform knowledge about the way machines are put together, and the new code is, as far as I can tell, less efficient -- contrary to your assertion. My comment about efficiency was related to the fact that the sound chip detection code would run only on the i2c-power mac bus, rather than all I2C buses on the system. Admittedly I had missed the larger picture of all detections running on all powermac systems, which wasn't the case so far. Anyway, the key point of my patch is to get rid of the legacy i2c binding model, not efficiency. Since I'm away from all machines I could test this with I have no data on any that or the module device table thing I pointed out for now. Anyway, some more technical comments on your patch: * I realise you just copied things around but it would be nice to clean up the coding style, especially comment style, a little while at it. (yeah, it's my fault) I can fix anything you like, just tell me what :) * aoa_codec_* is the module name, I see no reason to use that as the i2c name, that should be the codec's name instead (aka pcm3052 etc) The names are totally arbitrary and we can change them if you like. Keep in mind though that we should avoid using mere chip names for non-generic drivers. The aoa drivers are powermac-specific, we don't want the names we pick to collide with another driver, that's why I chose to keep the aoa prefix. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete
On Tue, 10 Feb 2009 08:59:57 -0600, Timur Tabi wrote: On Fri, Feb 6, 2009 at 8:00 AM, Timur Tabi ti...@freescale.com wrote: The i2c_wait() function is using wait_event_interruptible_timeout() to wait for the I2C controller to signal that it has completed an I2C bus operation. If the process that causes the I2C operation terminated abruptly, the wait will be interrupted, returning an error. It is better to let the I2C operation finished before the process exits. It is safe to use wait_event_timeout() instead, because the timeout will allow the process to exit if the I2C bus hangs. It's also better to allow the I2C operation to finish, because unacknowledged I2C operations can cause the I2C bus to hang. Signed-off-by: Timur Tabi ti...@freescale.com Jean, Could you pick up this patch for 2.6.30? No, that's something for either Ben Dooks (Cc'd) or the powerpc tree. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] i2c-mpc: do not allow interruptions when waiting for I2C to complete
On Tue, 10 Feb 2009 10:01:54 -0600, Timur Tabi wrote: Jean Delvare wrote: No, that's something for either Ben Dooks (Cc'd) or the powerpc tree. This patch has nothing to do with ARM, so Kumar will pick it up, if you ACK it. Why are you mentioning ARM? From MAINTAINERS: I2C SUBSYSTEM P: Jean Delvare (PC drivers, core) M: kh...@linux-fr.org P: Ben Dooks (embedded platforms) M: ben-li...@fluff.org To the best of my knowledge i2c-mpc falls into the embedded platforms category and is thus under Ben's responsibility. Thanks, -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] i2c: i2c-ibm_iic message can be confusing
Hi Ben, On Wed, 04 Feb 2009 14:55:33 +1100, Benjamin Herrenschmidt wrote: Acked-by: Jean Delvare kh...@linux-fr.org Jean, you'll take that in your tree or should I take it in mine ? No, I'm not taking it, i2c-ibm_iic is under Ben Dooks' jurisdiction. So it's up to either him or you. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] i2c: i2c-ibm_iic message can be confusing
On Mon, 2 Feb 2009 12:01:59 -0500, Sean MacLennan wrote: This is a trivial patch that does not need to be in 2.6.29. While tracking down an EEPROM problem, I found the messages confusing... it looked like the EEPROM was being started before the I2C driver! Here is an example: at24 0-0052: 512 byte 24c04 EEPROM (writable) ibm-iic ef600700.i2c: using standard (100 kHz) mode ad7414 0-004a: chip found It looks like the at24 starts first, then the i2c driver, then the ad7414. By moving the message to after the of scan, we always get the driver, then the devices. Cheers, Sean Print the i2c driver message before scanning for devices so that the logs show the driver, then the devices. Currently you can get device(s), driver, device(s). Signed-off-by: Sean MacLennan smaclen...@pikatech.com --- diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index 88f0db7..7fc0729 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -756,12 +756,12 @@ static int __devinit iic_probe(struct of_device *ofdev, goto error_cleanup; } - /* Now register all the child nodes */ - of_register_i2c_devices(adap, np); - dev_info(ofdev-dev, using %s mode\n, dev-fast_mode ? fast (400 kHz) : standard (100 kHz)); + /* Now register all the child nodes */ + of_register_i2c_devices(adap, np); + return 0; error_cleanup: Indeed. Acked-by: Jean Delvare kh...@linux-fr.org -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc/83xx: Move mcu_mpc8349emitx driver out of drivers/i2c/chips/
On Tue, 13 Jan 2009 08:39:37 -0600, Kumar Gala wrote: On Jan 11, 2009, at 10:55 AM, Anton Vorontsov wrote: This patch is used to help Jean Delvare to get rid of drivers/i2c/ chips/ directory. The new location suggested by Kumar Gala: as the driver is 83xx specific it's placed into arch/powerpc/platforms/83xx/. Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com --- The same patch but suitable for patch(1). arch/powerpc/platforms/83xx/Makefile |1 + arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c | 209 +++ + arch/powerpc/platforms/Kconfig | 11 ++ drivers/i2c/chips/Kconfig | 11 -- drivers/i2c/chips/Makefile |1 - drivers/i2c/chips/mcu_mpc8349emitx.c | 209 6 files changed, 221 insertions(+), 221 deletions(-) create mode 100644 arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c delete mode 100644 drivers/i2c/chips/mcu_mpc8349emitx.c applied Great, thanks Anton and Kumar! -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc/83xx: Move mcu_mpc8349emitx driver out of drivers/i2c/chips/
Hi Anton, On Sun, 11 Jan 2009 19:51:36 +0300, Anton Vorontsov wrote: This patch is used to help Jean Delvare to get rid of drivers/i2c/chips/ directory. The new location suggested by Kumar Gala: as the driver is 83xx specific it's placed into arch/powerpc/platforms/83xx/. Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com Thanks for doing this. Do you expect me to take this patch in my i2c tree, or will it go in some powerpc tree? -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc/83xx: Move mcu_mpc8349emitx driver out of drivers/i2c/chips/
On Sun, 11 Jan 2009 20:24:10 +0300, Anton Vorontsov wrote: On Sun, Jan 11, 2009 at 06:10:55PM +0100, Jean Delvare wrote: Hi Anton, On Sun, 11 Jan 2009 19:51:36 +0300, Anton Vorontsov wrote: This patch is used to help Jean Delvare to get rid of drivers/i2c/chips/ directory. The new location suggested by Kumar Gala: as the driver is 83xx specific it's placed into arch/powerpc/platforms/83xx/. Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com Thanks for doing this. Do you expect me to take this patch in my i2c tree, or will it go in some powerpc tree? As the patch adds some code to arch/powerpc/, I believe Kumar would want to merge it into his powerpc tree. In that case I think we'll need your Acked-by line for formality. Sure: Acked-by: Jean Delvare kh...@linux-fr.org But let's wait for Kumar's decision. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/7] i2c: add info-archdata field
On Wed, 22 Oct 2008 11:27:34 +1100, Benjamin Herrenschmidt wrote: On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote: Hi Anton, On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote: If present the info-archdata is copied into the dev-archdata. Some (OpenFirmware) platforms need it. Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] --- So who's pushing this one ? Jean ? As I wrote before, I'm happy to take this patch if you want me to, but I also have no objection if the author of this patchset wants to push it himself together with the rest of the set. Just let me know what you expect from me (preferably in the next few hours, as I will be sending my second and last round of i2c patches for kernel 2.6.28 to Linus today.) -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/7] i2c: add info-archdata field
On Wed, 22 Oct 2008 14:08:13 +0400, Anton Vorontsov wrote: On Wed, Oct 22, 2008 at 06:37:55PM +1100, Benjamin Herrenschmidt wrote: On Wed, 2008-10-22 at 08:50 +0200, Jean Delvare wrote: On Wed, 22 Oct 2008 11:27:34 +1100, Benjamin Herrenschmidt wrote: On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote: Hi Anton, On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote: If present the info-archdata is copied into the dev-archdata. Some (OpenFirmware) platforms need it. Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] --- So who's pushing this one ? Jean ? As I wrote before, I'm happy to take this patch if you want me to, but I also have no objection if the author of this patchset wants to push it himself together with the rest of the set. Just let me know what you expect from me (preferably in the next few hours, as I will be sending my second and last round of i2c patches for kernel 2.6.28 to Linus today.) Anton, I've done my last batch for 2.6.28 before -rc1 so it might be better if it goes through Jean no ? Sorry, I guess the few hours limit was done exactly when I was sleeping. ;-) Anyway, I'm fine either way. Don't see any problem if it goes i2c or powerpc tree. OK, then I'm taking the patch in my tree and it will go to Linus later today. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] [RESEND] i2c-cpm: Suppress autoprobing for devices
Hi Wolfram, On Thu, 16 Oct 2008 21:12:00 +0200, Wolfram Sang wrote: Similar to commit 618b26d52843c0f85b8eb143cf2695d7f6fd072d, also remove automatic probing for this i2c controller. Might need updates to dts files using it. Signed-off-by: Wolfram Sang [EMAIL PROTECTED] Acked-by: Jochen Friedrich [EMAIL PROTECTED] --- Resend with cc to i2c. drivers/i2c/busses/i2c-cpm.c |1 - 1 file changed, 1 deletion(-) Index: linux-2.6/drivers/i2c/busses/i2c-cpm.c === --- linux-2.6.orig/drivers/i2c/busses/i2c-cpm.c +++ linux-2.6/drivers/i2c/busses/i2c-cpm.c @@ -423,7 +423,6 @@ static const struct i2c_adapter cpm_ops .owner = THIS_MODULE, .name = i2c-cpm, .algo = cpm_i2c_algo, - .class = I2C_CLASS_HWMON | I2C_CLASS_SPD, }; static int __devinit cpm_i2c_setup(struct cpm_i2c *cpm) Acked-by: Jean Delvare [EMAIL PROTECTED] I guess you do _not_ want me to push this patch upstream now and it better goes through the powerpc tree to allow a synchronization with dts file changes? If I'm wrong and you want me to take this patch in my tree then please tell me. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
