Re: [PATCH 10/10] i2c: pca954x: get rid of the i2c deadlock workaround
On 01/04/2016 04:10 PM, Peter Rosin wrote: > From: Peter Rosin> > Signed-off-by: Peter Rosin It would be quite good if the commit messaged said why it is now safe to remove the workaround. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"
On 11/17/2015 03:28 PM, Shubhrajyoti Datta wrote: > On Tue, Nov 17, 2015 at 1:04 PM, Lars-Peter Clausen <l...@metafoo.de> wrote: >> On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote: >>> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen <l...@metafoo.de> wrote: >>>> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every >>>> transfer") removed the reinitialization of the controller before the start >>>> of each transfer. Apparently this change is not safe to make and the commit >>>> results in random I2C bus failures. >>> >>> Which is the platform and the ip version that you saw the issue. >>> Did you see the issue with read and write as well? >> >> The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few >> platforms, custom ones and standard ones and I could reproduce it on most. >> One of them was on the ZED board. The one where I couldn't reproduce it was >> the ZC706. But that doesn't necessarily mean it doesn't happen there, just >> that it is not triggered by the testcase. > All the boards having the same version of the ip is what I have understood. > > Thanks for the info I will try to reproduce the issue. > >> >> The problem is that it is random corruption, > Of registers? To be honest I don't know if there is corruption during I2C write transfers, but there is definitely corruption during read transactions. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] i2c: xiic: Prevent concurrent running of the IRQ handler and __xiic_start_xfer()
Prior to commit e6c9a037bc8a ("i2c: xiic: Remove the disabling of interrupts") IRQs where disabled when the initial __xiic_start_xfer() was called. After the commit the interrupt is enabled while the function is running, this means it is possible for the interrupt to be triggered while the function is still running. When this happens the internal data structures get corrupted and undefined behavior can occur like the following crash: Internal error: Oops: 17 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 2040 Comm: i2cdetect Not tainted 4.0.0-02856-g047a308 #10956 Hardware name: Xilinx Zynq Platform task: ee0c9500 ti: e99a2000 task.ti: e99a2000 PC is at __xiic_start_xfer+0x6c4/0x7c8 LR is at __xiic_start_xfer+0x690/0x7c8 pc : []lr : []psr: 800f0013 sp : e99a3da8 ip : fp : r10: 0001 r9 : 600f0013 r8 : f018 r7 : f018 r6 : c064e444 r5 : 0017 r4 : ee031010 r3 : r2 : r1 : 600f0013 r0 : 000f Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 18c5387d Table: 29a5404a DAC: 0015 Process i2cdetect (pid: 2040, stack limit = 0xe99a2210) Stack: (0xe99a3da8 to 0xe99a4000) 3da0: ee031010 0001 ee031020 ee031224 c02bc5ec 3dc0: ee34c604 ee0c9500 e99a3dcc e99a3dd0 e99a3dd0 e99a3dd8 c069f0e8 3de0: ee031020 c064e100 90bb e99a3e48 c02b6590 ee031020 0001 3e00: e99a3e48 ee031020 e99a3e63 0001 c02b6ec4 3e20: c02b7320 e99a3ef0 e99e3df0 3e40: 0103 2814575f 003e c00a e99a3e85 0001003e ee0c e99a3e63 3e60: eefd3578 c064e61c ee0c9500 c0041e04 056c e9a56db8 6e5a b6f5c000 3e80: ee0c9548 eefd0040 0001 eefd3540 ee0c9500 eefd39a0 c064b540 ee0c9500 3ea0: ee92b000 bef4862c ee34c600 e99ecdc0 0720 0003 3ec0: e99a2000 c02b8b30 e99a3f24 3ee0: b6e8 c04257e8 e99a3f24 c02b8f08 0703 3f00: 0003 c02116bc ee935300 bef4862c ee34c600 e99ecdc0 c02b91f0 3f20: e99ecdc0 0720 bef4862c eeb725f8 e99ecdc0 c00c9e2c 0003 0003 3f40: ee248dc0 ee248dc8 0002 eeb7c1a8 c00bb360 3f60: 0003 ee248dc0 bef4862c e99ecdc0 e99ecdc0 0720 3f80: 0003 e99a2000 c00c9f68 b6f22000 0036 3fa0: c000dfa4 c000de20 0003 0720 bef4862c bef4862c 3fc0: b6f22000 0036 b6f6 3fe0: 00013040 bef48614 8cab b6ecdbe6 400f0030 0003 2f7fd821 2f7fdc21 [] (__xiic_start_xfer) from [] (xiic_xfer+0x94/0x168) [] (xiic_xfer) from [] (__i2c_transfer+0x4c/0x7c) [] (__i2c_transfer) from [] (i2c_transfer+0x9c/0xc4) [] (i2c_transfer) from [] (i2c_smbus_xfer+0x3a0/0x4ec) [] (i2c_smbus_xfer) from [] (i2cdev_ioctl_smbus+0xb0/0x214) [] (i2cdev_ioctl_smbus) from [] (i2cdev_ioctl+0xa0/0x1d4) [] (i2cdev_ioctl) from [] (do_vfs_ioctl+0x4b0/0x5b8) [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c) [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x34) Code: e283300c e5843210 eafffe64 e5943210 (e1d320b4) The issue can easily be reproduced by performing I2C access under high system load or IO load. To fix the issue protect the invocation to __xiic_start_xfer() form xiic_start_xfer() with the same lock that is used to protect the interrupt handler. Fixes: e6c9a037bc8a ("i2c: xiic: Remove the disabling of interrupts") Signed-off-by: Lars-Peter Clausen <l...@metafoo.de> --- drivers/i2c/busses/i2c-xiic.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 705cf69..0b20449 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -664,9 +664,8 @@ static void xiic_start_xfer(struct xiic_i2c *i2c) { spin_lock(>lock); xiic_reinit(i2c); - spin_unlock(>lock); - __xiic_start_xfer(i2c); + spin_unlock(>lock); } static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"
Commit d701667bb331 ("i2c: xiic: Do not reset controller before every transfer") removed the reinitialization of the controller before the start of each transfer. Apparently this change is not safe to make and the commit results in random I2C bus failures. An easy way to trigger the issue is to run i2cdetect. Without the patch applied: 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: -- -- -- -- -- -- -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 70: -- -- -- -- -- -- -- -- With the patch applied every other or so invocation: 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 70: -- -- -- -- -- -- -- -- So revert the commit for now. Fixes: d701667bb331 ("i2c: xiic: Do not reset controller before every transfer") Signed-off-by: Lars-Peter Clausen <l...@metafoo.de> --- drivers/i2c/busses/i2c-xiic.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index e23a7b0..705cf69 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -662,6 +662,9 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c) static void xiic_start_xfer(struct xiic_i2c *i2c) { + spin_lock(>lock); + xiic_reinit(i2c); + spin_unlock(>lock); __xiic_start_xfer(i2c); } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] i2c: xiic: Replace spinlock with mutex
All protected sections are only called from sleep-able context, so there is no need to use a spinlock. Signed-off-by: Lars-Peter Clausen <l...@metafoo.de> --- drivers/i2c/busses/i2c-xiic.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 0b20449..6efd200 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -70,7 +70,7 @@ struct xiic_i2c { wait_queue_head_t wait; struct i2c_adapter adap; struct i2c_msg *tx_msg; - spinlock_t lock; + struct mutexlock; unsigned inttx_pos; unsigned intnmsgs; enum xilinx_i2c_state state; @@ -369,7 +369,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id) * To find which interrupts are pending; AND interrupts pending with * interrupts masked. */ - spin_lock(>lock); + mutex_lock(>lock); isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET); ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET); pend = isr & ier; @@ -497,7 +497,7 @@ out: dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr); xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr); - spin_unlock(>lock); + mutex_unlock(>lock); return IRQ_HANDLED; } @@ -662,10 +662,10 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c) static void xiic_start_xfer(struct xiic_i2c *i2c) { - spin_lock(>lock); + mutex_lock(>lock); xiic_reinit(i2c); __xiic_start_xfer(i2c); - spin_unlock(>lock); + mutex_unlock(>lock); } static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) @@ -745,7 +745,7 @@ static int xiic_i2c_probe(struct platform_device *pdev) i2c->adap.dev.parent = >dev; i2c->adap.dev.of_node = pdev->dev.of_node; - spin_lock_init(>lock); + mutex_init(>lock); init_waitqueue_head(>wait); ret = devm_request_threaded_irq(>dev, irq, xiic_isr, -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"
On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote: > On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen <l...@metafoo.de> wrote: >> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every >> transfer") removed the reinitialization of the controller before the start >> of each transfer. Apparently this change is not safe to make and the commit >> results in random I2C bus failures. > > Which is the platform and the ip version that you saw the issue. > Did you see the issue with read and write as well? The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few platforms, custom ones and standard ones and I could reproduce it on most. One of them was on the ZED board. The one where I couldn't reproduce it was the ZC706. But that doesn't necessarily mean it doesn't happen there, just that it is not triggered by the testcase. The problem is that it is random corruption, so some I2C devices might start to behave strangely at some point. The only good more or less reliable way to reproduce it that I found was to run i2cdetect a couple of times and at least one of them will produce strange behavior. >> >> An easy way to trigger the issue is to run i2cdetect. >> >> Without the patch applied: >> 0 1 2 3 4 5 6 7 8 9 a b c d e f >> 00: -- -- -- -- -- -- -- -- -- -- -- -- -- >> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU >> 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 70: -- -- -- -- -- -- -- -- >> >> With the patch applied every other or so invocation: >> 0 1 2 3 4 5 6 7 8 9 a b c d e f >> 00: 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f >> 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f >> 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f >> 30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU >> 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 70: -- -- -- -- -- -- -- -- >> > I assume that you have these many peripherals. > on the bus am I right? Sorry, I should have been more clear. The first one is before the patch that introduces the issue, the second one is with the patch that introduces the issue. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode
>> So maybe more like this: >> >> static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks) >> { >> if (!adap->quirks) >> return false; >> return (adap->quirks->flags & quirks) == quirks; >> } > > Should I use bool (like in your snippet) or int (like > i2c_check_functionality) as return type? I'd use bool, given that the result is a boolean value. It's semantically more clear this way. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode
On 10/28/2015 07:58 AM, Nicola Corna wrote: [...] > + holdmode = !((*client)->adapter->quirks && > + (*client)->adapter->quirks->flags & [...] > +client->adapter->quirks && > +client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH) This is rather ugly, can we get a helper in the I2C core something along the lines of i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH) - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode
On 10/28/2015 07:35 PM, Nicola Corna wrote: > October 28 2015 10:38 AM, "Lars-Peter Clausen" <l...@metafoo.de> wrote: > >> On 10/28/2015 07:58 AM, Nicola Corna wrote: >> [...] >> >>> + holdmode = !((*client)->adapter->quirks && >>> + (*client)->adapter->quirks->flags & >> >> [...] >> >>> + client->adapter->quirks && >>> + client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH) >> >> This is rather ugly, can we get a helper in the I2C core something along the >> lines of >> >> i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH) >> >> - Lars > > Something like this? > > --- > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index a69a9a0..a06ffc0 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -613,6 +613,12 @@ static inline int i2c_check_functionality(struct > i2c_adapter *adap, u32 func) > return (func & i2c_get_functionality(adap)) == func; > } > > +/* Return 1 if adapter has the specified quirks, 0 if not. */ > +static inline int i2c_check_quirks(struct i2c_adapter *adap, u64 quirks) > +{ > + return (quirks & (adap->quirks ? adap->quirks->flags : 0)) == quirks; > +} This is not a code obfuscation contest ;) So maybe more like this: static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks) { if (!adap->quirks) return false; return (adap->quirks->flags & quirks) == quirks; } And please use kernel-doc for the documentation. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: s3c2410: Don't enable PM runtime on the adapter device
On 04/16/2015 12:33 PM, Sylwester Nawrocki wrote: On 16/04/15 12:10, Charles Keepax wrote: Commit 523c5b89640e (i2c: Remove support for legacy PM) removed the PM ops from the bus type, which causes the pm operations on the s3c2410 adapter device to fail (-ENOSUPP in rpm_callback). The adapter device doesn't get bound to a driver and as such can't have its own pm_runtime callbacks. Previously this was fine as the bus callbacks would have been used, but now this can cause devices which use PM runtime and are attached over I2C to fail to resume. This commit fixes this issue by just doing the PM operations directly on the I2C device, rather than the adapter device in the driver and adding some stub callbacks for runtime suspend and resume. Signed-off-by: Charles Keepax ckee...@opensource.wolfsonmicro.com --- drivers/i2c/busses/i2c-s3c2410.c | 21 - 1 files changed, 16 insertions(+), 5 deletions(-) @@ -1253,7 +1253,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev) platform_set_drvdata(pdev, i2c); Wouldn't adding pm_runtime_no_callbacks(pdev-dev); here let us avoid the runtime resume/suspend stubs? Or just do pm_runtime_no_callbacks on the adapter device. Preferably in the I2C core. That should solve the issue without requiring any other changes. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/66] rtl2830: implement own I2C locking
On 02/02/2015 09:33 PM, Wolfram Sang wrote: Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. Jean, Are you ok with such patch? If so, please ack. Jean handed over I2C to me in late 2012 :) Basic problem here is that I2C-mux itself is controlled by same I2C device which implements I2C adapter for tuner. Here is what connections looks like: ___ | USB IF | | demod| |tuner | |---| || || | |--I2C--|-/ -|--I2C--|| |I2C master | | I2C mux | | I2C slave | |___| || || So when tuner is called via I2C, it needs recursively call same I2C adapter which is already locked. More elegant solution would be indeed nice. So, AFAIU this is the same problem that I2C based mux devices have (like drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked transfers... But those are all called with the lock for the adapter being held. I'm not convinced this is the same for this patch. This patch seems to add a device mutex which protects against concurrent access to the bus with the device itself, but not against concurrent access with other devices on the same bus. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI
Added I2C to Cc. On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote: Using i2c auto detect feature and auto device creation feature, enumerate ak8975 device, by checking their presence. This is needed because when this device sits behind an i2c mux, there is no way to define i2c mux in ACPI. This will enable ak8975 on windows based tablets/laptops running Linux when connected via a mux. Since DT model already can define an i2c mux and devices connected to it, this feature is only enabled for ACPI. This is quite a bit of a hack. Did they decide to not include the device in the ACPI description at all or is there a special id for INV6050+AK8975? How does Windows decide whether there is a device or not? Signed-off-by: Srinivas Pandruvada srinivas.pandruv...@linux.intel.com --- drivers/iio/magnetometer/ak8975.c | 35 +++ 1 file changed, 35 insertions(+) diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c index 0d10a4b..c3455bd 100644 --- a/drivers/iio/magnetometer/ak8975.c +++ b/drivers/iio/magnetometer/ak8975.c @@ -820,6 +820,36 @@ static const struct i2c_device_id ak8975_id[] = { MODULE_DEVICE_TABLE(i2c, ak8975_id); +#if defined(CONFIG_ACPI) +static int ak8975_detect(struct i2c_client *temp_client, +struct i2c_board_info *info) +{ + struct i2c_adapter *adapter; + int i, j; + int ret; + + /* autodetect only when we are behind a mux */ + adapter = i2c_parent_is_i2c_adapter(temp_client-adapter); + if (!adapter) + return -ENODEV; + + for (i = 0; i AK_MAX_TYPE; ++i) { + ret = ak8975_who_i_am(temp_client, i); + if (ret = 0) { + for (j = 0; j ARRAY_SIZE(ak8975_id) - 1; ++j) { + if (i == (int)ak8975_id[j].driver_data) { + strlcpy(info-type, ak8975_id[j].name, + I2C_NAME_SIZE); + return 0; + } + } + } + } + + return -ENODEV; +} +#endif + static const struct of_device_id ak8975_of_match[] = { { .compatible = asahi-kasei,ak8975, }, { .compatible = ak8975, }, @@ -841,6 +871,11 @@ static struct i2c_driver ak8975_driver = { }, .probe = ak8975_probe, .id_table = ak8975_id, +#if defined(CONFIG_ACPI) + .class = I2C_CLASS_HWMON, + .address_list = I2C_ADDRS(0x0C, 0x0D, 0x0E, 0x0F), + .detect = ak8975_detect, In contrast to the commit message this is always enabled if the kernel contains ACPI support, not if the device is a ACPI device. +#endif }; module_i2c_driver(ak8975_driver); -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI
On 12/18/2014 05:52 PM, Srinivas Pandruvada wrote: On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote: Added I2C to Cc. On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote: Using i2c auto detect feature and auto device creation feature, enumerate ak8975 device, by checking their presence. This is needed because when this device sits behind an i2c mux, there is no way to define i2c mux in ACPI. This will enable ak8975 on windows based tablets/laptops running Linux when connected via a mux. Since DT model already can define an i2c mux and devices connected to it, this feature is only enabled for ACPI. This is quite a bit of a hack. Why? Auto detect is standard feature of i2c devices. This is using standard auto detect feature provided by the framework. Auto detect is ugly, slow and unreliable, it's kind of like the last straw if nothing else works. Autodetect will be executed for every adapter with every device that supports auto detection, so you want to keep the amount of devices that do auto detection to a minimum in order to avoid both false positives and reduce boot time. Did they decide to not include the device in the ACPI description at all or is there a special id for INV6050+AK8975? This device needs has one combined id. Windows has a singe driver processing both as a combo device. Ok, then use the combined id to instantiate INV6050+AK8975. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] iio: ak8975: Added autodetect feature for ACPI
On 12/18/2014 06:30 PM, Srinivas Pandruvada wrote: On Thu, 2014-12-18 at 18:05 +0100, Lars-Peter Clausen wrote: On 12/18/2014 05:52 PM, Srinivas Pandruvada wrote: On Thu, 2014-12-18 at 17:28 +0100, Lars-Peter Clausen wrote: Added I2C to Cc. On 12/15/2014 10:19 PM, Srinivas Pandruvada wrote: Using i2c auto detect feature and auto device creation feature, enumerate ak8975 device, by checking their presence. This is needed because when this device sits behind an i2c mux, there is no way to define i2c mux in ACPI. This will enable ak8975 on windows based tablets/laptops running Linux when connected via a mux. Since DT model already can define an i2c mux and devices connected to it, this feature is only enabled for ACPI. This is quite a bit of a hack. Why? Auto detect is standard feature of i2c devices. This is using standard auto detect feature provided by the framework. Auto detect is ugly, slow and unreliable, it's kind of like the last straw if nothing else works. That is true here. You can't enumerate this device by ACPI. As discussed before we created i2c mux in inv6050 so that we can use AK8975 in bypass mode. I added some API to create i2c device on this mux, which Wolfram didn't like. He wanted to enumerate using existing mechanisms. If there is only a single ACPI ID that says this is a INV6050 with a AK8975 attached then the way to handle this is to have a driver that binds to that id and creates both devices. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: Remove support for legacy PM
On 12/04/2014 07:11 PM, Wolfram Sang wrote: On Sun, Nov 30, 2014 at 05:52:32PM +0100, Lars-Peter Clausen wrote: There haven't been any I2C driver that use the legacy suspend/resume callbacks for a while now and new drivers are supposed to use PM ops. So remove support for legacy suspend/resume for I2C drivers. Since there aren't any special bus specific things to do during suspend/resume and since the PM core will automatically fallback directly to using the device's PM ops if no bus PM ops are specified there is no need to have any I2C bus PM ops. Signed-off-by: Lars-Peter Clausen l...@metafoo.de I'll keep this for 3.20, 3.19 is too close now. But thanks, the diffstat looks great :) Just curious, has buildbot tested this already? No, but I did allyesconfig on both ARM and x86. As well as a `grep i2c_driver -A20 * -r | grep '\(suspend\|resume\)'` and running the following cocci script. @@ identifier fn; identifier drv; @@ struct i2c_driver drv = { ..., * .suspend = fn, ..., }; @@ identifier fn; identifier drv; @@ struct i2c_driver drv = { ..., * .resume = fn, ..., }; None of those methods found anything, so I'm quite confident that there are no users left. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C
On 12/02/2014 03:15 PM, Wolfram Sang wrote: What do you do when disable repeated start? Sending STOP and START? If so, this is really something different than repeated start. By using I2C_FUNC_I2C a user expects repeated start, so if the HW does not support it, we should say so and don't try to emulate it with something different. Yes, we send stop. As said before, this is wrong. Another master could interfere between the messages when using stop+start. This is no replacement for repeated start. More importantly a lot of I2C slaves also reset their internal state machine on a stop. So e.g. if reading a register is implemented by doing start,write,repeated start,read,stop and you replace that with start,write,stop,start,read,stop you'll always read register zero instead of the register you wanted to read. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Documentation: i2c: Use PM ops instead of legacy suspend/resume
New drivers should use PM ops instead of the legacy suspend/resume callbacks. Update the I2C device driver guides to reflect this. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- Documentation/i2c/upgrading-clients | 6 ++ Documentation/i2c/writing-clients | 8 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Documentation/i2c/upgrading-clients b/Documentation/i2c/upgrading-clients index 8e5fbd8..ccba3ff 100644 --- a/Documentation/i2c/upgrading-clients +++ b/Documentation/i2c/upgrading-clients @@ -79,11 +79,10 @@ static struct i2c_driver example_driver = { .driver = { .owner = THIS_MODULE, .name = example, + .pm = example_pm_ops, }, .attach_adapter = example_attach_adapter, .detach_client = example_detach, - .suspend= example_suspend, - .resume = example_resume, }; @@ -272,10 +271,9 @@ static struct i2c_driver example_driver = { .driver = { .owner = THIS_MODULE, .name = example, + .pm = example_pm_ops, }, .id_table = example_idtable, .probe = example_probe, .remove = example_remove, - .suspend= example_suspend, - .resume = example_resume, }; diff --git a/Documentation/i2c/writing-clients b/Documentation/i2c/writing-clients index 6b344b5..a755b14 100644 --- a/Documentation/i2c/writing-clients +++ b/Documentation/i2c/writing-clients @@ -36,6 +36,7 @@ MODULE_DEVICE_TABLE(i2c, foo_idtable); static struct i2c_driver foo_driver = { .driver = { .name = foo, + .pm = foo_pm_ops, /* optional */ }, .id_table = foo_idtable, @@ -47,8 +48,6 @@ static struct i2c_driver foo_driver = { .address_list = normal_i2c, .shutdown = foo_shutdown, /* optional */ - .suspend= foo_suspend, /* optional */ - .resume = foo_resume, /* optional */ .command= foo_command, /* optional, deprecated */ } @@ -279,8 +278,9 @@ Power Management If your I2C device needs special handling when entering a system low power state -- like putting a transceiver into a low power mode, or -activating a system wakeup mechanism -- do that in the suspend() method. -The resume() method should reverse what the suspend() method does. +activating a system wakeup mechanism -- do that by implementing the +appropriate callbacks for the dev_pm_ops of the driver (like suspend +and resume). These are standard driver model calls, and they work just like they would for any other driver stack. The calls can sleep, and can use -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] i2c: Remove support for legacy PM
There haven't been any I2C driver that use the legacy suspend/resume callbacks for a while now and new drivers are supposed to use PM ops. So remove support for legacy suspend/resume for I2C drivers. Since there aren't any special bus specific things to do during suspend/resume and since the PM core will automatically fallback directly to using the device's PM ops if no bus PM ops are specified there is no need to have any I2C bus PM ops. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/i2c/i2c-core.c | 118 - include/linux/i2c.h| 4 -- 2 files changed, 122 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 3105bd2..f23c03b 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -695,101 +695,6 @@ static void i2c_device_shutdown(struct device *dev) driver-shutdown(client); } -#ifdef CONFIG_PM_SLEEP -static int i2c_legacy_suspend(struct device *dev, pm_message_t mesg) -{ - struct i2c_client *client = i2c_verify_client(dev); - struct i2c_driver *driver; - - if (!client || !dev-driver) - return 0; - driver = to_i2c_driver(dev-driver); - if (!driver-suspend) - return 0; - return driver-suspend(client, mesg); -} - -static int i2c_legacy_resume(struct device *dev) -{ - struct i2c_client *client = i2c_verify_client(dev); - struct i2c_driver *driver; - - if (!client || !dev-driver) - return 0; - driver = to_i2c_driver(dev-driver); - if (!driver-resume) - return 0; - return driver-resume(client); -} - -static int i2c_device_pm_suspend(struct device *dev) -{ - const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL; - - if (pm) - return pm_generic_suspend(dev); - else - return i2c_legacy_suspend(dev, PMSG_SUSPEND); -} - -static int i2c_device_pm_resume(struct device *dev) -{ - const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL; - - if (pm) - return pm_generic_resume(dev); - else - return i2c_legacy_resume(dev); -} - -static int i2c_device_pm_freeze(struct device *dev) -{ - const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL; - - if (pm) - return pm_generic_freeze(dev); - else - return i2c_legacy_suspend(dev, PMSG_FREEZE); -} - -static int i2c_device_pm_thaw(struct device *dev) -{ - const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL; - - if (pm) - return pm_generic_thaw(dev); - else - return i2c_legacy_resume(dev); -} - -static int i2c_device_pm_poweroff(struct device *dev) -{ - const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL; - - if (pm) - return pm_generic_poweroff(dev); - else - return i2c_legacy_suspend(dev, PMSG_HIBERNATE); -} - -static int i2c_device_pm_restore(struct device *dev) -{ - const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL; - - if (pm) - return pm_generic_restore(dev); - else - return i2c_legacy_resume(dev); -} -#else /* !CONFIG_PM_SLEEP */ -#define i2c_device_pm_suspend NULL -#define i2c_device_pm_resume NULL -#define i2c_device_pm_freeze NULL -#define i2c_device_pm_thaw NULL -#define i2c_device_pm_poweroff NULL -#define i2c_device_pm_restore NULL -#endif /* !CONFIG_PM_SLEEP */ - static void i2c_client_dev_release(struct device *dev) { kfree(to_i2c_client(dev)); @@ -834,27 +739,12 @@ static const struct attribute_group *i2c_dev_attr_groups[] = { NULL }; -static const struct dev_pm_ops i2c_device_pm_ops = { - .suspend = i2c_device_pm_suspend, - .resume = i2c_device_pm_resume, - .freeze = i2c_device_pm_freeze, - .thaw = i2c_device_pm_thaw, - .poweroff = i2c_device_pm_poweroff, - .restore = i2c_device_pm_restore, - SET_RUNTIME_PM_OPS( - pm_generic_runtime_suspend, - pm_generic_runtime_resume, - NULL - ) -}; - struct bus_type i2c_bus_type = { .name = i2c, .match = i2c_device_match, .probe = i2c_device_probe, .remove = i2c_device_remove, .shutdown = i2c_device_shutdown, - .pm = i2c_device_pm_ops, }; EXPORT_SYMBOL_GPL(i2c_bus_type); @@ -1850,14 +1740,6 @@ int i2c_register_driver(struct module *owner, struct i2c_driver *driver) if (res) return res; - /* Drivers should switch to dev_pm_ops instead. */ - if (driver-suspend) - pr_warn(i2c-core: driver [%s] using legacy suspend method\n, - driver-driver.name); - if (driver-resume) - pr_warn(i2c-core: driver [%s] using legacy resume method\n
Re: [PATCH] i2c: Add generic support passing secondary devices addresses
On 09/22/2014 12:45 PM, Mika Westerberg wrote: On Fri, Sep 05, 2014 at 04:02:19PM +0200, Jean-Michel Hautbois wrote: Some I2C devices have multiple addresses assigned, for example each address corresponding to a different internal register map page of the device. So far drivers which need support for this have handled this with a driver specific and non-generic implementation, e.g. passing the additional address via platform data. This patch provides a new helper function called i2c_new_secondary_device() which is intended to provide a generic way to get the secondary address as well as instantiate a struct i2c_client for the secondary address. The function expects a pointer to the primary i2c_client, a name for the secondary address and an optional default address. The name is used as a handle to specify which secondary address to get. The default address is used as a fallback in case no secondary address was explicitly specified. In case no secondary address and no default address were specified the function returns NULL. For now the function only supports look-up of the secondary address from devicetree, but it can be extended in the future to for example support board files and/or ACPI. Signed-off-by: Jean-Michel Hautbois jean-michel.hautb...@vodalys.com Sorry, just noticed this one. Srinivas (CC'd) and I did similar patch series here: http://patchwork.ozlabs.org/patch/338342/ We should probably collaborate on this one to get both DT and ACPI supported. Yes. The idea was to keep the interface of the API generic so it can be used by ACPI or other device topology description mechanisms as well. But it looks as if the ACPI case is a bit more complex and we may need a revision of the API. How for example in the ACPI case do you know which address is which, when different parts of a chip are addressed using different addresses? - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: Add generic support passing secondary devices addresses
On 09/22/2014 03:45 PM, Mika Westerberg wrote: On Mon, Sep 22, 2014 at 03:27:36PM +0200, Lars-Peter Clausen wrote: On 09/22/2014 12:45 PM, Mika Westerberg wrote: On Fri, Sep 05, 2014 at 04:02:19PM +0200, Jean-Michel Hautbois wrote: Some I2C devices have multiple addresses assigned, for example each address corresponding to a different internal register map page of the device. So far drivers which need support for this have handled this with a driver specific and non-generic implementation, e.g. passing the additional address via platform data. This patch provides a new helper function called i2c_new_secondary_device() which is intended to provide a generic way to get the secondary address as well as instantiate a struct i2c_client for the secondary address. The function expects a pointer to the primary i2c_client, a name for the secondary address and an optional default address. The name is used as a handle to specify which secondary address to get. The default address is used as a fallback in case no secondary address was explicitly specified. In case no secondary address and no default address were specified the function returns NULL. For now the function only supports look-up of the secondary address from devicetree, but it can be extended in the future to for example support board files and/or ACPI. Signed-off-by: Jean-Michel Hautbois jean-michel.hautb...@vodalys.com Sorry, just noticed this one. Srinivas (CC'd) and I did similar patch series here: http://patchwork.ozlabs.org/patch/338342/ Sorry I gave wrong link. That one is older version. Here is the current: http://patchwork.ozlabs.org/patch/386409/ http://patchwork.ozlabs.org/patch/386410/ We should probably collaborate on this one to get both DT and ACPI supported. Yes. The idea was to keep the interface of the API generic so it can be used by ACPI or other device topology description mechanisms as well. But it looks as if the ACPI case is a bit more complex and we may need a revision of the API. How for example in the ACPI case do you know which address is which, when different parts of a chip are addressed using different addresses? Unfortunately there is no way in ACPI 5.0 to find out which is which. So we trust the ordering of I2cSerialBus() resources. Even that has been problematic because some vendors then list things like SMBus ARA addresses there in random order :-( Our API has following signature: int i2c_address_by_index(struct i2c_client *client, int index, struct i2c_board_info *info, struct i2c_adapter **adapter) and we use index to find out which address to use. Note also that in ACPI it is possible that the I2cSerialBus() resource points to another I2C host controller, so we need to have 'adapter' parameter as well. Ok, looks like there are two main differences in the two implementations. 1) The ACPI one uses a integer index and the DT one uses a string index to lookup the device. The problem with the index lookup is that the order is binding specific. So it might be different between e.g. the devicetree binding and the ACPI binding. This makes it quite hard to use the API in a generic way and you'd end up with hacks like: if (client-dev.of_node) index = 3; else if (ACPI_COMPANION(client-dev)) index = 1; else index = 5; So we might need a extra translation table which maps a name to a ACPI index and then we could use the name as the generic index in the driver. 2) The ACPI implementation returns the i2c_board_info and the adapter, while the DT implementation returns the instantiated I2C client device. It might make sense to have both. I image that most drivers are just interested in creating a new client device and will simply pass the board info and adapter they got to i2c_new_device(). In this case it makes sense to have a helper function which already does this internally to avoid boilerplate code duplication. There will probably some special cases though in which case the driver wants to get the adapter and the board info and then manually call i2c_new_device() after having done some additional steps. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: Add generic support passing secondary devices addresses
On 09/21/2014 07:49 PM, Wolfram Sang wrote: This raises the first question for me: Are the additional addresses configurable? Sadly, I can't find good documentation for the adv7604. Otherwise, if I know I have a adv7604 and know its addresses, this information should go into the driver and not the DT. They are. The current driver hard codes the other addresses, but that's not working when you have multiple adv7604s on the same I2C bus. How is this configured? I can't imagine every address has its own setting, but rather some 1-3 pins which select a certain block of addresses? Every secondary address has its own register and can be set to any valid I2C address. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: Add generic support passing secondary devices addresses
On 09/20/2014 06:49 PM, Wolfram Sang wrote: On Fri, Sep 05, 2014 at 04:02:19PM +0200, Jean-Michel Hautbois wrote: Some I2C devices have multiple addresses assigned, for example each address corresponding to a different internal register map page of the device. So far drivers which need support for this have handled this with a driver specific and non-generic implementation, e.g. passing the additional address via platform data. This raises the first question for me: Are the additional addresses configurable? Sadly, I can't find good documentation for the adv7604. Otherwise, if I know I have a adv7604 and know its addresses, this information should go into the driver and not the DT. They are. The current driver hard codes the other addresses, but that's not working when you have multiple adv7604s on the same I2C bus. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] Allow DT parsing of secondary devices
On 08/29/2014 05:15 PM, Jean-Michel Hautbois wrote: This is based on reg and reg-names in DT. Example: reg = 0x10 0x20 0x30; reg-names = main, io, test; This function will create dummy devices io and test with addresses 0x20 and 0x30 respectively. Signed-off-by: Jean-Michel Hautbois jean-michel.hautb...@vodalys.com This needs a better description explaining the problem and how it is solved. How about i2c: Add generic support passing secondary devices addresses Some I2C devices have multiple addresses assigned, for example each address corresponding to a different internal register map page of the device. So far drivers which need support for this have handled this with a driver specific and non-generic implementation, e.g. passing the additional address via platform data. This patch provides a new helper function called i2c_new_secondary_device() which is intended to provide a generic way to get the secondary address as well as instantiate a struct i2c_client for the secondary address. The function expects a pointer to the primary i2c_client, a name for the secondary address and an optional default address. The name is used as a handle to specify which secondary address to get. The default address is used as a fallback in case no secondary address was explicitly specified. In case no secondary address and no default address were specified the function returns NULL. For now the function only supports look-up of the secondary address from devicetree, but it can be extended in the future to for example support board files and/or ACPI. --- drivers/i2c/i2c-core.c | 20 include/linux/i2c.h| 6 ++ 2 files changed, 26 insertions(+) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 632057a..5eb414d 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -798,6 +798,26 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) } EXPORT_SYMBOL_GPL(i2c_new_dummy); The function needs a kernel doc description. +struct i2c_client *i2c_new_secondary_device(struct i2c_client *client, + const char *name, + u32 default_addr) The I2C framework commonly uses u16 for the type of a I2C address. +{ + int i, addr; addr needs to be u32 here since it is passed to of_property_read_u32_index(). + struct device_node *np; + + np = client-dev.of_node; of_node can be NULL, this needs to be handled. Ideally by using the default address. + i = of_property_match_string(np, reg-names, name); + if (i = 0) + of_property_read_u32_index(np, reg, i, addr); of_property_read_u32_index() can fail, this needs to be handled. + else + addr = default_addr; If no address was specified and default_addr is 0 the function should return NULL. + + dev_dbg(client-adapter-dev, Address for %s : 0x%x\n, name, addr); + return i2c_new_dummy(client-adapter, addr); +} +EXPORT_SYMBOL_GPL(i2c_new_secondary_device); + + /* - */ /* I2C bus adapters -- one roots each I2C or SMBUS segment */ diff --git a/include/linux/i2c.h b/include/linux/i2c.h index a95efeb..2d143d7 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -322,6 +322,12 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr); extern struct i2c_client * i2c_new_dummy(struct i2c_adapter *adap, u16 address); +/* Use reg/reg-names in DT in order to get extra addresses */ The comment should go into i2c-core.c and be in proper kernel doc fully explaining the function, its parameters and the behavior. +extern struct i2c_client * +i2c_new_secondary_device(struct i2c_client *client, + const char *name, + u32 default_addr); + extern void i2c_unregister_device(struct i2c_client *); #endif /* I2C */ -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] adv7604: Use DT parsing in dummy creation
On 08/29/2014 05:15 PM, Jean-Michel Hautbois wrote: This patch uses DT in order to parse addresses for dummy devices of adv7604. The ADV7604 has thirteen 256-byte maps that can be accessed via the main I²C ports. Each map has it own I²C address and acts as a standard slave device on the I²C bus. If nothing is defined, it uses default addresses. The main prupose is using two adv76xx on the same i2c bus. Ideally this patch is split up in two patches. One patch adding support for i2c_new_secondary_device() and one patch adding support for DT for the adv7604. [...] +static const char const *adv7604_secondary_names[] = { + main, /* ADV7604_PAGE_IO */ How about [ADV7604_PAGE_IO] = main, instead of having the comment, this makes things more explicit. + avlink, /* ADV7604_PAGE_AVLINK */ + cec, /* ADV7604_PAGE_CEC */ + infoframe, /* ADV7604_PAGE_INFOFRAME */ + esdp, /* ADV7604_PAGE_ESDP */ + dpp, /* ADV7604_PAGE_DPP */ + afe, /* ADV7604_PAGE_AFE */ + rep, /* ADV7604_PAGE_REP */ + edid, /* ADV7604_PAGE_EDID */ + hdmi, /* ADV7604_PAGE_HDMI */ + test, /* ADV7604_PAGE_TEST */ + cp, /* ADV7604_PAGE_CP */ + vdp /* ADV7604_PAGE_VDP */ +}; + /* --- */ static inline struct adv7604_state *to_state(struct v4l2_subdev *sd) @@ -2528,13 +2544,31 @@ static void adv7604_unregister_clients(struct adv7604_state *state) } static struct i2c_client *adv7604_dummy_client(struct v4l2_subdev *sd, - u8 addr, u8 io_reg) + unsigned int i) { struct i2c_client *client = v4l2_get_subdevdata(sd); + struct adv7604_platform_data *pdata = client-dev.platform_data; + unsigned int io_reg = 0xf2 + i; + unsigned int default_addr = io_read(sd, io_reg) 1; + struct i2c_client *new_client; + + if (IS_ENABLED(CONFIG_OF)) { No CONFIG_OF. i2c_new_secondary_device() is supposed to be the generic method of instantiating the secondary i2c_client, regardless of how the address is specified. For this driver we still need to keep the old method of instantiation via platform data for legacy reasons for now. So what this should look like is: if (pdata pdata-i2c_addresses[i]) new_client = i2c_new_dummy(client-adapter, pdata-i2c_addresses[i]); else new_client = i2c_new_secondary_device(client, adv7604_secondary_names[i], default_addr); + /* Try to find it in DT */ + new_client = i2c_new_secondary_device(client, + adv7604_secondary_names[i], default_addr); + } else if (pdata) { + if (pdata-i2c_addresses[i]) + new_client = i2c_new_dummy(client-adapter, + pdata-i2c_addresses[i]); + else + new_client = i2c_new_dummy(client-adapter, + default_addr); + } [...] -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] Allow DT parsing of secondary devices
On 08/29/2014 05:15 PM, Jean-Michel Hautbois wrote: This is based on reg and reg-names in DT. Example: reg = 0x10 0x20 0x30; reg-names = main, io, test; This function will create dummy devices io and test with addresses 0x20 and 0x30 respectively. Signed-off-by: Jean-Michel Hautbois jean-michel.hautb...@vodalys.com This needs a better description explaining the problem and how it is solved. How about i2c: Add generic support passing secondary devices addresses Some I2C devices have multiple addresses assigned, for example each address corresponding to a different internal register map page of the device. So far drivers which need support for this have handled this with a driver specific and non-generic implementation, e.g. passing the additional address via platform data. This patch provides a new helper function called i2c_new_secondary_device() which is intended to provide a generic way to get the secondary address as well as instantiate a struct i2c_client for the secondary address. The function expects a pointer to the primary i2c_client, a name for the secondary address and an optional default address. The name is used as a handle to specify which secondary address to get. The default address is used as a fallback in case no secondary address was explicitly specified. In case no secondary address and no default address were specified the function returns NULL. For now the function only supports look-up of the secondary address from devicetree, but it can be extended in the future to for example support board files and/or ACPI. The patch should also update the I2C devicetree bindings documentation to explain how bindings for devices with multiple addresses work. --- drivers/i2c/i2c-core.c | 20 include/linux/i2c.h| 6 ++ 2 files changed, 26 insertions(+) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 632057a..5eb414d 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -798,6 +798,26 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) } EXPORT_SYMBOL_GPL(i2c_new_dummy); The function needs a kernel doc description. +struct i2c_client *i2c_new_secondary_device(struct i2c_client *client, + const char *name, + u32 default_addr) The I2C framework commonly uses u16 for the type of a I2C address. +{ + int i, addr; addr needs to be u32 here since it is passed to of_property_read_u32_index(). + struct device_node *np; + + np = client-dev.of_node; of_node can be NULL, this needs to be handled. Ideally by using the default address. + i = of_property_match_string(np, reg-names, name); + if (i = 0) + of_property_read_u32_index(np, reg, i, addr); of_property_read_u32_index() can fail, this needs to be handled. + else + addr = default_addr; If no address was specified and default_addr is 0 the function should return NULL. + + dev_dbg(client-adapter-dev, Address for %s : 0x%x\n, name, addr); + return i2c_new_dummy(client-adapter, addr); +} +EXPORT_SYMBOL_GPL(i2c_new_secondary_device); + + /* - */ /* I2C bus adapters -- one roots each I2C or SMBUS segment */ diff --git a/include/linux/i2c.h b/include/linux/i2c.h index a95efeb..2d143d7 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -322,6 +322,12 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr); extern struct i2c_client * i2c_new_dummy(struct i2c_adapter *adap, u16 address); +/* Use reg/reg-names in DT in order to get extra addresses */ The comment should go into i2c-core.c and be in proper kernel doc fully explaining the function, its parameters and the behavior. +extern struct i2c_client * +i2c_new_secondary_device(struct i2c_client *client, + const char *name, + u32 default_addr); + extern void i2c_unregister_device(struct i2c_client *); #endif /* I2C */ -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c-mux-gpio: test if the gpio can sleep
On 10/10/2013 10:39 AM, Ionut Nicu wrote: Some gpio chips may have get/set operations that can sleep. For this type of chips we must use the _cansleep() version of gpio_set_value. Signed-off-by: Ionut Nicu ioan.nicu@nsn.com --- drivers/i2c/muxes/i2c-mux-gpio.c | 11 --- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c index a764da7..b5f17ef 100644 --- a/drivers/i2c/muxes/i2c-mux-gpio.c +++ b/drivers/i2c/muxes/i2c-mux-gpio.c @@ -27,11 +27,16 @@ struct gpiomux { static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val) { + unsigned gpio; int i; - for (i = 0; i mux-data.n_gpios; i++) - gpio_set_value(mux-gpio_base + mux-data.gpios[i], -val (1 i)); + for (i = 0; i mux-data.n_gpios; i++) { + gpio = mux-gpio_base + mux-data.gpios[i]; + if (gpio_cansleep(gpio)) + gpio_set_value_cansleep(gpio, val (1 i)); + else + gpio_set_value(gpio, val (1 i)); The proper way to do this is just always use the _cansleep() version. gpio_set_value() only works for chips which do not sleep, gpio_set_value_cansleep() works for both those who do sleep and those who do not. + } } static int i2c_mux_gpio_select(struct i2c_adapter *adap, void *data, u32 chan) -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c-mux-gpio: test if the gpio can sleep
On 10/10/2013 09:43 PM, Wolfram Sang wrote: On Thu, Oct 10, 2013 at 10:46:41AM +0200, Lars-Peter Clausen wrote: + if (gpio_cansleep(gpio)) + gpio_set_value_cansleep(gpio, val (1 i)); + else + gpio_set_value(gpio, val (1 i)); The proper way to do this is just always use the _cansleep() version. gpio_set_value() only works for chips which do not sleep, gpio_set_value_cansleep() works for both those who do sleep and those who do not. To the gpio-list: Has it been considered to have sth. like gpio_set_value and gpio_set_value_nosleep? I'd think it makes more sense to have the specific function have the specific name. It has been a few times, but I think the conclusion has always been that it is now too late to invert the semantics of gpio_set_value(). If you want to look up the discussions the keyword is gpio_set_value_atomic(). - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation
On 10/04/2013 01:55 PM, Wolfram Sang wrote: On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote: On 10/04/2013 07:46 AM, Wolfram Sang wrote: + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); + cr |= XIIC_CR_DIR_IS_TX_MASK; + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); + Is there no need to clear the bit again when receiving? This bit is cleared in xiic_xfer() - xiic_start_xfer() -xiic_reinit() xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK); A bit implicit, but OK. And did transferring ever work if this bit was never set before? I really don't know. We have switched from old driver to this new mainline one and based on our eeprom testing we have found that this bit hasn't been setup properly. It is described here. http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf page 28 - step 3. IIC Master Transmitter with a Repeated Start 1. Write the IIC device address to the TX_FIFO. 2. Write data to TX_FIFO. 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. 4. Continue writing data to TX_FIFO. 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. 6. Write to CR to set RSTA = 1. Repeated start is not happening in the driver as well, or am I overlooking something? 7. Write IIC device address to TX_FIFO. 8. Write all data except last byte to TX_FIFO. 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte. 11. Write last byte of data to TX_FIFO. CCing more people who worked on the driver in the past and might have experiences The current version works fine here. The driver uses whats described in the datasheet as dynamic controller logic flow and not the standard controller logic flow. The sequence Michal mentioned above is from the standard controller logic flow section. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] [media] s5c73m3: Don't use i2c_client-driver
The 'driver' field of the i2c_client struct is redundant and is going to be removed. The results of the expressions 'client-driver.driver-field' and 'client-dev.driver-field' are identical, so replace all occurrences of the former with the later. Signed-off-by: Lars-Peter Clausen l...@metafoo.de Cc: Kyungmin Park kyungmin.p...@samsung.com Cc: Andrzej Hajda a.ha...@samsung.com --- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c index b76ec0e..1083890 100644 --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c @@ -1581,7 +1581,7 @@ static int s5c73m3_probe(struct i2c_client *client, oif_sd = state-oif_sd; v4l2_subdev_init(sd, s5c73m3_subdev_ops); - sd-owner = client-driver-driver.owner; + sd-owner = client-dev.driver-owner; v4l2_set_subdevdata(sd, state); strlcpy(sd-name, S5C73M3, sizeof(sd-name)); -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 8/8] i2c: Remove redundant 'driver' field from the i2c_client struct
The 'driver' field of the i2c_client struct is redundant. The same data can be accessed through to_i2c_driver(client-dev.driver). The generated code for both approaches in more or less the same. E.g. on ARM the expression client-driver-command(...) generates ... ldr r3, [r0, #28] ldr r3, [r3, #32] blx r3 ... and the expression to_i2c_driver(client-dev.driver)-command(...) generates ... ldr r3, [r0, #160] ldr r3, [r3, #-4] blx r3 ... Other architectures will generate similar code. All users of the 'driver' field outside of the I2C core have already been converted. So this only leaves the core itself. This patch converts the remaining few users in the I2C core and then removes the 'driver' field from the i2c_client struct. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/i2c/i2c-core.c | 21 - drivers/i2c/i2c-smbus.c | 10 ++ include/linux/i2c.h | 2 -- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 29d3f04..430c001 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -248,17 +248,16 @@ static int i2c_device_probe(struct device *dev) driver = to_i2c_driver(dev-driver); if (!driver-probe || !driver-id_table) return -ENODEV; - client-driver = driver; + if (!device_can_wakeup(client-dev)) device_init_wakeup(client-dev, client-flags I2C_CLIENT_WAKE); dev_dbg(dev, probe\n); status = driver-probe(client, i2c_match_id(driver-id_table, client)); - if (status) { - client-driver = NULL; + if (status) i2c_set_clientdata(client, NULL); - } + return status; } @@ -279,10 +278,9 @@ static int i2c_device_remove(struct device *dev) dev-driver = NULL; status = 0; } - if (status == 0) { - client-driver = NULL; + if (status == 0) i2c_set_clientdata(client, NULL); - } + return status; } @@ -1606,9 +1604,14 @@ static int i2c_cmd(struct device *dev, void *_arg) { struct i2c_client *client = i2c_verify_client(dev); struct i2c_cmd_arg *arg = _arg; + struct i2c_driver *driver; + + if (!client || !client-dev.driver) + return 0; - if (client client-driver client-driver-command) - client-driver-command(client, arg-cmd, arg-arg); + driver = to_i2c_driver(client-dev.driver); + if (driver-command) + driver-command(client, arg-cmd, arg-arg); return 0; } diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c index 44d4c60..c99b229 100644 --- a/drivers/i2c/i2c-smbus.c +++ b/drivers/i2c/i2c-smbus.c @@ -46,6 +46,7 @@ static int smbus_do_alert(struct device *dev, void *addrp) { struct i2c_client *client = i2c_verify_client(dev); struct alert_data *data = addrp; + struct i2c_driver *driver; if (!client || client-addr != data-addr) return 0; @@ -54,12 +55,13 @@ static int smbus_do_alert(struct device *dev, void *addrp) /* * Drivers should either disable alerts, or provide at least -* a minimal handler. Lock so client-driver won't change. +* a minimal handler. Lock so the driver won't change. */ device_lock(dev); - if (client-driver) { - if (client-driver-alert) - client-driver-alert(client, data-flag); + if (client-dev.driver) { + driver = to_i2c_driver(client-dev.driver); + if (driver-alert) + driver-alert(client, data-flag); else dev_warn(client-dev, no driver alert()!\n); } else diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 2ab11dc..eff50e0 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -205,7 +205,6 @@ struct i2c_driver { * @name: Indicates the type of the device, usually a chip name that's * generic enough to hide second-sourcing and compatible revisions. * @adapter: manages the bus segment hosting this I2C device - * @driver: device's driver, hence pointer to access routines * @dev: Driver model device node for the slave. * @irq: indicates the IRQ generated by this device (if any) * @detected: member of an i2c_driver.clients list or i2c-core's @@ -222,7 +221,6 @@ struct i2c_client { /* _LOWER_ 7 bits */ char name[I2C_NAME_SIZE]; struct i2c_adapter *adapter;/* the adapter we sit on*/ - struct i2c_driver *driver; /* and our access routines
[PATCH 6/8] ALSA: ppc: keywest: Don't use i2c_client-driver
The 'driver' field of the i2c_client struct is redundant and is going to be removed. Use 'to_i2c_driver(client-dev.driver)' instead to get direct access to the i2c_driver struct. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- sound/ppc/keywest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c index 01aecc2..0d1c27e 100644 --- a/sound/ppc/keywest.c +++ b/sound/ppc/keywest.c @@ -65,7 +65,7 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter) * already bound. If not it means binding failed, and then there * is no point in keeping the device instantiated. */ - if (!keywest_ctx-client-driver) { + if (!keywest_ctx-client-dev.driver) { i2c_unregister_device(keywest_ctx-client); keywest_ctx-client = NULL; return -ENODEV; @@ -76,7 +76,7 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter) * 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); + to_i2c_driver(keywest_ctx-client-dev.driver)-clients); return 0; } -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/8] drm: encoder_slave: Don't use i2c_client-driver
The 'driver' field of the i2c_client struct is redundant and is going to be removed. The results of the expressions 'client-driver.driver-field' and 'client-dev.driver-field' are identical, so replace all occurrences of the former with the later. To get direct access to the i2c_driver struct use 'to_i2c_driver(client-dev.driver)'. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/gpu/drm/drm_encoder_slave.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_encoder_slave.c b/drivers/gpu/drm/drm_encoder_slave.c index 0cfb60f..d18b88b 100644 --- a/drivers/gpu/drm/drm_encoder_slave.c +++ b/drivers/gpu/drm/drm_encoder_slave.c @@ -67,12 +67,12 @@ int drm_i2c_encoder_init(struct drm_device *dev, goto fail; } - if (!client-driver) { + if (!client-dev.driver) { err = -ENODEV; goto fail_unregister; } - module = client-driver-driver.owner; + module = client-dev.driver-owner; if (!try_module_get(module)) { err = -ENODEV; goto fail_unregister; @@ -80,7 +80,7 @@ int drm_i2c_encoder_init(struct drm_device *dev, encoder-bus_priv = client; - encoder_drv = to_drm_i2c_encoder_driver(client-driver); + encoder_drv = to_drm_i2c_encoder_driver(to_i2c_driver(client-dev.driver)); err = encoder_drv-encoder_init(client, dev, encoder); if (err) @@ -111,7 +111,7 @@ void drm_i2c_encoder_destroy(struct drm_encoder *drm_encoder) { struct drm_encoder_slave *encoder = to_encoder_slave(drm_encoder); struct i2c_client *client = drm_i2c_encoder_get_client(drm_encoder); - struct module *module = client-driver-driver.owner; + struct module *module = client-dev.driver-owner; i2c_unregister_device(client); encoder-bus_priv = NULL; -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/8] i2c: Remove redundant driver field from the i2c_client struct
Hi, This series removes the redundant driver field from the i2c_client struct. The field is redundant since the same pointer can be accessed through to_i2c_driver(client-dev.driver). The commit log suggests that the field has been around since forever (since before v2.6.12-rc2) and it looks as if it was simply forgotten to remove it during the conversion of the i2c framework to the generic device driver model. Nevertheless there are a still a few users of the field around. This series first updates all users to use an alternative method of accessing the same data and then the last patch removes the driver field from the i2c_client struct. Note that due to this changes on most architectures neither the code size nor the type of generated instructions will change. This is due to the fact that we aren't really interested in the pointer value itself, but rather want to dereference it to access one of the fields of the struct. offset_of() (and hence to_i2c_driver) works by subtracting a offset from the pointer, so the compiler can internally create the sum of these two offsets and use that to access the field. E.g. on ARM the expression client-driver-command(...) generates ... ldr r3, [r0, #28] ldr r3, [r3, #32] blx r3 ... and the expression to_i2c_driver(client-dev.driver)-command(...) generates ... ldr r3, [r0, #160] ldr r3, [r3, #-4] blx r3 ... Other architectures will generate similar code. The most common pattern is to use the i2c_driver to get to the device_driver struct embedded in it. The same struct can easily be accessed through the device struct embedded in the i2c_client struct. E.g. client-driver-driver.field can be replaced by client-dev.driver-field. Here again the generated code is almost identical and only the offsets differ. E.g. on ARM the expression 'client-driver-driver.owner' generates ldr r3, [r0, #28] ldr r0, [r3, #44] and 'client-dev.driver-owner' generates ldr r3, [r0, #160] ldr r0, [r3, #8] The kernel overall code size is slightly reduced since the code that manages the driver field is removed and of course also the runtime memory footprint of the i2c_client struct is reduced. - Lars Lars-Peter Clausen (8): [media] s5c73m3: Don't use i2c_client-driver [media] exynos4-is: Don't use i2c_client-driver [media] core: Don't use i2c_client-driver drm: encoder_slave: Don't use i2c_client-driver drm: nouveau: Don't use i2c_client-driver ALSA: ppc: keywest: Don't use i2c_client-driver ASoC: imx-wm8962: Don't use i2c_client-driver i2c: Remove redundant 'driver' field from the i2c_client struct drivers/gpu/drm/drm_encoder_slave.c| 8 drivers/gpu/drm/nouveau/core/subdev/therm/ic.c | 3 ++- drivers/i2c/i2c-core.c | 21 - drivers/i2c/i2c-smbus.c| 10 ++ drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +- drivers/media/platform/exynos4-is/media-dev.c | 6 +++--- drivers/media/v4l2-core/tuner-core.c | 6 +++--- drivers/media/v4l2-core/v4l2-common.c | 10 +- include/linux/i2c.h| 2 -- include/media/v4l2-common.h| 2 +- sound/ppc/keywest.c| 4 ++-- sound/soc/fsl/imx-wm8962.c | 2 +- 12 files changed, 40 insertions(+), 36 deletions(-) -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/8] drm: nouveau: Don't use i2c_client-driver
The 'driver' field of the i2c_client struct is redundant and is going to be removed. Use 'to_i2c_driver(client-dev.driver)' instead to get direct access to the i2c_driver struct. Signed-off-by: Lars-Peter Clausen l...@metafoo.de Cc: Martin Peres martin.pe...@labri.fr --- drivers/gpu/drm/nouveau/core/subdev/therm/ic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c b/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c index 8b3adec..eae939d 100644 --- a/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c +++ b/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c @@ -41,7 +41,8 @@ probe_monitoring_device(struct nouveau_i2c_port *i2c, if (!client) return false; - if (!client-driver || client-driver-detect(client, info)) { + if (!client-dev.driver || + to_i2c_driver(client-dev.driver)-detect(client, info)) { i2c_unregister_device(client); return false; } -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/8] ASoC: imx-wm8962: Don't use i2c_client-driver
The 'driver' field of the i2c_client struct is redundant and is going to be removed. Check i2c_client-dev.driver instead to see if a driver is bound to the device. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- sound/soc/fsl/imx-wm8962.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c index 6c60666..f84ecbf 100644 --- a/sound/soc/fsl/imx-wm8962.c +++ b/sound/soc/fsl/imx-wm8962.c @@ -215,7 +215,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) goto fail; } codec_dev = of_find_i2c_device_by_node(codec_np); - if (!codec_dev || !codec_dev-driver) { + if (!codec_dev || !codec_dev-dev.driver) { dev_err(pdev-dev, failed to find codec platform device\n); ret = -EINVAL; goto fail; -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/8] [media] core: Don't use i2c_client-driver
The 'driver' field of the i2c_client struct is redundant and is going to be removed. The results of the expressions 'client-driver.driver-field' and 'client-dev.driver-field' are identical, so replace all occurrences of the former with the later. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/media/v4l2-core/tuner-core.c | 6 +++--- drivers/media/v4l2-core/v4l2-common.c | 10 +- include/media/v4l2-common.h | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c index ddc9379..4133af0 100644 --- a/drivers/media/v4l2-core/tuner-core.c +++ b/drivers/media/v4l2-core/tuner-core.c @@ -43,7 +43,7 @@ #define UNSET (-1U) -#define PREFIX (t-i2c-driver-driver.name) +#define PREFIX (t-i2c-dev.driver-name) /* * Driver modprobe parameters @@ -452,7 +452,7 @@ static void set_type(struct i2c_client *c, unsigned int type, } tuner_dbg(%s %s I2C addr 0x%02x with type %d used for 0x%02x\n, - c-adapter-name, c-driver-driver.name, c-addr 1, type, + c-adapter-name, c-dev.driver-name, c-addr 1, type, t-mode_mask); return; @@ -556,7 +556,7 @@ static void tuner_lookup(struct i2c_adapter *adap, int mode_mask; if (pos-i2c-adapter != adap || - strcmp(pos-i2c-driver-driver.name, tuner)) + strcmp(pos-i2c-dev.driver-name, tuner)) continue; mode_mask = pos-mode_mask; diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 037d7a5..433d6d7 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -236,14 +236,14 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client, v4l2_subdev_init(sd, ops); sd-flags |= V4L2_SUBDEV_FL_IS_I2C; /* the owner is the same as the i2c_client's driver owner */ - sd-owner = client-driver-driver.owner; + sd-owner = client-dev.driver-owner; sd-dev = client-dev; /* i2c_client and v4l2_subdev point to one another */ v4l2_set_subdevdata(sd, client); i2c_set_clientdata(client, sd); /* initialize name */ snprintf(sd-name, sizeof(sd-name), %s %d-%04x, - client-driver-driver.name, i2c_adapter_id(client-adapter), + client-dev.driver-name, i2c_adapter_id(client-adapter), client-addr); } EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init); @@ -274,11 +274,11 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev, loaded. This delay-load mechanism doesn't work if other drivers want to use the i2c device, so explicitly loading the module is the best alternative. */ - if (client == NULL || client-driver == NULL) + if (client == NULL || client-dev.driver == NULL) goto error; /* Lock the module so we can safely get the v4l2_subdev pointer */ - if (!try_module_get(client-driver-driver.owner)) + if (!try_module_get(client-dev.driver-owner)) goto error; sd = i2c_get_clientdata(client); @@ -287,7 +287,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev, if (v4l2_device_register_subdev(v4l2_dev, sd)) sd = NULL; /* Decrease the module use count to match the first try_module_get. */ - module_put(client-driver-driver.owner); + module_put(client-dev.driver-owner); error: /* If we have a client but no subdev, then something went wrong and diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index 16550c4..a707529 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -35,7 +35,7 @@ printk(level %s %d-%04x: fmt, name, i2c_adapter_id(adapter), addr , ## arg) #define v4l_client_printk(level, client, fmt, arg...) \ - v4l_printk(level, (client)-driver-driver.name, (client)-adapter, \ + v4l_printk(level, (client)-dev.driver-name, (client)-adapter, \ (client)-addr, fmt , ## arg) #define v4l_err(client, fmt, arg...) \ -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/8] [media] exynos4-is: Don't use i2c_client-driver
The 'driver' field of the i2c_client struct is redundant and is going to be removed. The results of the expressions 'client-driver.driver-field' and 'client-dev.driver-field' are identical, so replace all occurrences of the former with the later. Signed-off-by: Lars-Peter Clausen l...@metafoo.de Cc: Kyungmin Park kyungmin.p...@samsung.com Cc: Sylwester Nawrocki s.nawro...@samsung.com --- drivers/media/platform/exynos4-is/media-dev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index a835112..7a4ee4c 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -411,8 +411,8 @@ static int fimc_md_of_add_sensor(struct fimc_md *fmd, device_lock(client-dev); - if (!client-driver || - !try_module_get(client-driver-driver.owner)) { + if (!client-dev.driver || + !try_module_get(client-dev.driver-owner)) { ret = -EPROBE_DEFER; v4l2_info(fmd-v4l2_dev, No driver found for %s\n, node-full_name); @@ -442,7 +442,7 @@ static int fimc_md_of_add_sensor(struct fimc_md *fmd, fmd-num_sensors++; mod_put: - module_put(client-driver-driver.owner); + module_put(client-dev.driver-owner); dev_put: device_unlock(client-dev); put_device(client-dev); -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] i2c: Ignore return value of i2c_del_adapter()
Hi Jean, On 03/30/2013 08:55 AM, Jean Delvare wrote: Hi Lars-Peter, On Sat, 9 Mar 2013 19:16:46 +0100, Lars-Peter Clausen wrote: i2c_del_adapter() always returns 0. I beg you pardon? i2c_del_adapter() starts with: int i2c_del_adapter(struct i2c_adapter *adap) { int res = 0; struct i2c_adapter *found; struct i2c_client *client, *next; /* First make sure that this adapter was ever added */ mutex_lock(core_lock); found = idr_find(i2c_adapter_idr, adap-nr); mutex_unlock(core_lock); if (found != adap) { pr_debug(i2c-core: attempting to delete unregistered adapter [%s]\n, adap-name); return -EINVAL; } /* Tell drivers about this removal */ mutex_lock(core_lock); res = bus_for_each_drv(i2c_bus_type, NULL, adap, __process_removed_adapter); mutex_unlock(core_lock); if (res) return res; (...) So, no, it doesn't always return 0. Patch 1 and 2 in this series remove those two instances: https://lkml.org/lkml/2013/3/9/72 https://lkml.org/lkml/2013/3/9/86 For an explanation why this should be done please take a look at the cover letter to this patch series https://lkml.org/lkml/2013/3/9/73 - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Make return type of i2c_del_adapter() (and i2c_del_mux_adapter()) void
On 03/30/2013 09:13 PM, Jean Delvare wrote: Hi Lars, On Sat, 9 Mar 2013 19:16:43 +0100, Lars-Peter Clausen wrote: Currently i2c_del_adapter() returns 0 on success and potentially an error code on failure. Unfortunately this doesn't mix too well with the Linux device driver model. (...) I see: struct device_driver { (...) int (*probe) (struct device *dev); int (*remove) (struct device *dev); So the driver core does allow remove functions to return an error. Well, the return type is int, but the return value is never checked. So you can return an error value, but the device driver core is going to care and the device is still removed. So any code which does return an error in it's probe function in the assumption that this means the device will still be present is broken and leaves the system in an undefined state. So if that happens the kernel will probably crash sooner or later, or if you are lucky you only created a few resources leaks. And no we can't change the core to handle errors from a drivers remove callback. It's a basic inherent property of the Linux device driver model that any device can be removed at any time. Are you going to fix all subsystems as you are doing for i2c now, and then change device_driver.remove to return void? If not, I don't see the point of changing it in i2c. As I said it's a bug if a driver returns an error in its remove function. And the fact that the return type of the remove callback is int is pretty much misleading in this regard, so the long term goal is to make the return type void. But that's a long way to go until we get there, fixing the return type of i2c_del_adapter() is kind of the low hanging fruit. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] i2c: Remove detach_adapter
The detach_adapter callback has been deprecated for quite some time and has no user left. Keeping it alive blocks other cleanups, so remove it. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/i2c/i2c-core.c | 33 ++--- include/linux/i2c.h| 7 ++- 2 files changed, 12 insertions(+), 28 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index f7dfe87..a853cb3 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -47,7 +47,7 @@ /* core_lock protects i2c_adapter_idr, and guarantees that device detection, deletion of detected devices, and attach_adapter - and detach_adapter calls are serialized */ + calls are serialized */ static DEFINE_MUTEX(core_lock); static DEFINE_IDR(i2c_adapter_idr); @@ -1013,11 +1013,10 @@ int i2c_add_numbered_adapter(struct i2c_adapter *adap) } EXPORT_SYMBOL_GPL(i2c_add_numbered_adapter); -static int i2c_do_del_adapter(struct i2c_driver *driver, +static void i2c_do_del_adapter(struct i2c_driver *driver, struct i2c_adapter *adapter) { struct i2c_client *client, *_n; - int res; /* Remove the devices we created ourselves as the result of hardware * probing (using a driver's detect method) */ @@ -1029,16 +1028,6 @@ static int i2c_do_del_adapter(struct i2c_driver *driver, i2c_unregister_device(client); } } - - if (!driver-detach_adapter) - return 0; - dev_warn(adapter-dev, %s: detach_adapter method is deprecated\n, -driver-driver.name); - res = driver-detach_adapter(adapter); - if (res) - dev_err(adapter-dev, detach_adapter failed (%d) - for driver [%s]\n, res, driver-driver.name); - return res; } static int __unregister_client(struct device *dev, void *dummy) @@ -1059,7 +1048,8 @@ static int __unregister_dummy(struct device *dev, void *dummy) static int __process_removed_adapter(struct device_driver *d, void *data) { - return i2c_do_del_adapter(to_i2c_driver(d), data); + i2c_do_del_adapter(to_i2c_driver(d), data); + return 0; } /** @@ -1072,7 +1062,6 @@ static int __process_removed_adapter(struct device_driver *d, void *data) */ int i2c_del_adapter(struct i2c_adapter *adap) { - int res = 0; struct i2c_adapter *found; struct i2c_client *client, *next; @@ -1088,11 +1077,9 @@ int i2c_del_adapter(struct i2c_adapter *adap) /* Tell drivers about this removal */ mutex_lock(core_lock); - res = bus_for_each_drv(i2c_bus_type, NULL, adap, + bus_for_each_drv(i2c_bus_type, NULL, adap, __process_removed_adapter); mutex_unlock(core_lock); - if (res) - return res; /* Remove devices instantiated from sysfs */ mutex_lock_nested(adap-userspace_clients_lock, @@ -,8 +1098,8 @@ int i2c_del_adapter(struct i2c_adapter *adap) * we can't remove the dummy devices during the first pass: they * could have been instantiated by real devices wishing to clean * them up properly, so we give them a chance to do that first. */ - res = device_for_each_child(adap-dev, NULL, __unregister_client); - res = device_for_each_child(adap-dev, NULL, __unregister_dummy); + device_for_each_child(adap-dev, NULL, __unregister_client); + device_for_each_child(adap-dev, NULL, __unregister_dummy); #ifdef CONFIG_I2C_COMPAT class_compat_remove_link(i2c_adapter_compat_class, adap-dev, @@ -1208,9 +1195,9 @@ EXPORT_SYMBOL(i2c_register_driver); static int __process_removed_driver(struct device *dev, void *data) { - if (dev-type != i2c_adapter_type) - return 0; - return i2c_do_del_adapter(data, to_i2c_adapter(dev)); + if (dev-type == i2c_adapter_type) + i2c_do_del_adapter(data, to_i2c_adapter(dev)); + return 0; } /** diff --git a/include/linux/i2c.h b/include/linux/i2c.h index d0c4db7..8ffab3f 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -125,7 +125,6 @@ extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, * struct i2c_driver - represent an I2C device driver * @class: What kind of i2c device we instantiate (for detect) * @attach_adapter: Callback for bus addition (deprecated) - * @detach_adapter: Callback for bus removal (deprecated) * @probe: Callback for device binding * @remove: Callback for device unbinding * @shutdown: Callback for device shutdown @@ -162,12 +161,10 @@ extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, struct i2c_driver { unsigned int class; - /* Notifies the driver that a new bus has appeared or is about to be -* removed. You should avoid using this, it will be removed in a -* near future. + /* Notifies the driver
[PATCH 0/6] Make return type of i2c_del_adapter() (and i2c_del_mux_adapter()) void
Currently i2c_del_adapter() returns 0 on success and potentially an error code on failure. Unfortunately this doesn't mix too well with the Linux device driver model. An i2c_adapter is usually registered in a drivers probe callback and removed in the drivers remove callback. The Linux device driver model assumes that a device may disappear at any time, e.g. because it is on a hot-plug-able bus and the device is physically removed or because it is unbound from it's driver. This means that a driver's remove callback is not allowed to fail. This has lead to code fragments like the following: rc = i2c_del_adapter(board-i2c_adap); BUG_ON(rc); Currently there are two code paths which can cause to i2c_del_adapter() to return an error code: 1) If the adapter that is passed to i2c_del_adapter() is not registered i2c_del_adapter() returns -EINVAL 2) If an I2C client, which is registered as a child to the adapter, implements the detach_adapter callback and detach_adapter returns an error the removal of the adapter is aborted and i2c_del_adapter() returns the error returned by the clients detach_adapter callback. The first probably never happens unless there is a serious bug in the driver calling i2c_del_adapter(), but I think even then, for the sake of sanitizing the API we can argue that the purpose of i2c_del_adapter() is to remove the adapter from the system. If the adapter currently isn't registered this becomes a no-op and we can return success, without having to do anything. The second also never happens, since the detach_adapter callback has been deprecated for quite some time now and there are currently no drivers left which implement it. So realisticly i2c_del_adapter() already always returns 0 and all code checking for errors is more or less dead code. And in fact the majority of the users of i2c_del_adapter() already ignore the return value, but there are still some drivers (both newer and older) which assume that i2c_del_adapter() might fail. This series aims at making it explicit that i2c_del_adapter() can not fail by making its return type void. The series will start by making i2c_del_adapter() always return success. For this it will update the case, where a non-registered adapter is passed in, to return 0 instead of -EINVAL and remove all code related to the unused detach_adapter callback. Afterward the series will update all users of i2c_del_adapter() to ignore the return value (since it is always 0 now). And finally the series will change the return type of i2c_del_adapter() to void. The same is true for i2c_del_mux_adapter() which is mostly just a wrapper around i2c_del_adapter(), so it is also updated in this series. - Lars Lars-Peter Clausen (6): i2c: Remove detach_adapter i2c: i2c_del_adapter: Don't treat removing a non-registered adapter as error i2c: Ignore return value of i2c_del_adapter() i2c: Make return type of i2c_del_adapter() void i2c: Ignore the return value of i2c_del_mux_adapter() i2c: Make the return type of i2c_del_mux_adapter() void drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c | 3 +-- drivers/i2c/busses/i2c-amd756-s4882.c| 6 + drivers/i2c/busses/i2c-at91.c| 5 ++-- drivers/i2c/busses/i2c-cbus-gpio.c | 4 ++- drivers/i2c/busses/i2c-intel-mid.c | 3 +-- drivers/i2c/busses/i2c-mv64xxx.c | 5 ++-- drivers/i2c/busses/i2c-mxs.c | 5 +--- drivers/i2c/busses/i2c-nforce2-s4985.c | 6 + drivers/i2c/busses/i2c-powermac.c| 10 ++- drivers/i2c/busses/i2c-puv3.c| 10 ++- drivers/i2c/busses/i2c-viperboard.c | 5 ++-- drivers/i2c/i2c-core.c | 39 +--- drivers/i2c/i2c-mux.c| 9 ++- drivers/i2c/muxes/i2c-mux-pca954x.c | 6 ++--- drivers/media/pci/bt8xx/bttv-input.c | 6 ++--- drivers/media/pci/mantis/mantis_i2c.c| 4 ++- drivers/net/ethernet/sfc/falcon.c| 6 ++--- drivers/staging/media/go7007/go7007-driver.c | 7 ++--- include/linux/i2c-mux.h | 2 +- include/linux/i2c.h | 9 +++ 20 files changed, 48 insertions(+), 102 deletions(-) -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] i2c: i2c_del_adapter: Don't treat removing a non-registered adapter as error
Currently i2c_del_adapter() returns -EINVAL when it gets an adapter which is not registered. But none of the users of i2c_del_adapter() depend on this behavior, so for the sake of being able to sanitize the return type of i2c_del_adapter argue, that the purpose of i2c_del_adapter() is to remove an I2C adapter from the system. If the adapter is not registered in the first place this becomes a no-op. So we can return success without having to do anything. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/i2c/i2c-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index a853cb3..7727d33 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -1072,7 +1072,7 @@ int i2c_del_adapter(struct i2c_adapter *adap) if (found != adap) { pr_debug(i2c-core: attempting to delete unregistered adapter [%s]\n, adap-name); - return -EINVAL; + return 0; } /* Tell drivers about this removal */ -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] i2c: Ignore return value of i2c_del_adapter()
i2c_del_adapter() always returns 0. So all checks testing whether it will be non zero will always evaluate to false and the conditional code is dead code. This patch updates all callers of i2c_del_mux_adapter() to ignore the return value and assume that it will always succeed (which it will). In a subsequent patch the return type of i2c_del_adapter() will be made void. Cc: Jean Delvare kh...@linux-fr.org Cc: Guan Xuetao g...@mprc.pku.edu.cn Cc: Mauro Carvalho Chehab mche...@redhat.com Cc: Ben Hutchings bhutchi...@solarflare.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Alan Cox a...@linux.intel.com Cc: Nicolas Ferre nicolas.fe...@atmel.com Cc: Aaro Koskinen aaro.koski...@iki.fi Cc: Marek Vasut ma...@denx.de (commit_signer:11/20=55%) Cc: Shawn Guo shawn@linaro.org Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Lars Poeschel poesc...@lemonage.de Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c | 3 +-- drivers/i2c/busses/i2c-amd756-s4882.c| 6 +- drivers/i2c/busses/i2c-at91.c| 5 ++--- drivers/i2c/busses/i2c-cbus-gpio.c | 4 +++- drivers/i2c/busses/i2c-intel-mid.c | 3 +-- drivers/i2c/busses/i2c-mv64xxx.c | 5 ++--- drivers/i2c/busses/i2c-mxs.c | 5 + drivers/i2c/busses/i2c-nforce2-s4985.c | 6 +- drivers/i2c/busses/i2c-powermac.c| 10 ++ drivers/i2c/busses/i2c-puv3.c| 10 ++ drivers/i2c/busses/i2c-viperboard.c | 5 ++--- drivers/i2c/i2c-mux.c| 5 + drivers/media/pci/bt8xx/bttv-input.c | 6 +++--- drivers/media/pci/mantis/mantis_i2c.c| 4 +++- drivers/net/ethernet/sfc/falcon.c| 6 ++ drivers/staging/media/go7007/go7007-driver.c | 7 ++- 16 files changed, 29 insertions(+), 61 deletions(-) diff --git a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c index 88627e3..1eb86c7 100644 --- a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c +++ b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c @@ -319,8 +319,7 @@ void oaktrail_hdmi_i2c_exit(struct pci_dev *dev) struct hdmi_i2c_dev *i2c_dev; hdmi_dev = pci_get_drvdata(dev); - if (i2c_del_adapter(oaktrail_hdmi_i2c_adapter)) - DRM_DEBUG_DRIVER(Failed to delete hdmi-i2c adapter\n); + i2c_del_adapter(oaktrail_hdmi_i2c_adapter); i2c_dev = hdmi_dev-i2c_dev; kfree(i2c_dev); diff --git a/drivers/i2c/busses/i2c-amd756-s4882.c b/drivers/i2c/busses/i2c-amd756-s4882.c index 378fcb5..07f01ac 100644 --- a/drivers/i2c/busses/i2c-amd756-s4882.c +++ b/drivers/i2c/busses/i2c-amd756-s4882.c @@ -169,11 +169,7 @@ static int __init amd756_s4882_init(void) } /* Unregister physical bus */ - error = i2c_del_adapter(amd756_smbus); - if (error) { - dev_err(amd756_smbus.dev, Physical bus removal failed\n); - goto ERROR0; - } + i2c_del_adapter(amd756_smbus); printk(KERN_INFO Enabling SMBus multiplexing for Tyan S4882\n); /* Define the 5 virtual adapters and algorithms structures */ diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c index 75195e3..6604587 100644 --- a/drivers/i2c/busses/i2c-at91.c +++ b/drivers/i2c/busses/i2c-at91.c @@ -785,12 +785,11 @@ static int at91_twi_probe(struct platform_device *pdev) static int at91_twi_remove(struct platform_device *pdev) { struct at91_twi_dev *dev = platform_get_drvdata(pdev); - int rc; - rc = i2c_del_adapter(dev-adapter); + i2c_del_adapter(dev-adapter); clk_disable_unprepare(dev-clk); - return rc; + return 0; } #ifdef CONFIG_PM diff --git a/drivers/i2c/busses/i2c-cbus-gpio.c b/drivers/i2c/busses/i2c-cbus-gpio.c index 98386d6..1be13ac 100644 --- a/drivers/i2c/busses/i2c-cbus-gpio.c +++ b/drivers/i2c/busses/i2c-cbus-gpio.c @@ -206,7 +206,9 @@ static int cbus_i2c_remove(struct platform_device *pdev) { struct i2c_adapter *adapter = platform_get_drvdata(pdev); - return i2c_del_adapter(adapter); + i2c_del_adapter(adapter); + + return 0; } static int cbus_i2c_probe(struct platform_device *pdev) diff --git a/drivers/i2c/busses/i2c-intel-mid.c b/drivers/i2c/busses/i2c-intel-mid.c index 323fa01..0fb6597 100644 --- a/drivers/i2c/busses/i2c-intel-mid.c +++ b/drivers/i2c/busses/i2c-intel-mid.c @@ -1082,8 +1082,7 @@ static void intel_mid_i2c_remove(struct pci_dev *dev) { struct intel_mid_i2c_private *mrst = pci_get_drvdata(dev); intel_mid_i2c_disable(mrst-adap); - if (i2c_del_adapter(mrst-adap)) - dev_err(dev-dev, Failed to delete i2c adapter); + i2c_del_adapter(mrst-adap); free_irq(dev-irq, mrst); iounmap(mrst-base); diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index 8b20ef8..3bbd65d
[PATCH 4/6] i2c: Make return type of i2c_del_adapter() void
i2c_del_adapter() is usually called from a drivers remove callback. The Linux device driver model does not allow the remove callback to fail and all resources allocated in the probe callback need to be freed, as well as all resources which have been provided to the rest of the kernel(for example a I2C adapter) need to be revoked. So any function revoking such resources isn't allowed to fail either. i2c_del_adapter() adheres to this requirement and will never fail. But i2c_del_adapter()'s return type is int, which may cause driver authors to think that it can fail. This led to code constructs like: ret = i2c_del_adapter(...); BUG_ON(ret); Since i2c_del_adapter() always returns 0 the BUG_ON is never hit and essentially becomes dead code, which means it can be removed. Making the return type of i2c_del_adapter() void makes it explicit that the function will never fail and should prevent constructs like the above from re-appearing in the kernel code. All callers of i2c_del_adapter() have already been updated in a previous patch to ignore the return value, so the conversion of the return type from int to void can be done without causing any build failures. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/i2c/i2c-core.c | 6 ++ include/linux/i2c.h| 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 7727d33..838d030 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -1060,7 +1060,7 @@ static int __process_removed_adapter(struct device_driver *d, void *data) * This unregisters an I2C adapter which was previously registered * by @i2c_add_adapter or @i2c_add_numbered_adapter. */ -int i2c_del_adapter(struct i2c_adapter *adap) +void i2c_del_adapter(struct i2c_adapter *adap) { struct i2c_adapter *found; struct i2c_client *client, *next; @@ -1072,7 +1072,7 @@ int i2c_del_adapter(struct i2c_adapter *adap) if (found != adap) { pr_debug(i2c-core: attempting to delete unregistered adapter [%s]\n, adap-name); - return 0; + return; } /* Tell drivers about this removal */ @@ -1124,8 +1124,6 @@ int i2c_del_adapter(struct i2c_adapter *adap) /* Clear the device structure in case this adapter is ever going to be added again */ memset(adap-dev, 0, sizeof(adap-dev)); - - return 0; } EXPORT_SYMBOL(i2c_del_adapter); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 8ffab3f..dec1702 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -447,7 +447,7 @@ void i2c_unlock_adapter(struct i2c_adapter *); */ #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) extern int i2c_add_adapter(struct i2c_adapter *); -extern int i2c_del_adapter(struct i2c_adapter *); +extern void i2c_del_adapter(struct i2c_adapter *); extern int i2c_add_numbered_adapter(struct i2c_adapter *); extern int i2c_register_driver(struct module *, struct i2c_driver *); -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] i2c: Ignore the return value of i2c_del_mux_adapter()
i2c_del_mux_adapter() always returns 0. So all checks testing whether it will be non zero will always evaluate to false and the conditional code is dead code. This patch updates all callers of i2c_del_mux_adapter() to ignore its return value and assume that it will always succeed (which it will). A subsequent patch will make the return type of i2c_del_mux_adapter() void. Cc: Guenter Roeck guenter.ro...@ericsson.com Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- drivers/i2c/muxes/i2c-mux-pca954x.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c index 8e43872..a531d80 100644 --- a/drivers/i2c/muxes/i2c-mux-pca954x.c +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c @@ -262,13 +262,11 @@ static int pca954x_remove(struct i2c_client *client) { struct pca954x *data = i2c_get_clientdata(client); const struct chip_desc *chip = chips[data-type]; - int i, err; + int i; for (i = 0; i chip-nchans; ++i) if (data-virt_adaps[i]) { - err = i2c_del_mux_adapter(data-virt_adaps[i]); - if (err) - return err; + i2c_del_mux_adapter(data-virt_adaps[i]); data-virt_adaps[i] = NULL; } -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] I2C: xiic: Add OF binding support
Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- Documentation/devicetree/bindings/i2c/xiic.txt | 22 ++ drivers/i2c/busses/i2c-xiic.c | 23 ++- 2 files changed, 40 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/xiic.txt diff --git a/Documentation/devicetree/bindings/i2c/xiic.txt b/Documentation/devicetree/bindings/i2c/xiic.txt new file mode 100644 index 000..ceabbe9 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/xiic.txt @@ -0,0 +1,22 @@ +Xilinx IIC controller: + +Required properties: +- compatible : Must be xlnx,xps-iic-2.00.a +- reg : IIC register location and length +- interrupts : IIC controller unterrupt +- #address-cells = 1 +- #size-cells = 0 + +Optional properties: +- Child nodes conforming to i2c bus binding + +Example: + + axi_iic_0: i2c@4080 { + compatible = xlnx,xps-iic-2.00.a; + interrupts = 1 2 ; + reg = 0x4080 0x1 ; + + #size-cells = 0; + #address-cells = 1; + }; diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 2bded76..641d0e5 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -40,6 +40,7 @@ #include linux/i2c-xiic.h #include linux/io.h #include linux/slab.h +#include linux/of_i2c.h #define DRIVER_NAME xiic-i2c @@ -705,8 +706,6 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev) goto resource_missing; pdata = (struct xiic_i2c_platform_data *) pdev-dev.platform_data; - if (!pdata) - return -EINVAL; i2c = kzalloc(sizeof(*i2c), GFP_KERNEL); if (!i2c) @@ -730,6 +729,7 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev) i2c-adap = xiic_adapter; i2c_set_adapdata(i2c-adap, i2c); i2c-adap.dev.parent = pdev-dev; + i2c-adap.dev.of_node = pdev-dev.of_node; xiic_reinit(i2c); @@ -748,9 +748,13 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev) goto add_adapter_failed; } - /* add in known devices to the bus */ - for (i = 0; i pdata-num_devices; i++) - i2c_new_device(i2c-adap, pdata-devices + i); + if (pdata) { + /* add in known devices to the bus */ + for (i = 0; i pdata-num_devices; i++) + i2c_new_device(i2c-adap, pdata-devices + i); + } + + of_i2c_register_devices(i2c-adap); return 0; @@ -795,12 +799,21 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev) return 0; } +#if defined(CONFIG_OF) +static const struct of_device_id xiic_of_match[] __devinitconst = { + { .compatible = xlnx,xps-iic-2.00.a, }, + {}, +}; +MODULE_DEVICE_TABLE(of, xiic_of_match); +#endif + static struct platform_driver xiic_i2c_driver = { .probe = xiic_i2c_probe, .remove = __devexit_p(xiic_i2c_remove), .driver = { .owner = THIS_MODULE, .name = DRIVER_NAME, + .of_match_table = of_match_ptr(xiic_of_match), }, }; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] i2c/of: Populate multiplexed i2c busses from the device tree.
On 04/12/2012 11:14 PM, David Daney wrote: From: David Daney david.da...@cavium.com v3: Integrate changes from Lars-Peter Clausen to make better use of the of_*() infrastructure. Get rid of ugly #ifdefs. v2: Update bindings to use reg insutead of cell-index v1: Unchanged from the original RFC where I said: We need to populate our I2C devices from the device tree, but some of our systems have multiplexers which currently break this process. The basic idea is to have the generic i2c-mux framework propagate the device_node for the child bus into the corresponding i2c_adapter, and then call of_i2c_register_devices(). This means that the device tree bindings for *all* i2c muxes must have some common properties. I try to document these in mux.txt. This is now tested against 3.4-rc2 and is still working well. I've been using these patches with a pca9548 and a pca9546 for a while now. Both: Tested-by: Lars-Peter Clausen l...@metafoo.de -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] SPI: Add helper macro for spi_driver boilerplate
On 11/24/2011 01:13 AM, Ben Dooks wrote: On Wed, Nov 16, 2011 at 10:12:54AM -0700, Grant Likely wrote: On Wed, Nov 16, 2011 at 2:13 AM, Lars-Peter Clausen l...@metafoo.de wrote: This patch introduces the module_spi_driver macro which is a convenience macro for SPI driver modules similar to module_platform_driver. It is intended to be used by drivers which init/exit section does nothing but register/unregister the SPI driver. By using this macro it is possible to eliminate a few lines of boilerplate code per SPI driver. Signed-off-by: Lars-Peter Clausen l...@metafoo.de Acked-by: Grant Likely grant.lik...@secretlab.ca I'm begining to think we need to make some of these driver and device bits more generic... there seems to be so much similar but not quite the same code. I've been thinking the same. A good start would probably be consolidating the platform/spi/i2c device id handling code. Since those are all name based ids the code for handling them looks rather similar, though there are some minor differences. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] Generalize module_platform_driver
Grant Likely recently introduced the module_platform_driver macro which can be used to eliminate a few lines on boilerplate code in platform driver modules. The same approach can be used to do the same for other bus type drivers. The first patch of this series generalizes the module_platform_driver macro and introduces the module_driver macro. It is similar to module_platform_driver macro but has two additional parameters to pass in the driver register and unregister function. The intend is that this macro is used to construct bus specific macros for generating the driver register/unregister boilerplate code. The next two patches in this series add the module_i2c_driver and module_spi_driver macro which use the module_driver macro to generate the I2C and SPI driver boilerplate code. The last two patches convert the driver found in the IIO framework to use the new module_i2c_driver and module_spi_driver macros to demonstrate their potential and remove over 700 lines of code. While this series only introduces these kind of helper macros for I2C and SPI bus drivers the same scheme should be applicable to most other bus driver types. For example PCI and USB seem to be good candidates. It probably makes sense to merge the first three patches together. The last two can probably, since this is not urgent, wait until the first three have reached mainline. - Lars Lars-Peter Clausen (5): drivercore: Generalize module_platform_driver I2C: Add helper macro for i2c_driver boilerplate SPI: Add helper macro for spi_driver boilerplate staging:iio: Use module_i2c_driver to register I2C drivers staging:iio: Use module_spi_driver to register SPI driver drivers/staging/iio/accel/adis16201_core.c | 13 + drivers/staging/iio/accel/adis16203_core.c | 13 + drivers/staging/iio/accel/adis16204_core.c | 13 + drivers/staging/iio/accel/adis16209_core.c | 13 + drivers/staging/iio/accel/adis16220_core.c | 13 + drivers/staging/iio/accel/adis16240_core.c | 13 + drivers/staging/iio/accel/kxsd9.c | 13 + drivers/staging/iio/accel/lis3l02dq_core.c | 13 + drivers/staging/iio/accel/sca3000_core.c| 13 + drivers/staging/iio/adc/ad7192.c| 13 + drivers/staging/iio/adc/ad7280a.c | 13 + drivers/staging/iio/adc/ad7291.c| 14 +- drivers/staging/iio/adc/ad7298_core.c | 13 + drivers/staging/iio/adc/ad7476_core.c | 13 + drivers/staging/iio/adc/ad7606_spi.c| 13 + drivers/staging/iio/adc/ad7780.c| 13 + drivers/staging/iio/adc/ad7793.c| 13 + drivers/staging/iio/adc/ad7816.c| 14 +- drivers/staging/iio/adc/ad7887_core.c | 13 + drivers/staging/iio/adc/ad799x_core.c | 14 +- drivers/staging/iio/adc/adt7310.c | 14 +- drivers/staging/iio/adc/adt7410.c | 14 +- drivers/staging/iio/adc/max1363_core.c | 14 +- drivers/staging/iio/addac/adt7316-i2c.c | 14 +- drivers/staging/iio/addac/adt7316-spi.c | 14 +- drivers/staging/iio/cdc/ad7150.c| 14 +- drivers/staging/iio/cdc/ad7152.c| 14 +- drivers/staging/iio/cdc/ad7746.c| 14 +- drivers/staging/iio/dac/ad5064.c| 13 + drivers/staging/iio/dac/ad5360.c| 13 + drivers/staging/iio/dac/ad5446.c| 13 + drivers/staging/iio/dac/ad5504.c| 13 + drivers/staging/iio/dac/ad5624r_spi.c | 13 + drivers/staging/iio/dac/ad5686.c| 13 + drivers/staging/iio/dac/ad5791.c| 13 + drivers/staging/iio/dac/max517.c| 14 +- drivers/staging/iio/dds/ad5930.c| 13 + drivers/staging/iio/dds/ad9832.c| 13 + drivers/staging/iio/dds/ad9834.c| 13 + drivers/staging/iio/dds/ad9850.c| 13 + drivers/staging/iio/dds/ad9852.c| 13 + drivers/staging/iio/dds/ad9910.c| 13 + drivers/staging/iio/dds/ad9951.c| 13 + drivers/staging/iio/gyro/adis16080_core.c | 13 + drivers/staging/iio/gyro/adis16130_core.c | 13 + drivers/staging/iio/gyro/adis16260_core.c | 13 + drivers/staging/iio/gyro/adxrs450_core.c| 13 + drivers/staging
[PATCH 3/5] SPI: Add helper macro for spi_driver boilerplate
This patch introduces the module_spi_driver macro which is a convenience macro for SPI driver modules similar to module_platform_driver. It is intended to be used by drivers which init/exit section does nothing but register/unregister the SPI driver. By using this macro it is possible to eliminate a few lines of boilerplate code per SPI driver. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- include/linux/spi/spi.h | 11 +++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index bb4f5fb..176fce9 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -200,6 +200,17 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) driver_unregister(sdrv-driver); } +/** + * module_spi_driver() - Helper macro for registering a SPI driver + * @__spi_driver: spi_driver struct + * + * Helper macro for SPI drivers which do not do anything special in module + * init/exit. This eliminates a lot of boilerplate. Each module may only + * use this macro once, and calling it replaces module_init() and module_exit() + */ +#define module_spi_driver(__spi_driver) \ + module_driver(__spi_driver, spi_register_driver, \ + spi_unregister_driver) /** * struct spi_master - interface to SPI master controller -- 1.7.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] I2C: Add helper macro for i2c_driver boilerplate
This patch introduces the module_i2c_driver macro which is a convenience macro for I2C driver modules similar to module_platform_driver. It is intended to be used by drivers which init/exit section does nothing but register/unregister the I2C driver. By using this macro it is possible to eliminate a few lines of boilerplate code per I2C driver. Signed-off-by: Lars-Peter Clausen l...@metafoo.de --- include/linux/i2c.h | 13 + 1 files changed, 13 insertions(+), 0 deletions(-) diff --git a/include/linux/i2c.h b/include/linux/i2c.h index a81bf6d..7e92854 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -485,6 +485,19 @@ static inline int i2c_adapter_id(struct i2c_adapter *adap) { return adap-nr; } + +/** + * module_i2c_driver() - Helper macro for registering a I2C driver + * @__i2c_driver: i2c_driver struct + * + * Helper macro for I2C drivers which do not do anything special in module + * init/exit. This eliminates a lot of boilerplate. Each module may only + * use this macro once, and calling it replaces module_init() and module_exit() + */ +#define module_i2c_driver(__i2c_driver) \ + module_driver(__i2c_driver, i2c_add_driver, \ + i2c_del_driver) + #endif /* I2C */ #endif /* __KERNEL__ */ -- 1.7.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html