Re: [PATCH] i2c/designware: enable i2c controller to suspend/resume asynchronously
Hi On 12/24/2015 04:30 PM, Fu, Zhonghui wrote: Now, PM core supports asynchronous suspend/resume mode for devices during system suspend/resume, and the power state transition of one device may be completed in separate kernel thread. PM core ensures all power state transition dependency between devices. This patch enables designware i2c controllers to suspend/resume asynchronously. This will take advantage of multicore and improve system suspend/resume speed. After enabling all i2c devices, i2c adapters and i2c controllers on ASUS T100TA tablet, the system suspend-to-idle time is reduced to about 510ms from 750ms, and the system resume time is reduced to about 790ms from 900ms. Nice reduction :-) diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 6b00061..395130b 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -230,6 +230,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) } adap = &dev->adapter; + device_enable_async_suspend(&pdev->dev); adap->owner = THIS_MODULE; adap->class = I2C_CLASS_DEPRECATED; ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); Does device_enable_async_suspend() need to be called before enabling runtime PM? I suppose not since there appears to have also related sysfs node for toggling it runtime. I'm thinking if you could move the device_enable_async_suspend() call into drivers/i2c/busses/i2c-designware-core.c: i2c_dw_probe() and then also PCI enumerated adapter could take advantage of it. -- Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support
On 12/16/2015 11:04 AM, Mika Westerberg wrote: On Tue, Dec 15, 2015 at 11:20:03AM -0800, Loc Ho wrote: Can we not just add an AML method that will return the operation clock frequency that the I2C driver can use? If the method doesn't existed, we will just bail and do nothing or assume what ever default behavior? Why would you add yet another method? There already are existing ACPI *CNT methods that you can use, and are used in systems out there. One thing to remember that *CNT methods allow more precise machine and per bus specific tunings against different HW characteristics like pull-up resistance, line load, propagation delay, etc. -- Jarkko -- 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
[RFC] i2c: Prevent runtime suspend of parent during adapter registration
If runtime PM of parent device is enabled before it registers the I2C adapter there can be unnecessary runtime suspend during adapter device registration. This can happen when adapter is registered from parent's probe and if parent device is initially active platform device. In that case power.usage_count of parent device is zero and pm_runtime_get()/pm_runtime_put() cycle during probe could put the parent into runtime suspend. This happens when the i2c_register_adapter() calls the device_register(): i2c_register_adapter device_register device_add bus_probe_device device_initial_probe __device_attach if (dev->parent) pm_runtime_get_sync(dev->parent) ... if (dev->parent) pm_runtime_put(dev->parent) After that i2c_register_adapter() continues registering I2C slave devices. In case slave device probe does I2C transfers the parent will resume again and thus get a needless runtime suspend/resume cycle during adapter registration. Prevent this while retaining the runtime PM status of parent by only incrementing/decrementing parent device power usage count during I2C adapter registration. That makes sure there won't be spurious runtime PM status changes for parent's probe and lets the driver core to idle the device after probe finishes. Signed-off-by: Jarkko Nikula --- I noticed this with i2c-designware-platdrv.c which started to do so after commit 36d48fb5766a ("i2c: designware-platdrv: enable RuntimePM before registering to the core"). I guess the same could happen also with a few other I2C drivers that enable runtime PM similar way. For instance i2c-at91.c, i2c-cadence.c, i2c-hix5hd2.c and i2c-qup.c. That made me thinking if issue might be best to handle in i2c-core.c. Device core drivers/base/dd.c: driver_probe_device() or drivers/base/platform.c: platform_drv_probe() could be other alternatives but that would cause a regression to a driver that purposively tries to suspend the device in its probe(). --- drivers/i2c/i2c-core.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index ba8eb087f224..4f8b5c80cf1e 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -1556,6 +1556,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) dev_set_name(&adap->dev, "i2c-%d", adap->nr); adap->dev.bus = &i2c_bus_type; adap->dev.type = &i2c_adapter_type; + if (adap->dev.parent) + pm_runtime_get_noresume(adap->dev.parent); res = device_register(&adap->dev); if (res) goto out_list; @@ -1617,6 +1619,8 @@ exit_recovery: mutex_lock(&core_lock); bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter); mutex_unlock(&core_lock); + if (adap->dev.parent) + pm_runtime_put_noidle(adap->dev.parent); return 0; @@ -1624,6 +1628,8 @@ out_list: mutex_lock(&core_lock); idr_remove(&i2c_adapter_idr, adap->nr); mutex_unlock(&core_lock); + if (adap->dev.parent) + pm_runtime_put_noidle(adap->dev.parent); return res; } -- 2.6.2 -- 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] I2C: designware: fix IO timeout issue for AMD controller
On 12/11/2015 02:02 PM, Xiangliang Yu wrote: Because of some hardware limitation, AMD I2C controller can't trigger pending interrupt if interrupt status has been changed after clearing interrupt status bits. Then, I2C will lost interrupt and IO timeout. According to hardware design, this patch implements a workaround to disable i2c controller interrupt and re-enable i2c interrupt before exiting ISR. To reduce the performance impacts on other vendors, use unlikely function to check flag in ISR. --- Changes in v2: - pass flags with ->driver_data - unmask interrupt right after masking Signed-off-by: Xiangliang Yu --- drivers/i2c/busses/i2c-designware-core.c| 6 ++ drivers/i2c/busses/i2c-designware-core.h| 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 7 ++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 8c48b27..de7fbbb 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -813,6 +813,12 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) tx_aborted: if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err) complete(&dev->cmd_complete); + else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) { + /* workaround to trigger pending interrupt */ + stat = dw_readl(dev, DW_IC_INTR_MASK); + i2c_dw_disable_int(dev); + dw_writel(dev, stat, DW_IC_INTR_MASK); + } Looks ok to me. Acked-by: Jarkko Nikula -- 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: designware: Allow build Baytrail semaphore support when IOSF_MBI=m
On 12/10/2015 02:59 PM, Andy Shevchenko wrote: On Thu, 2015-12-10 at 13:48 +0200, Jarkko Nikula wrote: I believe i2c-designware-baytrail.c doesn't have strict dependency that Intel SoC IOSF Sideband support must be always built-in in order to be able to compile support for Intel Baytrail I2C bus sharing HW semaphore. Redefine build dependencies so that CONFIG_IOSF_MBI=y is required only when CONFIG_I2C_DESIGNWARE_PLATFORM is built-in. Signed-off-by: Jarkko Nikula --- Hi David. Can you ack/nak this patch as I'm not fully familiar with this HW semaphore can there be problems when IOSF_MBI is built as a module. At least I'm getting similar sensible looking "punit semaphore acquired/held for x ms" debug messages when I modprobe/rmmod i2c_designware_platform independently is the CONFIG_IOSF_MBI=y or =m. --- drivers/i2c/busses/Kconfig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 69c46fe13777..76f4d024def0 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -490,7 +490,9 @@ config I2C_DESIGNWARE_PCI config I2C_DESIGNWARE_BAYTRAIL bool "Intel Baytrail I2C semaphore support" - depends on I2C_DESIGNWARE_PLATFORM && IOSF_MBI=y && ACPI + depends on ACPI + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \ + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y) Would it be more readable in the following way depends on ACPI depends on I2C_DESIGNWARE_PLATFORM depends on IOSF_MBI if I2C_DESIGNWARE_PLATFORM=m depends on IOSF_MBI=y if I2C_DESIGNWARE_PLATFORM=y For my eyes it looks a bit more complex but I think it's matter of taste. However, the syntax you are proposing is not supported for "depends on" like it is for "select" statements. -- Jarkko -- 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] i2c: designware: Keep pm_runtime_enable/_disable calls in sync
On an hardware shared I2C bus (certain Intel Baytrail SoC platforms) the runtime PM disable depth keeps increasing over repeated modprobe/rmmod cycle because pm_runtime_disable() is called without checking should it be disabled already because of bus sharing. This hasn't made any other harm than dev->power.disable_depth keeps increasing but keep it sync by calling pm_runtime_disable() only when runtime PM is not disabled. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-platdrv.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 809579ecb5a4..1308666b054b 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -240,12 +240,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) } r = i2c_dw_probe(dev); - if (r) { + if (r && !dev->pm_runtime_disabled) pm_runtime_disable(&pdev->dev); - return r; - } - return 0; + return r; } static int dw_i2c_plat_remove(struct platform_device *pdev) @@ -260,7 +258,8 @@ static int dw_i2c_plat_remove(struct platform_device *pdev) pm_runtime_dont_use_autosuspend(&pdev->dev); pm_runtime_put_sync(&pdev->dev); - pm_runtime_disable(&pdev->dev); + if (!dev->pm_runtime_disabled) + pm_runtime_disable(&pdev->dev); return 0; } -- 2.6.2 -- 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: designware: Allow build Baytrail semaphore support when IOSF_MBI=m
I believe i2c-designware-baytrail.c doesn't have strict dependency that Intel SoC IOSF Sideband support must be always built-in in order to be able to compile support for Intel Baytrail I2C bus sharing HW semaphore. Redefine build dependencies so that CONFIG_IOSF_MBI=y is required only when CONFIG_I2C_DESIGNWARE_PLATFORM is built-in. Signed-off-by: Jarkko Nikula --- Hi David. Can you ack/nak this patch as I'm not fully familiar with this HW semaphore can there be problems when IOSF_MBI is built as a module. At least I'm getting similar sensible looking "punit semaphore acquired/held for x ms" debug messages when I modprobe/rmmod i2c_designware_platform independently is the CONFIG_IOSF_MBI=y or =m. --- drivers/i2c/busses/Kconfig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 69c46fe13777..76f4d024def0 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -490,7 +490,9 @@ config I2C_DESIGNWARE_PCI config I2C_DESIGNWARE_BAYTRAIL bool "Intel Baytrail I2C semaphore support" - depends on I2C_DESIGNWARE_PLATFORM && IOSF_MBI=y && ACPI + depends on ACPI + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \ + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y) help This driver enables managed host access to the PMIC I2C bus on select Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows -- 2.6.2 -- 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 RESEND] i2c: Remove setting for 1 second timeout from adapter drivers
On 12/03/2015 10:52 PM, Wolfram Sang wrote: On Wed, Dec 02, 2015 at 01:02:34PM +0200, Jarkko Nikula wrote: I2C adapter drivers that are using 1 second timeout can leave the timeout unset and let the i2c-core.c: i2c_register_adapter() to set it instead. Signed-off-by: Jarkko Nikula My take on this is that I prefer the explicit (and thus visible) assignment. What the core does was intended as a fallback to prevent strange things happening with a 0 value. Fair enough. It's hard to say if some of these really needs to be 1 second instead of being stetson guessed or copy-pasted. Now if we ever change the fall back value from HZ to HZ/10 or similar we might get a regression. -- Jarkko -- 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 RESEND] i2c: Remove setting for 1 second timeout from adapter drivers
I2C adapter drivers that are using 1 second timeout can leave the timeout unset and let the i2c-core.c: i2c_register_adapter() to set it instead. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-ibm_iic.c | 1 - drivers/i2c/busses/i2c-iop3xx.c | 1 - drivers/i2c/busses/i2c-mpc.c | 1 - drivers/i2c/busses/i2c-mv64xxx.c | 5 - drivers/i2c/busses/i2c-pca-platform.c | 1 - 5 files changed, 9 deletions(-) diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index ab492301581a..5dc0bb7e5dbc 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -748,7 +748,6 @@ static int iic_probe(struct platform_device *ofdev) i2c_set_adapdata(adap, dev); adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD; adap->algo = &iic_algo; - adap->timeout = HZ; ret = i2c_add_adapter(adap); if (ret < 0) { diff --git a/drivers/i2c/busses/i2c-iop3xx.c b/drivers/i2c/busses/i2c-iop3xx.c index 72d6161cf77c..c4ed8281bec2 100644 --- a/drivers/i2c/busses/i2c-iop3xx.c +++ b/drivers/i2c/busses/i2c-iop3xx.c @@ -479,7 +479,6 @@ iop3xx_i2c_probe(struct platform_device *pdev) /* * Default values...should these come in from board code? */ - new_adapter->timeout = HZ; new_adapter->algo = &iop3xx_i2c_algo; init_waitqueue_head(&adapter_data->waitq); diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 48ecffecc0ed..b4bb64ffe3cd 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -640,7 +640,6 @@ static const struct i2c_algorithm mpc_algo = { static struct i2c_adapter mpc_ops = { .owner = THIS_MODULE, .algo = &mpc_algo, - .timeout = HZ, }; static const struct of_device_id mpc_i2c_of_match[]; diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index 43207f52e5a3..eaef7763f5fc 100644 --- a/drivers/i2c/busses/i2c-mv64xxx.c +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -839,11 +839,6 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, reset_control_deassert(drv_data->rstc); } - /* Its not yet defined how timeouts will be specified in device tree. -* So hard code the value to 1 second. -*/ - drv_data->adapter.timeout = HZ; - device = of_match_device(mv64xxx_i2c_of_match_table, dev); if (!device) return -ENODEV; diff --git a/drivers/i2c/busses/i2c-pca-platform.c b/drivers/i2c/busses/i2c-pca-platform.c index 3bd2e7d06e4b..2b930df0e6b5 100644 --- a/drivers/i2c/busses/i2c-pca-platform.c +++ b/drivers/i2c/busses/i2c-pca-platform.c @@ -183,7 +183,6 @@ static int i2c_pca_pf_probe(struct platform_device *pdev) i2c->algo_data.i2c_clock = platform_data->i2c_clock_speed; i2c->gpio = platform_data->gpio; } else { - i2c->adap.timeout = HZ; i2c->algo_data.i2c_clock = 59000; i2c->gpio = -1; } -- 2.6.2 -- 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 v1 13/13] i2c: designware: Convert to use unified device property API
On 11/24/2015 12:22 PM, Andy Shevchenko wrote: From: Mika Westerberg With ACPI _DSD (introduced in ACPI v5.1) it is now possible to pass device configuration information from ACPI in addition to DT. In order to support this, convert the driver to use the unified device property accessors instead of DT specific. Change to ordering a bit so that we first try platform data and if that's not available look from device properties. ACPI *CNT methods are then used as last resort to override everything else. Signed-off-by: Mika Westerberg Signed-off-by: Andy Shevchenko --- drivers/i2c/busses/i2c-designware-platdrv.c | 48 + 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 809579e..e9062be 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -156,33 +157,28 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) /* fast mode by default because of legacy reasons */ clk_freq = 40; - if (has_acpi_companion(&pdev->dev)) { - dw_i2c_acpi_configure(pdev); - } else if (pdev->dev.of_node) { - of_property_read_u32(pdev->dev.of_node, - "i2c-sda-hold-time-ns", &ht); - - of_property_read_u32(pdev->dev.of_node, -"i2c-sda-falling-time-ns", -&dev->sda_falling_time); - of_property_read_u32(pdev->dev.of_node, -"i2c-scl-falling-time-ns", -&dev->scl_falling_time); - - of_property_read_u32(pdev->dev.of_node, "clock-frequency", -&clk_freq); - - /* Only standard mode at 100kHz and fast mode at 400kHz -* are supported. -*/ - if (clk_freq != 10 && clk_freq != 40) { - dev_err(&pdev->dev, "Only 100kHz and 400kHz supported"); - return -EINVAL; - } + if ((pdata = dev_get_platdata(&pdev->dev))) { + clk_freq = pdata->i2c_scl_freq; } else { - pdata = dev_get_platdata(&pdev->dev); - if (pdata) - clk_freq = pdata->i2c_scl_freq; + device_property_read_u32(&pdev->dev, "i2c-sda-hold-time-ns", +&ht); + device_property_read_u32(&pdev->dev, "i2c-sda-falling-time-ns", +&dev->sda_falling_time); + device_property_read_u32(&pdev->dev, "i2c-scl-falling-time-ns", +&dev->scl_falling_time); + device_property_read_u32(&pdev->dev, "clock-frequency", + &clk_freq); Mika, Andy: Was this one able to go separately? At least it builds without rest of the set but is there anything that could break DT based system if there are no patches 1-8/13? Acked-by: Jarkko Nikula -- 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: designware platdrv and runtime pm?
On 09.11.2015 23:15, Wolfram Sang wrote: Hi guys, while handling the merge conflict for the designware-platdrv, I noticed an asymmetry in the runtime PM handling. Currently, code looks like this: if (dev->pm_runtime_disabled) { pm_runtime_forbid(&pdev->dev); } else { pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev); } r = i2c_dw_probe(dev); if (r) { pm_runtime_disable(&pdev->dev); return r; } But shouldn't the above error path (and the remove path) also take dev->pm_runtime_disabled into account and act accordingly? I think you are right. Which brings another question to my mind do we need to have a patch to linux-stable too? David: Your original commit 894acb2f823b ("i2c: designware: Add Intel Baytrail PMIC I2C bus support") doesn't add pm_runtime_disabled test to dw_i2c_remove(). I guess there is possibility power down the shared controller by having CONFIG_I2C_DESIGNWARE_PLATFORM=m and then unloading the driver? -- Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller
On 06.11.2015 06:34, Yu, Xiangliang wrote: -Original Message- From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com] --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) stat = i2c_dw_read_clear_intrbits(dev); What if the status changes right here, before you go and mask the interrupt? Have no effect, because i2c controller can't trigger next interrupt. Does it mean possible lost interrupt too? I guess it can be debugged by placing a few ms long mdelay() between clearing and masking. How frequent is this timeout? I guess lost interrupt is somehow nearly related to clearing, masking and unmasking if that cycle helps. One thing that comes to my mind is masking needed and could plain unmasking be also a working workaround? Have you tried to print DW_IC_INTR_STAT, DW_IC_INTR_MASK and DW_IC_RAW_INTR_STAT when timeout occurs? E.g. just after printing the timeout out error in i2c_dw_xfer(). Probably good in i2c_dw_isr() too if if doesn't produce too much data and make debugging impossible. -- Jarkko -- 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] platform/chrome: Fix i2c-designware adapter name
Commit d80d134182ba ("i2c: designware: Move common probe code into i2c_dw_probe()") caused the I2C adapter lookup code here to fail for PCI enumerated i2c-designware because commit changed the adapter name but didn't update it here. Fix the I2C adapter lookup by using the "Synopsys DesignWare I2C adapter" name. Reported-by: Jeremiah Mahler Fixes: d80d134182ba ("i2c: designware: Move common probe code into i2c_dw_probe()") Signed-off-by: Jarkko Nikula --- Hi Jeremiah. This is the same diff I had in a reply to your bug report. Can you test does this fix work for you as I don't have the HW. --- drivers/platform/chrome/chromeos_laptop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/platform/chrome/chromeos_laptop.c b/drivers/platform/chrome/chromeos_laptop.c index 02072749fff3..2b441e9ae593 100644 --- a/drivers/platform/chrome/chromeos_laptop.c +++ b/drivers/platform/chrome/chromeos_laptop.c @@ -47,8 +47,8 @@ static const char *i2c_adapter_names[] = { "SMBus I801 adapter", "i915 gmbus vga", "i915 gmbus panel", - "i2c-designware-pci", - "i2c-designware-pci", + "Synopsys DesignWare I2C adapter", + "Synopsys DesignWare I2C adapter", }; /* Keep this enum consistent with i2c_adapter_names */ -- 2.6.2 -- 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: designware: Don't mask TX_EMPTY if write is in progress
Hi On 10/30/2015 09:37 AM, Jisheng Zhang wrote: Currently when i2c_msg index search is completed, TX_EMPTY interrupt will be masked. But if the size of i2c_msg data is longer than the size of the tx buffer, we still need TX_EMPTY interrupt, otherwise we will get "controller timed out" error. This patch fixes this issue by only masking TX_EMPTY if i2c_msg index search is completed and write is not in progress. Signed-off-by: Jisheng Zhang --- drivers/i2c/busses/i2c-designware-core.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 7441cdc..a2eb212 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -542,10 +542,11 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev) } /* -* If i2c_msg index search is completed, we don't need TX_EMPTY -* interrupt any more. +* If i2c_msg index search is completed and writing is not in progress, +* we don't need TX_EMPTY interrupt any more. */ - if (dev->msg_write_idx == dev->msgs_num) + if (dev->msg_write_idx == dev->msgs_num && + !(dev->status & STATUS_WRITE_IN_PROGRESS)) intr_mask &= ~DW_IC_INTR_TX_EMPTY; This looks a possible scenario here. What I'm wondering how is the transfer ending and what condition in i2c_dw_isr() causes the complete() call? Assuming we break the for loop in i2c_dw_xfer_msg() with buf_len > 0 and dev->msg_write_idx == dev->msgs_num how we get back inside the for loop for writing the remaining bytes and stop command? -- Jarkko -- 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: Issues with touchpad / touchscreen on yoga 900
Hi On 10/29/2015 08:30 PM, Wolfram Sang wrote: On Thu, Oct 29, 2015 at 11:29:13AM -0600, Kevin Fenzi wrote: Greetings. I'm having problems with a lenovo yoga 900 not seeing it's touchscreen or touchpad. Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1275718 The problem parts of dmesg seem to be: Oct 26 15:20:08 localhost kernel: i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration Oct 26 15:20:08 localhost kernel: i2c_hid i2c-SYNA2B29:00: hid_descr_cmd failed Oct 26 15:20:08 localhost kernel: i2c_designware i2c_designware.2: i2c_dw_handle_tx_abort: lost arbitration Oct 26 15:20:08 localhost kernel: i2c_hid i2c-ELAN21EF:00: hid_descr_cmd failed This is with 4.3.0-0.rc7.git1.1.fc24.x86_64 (post rc7 4.3.0). Happy to gather further info or try patches. ;) Could you try this test patch from Mika that prints the parameters it gets from ACPI and forces using calculated ones (your quirk was supposed to be doing the same): http://www.spinics.net/lists/linux-i2c/msg21239.html Another fix from Mika that comes to my mind is "HID: multitouch: Fetch feature reports on demand for Win8 devices" which is sitting in linux-next. Not sure is it related but probably worth to try. https://lkml.org/lkml/2015/9/28/404 -- Jarkko -- 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: i801: Document Intel DNV and Broxton
Add missing entries into i2c-i801 documentation and Kconfig about recently added Intel DNV and Broxton. Suggested-by: Jean Delvare Signed-off-by: Jarkko Nikula Cc: Mika Westerberg --- Documentation/i2c/busses/i2c-i801 | 2 ++ drivers/i2c/busses/Kconfig| 2 ++ 2 files changed, 4 insertions(+) diff --git a/Documentation/i2c/busses/i2c-i801 b/Documentation/i2c/busses/i2c-i801 index 82f48f774afb..6a4b1af724f8 100644 --- a/Documentation/i2c/busses/i2c-i801 +++ b/Documentation/i2c/busses/i2c-i801 @@ -30,6 +30,8 @@ Supported adapters: * Intel BayTrail (SOC) * Intel Sunrise Point-H (PCH) * Intel Sunrise Point-LP (PCH) + * Intel DNV (SOC) + * Intel Broxton (SOC) Datasheets: Publicly available at the Intel website On Intel Patsburg and later chipsets, both the normal host SMBus controller diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 0774f18f5716..45e1a7ec617d 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -124,6 +124,8 @@ config I2C_I801 BayTrail (SOC) Sunrise Point-H (PCH) Sunrise Point-LP (PCH) + DNV (SOC) + Broxton (SOC) This driver can also be built as a module. If so, the module will be called i2c-i801. -- 2.6.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
Re: [PATCH] i2c: i801: Add support for Intel Broxton
Hi On 10/25/2015 12:50 PM, Jean Delvare wrote: @@ -204,6 +205,7 @@ #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS 0xa123 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS 0x9d23 #define PCI_DEVICE_ID_INTEL_DNV_SMBUS 0x19df +#define PCI_DEVICE_ID_INTEL_BROXTON_SMBUS 0x5ad4 Can you please get this added to pci.ids? http://pci-ids.ucw.cz/read/PC/8086 We seems to miss quite many other recent IDs too. I'll take this forward. struct i801_mux_config { char *gpio_chip; @@ -866,6 +868,7 @@ static const struct pci_device_id i801_ids[] = { { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) }, { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) }, { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) }, + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BROXTON_SMBUS) }, { 0, } }; Does this one not have FEATURE_TCO as DNV does? If it does, you'll need to add a line for it in the switch block in i801_probe(). No, it's specific to sunrisepoint and dnv only at the moment. Please also update Documentation/i2c/busses/i2c-i801 and drivers/i2c/busses/Kconfig. Ok, will do. -- Jarkko -- 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 2/2] i2c: add ACPI support for I2C mux ports
On 10/23/2015 04:40 PM, Mika Westerberg wrote: On Fri, Oct 23, 2015 at 04:13:11PM +0300, Mika Westerberg wrote: On Fri, Oct 23, 2015 at 12:16:11PM +0200, Wolfram Sang wrote: On Fri, Oct 23, 2015 at 11:40:13AM +0300, Mika Westerberg wrote: On Thu, Oct 22, 2015 at 02:17:42AM -0700, Dustin Byford wrote: Ah, forgot to mention that the i2c-designware-pcidrv.c misses include of so that needs to be fixed. Otherwise we get: drivers/i2c/busses/i2c-designware-pcidrv.c: In function ‘i2c_dw_pci_probe’: drivers/i2c/busses/i2c-designware-pcidrv.c:259:2: error: implicit declaration of function ‘ACPI_COMPANION_SET’ [-Werror=implicit-function-declaration] drivers/i2c/busses/i2c-designware-pcidrv.c:259:33: error: implicit declaration of function ‘ACPI_COMPANION’ [-Werror=implicit-function-declaration] Please also check that patches apply on top of i2c/for-next due recently applied cleanup patches from me. I think commit d80d134182ba ("i2c: designware: Move common probe code into i2c_dw_probe()") makes your patch only a 2 liner to drivers/i2c/busses/i2c-designware-core.c :-) -- Jarkko -- 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: i801: Add support for Intel Broxton
This patch adds the SMBUS PCI ID of Intel Broxton. Signed-off-by: Jarkko Nikula --- This goes on top of Mika's "[PATCH] i2c: i801: Add support for Intel DNV": http://marc.info/?l=linux-i2c&m=14447401042&w=2 --- drivers/i2c/busses/i2c-i801.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 47c2ddf76264..d8219bc2ac4e 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -61,6 +61,7 @@ * Sunrise Point-H (PCH) 0xa123 32 hardyes yes yes * Sunrise Point-LP (PCH) 0x9d23 32 hardyes yes yes * DNV (SOC) 0x19df 32 hardyes yes yes + * Broxton (SOC) 0x5ad4 32 hardyes yes yes * * Features supported by this driver: * Software PECno @@ -204,6 +205,7 @@ #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS 0xa123 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS 0x9d23 #define PCI_DEVICE_ID_INTEL_DNV_SMBUS 0x19df +#define PCI_DEVICE_ID_INTEL_BROXTON_SMBUS 0x5ad4 struct i801_mux_config { char *gpio_chip; @@ -866,6 +868,7 @@ static const struct pci_device_id i801_ids[] = { { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) }, { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) }, { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) }, + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BROXTON_SMBUS) }, { 0, } }; -- 2.6.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
Re: linux-next: Tree for Oct 21 (drivers/i2c/busses/i2c-designware-platdrv.c)
Hi On 10/21/2015 11:28 PM, Randy Dunlap wrote: On 10/20/15 23:16, Stephen Rothwell wrote: Hi all, There will be no linux-next releases after tomorrow until Nov 2 (kernel summit). Changes since 20151020: on i386 or x86_64: when CONFIG_PM_SLEEP is not enabled: ../drivers/i2c/busses/i2c-designware-platdrv.c:340:13: error: 'dw_i2c_plat_prepare' undeclared here (not in a function) .prepare = dw_i2c_plat_prepare, ^ ../drivers/i2c/busses/i2c-designware-platdrv.c:341:14: error: 'dw_i2c_plat_complete' undeclared here (not in a function) .complete = dw_i2c_plat_complete, ^ Here's the fix for this: http://marc.info/?l=linux-i2c&m=144541136917692&w=2 -- Jarkko -- 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: designware: Disable runtime PM in case i2c_dw_probe() fails
Call to pm_runtime_disable() got dropped while handling the merge conflict between commit 36d48fb5766a ("i2c: designware-platdrv: enable RuntimePM before registering to the core") and commit d80d134182ba ("i2c: designware: Move common probe code into i2c_dw_probe()"). Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index eb55760ccd9f..8fd9d4a18bd5 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -261,8 +261,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) } r = i2c_dw_probe(dev); - if (r) + if (r) { + pm_runtime_disable(&pdev->dev); return r; + } return 0; } -- 2.6.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
Re: [PATCH v2 6/6] i2c: designware: Move common probe code into i2c_dw_probe()
Hi On 10/20/2015 07:32 PM, Wolfram Sang wrote: There was a merge conflict with a bugfix from i2c/for-current. I think it is okay, but you may want to double check my i2c/for-next. Looks like pm_runtime_disable() got dropped from your 36d48fb5766a ("i2c: designware-platdrv: enable RuntimePM before registering to the core") while handling the merge conflict. I'll send a fix. What about this irq-clearing-in-probe thingie on top of this series? :) I'll have a look at it. What's not entirely clear to me would it be no-op or not. HW is actually disabled after i2c_dw_init() which is called before requesting the interrupt but is not clear to me from the spec does HW clear interrupts while it goes idle. So as a result I'd expect either a explicit interrupt clearing patch (to be more robust against potential unmasking changes) or a comment in __i2c_dw_enable() :-) -- Jarkko -- 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: designware: Fix build error when !CONFIG_PM_SLEEP
Commit 6ad6fde3970c ("i2c: designware: Rename platform driver probe and PM functions") introduced "'dw_i2c_plat_prepare' undeclared here" and "'dw_i2c_plat_complete' undeclared here" build errors when CONFIG_PM_SLEEP is not set. Fix this by renaming NULL defined dw_i2c_prepare and dw_i2c_complete PM hooks to dw_i2c_plat_prepare and dw_i2c_plat_complete since this was obviously missing from the commit. Reported-by: kbuild test robot Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index f6b252a8ffd1..eb55760ccd9f 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -307,8 +307,8 @@ static void dw_i2c_plat_complete(struct device *dev) pm_request_resume(dev); } #else -#define dw_i2c_prepare NULL -#define dw_i2c_completeNULL +#define dw_i2c_plat_prepareNULL +#define dw_i2c_plat_complete NULL #endif #ifdef CONFIG_PM -- 2.6.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
Re: [PATCH 0/6] i2c: designware: Code duplication removal and cleanups
Hi On 10/15/2015 03:18 PM, Wolfram Sang wrote: Jarkko, would you be interested in maintaining the designware driver? For any non-trivial patch to this driver, I'd need assistance. Yes I can. I think you could add also Mika and Andy as all of us are actively supporting it. -- Jarkko -- 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: designware: enable High-speed mode for pcidrv
On 10/15/2015 08:46 AM, Xiang Wang wrote: 1. "bus speed mode" is a bit different from other parameters. Actually it can be determined by the speed setting of "i2c devices" in ACPI (I2CSerialBus). E.g. If i2c device uses 3MHz, we use High-speed mode for this i2c bus. 2. If we hardcode speed setting in pci driver, we lose the flexibility. A high-speed device may be connected to this i2c bus on this board, but may be connected to another i2c bus on another board design. In this patch, we enumerate the i2c device in ACPI table to determine the frequency setting. Then we set corresponding speed mode for this i2c controller. The ACPI stuff is common for pci and plat driver. If board design changes, we only change BIOS. In conclusion, we have 2 solutions to set the i2c controller speed mode (pci driver): 1) use hardcode value in pci driver 2) use frequency setting of "i2c device" in ACPI table (more flexible, but looks a bit strange) Do you have any preference/suggestions for above solutions? Thanks I don't think we can hard code especially the high-speed mode because most typically buses are populated with slower devices. Things are a bit more clear when ACPI provides timing parameters for the bus (for standard and fast speed modes at the moment in i2c-designware-platdrv.c: dw_i2c_acpi_configure()) but still I think the ACPI namespace walk may be needed against potential BIOS misconfigurations. For instance if it provides timing parameters for all speeds but there are devices with lower speed on the same bus. I'd take these timing parameters as configuration data for bus features but actual speed (speed bits in IC_CON register) is defined separately. To me it looks only way to achieve that is to pick slowest device from I2cSerialBus resource descriptors. -- Jarkko -- 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 v2 6/6] i2c: designware: Move common probe code into i2c_dw_probe()
There is some code duplication in i2c-designware-platdrv and i2c-designware-pcidrv probe functions. What is even worse that duplication requires i2c_dw_xfer(), i2c_dw_func() and i2c_dw_isr() i2c-designware-core functions to be exported. Therefore move common code into new i2c_dw_probe() and make functions above local to i2c-designware-core. While merging the code patch does following functional changes: - I2C Adapter name will be "Synopsys DesignWare I2C adapter". Previously it was used for platform and ACPI devices but PCI device used "i2c-designware-pci". - Using device name for interrupt name. Previous it was platform device name, ACPI device name or "i2c-designware-pci". - Error code from devm_request_irq() and i2c_add_numbered_adapter() will be printed in case of error. Signed-off-by: Jarkko Nikula --- v2: - Adapter name changed to "Synopsys DesignWare I2C adapter" - Using device name for interrupt name --- drivers/i2c/busses/i2c-designware-core.c| 49 + drivers/i2c/busses/i2c-designware-core.h| 5 +-- drivers/i2c/busses/i2c-designware-pcidrv.c | 30 ++ drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++--- 4 files changed, 49 insertions(+), 63 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 52272a01c679..8c48b27ba059 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -618,7 +618,7 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev) /* * Prepare controller for a transaction and call i2c_dw_xfer_msg */ -int +static int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) { struct dw_i2c_dev *dev = i2c_get_adapdata(adap); @@ -702,14 +702,17 @@ done_nolock: return ret; } -EXPORT_SYMBOL_GPL(i2c_dw_xfer); -u32 i2c_dw_func(struct i2c_adapter *adap) +static u32 i2c_dw_func(struct i2c_adapter *adap) { struct dw_i2c_dev *dev = i2c_get_adapdata(adap); return dev->functionality; } -EXPORT_SYMBOL_GPL(i2c_dw_func); + +static struct i2c_algorithm i2c_dw_algo = { + .master_xfer= i2c_dw_xfer, + .functionality = i2c_dw_func, +}; static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev) { @@ -770,7 +773,7 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev) * Interrupt service routine. This gets called whenever an I2C interrupt * occurs. */ -irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) { struct dw_i2c_dev *dev = dev_id; u32 stat, enabled; @@ -813,7 +816,6 @@ tx_aborted: return IRQ_HANDLED; } -EXPORT_SYMBOL_GPL(i2c_dw_isr); void i2c_dw_disable(struct dw_i2c_dev *dev) { @@ -838,5 +840,40 @@ u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev) } EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param); +int i2c_dw_probe(struct dw_i2c_dev *dev) +{ + struct i2c_adapter *adap = &dev->adapter; + int r; + + init_completion(&dev->cmd_complete); + mutex_init(&dev->lock); + + r = i2c_dw_init(dev); + if (r) + return r; + + snprintf(adap->name, sizeof(adap->name), +"Synopsys DesignWare I2C adapter"); + adap->algo = &i2c_dw_algo; + adap->dev.parent = dev->dev; + i2c_set_adapdata(adap, dev); + + i2c_dw_disable_int(dev); + r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED, +dev_name(dev->dev), dev); + if (r) { + dev_err(dev->dev, "failure requesting irq %i: %d\n", + dev->irq, r); + return r; + } + + r = i2c_add_numbered_adapter(adap); + if (r) + dev_err(dev->dev, "failure adding adapter: %d\n", r); + + return r; +} +EXPORT_SYMBOL_GPL(i2c_dw_probe); + MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core"); MODULE_LICENSE("GPL"); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 0e73b8672402..1d50898e7b24 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -113,13 +113,10 @@ struct dw_i2c_dev { #define ACCESS_16BIT 0x0002 extern int i2c_dw_init(struct dw_i2c_dev *dev); -extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], - int num); -extern u32 i2c_dw_func(struct i2c_adapter *adap); -extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id); extern void i2c_dw_disable(struct dw_i2c_dev *dev); extern void i2c_dw_disable_int(struct dw_i2c_dev *dev); extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev); +extern int i2c_dw_probe(struct dw_i2c_dev *dev); #if IS_ENABLED(CONFIG_I2C_DE
Re: [PATCH 6/6] i2c: designware: Move common probe code into i2c_dw_probe()
On 10/10/2015 10:57 AM, Wolfram Sang wrote: - I2C Adapter name will be "i2c-designware". Previously adapter name was "Synopsys DesignWare I2C adapter" for platform and ACPI devices and "i2c-designware-pci" for PCI devices. I have a small tendency to make it "Synopsys DesignWare I2C adapter" for all cases. Not much, though. Ok, will change. - Interrupt name too will be "i2c-designware". Previous it was platform device name, ACPI device name or "i2c-designware-pci". The device name makes them easier to distinguish in case you have multiple instantiations. Good point. Bug reports definitely will benefit from it given that we have platforms where there are differences between i2c-designware instances. Same platform can have different instances enumerated from PCI or ACPI and those are registered as platform devices in drivers/mfd/intel-lpss.c. -- Jarkko -- 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/6] i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()
On 10/10/2015 10:53 AM, Wolfram Sang wrote: On Mon, Aug 31, 2015 at 05:31:28PM +0300, Jarkko Nikula wrote: There is no need to clear interrupts in i2c_dw_pci_probe() since only place where interrupts are unmasked is i2c_dw_xfer_init() and there interrupts are always cleared after commit 2a2d95e9d6d2 ("i2c: designware: always clear interrupts before enabling them"). Still, their status might be unclear when we init because firmware might have used it before. It should be easy to reintroduce this in patch 6, or? Yes and then it would be also uniform across different enumeration methods if we ever need to add it. Now this clearing was done only for PCI devices. Although I don't think there is any need to add clearing to probe path as clearing happens beginning of transmit before enabling the interrupts. -- Jarkko -- 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: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves
Hi On 10/10/2015 12:47 AM, Wolfram Sang wrote: On Fri, Oct 02, 2015 at 12:27:16PM +0300, Jarkko Nikula wrote: On 10/01/2015 11:37 PM, Wolfram Sang wrote: Do you foresee troubles already? I am still in favour of a symlink. I haven't looked at this for a while but one problem was that devices/ directory belongs to private structure of struct bus_type and in order to create a symlink there it needs to done in drivers/base/bus.c: bus_add_device() which felt quite hackish to me. This is just a quick prototype and untested; but I did something similar in the i2c-mux code: diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 5f89f1e3c2f24f..715dca57ba68fd 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -970,13 +970,15 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, { struct acpi_device *adev = ACPI_COMPANION(&client->dev); - if (adev) { - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); - return; - } - dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), i2c_encode_flags_to_addr(client)); + + if (adev) { + char symlink_name[256]; + + snprintf(symlink_name, sizeof(symlink_name), "i2c-%s", acpi_dev_name(adev)); + sysfs_create_link(&client->dev.kobj, &adap->dev.kobj, symlink_name); + } } /** Shouldn't something like this be enough? Not really. It would create the symlink under the device not under /sys/bus/i2c/devices/. Please note the sysfs_create_link() must be called after device_register(). I moved the symlink creation into i2c_new_device() for the example below. Now: /sys/bus/i2c/devices/i2c-ATML3432:00 -> ../../../devices/pci:00/:00:15.1/i2c_designware.1/i2c-9/i2c-ATML3432:00 After modified patch: /sys/bus/i2c/devices/9-004c -> ../../../devices/pci:00/:00:15.1/i2c_designware.1/i2c-9/9-004c which has now the symlink pointing to adapter /sys/devices/pci:00/:00:15.1/i2c_designware.1/i2c-9/9-004c/ i2c-ATML3432:00 -> ../../i2c-9 We would need a following symlink but I didn't figure out how to do it without touching drivers/base/bus.c. /sys/bus/i2c/devices/i2c-ATML3432:00 -> 9-004c Symlinks there are added/removed by using &bus->p->devices_kset->kobj where p is a private for driver core only. -- Jarkko -- 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: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves
On 10/01/2015 11:37 PM, Wolfram Sang wrote: This is for discussion so I didn't cc sta...@vger.kernel.org yet. I was thinking would it work if we'd keep the stable name but have an another symlink in /sys/bus/i2c/devices/ that uses "x-00yz" name. However this feels ill-use of devices directory and probably causes more troubles elsewhere. Do you foresee troubles already? I am still in favour of a symlink. I haven't looked at this for a while but one problem was that devices/ directory belongs to private structure of struct bus_type and in order to create a symlink there it needs to done in drivers/base/bus.c: bus_add_device() which felt quite hackish to me. -- Jarkko -- 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: Remove setting for 1 second timeout from adapter drivers
I2C adapter drivers that are using 1 second timeout can leave the timeout unset and let the i2c-core.c: i2c_register_adapter() to set it instead. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-ibm_iic.c | 1 - drivers/i2c/busses/i2c-iop3xx.c | 1 - drivers/i2c/busses/i2c-mpc.c | 1 - drivers/i2c/busses/i2c-mv64xxx.c | 5 - drivers/i2c/busses/i2c-pca-platform.c | 1 - 5 files changed, 9 deletions(-) diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index 722f839cfa3c..13fda39acffe 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -748,7 +748,6 @@ static int iic_probe(struct platform_device *ofdev) i2c_set_adapdata(adap, dev); adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD; adap->algo = &iic_algo; - adap->timeout = HZ; ret = i2c_add_adapter(adap); if (ret < 0) { diff --git a/drivers/i2c/busses/i2c-iop3xx.c b/drivers/i2c/busses/i2c-iop3xx.c index 72d6161cf77c..c4ed8281bec2 100644 --- a/drivers/i2c/busses/i2c-iop3xx.c +++ b/drivers/i2c/busses/i2c-iop3xx.c @@ -479,7 +479,6 @@ iop3xx_i2c_probe(struct platform_device *pdev) /* * Default values...should these come in from board code? */ - new_adapter->timeout = HZ; new_adapter->algo = &iop3xx_i2c_algo; init_waitqueue_head(&adapter_data->waitq); diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 48ecffecc0ed..b4bb64ffe3cd 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -640,7 +640,6 @@ static const struct i2c_algorithm mpc_algo = { static struct i2c_adapter mpc_ops = { .owner = THIS_MODULE, .algo = &mpc_algo, - .timeout = HZ, }; static const struct of_device_id mpc_i2c_of_match[]; diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index 30059c1df2a3..2640390be5dd 100644 --- a/drivers/i2c/busses/i2c-mv64xxx.c +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -832,11 +832,6 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, reset_control_deassert(drv_data->rstc); } - /* Its not yet defined how timeouts will be specified in device tree. -* So hard code the value to 1 second. -*/ - drv_data->adapter.timeout = HZ; - device = of_match_device(mv64xxx_i2c_of_match_table, dev); if (!device) return -ENODEV; diff --git a/drivers/i2c/busses/i2c-pca-platform.c b/drivers/i2c/busses/i2c-pca-platform.c index 3bd2e7d06e4b..2b930df0e6b5 100644 --- a/drivers/i2c/busses/i2c-pca-platform.c +++ b/drivers/i2c/busses/i2c-pca-platform.c @@ -183,7 +183,6 @@ static int i2c_pca_pf_probe(struct platform_device *pdev) i2c->algo_data.i2c_clock = platform_data->i2c_clock_speed; i2c->gpio = platform_data->gpio; } else { - i2c->adap.timeout = HZ; i2c->algo_data.i2c_clock = 59000; i2c->gpio = -1; } -- 2.5.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 1/6] i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe()
There is no need to clear interrupts in i2c_dw_pci_probe() since only place where interrupts are unmasked is i2c_dw_xfer_init() and there interrupts are always cleared after commit 2a2d95e9d6d2 ("i2c: designware: always clear interrupts before enabling them"). This allows to cleanup the code and replace i2c_dw_clear_int() in i2c_dw_xfer_init() by direct register read as there are no other callers. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-core.c | 8 +--- drivers/i2c/busses/i2c-designware-core.h | 1 - drivers/i2c/busses/i2c-designware-pcidrv.c | 1 - 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 7441cdc1b34a..dec0af7d0471 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -438,7 +438,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) __i2c_dw_enable(dev, true); /* Clear and enable interrupts */ - i2c_dw_clear_int(dev); + dw_readl(dev, DW_IC_CLR_INTR); dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK); } @@ -839,12 +839,6 @@ void i2c_dw_disable(struct dw_i2c_dev *dev) } EXPORT_SYMBOL_GPL(i2c_dw_disable); -void i2c_dw_clear_int(struct dw_i2c_dev *dev) -{ - dw_readl(dev, DW_IC_CLR_INTR); -} -EXPORT_SYMBOL_GPL(i2c_dw_clear_int); - void i2c_dw_disable_int(struct dw_i2c_dev *dev) { dw_writel(dev, 0, DW_IC_INTR_MASK); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 9630222abf32..f44aeeeb91c6 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -122,7 +122,6 @@ extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id); extern void i2c_dw_enable(struct dw_i2c_dev *dev); extern u32 i2c_dw_is_enabled(struct dw_i2c_dev *dev); extern void i2c_dw_disable(struct dw_i2c_dev *dev); -extern void i2c_dw_clear_int(struct dw_i2c_dev *dev); extern void i2c_dw_disable_int(struct dw_i2c_dev *dev); extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev); diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index df23e8c30e6f..e477dddcdae5 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -268,7 +268,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, } i2c_dw_disable_int(dev); - i2c_dw_clear_int(dev); r = i2c_add_numbered_adapter(adap); if (r) { dev_err(&pdev->dev, "failure adding adapter\n"); -- 2.5.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: designware: Remove unused functions
i2c_dw_is_enabled() became unused by the commit be58eda775c8 ("i2c: designware-pci: Cleanup driver power management") and i2c_dw_enable() by the commit 3a48d1c08fe0 ("i2c: prevent spurious interrupt on Designware controllers"). Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-core.c | 13 - drivers/i2c/busses/i2c-designware-core.h | 2 -- 2 files changed, 15 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index dec0af7d0471..c9e93a76a455 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -815,19 +815,6 @@ tx_aborted: } EXPORT_SYMBOL_GPL(i2c_dw_isr); -void i2c_dw_enable(struct dw_i2c_dev *dev) -{ - /* Enable the adapter */ - __i2c_dw_enable(dev, true); -} -EXPORT_SYMBOL_GPL(i2c_dw_enable); - -u32 i2c_dw_is_enabled(struct dw_i2c_dev *dev) -{ - return dw_readl(dev, DW_IC_ENABLE); -} -EXPORT_SYMBOL_GPL(i2c_dw_is_enabled); - void i2c_dw_disable(struct dw_i2c_dev *dev) { /* Disable controller */ diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index f44aeeeb91c6..10c79038834d 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -119,8 +119,6 @@ extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num); extern u32 i2c_dw_func(struct i2c_adapter *adap); extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id); -extern void i2c_dw_enable(struct dw_i2c_dev *dev); -extern u32 i2c_dw_is_enabled(struct dw_i2c_dev *dev); extern void i2c_dw_disable(struct dw_i2c_dev *dev); extern void i2c_dw_disable_int(struct dw_i2c_dev *dev); extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev); -- 2.5.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/6] i2c: designware: Code duplication removal and cleanups
It bothered to me to see "static struct i2c_algorithm i2c_dw_algo {}" defined twice both in i2c-designware-pcidrv.c and i2c-designware-platdrv.c and so many exported i2c-designware-core functions. It turned out some of them became unused or are local and there were also duplicated probe code that I moved to new common i2c_dw_probe(). Object sizes below before and after this set from CONFIG_X86_64=y build. textdata bss dec hex filename 64391096 075351d6f drivers/i2c/busses/i2c-designware-core.ko 51231588 1667271a47 drivers/i2c/busses/i2c-designware-pci.ko 52741096 16638618f2 drivers/i2c/busses/i2c-designware-platform.ko 168363780 32 2064850a8 (TOTALS) textdata bss dec hex filename 72251120 16836120a9 drivers/i2c/busses/i2c-designware-core.ko 42811524 0580516ad drivers/i2c/busses/i2c-designware-pci.ko 43371072 054091521 drivers/i2c/busses/i2c-designware-platform.ko 158433716 16 195754c77 (TOTALS) Jarkko Nikula (6): i2c: designware: Remove interrupt clearing from i2c_dw_pci_probe() i2c: designware: Disable interrupts before requesting PCI device interrupt i2c: designware: Remove unused functions i2c: designware: Make dw_readl() and dw_writel() static i2c: designware: Rename platform driver probe and PM functions i2c: designware: Move common probe code into i2c_dw_probe() drivers/i2c/busses/i2c-designware-core.c| 73 ++--- drivers/i2c/busses/i2c-designware-core.h| 10 +--- drivers/i2c/busses/i2c-designware-pcidrv.c | 31 ++-- drivers/i2c/busses/i2c-designware-platdrv.c | 52 ++-- 4 files changed, 63 insertions(+), 103 deletions(-) -- 2.5.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: designware: Rename platform driver probe and PM functions
Make it easier to distinguish between i2c-designware-platdrv and i2c-designware-core functions and to be consistent with i2c-designware-pcidrv. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-platdrv.c | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 3dd2de31a2f8..17167cd4812d 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -133,7 +133,7 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev) static inline void dw_i2c_acpi_unconfigure(struct platform_device *pdev) { } #endif -static int dw_i2c_probe(struct platform_device *pdev) +static int dw_i2c_plat_probe(struct platform_device *pdev) { struct dw_i2c_dev *dev; struct i2c_adapter *adap; @@ -271,7 +271,7 @@ static int dw_i2c_probe(struct platform_device *pdev) return 0; } -static int dw_i2c_remove(struct platform_device *pdev) +static int dw_i2c_plat_remove(struct platform_device *pdev) { struct dw_i2c_dev *dev = platform_get_drvdata(pdev); @@ -300,12 +300,12 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match); #endif #ifdef CONFIG_PM_SLEEP -static int dw_i2c_prepare(struct device *dev) +static int dw_i2c_plat_prepare(struct device *dev) { return pm_runtime_suspended(dev); } -static void dw_i2c_complete(struct device *dev) +static void dw_i2c_plat_complete(struct device *dev) { if (dev->power.direct_complete) pm_request_resume(dev); @@ -316,7 +316,7 @@ static void dw_i2c_complete(struct device *dev) #endif #ifdef CONFIG_PM -static int dw_i2c_suspend(struct device *dev) +static int dw_i2c_plat_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); @@ -327,7 +327,7 @@ static int dw_i2c_suspend(struct device *dev) return 0; } -static int dw_i2c_resume(struct device *dev) +static int dw_i2c_plat_resume(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); @@ -341,10 +341,10 @@ static int dw_i2c_resume(struct device *dev) } static const struct dev_pm_ops dw_i2c_dev_pm_ops = { - .prepare = dw_i2c_prepare, - .complete = dw_i2c_complete, - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_suspend, dw_i2c_resume) - SET_RUNTIME_PM_OPS(dw_i2c_suspend, dw_i2c_resume, NULL) + .prepare = dw_i2c_plat_prepare, + .complete = dw_i2c_plat_complete, + SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume) + SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL) }; #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops) @@ -356,8 +356,8 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = { MODULE_ALIAS("platform:i2c_designware"); static struct platform_driver dw_i2c_driver = { - .probe = dw_i2c_probe, - .remove = dw_i2c_remove, + .probe = dw_i2c_plat_probe, + .remove = dw_i2c_plat_remove, .driver = { .name = "i2c_designware", .of_match_table = of_match_ptr(dw_i2c_of_match), -- 2.5.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 6/6] i2c: designware: Move common probe code into i2c_dw_probe()
There is some code duplication in i2c-designware-platdrv and i2c-designware-pcidrv probe functions. What is even worse that duplication requires i2c_dw_xfer(), i2c_dw_func() and i2c_dw_isr() i2c-designware-core functions to be exported. Therefore move common code into new i2c_dw_probe() and make functions above local to i2c-designware-core. While merging the code patch does following functional changes: - I2C Adapter name will be "i2c-designware". Previously adapter name was "Synopsys DesignWare I2C adapter" for platform and ACPI devices and "i2c-designware-pci" for PCI devices. - Interrupt name too will be "i2c-designware". Previous it was platform device name, ACPI device name or "i2c-designware-pci". - Error code from devm_request_irq() and i2c_add_numbered_adapter() will be printed in case of error. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-core.c| 48 + drivers/i2c/busses/i2c-designware-core.h| 5 +-- drivers/i2c/busses/i2c-designware-pcidrv.c | 30 ++ drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++--- 4 files changed, 48 insertions(+), 63 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 52272a01c679..501c8ac0cf14 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -618,7 +618,7 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev) /* * Prepare controller for a transaction and call i2c_dw_xfer_msg */ -int +static int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) { struct dw_i2c_dev *dev = i2c_get_adapdata(adap); @@ -702,14 +702,17 @@ done_nolock: return ret; } -EXPORT_SYMBOL_GPL(i2c_dw_xfer); -u32 i2c_dw_func(struct i2c_adapter *adap) +static u32 i2c_dw_func(struct i2c_adapter *adap) { struct dw_i2c_dev *dev = i2c_get_adapdata(adap); return dev->functionality; } -EXPORT_SYMBOL_GPL(i2c_dw_func); + +static struct i2c_algorithm i2c_dw_algo = { + .master_xfer= i2c_dw_xfer, + .functionality = i2c_dw_func, +}; static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev) { @@ -770,7 +773,7 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev) * Interrupt service routine. This gets called whenever an I2C interrupt * occurs. */ -irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) { struct dw_i2c_dev *dev = dev_id; u32 stat, enabled; @@ -813,7 +816,6 @@ tx_aborted: return IRQ_HANDLED; } -EXPORT_SYMBOL_GPL(i2c_dw_isr); void i2c_dw_disable(struct dw_i2c_dev *dev) { @@ -838,5 +840,39 @@ u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev) } EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param); +int i2c_dw_probe(struct dw_i2c_dev *dev) +{ + struct i2c_adapter *adap = &dev->adapter; + int r; + + init_completion(&dev->cmd_complete); + mutex_init(&dev->lock); + + r = i2c_dw_init(dev); + if (r) + return r; + + snprintf(adap->name, sizeof(adap->name), "i2c-designware"); + adap->algo = &i2c_dw_algo; + adap->dev.parent = dev->dev; + i2c_set_adapdata(adap, dev); + + i2c_dw_disable_int(dev); + r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED, +adap->name, dev); + if (r) { + dev_err(dev->dev, "failure requesting irq %i: %d\n", + dev->irq, r); + return r; + } + + r = i2c_add_numbered_adapter(adap); + if (r) + dev_err(dev->dev, "failure adding adapter: %d\n", r); + + return r; +} +EXPORT_SYMBOL_GPL(i2c_dw_probe); + MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core"); MODULE_LICENSE("GPL"); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 0e73b8672402..1d50898e7b24 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -113,13 +113,10 @@ struct dw_i2c_dev { #define ACCESS_16BIT 0x0002 extern int i2c_dw_init(struct dw_i2c_dev *dev); -extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], - int num); -extern u32 i2c_dw_func(struct i2c_adapter *adap); -extern irqreturn_t i2c_dw_isr(int this_irq, void *dev_id); extern void i2c_dw_disable(struct dw_i2c_dev *dev); extern void i2c_dw_disable_int(struct dw_i2c_dev *dev); extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev); +extern int i2c_dw_probe(struct dw_i2c_dev *dev); #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL) extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev); diff --git a/drivers/i2c/buss
[PATCH 4/6] i2c: designware: Make dw_readl() and dw_writel() static
dw_readl() and dw_writel() are not used outside of i2c-designware-core and they are not exported so make them static and remove their forward declaration. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-core.c | 4 ++-- drivers/i2c/busses/i2c-designware-core.h | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index c9e93a76a455..52272a01c679 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -165,7 +165,7 @@ static char *abort_sources[] = { "lost arbitration", }; -u32 dw_readl(struct dw_i2c_dev *dev, int offset) +static u32 dw_readl(struct dw_i2c_dev *dev, int offset) { u32 value; @@ -181,7 +181,7 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset) return value; } -void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset) +static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset) { if (dev->accessor_flags & ACCESS_SWAP) b = swab32(b); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 10c79038834d..0e73b8672402 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -112,8 +112,6 @@ struct dw_i2c_dev { #define ACCESS_SWAP0x0001 #define ACCESS_16BIT 0x0002 -extern u32 dw_readl(struct dw_i2c_dev *dev, int offset); -extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); extern int i2c_dw_init(struct dw_i2c_dev *dev); extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num); -- 2.5.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: designware: Disable interrupts before requesting PCI device interrupt
Device must not generate interrupts before registering the interrupt handler so move i2c_dw_disable_int() before requesting it. There are no known issues with this. The code has been here since commit fe20ff5c7e9c ("i2c-designware: Add support for Designware core behind PCI devices."). Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-pcidrv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index e477dddcdae5..d9e8937f062f 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -260,6 +260,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci"); + i2c_dw_disable_int(dev); r = devm_request_irq(&pdev->dev, pdev->irq, i2c_dw_isr, IRQF_SHARED | IRQF_COND_SUSPEND, adap->name, dev); if (r) { @@ -267,7 +268,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, return r; } - i2c_dw_disable_int(dev); r = i2c_add_numbered_adapter(adap); if (r) { dev_err(&pdev->dev, "failure adding adapter\n"); -- 2.5.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: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves
On 08/25/2015 08:03 AM, Dustin Byford wrote: On Mon Aug 24 13:52, Jarkko Nikula wrote: I don't know how common ACPI enumerated I2C hwmon devices are but I guess they exists since Dustin notices this issue and wrote "With that change, /sys/bus/i2c/devices/i2c-0-004a, for example, became /sys/bus/i2c/devices/i2c-PNP:xx". I wouldn't take my particular use case as evidence they are common, or even they really exist. I got here because I loaded a hwmon driver using the _DSD/PRP0001 mechanism in ACPI. In that case, those devices have never worked with 'sensors' Therefore, in that context, this change is just fixing a bug. So does this then fall more into new use case rather than being a regression as such? What I'm trying to understand what kind of regression we actually do have here? Is it about listing the devices or does it also cease to show for instance temperature sensor readings? If it requires to add a match table to existing driver in order to trigger this then at least stable tree is not worry here. -- Jarkko -- 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
[RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore: lib/sysfs.c:665: if ((!subsys || !strcmp(subsys, "i2c")) && sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr, &entry.chip.addr) == 2) { This patch fixes this by reverting back the old device naming in i2c-core but at the same avoids regression to ALSA SoC drivers that depend on stable device binding. Reverted I2C slave device naming is handled in ALSA SoC core by using the ACPI name as a component name if device is ACPI probed. This keeps the component binding in ALSA SoC stable and requires only minimal changes to affected machine drivers. Fixes: 70762abb9f89 i2c: Use stable dev_name for ACPI enumerated I2C slaves Reported-by: Dustin Byford Signed-off-by: Jarkko Nikula --- This is on top of 4.2.0-rc8. I didn't check exact kernel versions where this still applies but I know we would need a few different versions for older stable versions. Mainly because of added and moved drivers in sound/soc/intel/. This is for discussion so I didn't cc sta...@vger.kernel.org yet. I was thinking would it work if we'd keep the stable name but have an another symlink in /sys/bus/i2c/devices/ that uses "x-00yz" name. However this feels ill-use of devices directory and probably causes more troubles elsewhere. I don't know how common ACPI enumerated I2C hwmon devices are but I guess they exists since Dustin notices this issue and wrote "With that change, /sys/bus/i2c/devices/i2c-0-004a, for example, became /sys/bus/i2c/devices/i2c-PNP:xx". It could be possible there is use for the new device naming. I found a comment from Documentation/hid/hid-sensor.txt introduced by commit b2eafd7282fd ("HID: sensor: Update document for custom sensor") that seems to indicate current i2c-INTABCD:xy naming may be used from userspace too but I didn't find examples of that. --- drivers/i2c/i2c-core.c | 21 - sound/soc/intel/boards/broadwell.c | 6 +++--- sound/soc/intel/boards/byt-max98090.c| 2 +- sound/soc/intel/boards/byt-rt5640.c | 2 +- sound/soc/intel/boards/bytcr_rt5640.c| 2 +- sound/soc/intel/boards/cht_bsw_max98090_ti.c | 4 ++-- sound/soc/intel/boards/cht_bsw_rt5645.c | 4 ++-- sound/soc/intel/boards/cht_bsw_rt5672.c | 6 +++--- sound/soc/intel/boards/haswell.c | 2 +- sound/soc/soc-core.c | 10 ++ 10 files changed, 28 insertions(+), 31 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index c83e4d13cfc5..e9c227b9dc92 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -913,22 +913,6 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter); -static void i2c_dev_set_name(struct i2c_adapter *adap, -struct i2c_client *client) -{ - struct acpi_device *adev = ACPI_COMPANION(&client->dev); - - if (adev) { - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); - return; - } - - /* For 10-bit clients, add an arbitrary offset to avoid collisions */ - dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), -client->addr | ((client->flags & I2C_CLIENT_TEN) -? 0xa000 : 0)); -} - /** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device @@ -987,7 +971,10 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; client->dev.fwnode = info->fwnode; - i2c_dev_set_name(adap, client); + /* For 10-bit clients, add an arbitrary offset to avoid collisions */ + dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), +client->addr | ((client->flags & I2C_CLIENT_TEN) +? 0xa000 : 0)); status = device_register(&client->dev); if (status) goto out_err; diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c index 8bafaf6ceab1..3abcf0d7682e 100644 --- a/sound/soc/intel/boards/broadwell.c +++ b/sound/soc/intel/boards/broadwell.c @@ -205,7 +205,7 @@ static struct snd_soc_dai_link broadwell_rt286_dais[] = { .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1,
Re: [RFC 0/1] i2c: acpi: revert setting a "stable" device name
On 15.08.2015 18:13, Guenter Roeck wrote: On Fri, Aug 14, 2015 at 12:37:13PM -0700, Dustin Byford wrote: 70762ab from 11/2014 (i2c: Use stable dev_name for ACPI enumerated I2C slaves) modified the sysfs-visible dev_name() for ACPI enumerated I2C devices. With that change, /sys/bus/i2c/devices/i2c-0-004a, for example, became /sys/bus/i2c/devices/i2c-PNP:xx That causes problems for userspace code such as 'sensors' which does this: lib/sysfs.c:665: if ((!subsys || !strcmp(subsys, "i2c")) && sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr, &entry.chip.addr) == 2) { ... Therefore, in theory, sensors that were previously visible by running 'sensors' no longer show up. On the other hand, there are probably few, if any, cases of this because ACPI enumerated I2C hwmon devices are not common. I'm not defending the 'sensors' code, I'm sure there are better ways to discover a hwmon I2C device from userspace. But, I'm also not sure Is it necessary to defend user space applications nowadays if a kernel change breaks a well established ABI ? WHat happened to "Thou Shalt Not Break Userspace" ? I absolutely agree that i2c bus renumbering across reboots is a problem. However, it seems to me that 70762ab doesn't solve that problem, it just paints it over. And, as you have noticed, it introduces new problems along the way. This indeed is unwanted and needs to be fixed. Looking at the lib/sysfs.c also SPI chip detection breaks because of commit e13ac47bec20 ("spi: Use stable dev_name for ACPI enumerated SPI slaves"). However plain revert won't work as it breaks those ALSA SoC drivers that depend on this. Good thing is that matching there is purely kernel space and at quick look "git grep '"i2c-.*:[0-9]'" and "git grep '"spi-.*:[0-9]'" don't find other in-kernel users. 70762ab achieved its stated goal in a meaningful way. Won't "i2c-" also vary with ACPI scan order, BIOS settings, firmware upgrades, etc...? If I remember correctly ACPI ID should not ever change and instance id :xy after INTABCD:xy should also be visible and keep the order even if device is disabled or not plugged. But I'm not absolute sure about this. At least on a test platform that allow disable devices will show those devices off-line (/sys/bus/acpi/devices/INTABCD:xy/status == 0). -- Jarkko -- 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: designware: Make debug print in i2c_dw_isr() shorter
Printing adapter name is irrelevant from this debug print and makes output needlessly long. Having already device and functions names printed here is enough for debugging. While at it remove extra space from "enabled= 0x" and use "%#x" for printing "0x" prefixed hexadecimal values. Signed-off-by: Jarkko Nikula --- This is my previous "[PATCH 1/2] i2c: designware: Remove space between variable and its value in debug print" and "[PATCH 2/2] i2c: designware: Remove adapter name from debug print in i2c_dw_isr()" squashed. --- drivers/i2c/busses/i2c-designware-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 6f19a33773fe..cc98b2a75591 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -777,8 +777,8 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) enabled = dw_readl(dev, DW_IC_ENABLE); stat = dw_readl(dev, DW_IC_RAW_INTR_STAT); - dev_dbg(dev->dev, "%s: %s enabled= 0x%x stat=0x%x\n", __func__, - dev->adapter.name, enabled, stat); + dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, + enabled, stat); if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY)) return IRQ_NONE; -- 2.4.6 -- 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: designware: Remove adapter name from debug print in i2c_dw_isr()
On 07.08.2015 14:10, Wolfram Sang wrote: On Fri, Aug 07, 2015 at 01:43:28PM +0300, Jarkko Nikula wrote: Printing adapter name is irrelevant from this debug print and makes output needlessly long. Having already device and functions names printed here is enough for debugging. Signed-off-by: Jarkko Nikula So much about the double space :) I think I'll squash those two. Fine to me. Actually long "Synopsys DesignWare I2C adapter" printed bothered me most. -- Jarkko -- 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] i2c: designware: Remove space between variable and its value in debug print
Remove extra space from "enabled= 0x" to make debug print a bit more dense and to be in sync with "stat=0x". While at it, use "%#x" for printing "0x" prefixed hexadecimal values. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 6f19a33773fe..7310051d4a50 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -777,7 +777,7 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) enabled = dw_readl(dev, DW_IC_ENABLE); stat = dw_readl(dev, DW_IC_RAW_INTR_STAT); - dev_dbg(dev->dev, "%s: %s enabled= 0x%x stat=0x%x\n", __func__, + dev_dbg(dev->dev, "%s: %s enabled=%#x stat=%#x\n", __func__, dev->adapter.name, enabled, stat); if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY)) return IRQ_NONE; -- 2.4.6 -- 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: designware: Remove adapter name from debug print in i2c_dw_isr()
Printing adapter name is irrelevant from this debug print and makes output needlessly long. Having already device and functions names printed here is enough for debugging. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 7310051d4a50..cc98b2a75591 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -777,8 +777,8 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) enabled = dw_readl(dev, DW_IC_ENABLE); stat = dw_readl(dev, DW_IC_RAW_INTR_STAT); - dev_dbg(dev->dev, "%s: %s enabled=%#x stat=%#x\n", __func__, - dev->adapter.name, enabled, stat); + dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, + enabled, stat); if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY)) return IRQ_NONE; -- 2.4.6 -- 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] i2c: core: Remove needless structure member zero initialization
No need to clear one struct i2c_client member variable since memset has already cleared all of them. Remove also one space intendation from err label. Signed-off-by: Jarkko Nikula --- drivers/i2c/i2c-core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 987c124432c5..051aa3a811a8 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -283,7 +283,6 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command, memset(&client, 0, sizeof(client)); client.adapter = adapter; client.addr = sb->slave_address; - client.flags = 0; if (sb->access_mode == ACPI_I2C_10BIT_MODE) client.flags |= I2C_CLIENT_TEN; @@ -361,7 +360,7 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command, gsb->status = status; - err: +err: ACPI_FREE(ares); return ret; } -- 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
[PATCHv3 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation
Simplifies the code a bit and makes easier to disable PCI device on driver detach by removing the pcim_pin_device() call in the future if needed. Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it made some systems to hang during power-off. See commit d6fcb3b9cf77 ("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled") and http://marc.info/?l=linux-kernel&m=115160053309535&w=2 Signed-off-by: Jarkko Nikula --- Changes from v2: - over 80 characters long pcim_iomap_regions line splitted - gotos and error labels removed --- drivers/i2c/busses/i2c-i801.c | 25 + 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 5fb35464f693..5ecbb3fdc27e 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1180,35 +1180,35 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) } priv->features &= ~disable_features; - err = pci_enable_device(dev); + err = pcim_enable_device(dev); if (err) { dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n", err); - goto exit; + return err; } + pcim_pin_device(dev); /* Determine the address of the SMBus area */ priv->smba = pci_resource_start(dev, SMBBAR); if (!priv->smba) { dev_err(&dev->dev, "SMBus base address uninitialized, upgrade BIOS\n"); - err = -ENODEV; - goto exit; + return -ENODEV; } err = acpi_check_resource_conflict(&dev->resource[SMBBAR]); if (err) { - err = -ENODEV; - goto exit; + return -ENODEV; } - err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev)); + err = pcim_iomap_regions(dev, 1 << SMBBAR, +dev_driver_string(&dev->dev)); if (err) { dev_err(&dev->dev, "Failed to request SMBus region 0x%lx-0x%Lx\n", priv->smba, (unsigned long long)pci_resource_end(dev, SMBBAR)); - goto exit; + return err; } pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &temp); @@ -1276,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = i2c_add_adapter(&priv->adapter); if (err) { dev_err(&dev->dev, "Failed to add SMBus adapter\n"); - goto exit_release; + return err; } i801_probe_optional_slaves(priv); @@ -1286,11 +1286,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) pci_set_drvdata(dev, priv); return 0; - -exit_release: - pci_release_region(dev, SMBBAR); -exit: - return err; } static void i801_remove(struct pci_dev *dev) @@ -1301,8 +1296,6 @@ static void i801_remove(struct pci_dev *dev) i2c_del_adapter(&priv->adapter); pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg); - pci_release_region(dev, SMBBAR); - /* * do not call pci_disable_device(dev) since it can cause hard hangs on * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010) -- 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
[PATCHv3 2/5] i2c: i801: Remove i801_driver forward declaration
struct pci_driver i801_driver forward declaration is needed only for accessing the name field. Remove it and use dev_driver_string() instead. Signed-off-by: Jarkko Nikula Reviewed-by: Jean Delvare --- drivers/i2c/busses/i2c-i801.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 7d1f4a478c54..5f827dfc671a 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -223,8 +223,6 @@ struct i801_priv { #endif }; -static struct pci_driver i801_driver; - #define FEATURE_SMBUS_PEC (1 << 0) #define FEATURE_BLOCK_BUFFER (1 << 1) #define FEATURE_BLOCK_PROC (1 << 2) @@ -1204,7 +1202,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) goto exit; } - err = pci_request_region(dev, SMBBAR, i801_driver.name); + err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev)); if (err) { dev_err(&dev->dev, "Failed to request SMBus region 0x%lx-0x%Lx\n", @@ -1256,7 +1254,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) init_waitqueue_head(&priv->waitq); err = request_irq(dev->irq, i801_isr, IRQF_SHARED, - i801_driver.name, priv); + dev_driver_string(&dev->dev), priv); if (err) { dev_err(&dev->dev, "Failed to allocate irq %d: %d\n", dev->irq, err); -- 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
[PATCHv3 1/5] i2c: i801: Don't break user-visible strings
It makes more difficult to grep these error prints from sources if they are split to multiple source lines. Signed-off-by: Jarkko Nikula Reviewed-by: Jean Delvare --- drivers/i2c/busses/i2c-i801.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 8fafb254e42a..7d1f4a478c54 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1192,8 +1192,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) /* Determine the address of the SMBus area */ priv->smba = pci_resource_start(dev, SMBBAR); if (!priv->smba) { - dev_err(&dev->dev, "SMBus base address uninitialized, " - "upgrade BIOS\n"); + dev_err(&dev->dev, + "SMBus base address uninitialized, upgrade BIOS\n"); err = -ENODEV; goto exit; } @@ -1206,8 +1206,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = pci_request_region(dev, SMBBAR, i801_driver.name); if (err) { - dev_err(&dev->dev, "Failed to request SMBus region " - "0x%lx-0x%Lx\n", priv->smba, + dev_err(&dev->dev, + "Failed to request SMBus region 0x%lx-0x%Lx\n", + priv->smba, (unsigned long long)pci_resource_end(dev, SMBBAR)); goto exit; } -- 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
[PATCHv3 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()
Since pci_disable_device() is not called from i801_suspend() and power state is set already it means that subsequent pci_enable_device() calls do practically nothing but monotonically increase struct pci_dev enable_cnt. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index b1d725d758bb..5fb35464f693 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev) { pci_set_power_state(dev, PCI_D0); pci_restore_state(dev); - return pci_enable_device(dev); + return 0; } #else #define i801_suspend NULL -- 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
[PATCHv3 3/5] i2c: i801: Use managed devm_* memory and irq allocation
This simplifies the error and remove paths. Signed-off-by: Jarkko Nikula Reviewed-by: Jean Delvare --- drivers/i2c/busses/i2c-i801.c | 17 ++--- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 5f827dfc671a..b1d725d758bb 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) int err, i; struct i801_priv *priv; - priv = kzalloc(sizeof(*priv), GFP_KERNEL); + priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; @@ -1253,8 +1253,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) if (priv->features & FEATURE_IRQ) { init_waitqueue_head(&priv->waitq); - err = request_irq(dev->irq, i801_isr, IRQF_SHARED, - dev_driver_string(&dev->dev), priv); + err = devm_request_irq(&dev->dev, dev->irq, i801_isr, + IRQF_SHARED, + dev_driver_string(&dev->dev), priv); if (err) { dev_err(&dev->dev, "Failed to allocate irq %d: %d\n", dev->irq, err); @@ -1275,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = i2c_add_adapter(&priv->adapter); if (err) { dev_err(&dev->dev, "Failed to add SMBus adapter\n"); - goto exit_free_irq; + goto exit_release; } i801_probe_optional_slaves(priv); @@ -1286,12 +1287,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) return 0; -exit_free_irq: - if (priv->features & FEATURE_IRQ) - free_irq(dev->irq, priv); +exit_release: pci_release_region(dev, SMBBAR); exit: - kfree(priv); return err; } @@ -1303,11 +1301,8 @@ static void i801_remove(struct pci_dev *dev) i2c_del_adapter(&priv->adapter); pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg); - if (priv->features & FEATURE_IRQ) - free_irq(dev->irq, priv); pci_release_region(dev, SMBBAR); - kfree(priv); /* * do not call pci_disable_device(dev) since it can cause hard hangs on * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010) -- 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: [PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation
Hi On 02/13/2015 12:47 PM, Jean Delvare wrote: Hi Jarkko, On Wed, 11 Feb 2015 14:32:08 +0200, Jarkko Nikula wrote: Simplifies the code a bit and makes easier to disable PCI device on driver detach by removing the pcim_pin_device() call in the future if needed. Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it made some systems to hang during power-off. See commit d6fcb3b9cf77 ("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled") and http://marc.info/?l=linux-kernel&m=115160053309535&w=2 Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) The checkpatch script complains: WARNING: line over 80 characters #90: FILE: drivers/i2c/busses/i2c-i801.c:1206: + err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev)); Ah, I was blind to see this. Will fix. diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 5fb35464f693..9f7b69743233 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) } priv->features &= ~disable_features; - err = pci_enable_device(dev); + err = pcim_enable_device(dev); if (err) { dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n", err); goto exit; } + pcim_pin_device(dev); /* Determine the address of the SMBus area */ priv->smba = pci_resource_start(dev, SMBBAR); What is the benefit of this change, compared to just leaving the call to pci_enable_device() in place? If you mean the patch itself I think easy pcim_pin_device() removal is the biggest benefit if we ever want to experiment does hang during power-off problem still exist in some machines. All PCI, ACPI and power-off code have evolved in 9 years so original problem may not exist anymore. Who knows. Without this patch pcm_disable_device() needs to be added to error paths and i801_remove(). (of course trivial but it's always more nice to get regression from one-liner). If you mean plain pcim_enable_device() it is required for other pcim_ functions since it allocates pcim_release which takes care of cleanup bunch of things including pci_release_region(). E.g. pcim_iomap_regions() without pcim_enable_device() causes an oops when doing "cat /proc/ioports" after module removal since pcim_iomap_release() only unmaps but does not release regions. @@ -1202,7 +1203,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) goto exit; } - err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev)); + err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev)); if (err) { dev_err(&dev->dev, "Failed to request SMBus region 0x%lx-0x%Lx\n", @@ -1276,7 +1277,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = i2c_add_adapter(&priv->adapter); if (err) { dev_err(&dev->dev, "Failed to add SMBus adapter\n"); - goto exit_release; + goto exit; } i801_probe_optional_slaves(priv); @@ -1287,8 +1288,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) return 0; -exit_release: - pci_release_region(dev, SMBBAR); exit: return err; } Now that the exit path is empty, wouldn't it make sense to return directly on error? My understanding is that this is one of the benefits of managed device resources. Yes it does. Will fix too. -- Jarkko -- 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: [PATCHv2 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()
Hi On 02/13/2015 12:33 PM, Jean Delvare wrote: Hi Jarkko, On Wed, 11 Feb 2015 14:32:07 +0200, Jarkko Nikula wrote: Since pci_disable_device() is not called from i801_suspend() and power state is set already it means that subsequent pci_enable_device() calls do practically nothing but monotonically increase struct pci_dev enable_cnt. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index b1d725d758bb..5fb35464f693 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev) { pci_set_power_state(dev, PCI_D0); pci_restore_state(dev); - return pci_enable_device(dev); + return 0; } #else #define i801_suspend NULL This looks reasonable but have you tested this change on a range of actual laptops to make sure it has no unexpected side effect? Unfortunately I have only limited amount of test hw which are working fine even if PCI device is disabled so I cannot hit those issues that were seen in the past. I suppose this patch unlikely cause regression since if you look at the call chain pci_enable_device() -> pci_enable_device_flags() it doesn't go beyond taking the current power state since enable_cnt is always greater than 1. drivers/pci/pci.c: pci_enable_device_flags(): if (dev->pm_cap) { u16 pmcsr; pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr); dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK); } if (atomic_inc_return(&dev->enable_cnt) > 1) return 0; /* already enabled */ To me it seems current power state taking is practically no-op when device is enabled since pci_set_power_state() calls in i801_suspend() and i801_resume() have already cached it. -- Jarkko -- 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
[PATCHv2 2/5] i2c: i801: Remove i801_driver forward declaration
struct pci_driver i801_driver forward declaration is needed only for accessing the name field. Remove it and use dev_driver_string() instead. Signed-off-by: Jarkko Nikula Reviewed-by: Jean Delvare --- drivers/i2c/busses/i2c-i801.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 7d1f4a478c54..5f827dfc671a 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -223,8 +223,6 @@ struct i801_priv { #endif }; -static struct pci_driver i801_driver; - #define FEATURE_SMBUS_PEC (1 << 0) #define FEATURE_BLOCK_BUFFER (1 << 1) #define FEATURE_BLOCK_PROC (1 << 2) @@ -1204,7 +1202,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) goto exit; } - err = pci_request_region(dev, SMBBAR, i801_driver.name); + err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev)); if (err) { dev_err(&dev->dev, "Failed to request SMBus region 0x%lx-0x%Lx\n", @@ -1256,7 +1254,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) init_waitqueue_head(&priv->waitq); err = request_irq(dev->irq, i801_isr, IRQF_SHARED, - i801_driver.name, priv); + dev_driver_string(&dev->dev), priv); if (err) { dev_err(&dev->dev, "Failed to allocate irq %d: %d\n", dev->irq, err); -- 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
[PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation
Simplifies the code a bit and makes easier to disable PCI device on driver detach by removing the pcim_pin_device() call in the future if needed. Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it made some systems to hang during power-off. See commit d6fcb3b9cf77 ("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled") and http://marc.info/?l=linux-kernel&m=115160053309535&w=2 Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 5fb35464f693..9f7b69743233 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) } priv->features &= ~disable_features; - err = pci_enable_device(dev); + err = pcim_enable_device(dev); if (err) { dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n", err); goto exit; } + pcim_pin_device(dev); /* Determine the address of the SMBus area */ priv->smba = pci_resource_start(dev, SMBBAR); @@ -1202,7 +1203,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) goto exit; } - err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev)); + err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev)); if (err) { dev_err(&dev->dev, "Failed to request SMBus region 0x%lx-0x%Lx\n", @@ -1276,7 +1277,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = i2c_add_adapter(&priv->adapter); if (err) { dev_err(&dev->dev, "Failed to add SMBus adapter\n"); - goto exit_release; + goto exit; } i801_probe_optional_slaves(priv); @@ -1287,8 +1288,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) return 0; -exit_release: - pci_release_region(dev, SMBBAR); exit: return err; } @@ -1301,8 +1300,6 @@ static void i801_remove(struct pci_dev *dev) i2c_del_adapter(&priv->adapter); pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg); - pci_release_region(dev, SMBBAR); - /* * do not call pci_disable_device(dev) since it can cause hard hangs on * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010) -- 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
[PATCHv2 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()
Since pci_disable_device() is not called from i801_suspend() and power state is set already it means that subsequent pci_enable_device() calls do practically nothing but monotonically increase struct pci_dev enable_cnt. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index b1d725d758bb..5fb35464f693 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev) { pci_set_power_state(dev, PCI_D0); pci_restore_state(dev); - return pci_enable_device(dev); + return 0; } #else #define i801_suspend NULL -- 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
[PATCHv2 0/5] i2c: i801: Few cleanups
Hi Jean Patches 2-3/5 are otherwise the same I sent last week but now on top of 1/5. Jarkko Nikula (5): i2c: i801: Don't break user-visible strings i2c: i801: Remove i801_driver forward declaration i2c: i801: Use managed devm_* memory and irq allocation i2c: i801: Remove pci_enable_device() call from i801_resume() i2c: i801: Use managed pcim_* PCI device initialization and reservation drivers/i2c/busses/i2c-i801.c | 37 ++--- 1 file changed, 14 insertions(+), 23 deletions(-) -- 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
[PATCHv2 3/5] i2c: i801: Use managed devm_* memory and irq allocation
This simplifies the error and remove paths. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 17 ++--- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 5f827dfc671a..b1d725d758bb 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) int err, i; struct i801_priv *priv; - priv = kzalloc(sizeof(*priv), GFP_KERNEL); + priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; @@ -1253,8 +1253,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) if (priv->features & FEATURE_IRQ) { init_waitqueue_head(&priv->waitq); - err = request_irq(dev->irq, i801_isr, IRQF_SHARED, - dev_driver_string(&dev->dev), priv); + err = devm_request_irq(&dev->dev, dev->irq, i801_isr, + IRQF_SHARED, + dev_driver_string(&dev->dev), priv); if (err) { dev_err(&dev->dev, "Failed to allocate irq %d: %d\n", dev->irq, err); @@ -1275,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = i2c_add_adapter(&priv->adapter); if (err) { dev_err(&dev->dev, "Failed to add SMBus adapter\n"); - goto exit_free_irq; + goto exit_release; } i801_probe_optional_slaves(priv); @@ -1286,12 +1287,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) return 0; -exit_free_irq: - if (priv->features & FEATURE_IRQ) - free_irq(dev->irq, priv); +exit_release: pci_release_region(dev, SMBBAR); exit: - kfree(priv); return err; } @@ -1303,11 +1301,8 @@ static void i801_remove(struct pci_dev *dev) i2c_del_adapter(&priv->adapter); pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg); - if (priv->features & FEATURE_IRQ) - free_irq(dev->irq, priv); pci_release_region(dev, SMBBAR); - kfree(priv); /* * do not call pci_disable_device(dev) since it can cause hard hangs on * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010) -- 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
[PATCHv2 1/5] i2c: i801: Don't break user-visible strings
It makes more difficult to grep these error prints from sources if they are split to multiple source lines. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 8fafb254e42a..7d1f4a478c54 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1192,8 +1192,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) /* Determine the address of the SMBus area */ priv->smba = pci_resource_start(dev, SMBBAR); if (!priv->smba) { - dev_err(&dev->dev, "SMBus base address uninitialized, " - "upgrade BIOS\n"); + dev_err(&dev->dev, + "SMBus base address uninitialized, upgrade BIOS\n"); err = -ENODEV; goto exit; } @@ -1206,8 +1206,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = pci_request_region(dev, SMBBAR, i801_driver.name); if (err) { - dev_err(&dev->dev, "Failed to request SMBus region " - "0x%lx-0x%Lx\n", priv->smba, + dev_err(&dev->dev, + "Failed to request SMBus region 0x%lx-0x%Lx\n", + priv->smba, (unsigned long long)pci_resource_end(dev, SMBBAR)); goto exit; } -- 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 v2] i2c: designware: fixup return handling of wait_for_completion_timeout
On 02/10/2015 10:11 AM, Nicholas Mc Guire wrote: return type of wait_for_completion_timeout is unsigned long not int, rather than introducing a new variable the wait_for_completion_timeout is moved into the if condition as the return value is only used to detect timeout. Signed-off-by: Nicholas Mc Guire --- v2: Aside from the newly added variable having the wrong type (...) the preferred solution is to simply wrap wait_for_completion_timeout into the condition as the remaining jiffies is actually not used - suggested be Jarkko Nikula cmd_complete, HZ); - if (ret == 0) { + if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) { dev_err(dev->dev, "controller timed out\n"); /* i2c_dw_init implicitly disables the adapter */ i2c_dw_init(dev); Reviewed-by: Jarkko Nikula -- 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-designware: fixup of wait_for_completion_timeout return handling
Hi On 02/09/2015 06:35 PM, Nicholas Mc Guire wrote: return type of wait_for_completion_timeout is unsigned long not int. An appropriate return variable is introduced and assignment fixed up. Signed-off-by: Nicholas Mc Guire --- Patch was compile tested with x86_64_defconfig + CONFIG_X86_AMD_PLATFORM_DEVICE=y, CONFIG_I2C_DESIGNWARE_PLATFORM=m (implies CONFIG_I2C_DESIGNWARE_CORE=y) Patch is against 3.19.0-rc7 (localversion-next is -next-20150209) drivers/i2c/busses/i2c-designware-core.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 6e25c01..05934e0 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -623,6 +623,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) { struct dw_i2c_dev *dev = i2c_get_adapdata(adap); int ret; + unsigned int timeout; Not unsigned long. @@ -656,8 +657,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) i2c_dw_xfer_init(dev); /* wait for tx to complete */ - ret = wait_for_completion_timeout(&dev->cmd_complete, HZ); - if (ret == 0) { + timeout = wait_for_completion_timeout(&dev->cmd_complete, HZ); + if (timeout == 0) { dev_err(dev->dev, "controller timed out\n"); /* i2c_dw_init implicitly disables the adapter */ i2c_dw_init(dev); I'd say better to test directly with "if (!wait_for_completion_timeout())" since remaining jiffies or potential error code from wait_for_completion_timeout() is not used here. -- Jarkko -- 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: i801: Use managed devm_* memory and irq allocation
On 02/06/2015 04:51 PM, Jean Delvare wrote: Hi Jarkko, On Fri, 6 Feb 2015 15:56:53 +0200, Jarkko Nikula wrote: This simplifies the error and remove paths. Signed-off-by: Jarkko Nikula --- Looks good, but what about also converting pci_request_region to devm_request_region? Yep, in progress but won't finish today so wanted to send these two first :-) -- Jarkko -- 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: i801: Use managed devm_* memory and irq allocation
This simplifies the error and remove paths. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 17 ++--- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index e31aa987b281..b4369d1835ca 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) int err, i; struct i801_priv *priv; - priv = kzalloc(sizeof(*priv), GFP_KERNEL); + priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; @@ -1252,8 +1252,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) if (priv->features & FEATURE_IRQ) { init_waitqueue_head(&priv->waitq); - err = request_irq(dev->irq, i801_isr, IRQF_SHARED, - dev_driver_string(&dev->dev), priv); + err = devm_request_irq(&dev->dev, dev->irq, i801_isr, + IRQF_SHARED, + dev_driver_string(&dev->dev), priv); if (err) { dev_err(&dev->dev, "Failed to allocate irq %d: %d\n", dev->irq, err); @@ -1274,7 +1275,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) err = i2c_add_adapter(&priv->adapter); if (err) { dev_err(&dev->dev, "Failed to add SMBus adapter\n"); - goto exit_free_irq; + goto exit_release; } i801_probe_optional_slaves(priv); @@ -1285,12 +1286,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) return 0; -exit_free_irq: - if (priv->features & FEATURE_IRQ) - free_irq(dev->irq, priv); +exit_release: pci_release_region(dev, SMBBAR); exit: - kfree(priv); return err; } @@ -1302,11 +1300,8 @@ static void i801_remove(struct pci_dev *dev) i2c_del_adapter(&priv->adapter); pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg); - if (priv->features & FEATURE_IRQ) - free_irq(dev->irq, priv); pci_release_region(dev, SMBBAR); - kfree(priv); /* * do not call pci_disable_device(dev) since it can cause hard hangs on * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010) -- 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/2] i2c: i801: Remove i801_driver forward declaration
struct pci_driver i801_driver forward declaration is needed only for accessing the name field. Remove it and use dev_driver_string() instead. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-i801.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 8fafb254e42a..e31aa987b281 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -223,8 +223,6 @@ struct i801_priv { #endif }; -static struct pci_driver i801_driver; - #define FEATURE_SMBUS_PEC (1 << 0) #define FEATURE_BLOCK_BUFFER (1 << 1) #define FEATURE_BLOCK_PROC (1 << 2) @@ -1204,7 +1202,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) goto exit; } - err = pci_request_region(dev, SMBBAR, i801_driver.name); + err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev)); if (err) { dev_err(&dev->dev, "Failed to request SMBus region " "0x%lx-0x%Lx\n", priv->smba, @@ -1255,7 +1253,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) init_waitqueue_head(&priv->waitq); err = request_irq(dev->irq, i801_isr, IRQF_SHARED, - i801_driver.name, priv); + dev_driver_string(&dev->dev), priv); if (err) { dev_err(&dev->dev, "Failed to allocate irq %d: %d\n", dev->irq, err); -- 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] i2c: designware: Do not calculate SCL timing parameters needlessly
Do SCL timing parameter calculation conditionally depending are custom parameters provided since calculated values will get instantly overwritten by provided parameters. Signed-off-by: Jarkko Nikula --- drivers/i2c/busses/i2c-designware-core.c | 45 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 23628b7bfb8d..2c7749774a5f 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -309,40 +309,39 @@ int i2c_dw_init(struct dw_i2c_dev *dev) sda_falling_time = dev->sda_falling_time ?: 300; /* ns */ scl_falling_time = dev->scl_falling_time ?: 300; /* ns */ - /* Standard-mode */ - hcnt = i2c_dw_scl_hcnt(input_clock_khz, - 4000, /* tHD;STA = tHIGH = 4.0 us */ - sda_falling_time, - 0, /* 0: DW default, 1: Ideal */ - 0); /* No offset */ - lcnt = i2c_dw_scl_lcnt(input_clock_khz, - 4700, /* tLOW = 4.7 us */ - scl_falling_time, - 0); /* No offset */ - - /* Allow platforms to specify the ideal HCNT and LCNT values */ + /* Set SCL timing parameters for standard-mode */ if (dev->ss_hcnt && dev->ss_lcnt) { hcnt = dev->ss_hcnt; lcnt = dev->ss_lcnt; + } else { + hcnt = i2c_dw_scl_hcnt(input_clock_khz, + 4000, /* tHD;STA = tHIGH = 4.0 us */ + sda_falling_time, + 0, /* 0: DW default, 1: Ideal */ + 0); /* No offset */ + lcnt = i2c_dw_scl_lcnt(input_clock_khz, + 4700, /* tLOW = 4.7 us */ + scl_falling_time, + 0); /* No offset */ } dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT); dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT); dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); - /* Fast-mode */ - hcnt = i2c_dw_scl_hcnt(input_clock_khz, - 600,/* tHD;STA = tHIGH = 0.6 us */ - sda_falling_time, - 0, /* 0: DW default, 1: Ideal */ - 0); /* No offset */ - lcnt = i2c_dw_scl_lcnt(input_clock_khz, - 1300, /* tLOW = 1.3 us */ - scl_falling_time, - 0); /* No offset */ - + /* Set SCL timing parameters for fast-mode */ if (dev->fs_hcnt && dev->fs_lcnt) { hcnt = dev->fs_hcnt; lcnt = dev->fs_lcnt; + } else { + hcnt = i2c_dw_scl_hcnt(input_clock_khz, + 600,/* tHD;STA = tHIGH = 0.6 us */ + sda_falling_time, + 0, /* 0: DW default, 1: Ideal */ + 0); /* No offset */ + lcnt = i2c_dw_scl_lcnt(input_clock_khz, + 1300, /* tLOW = 1.3 us */ + scl_falling_time, + 0); /* No offset */ } dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT); dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT); -- 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 v1 3/3] i2c: designware-pci: no need to provide clk_khz
On 01/22/2015 04:44 PM, Andy Shevchenko wrote: The clk_khz field makes sense only if SS counters are not provided. Since we provide them for Haswell and Baytrail explicitly we can omit the clk_khz parameter. Signed-off-by: Andy Shevchenko --- drivers/i2c/busses/i2c-designware-pcidrv.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index 2bbd2a7..506cbe8 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -112,7 +112,6 @@ static struct dw_pci_controller dw_pci_controllers[] = { .bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST, .tx_fifo_depth = 32, .rx_fifo_depth = 32, - .clk_khz = 10, .functionality = I2C_FUNC_10BIT_ADDR, .scl_sda_cfg = &byt_config, }, @@ -121,7 +120,6 @@ static struct dw_pci_controller dw_pci_controllers[] = { .bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST, .tx_fifo_depth = 32, .rx_fifo_depth = 32, - .clk_khz = 10, .functionality = I2C_FUNC_10BIT_ADDR, .scl_sda_cfg = &hsw_config, }, Yes, there are no other use for the clock rate than calculate these SS counters in drivers/i2c/busses/i2c-designware-core.c, and which will get overwritten by provided values. Reviewed-by: Jarkko Nikula -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support
Hi On 12/02/2014 02:09 AM, David E. Box wrote: Adds support for acquiring and releasing a hardware bus lock in the i2c designware core transfer function. This is needed for i2c bus controllers that are shared with but not controlled by the kernel. Signed-off-by: David E. Box --- drivers/i2c/busses/i2c-designware-core.c| 11 +++ drivers/i2c/busses/i2c-designware-core.h| 6 ++ drivers/i2c/busses/i2c-designware-platdrv.c | 18 +- 3 files changed, 30 insertions(+), 5 deletions(-) ... diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index a743115..afdff3b 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -261,10 +261,16 @@ static int dw_i2c_probe(struct platform_device *pdev) return r; } - pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); - pm_runtime_use_autosuspend(&pdev->dev); - pm_runtime_set_active(&pdev->dev); - pm_runtime_enable(&pdev->dev); + i2c_dw_eval_lock_support(dev); i2c_dw_eval_lock_support() is added in the next patch. + + if (dev->pm_runtime_disabled) { + pm_runtime_forbid(&pdev->dev); + } else { + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + } return 0; } @@ -314,7 +320,9 @@ static int dw_i2c_resume(struct device *dev) struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); clk_prepare_enable(i_dev->clk); - i2c_dw_init(i_dev); + + if (!i_dev->pm_runtime_disabled) + i2c_dw_init(i_dev); Should there be similar conditional call or locking around i2c_dw_init() and i2c_dw_disable_int() also in dw_i2c_probe()? -- Jarkko -- 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] fix module autoloading for ACPI enumerated devices
On 01/17/2014 05:57 PM, Mark Brown wrote: On Fri, Jan 17, 2014 at 09:37:56AM +0200, Jarkko Nikula wrote: Sidenote: actually this modalias/module loading issue is different and not related to stable ACPI i2c/spi slave device names. Oh, I'd been under the impression that it was the rewrite that was triggering this? IIRC issue has been there since when ACPI slave device support was added. I have a partial fix for it in cf9eb39c6f7b ("spi: Fix modalias for ACPI enumerated SPI devices") and when doing similar change for i2c Rui pointed me that he has a better fix that takes care of _CID string and platform code too. -- Jarkko -- 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] fix module autoloading for ACPI enumerated devices
On 01/16/2014 09:46 PM, Mark Brown wrote: On Thu, Jan 16, 2014 at 09:05:09PM +0800, Zhang Rui wrote: On Thu, 2014-01-16 at 13:27 +0100, Wolfram Sang wrote: This seems a gap to me but I'm not 100% sure. I saw Grant Likely introduced the OF style MODALIAS to platform bus, and OF style registration/binding to i2c bus, maybe he has an answer for this. This is needed for ACPI because we rewrite the device names to give us stable names. With OF for I2C and SPI nothing bus specific is done that affects this stuff so the default behaviour works. Sidenote: actually this modalias/module loading issue is different and not related to stable ACPI i2c/spi slave device names. -- Jarkko -- 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
[PATCHv4 3/3] spi: Use stable dev_name for ACPI enumerated SPI slaves
Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order. This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij". Signed-off-by: Jarkko Nikula Acked-by: Mark Brown --- Mark's ack added as diff between v3 and v4 is just: - dev_set_name(&spi->dev, "spi-%s", dev_name(&adev->dev)); + dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev)); --- drivers/spi/spi.c | 17 ++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index af4f971..be619e0 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -352,6 +352,19 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device); +static void spi_dev_set_name(struct spi_device *spi) +{ + struct acpi_device *adev = ACPI_COMPANION(&spi->dev); + + if (adev) { + dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev)); + return; + } + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), +spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -378,9 +391,7 @@ int spi_add_device(struct spi_device *spi) } /* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi); /* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash -- 1.8.4.2 -- 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
[PATCHv4 1/3] ACPI: Provide acpi_dev_name accessor for struct acpi_device device name
struct acpi_device fields are only available when CONFIG_ACPI is set. We may find use for dev_name(&adev->dev) in generic code that is build also without CONFIG_ACPI is set but currently this requires #ifdef CONFIG_ACPI churn. Provide here an accessor that returns dev_name of embedded struct device dev in struct acpi_device or NULL depending on CONFIG_ACPI setting. Signed-off-by: Jarkko Nikula --- include/linux/acpi.h | 10 ++ 1 file changed, 10 insertions(+) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 0da49ed..844e7ff 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -53,6 +53,11 @@ static inline acpi_handle acpi_device_handle(struct acpi_device *adev) #define ACPI_COMPANION_SET(dev, adev) ACPI_COMPANION(dev) = (adev) #define ACPI_HANDLE(dev) acpi_device_handle(ACPI_COMPANION(dev)) +static inline const char *acpi_dev_name(struct acpi_device *adev) +{ + return dev_name(&adev->dev); +} + enum acpi_irq_model_id { ACPI_IRQ_MODEL_PIC = 0, ACPI_IRQ_MODEL_IOAPIC, @@ -420,6 +425,11 @@ static inline bool acpi_driver_match_device(struct device *dev, #define ACPI_COMPANION_SET(dev, adev) do { } while (0) #define ACPI_HANDLE(dev) (NULL) +static inline const char *acpi_dev_name(struct acpi_device *adev) +{ + return NULL; +} + static inline void acpi_early_init(void) { } static inline int early_acpi_boot_init(void) -- 1.8.4.2 -- 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
[PATCHv4 2/3] i2c: Use stable dev_name for ACPI enumerated I2C slaves
Current I2C adapter id - client address "x-00yy" based device naming scheme is not always stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This is problematic in PC kind of platforms where I2C adapter numbers can change due variable amount of bus controllers, probe order, add-on cards or just because of BIOS settings. This patch addresses the problem by using the ACPI device name with "i2c-" prefix for ACPI enumerated I2C slaves. For them device name "x-00yz" becomes "i2c-INTABCD:ij" after this patch. Signed-off-by: Jarkko Nikula --- drivers/i2c/i2c-core.c | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index f74af33..bc9a294 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -618,6 +618,22 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter); +static void i2c_dev_set_name(struct i2c_adapter *adap, +struct i2c_client *client) +{ + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + + if (adev) { + dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); + return; + } + + /* For 10-bit clients, add an arbitrary offset to avoid collisions */ + dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), +client->addr | ((client->flags & I2C_CLIENT_TEN) +? 0xa000 : 0)); +} + /** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device @@ -676,10 +692,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; ACPI_COMPANION_SET(&client->dev, info->acpi_node.companion); - /* For 10-bit clients, add an arbitrary offset to avoid collisions */ - dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), -client->addr | ((client->flags & I2C_CLIENT_TEN) -? 0xa000 : 0)); + i2c_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err; -- 1.8.4.2 -- 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
[PATCHv4 0/3] I2C and SPI dev_name change for ACPI enumerated slaves
v4: This version adds and uses acpi_dev_name() accessor instead defining struct acpi_device stub for !CONFIG_ACPI v3: Third version of the set that changes I2C and SPI slave device names to be generated from stable ACPI device names on ACPI 5 based systems instead of using bus numbers which could change. Slave device name change goes as "x-00yz" -> "i2c-INTABCD:ij" "spix.y" -> "spi-INTABCD:ij" This version is actually a bit cleaner thanks to Rafael's "ACPI / driver core: Store an ACPI device pointer in struct acpi_dev_node" in git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm Set goes on top of linux-pm/linux-next due dependency to 1/3. As we are now in merge-window I'm fine if 1/3 goes in first and I queue 2-3/3 later to i2c and spi trees. Jarkko Nikula (3): ACPI: Provide acpi_dev_name accessor for struct acpi_device device name i2c: Use stable dev_name for ACPI enumerated I2C slaves spi: Use stable dev_name for ACPI enumerated SPI slaves drivers/i2c/i2c-core.c | 21 + drivers/spi/spi.c | 17 ++--- include/linux/acpi.h | 10 ++ 3 files changed, 41 insertions(+), 7 deletions(-) -- 1.8.4.2 -- 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: [alsa-devel] [PATCHv3 1/3] ACPI: Provide struct acpi_device stub for !CONFIG_ACPI builds
On 11/14/2013 12:03 PM, Takashi Iwai wrote: At Thu, 14 Nov 2013 11:00:59 +0200, Jarkko Nikula wrote: We have a few cases where we want to access struct device dev field in struct acpi_device from generic code that is build also without CONFIG_ACPI. Provide here a minimal struct acpi_device stub that allows to build such a code without adding new #ifdef CONFIG_ACPI churn. This should not increase section sizes if code is protected by ACPI_COMPANION() runtime checks as those will be optimized out by later compiler stages in case CONFIG_ACPI is not set. Signed-off-by: Jarkko Nikula --- Rafael, instead of this we could also have an accessor but adev->dev looked better in actual code and size vmlinux didn't change for x86_64_defconfig with CONFIG_ACPI is not set nor for omap2plus_defconfig. This looks too hackish, IMO. Defining a different dummy struct often gives more headaches. Thinking of the potential risk by misuses, it'd be simpler and safer to define a macro like acpi_dev_name() instead. BTW, is linux/device.h already included in !CONFIG_ACPI case? It is included unconditionally in include/linux/acpi.h. Your acpi_dev_name() proposal may be good enough for now as adev->dev access in generic code (now under CONFIG_ACPI) seems to be anyway around dev_name, using only pointer to struct acpi_device or has more things that prevents immediate #ifdef CONFIG_ACPI removal there. But one problem at time. I'll cook a version with acpi_dev_name. -- Jarkko -- 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
[PATCHv3 3/3] spi: Use stable dev_name for ACPI enumerated SPI slaves
Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order. This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij". Signed-off-by: Jarkko Nikula --- drivers/spi/spi.c | 17 ++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index af4f971..d9140bb 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -352,6 +352,19 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device); +static void spi_dev_set_name(struct spi_device *spi) +{ + struct acpi_device *adev = ACPI_COMPANION(&spi->dev); + + if (adev) { + dev_set_name(&spi->dev, "spi-%s", dev_name(&adev->dev)); + return; + } + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), +spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -378,9 +391,7 @@ int spi_add_device(struct spi_device *spi) } /* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi); /* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash -- 1.8.4.2 -- 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
[PATCHv3 1/3] ACPI: Provide struct acpi_device stub for !CONFIG_ACPI builds
We have a few cases where we want to access struct device dev field in struct acpi_device from generic code that is build also without CONFIG_ACPI. Provide here a minimal struct acpi_device stub that allows to build such a code without adding new #ifdef CONFIG_ACPI churn. This should not increase section sizes if code is protected by ACPI_COMPANION() runtime checks as those will be optimized out by later compiler stages in case CONFIG_ACPI is not set. Signed-off-by: Jarkko Nikula --- Rafael, instead of this we could also have an accessor but adev->dev looked better in actual code and size vmlinux didn't change for x86_64_defconfig with CONFIG_ACPI is not set nor for omap2plus_defconfig. --- include/linux/acpi.h | 4 1 file changed, 4 insertions(+) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 0da49ed..df444e1 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -414,6 +414,10 @@ static inline bool acpi_driver_match_device(struct device *dev, #else /* !CONFIG_ACPI */ +struct acpi_device { + struct device dev; +}; + #define acpi_disabled 1 #define ACPI_COMPANION(dev)(NULL) -- 1.8.4.2 -- 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
[PATCHv3 0/3] I2C and SPI dev_name change for ACPI enumerated slaves
Hi Third version of the set that changes I2C and SPI slave device names to be generated from stable ACPI device names on ACPI 5 based systems instead of using bus numbers which could change. Slave device name change goes as "x-00yz" -> "i2c-INTABCD:ij" "spix.y" -> "spi-INTABCD:ij" This version is actually a bit cleaner thanks to Rafael's "ACPI / driver core: Store an ACPI device pointer in struct acpi_dev_node" in git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm Set goes on top of linux-pm/linux-next due dependency to 1/3. As we are now in merge-window I'm fine if 1/3 goes in first and I queue 2-3/3 later to i2c and spi trees. Jarkko Nikula (3): ACPI: Provide struct acpi_device stub for !CONFIG_ACPI builds i2c: Use stable dev_name for ACPI enumerated I2C slaves spi: Use stable dev_name for ACPI enumerated SPI slaves drivers/i2c/i2c-core.c | 21 + drivers/spi/spi.c | 17 ++--- include/linux/acpi.h | 4 3 files changed, 35 insertions(+), 7 deletions(-) -- 1.8.4.2 -- 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
[PATCHv3 2/3] i2c: Use stable dev_name for ACPI enumerated I2C slaves
Current I2C adapter id - client address "x-00yy" based device naming scheme is not always stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This is problematic in PC kind of platforms where I2C adapter numbers can change due variable amount of bus controllers, probe order, add-on cards or just because of BIOS settings. This patch addresses the problem by using the ACPI device name with "i2c-" prefix for ACPI enumerated I2C slaves. For them device name "x-00yz" becomes "i2c-INTABCD:ij" after this patch. Signed-off-by: Jarkko Nikula --- drivers/i2c/i2c-core.c | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index f74af33..b8f2c32 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -618,6 +618,22 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter); +static void i2c_dev_set_name(struct i2c_adapter *adap, +struct i2c_client *client) +{ + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + + if (adev) { + dev_set_name(&client->dev, "i2c-%s", dev_name(&adev->dev)); + return; + } + + /* For 10-bit clients, add an arbitrary offset to avoid collisions */ + dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), +client->addr | ((client->flags & I2C_CLIENT_TEN) +? 0xa000 : 0)); +} + /** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device @@ -676,10 +692,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; ACPI_COMPANION_SET(&client->dev, info->acpi_node.companion); - /* For 10-bit clients, add an arbitrary offset to avoid collisions */ - dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), -client->addr | ((client->flags & I2C_CLIENT_TEN) -? 0xa000 : 0)); + i2c_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err; -- 1.8.4.2 -- 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: [PATCHv2 1/3] ACPI: Expose struct acpi_device and acpi_bus_get_device() to non-ACPI builds
On Mon, Nov 04, 2013 at 02:00:45AM +0100, Rafael J. Wysocki wrote: > On Saturday, November 02, 2013 11:18:31 PM Rafael J. Wysocki wrote: > > This turns out the cause build problems to happen on some architectures. > > > > I guess it's sufficient to simply define a stub struct acpi_device as > > > > struct acpi_device { > > struct device dev; > > }; > > > > for !CONFIG_ACPI instead. > > Generally, it is a bad idea to #include acpi_bus.h for !CONFIG_ACPI. > Ouch, indeed. > The appended patch works for me, sorry for stealing part of your changelog. > No problem. Thanks for fixing this up. -- Jarkko -- 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: [PATCHv2 0/3] I2C and SPI dev_name change for ACPI enumerated slaves
On 11/01/2013 03:20 PM, Wolfram Sang wrote: On Fri, Nov 01, 2013 at 02:18:06PM +0100, Rafael J. Wysocki wrote: Looks good to me. If there are no objections, I can merge these through my tree. Which is basically fine with me. Do you want to have it in 3.13 already? I mean renaming the devices could lead to regressions, so I'd rather be conservative and aim for 3.14. Valid concern. Quick grep below doesn't reveal any obvious device name matching outside of sound/soc/ but of course it doesn't prove it to be impossible. git grep '[0-9]\-00' |grep name git grep 'spi[0-9].[0-9]' |grep name -- Jarkko -- 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
[PATCHv2 2/3] i2c: Use stable dev_name for ACPI enumerated I2C slaves
Current I2C adapter id - client address "x-00yy" based device naming scheme is not always stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This is problematic in PC kind of platforms where I2C adapter numbers can change due variable amount of bus controllers, probe order, add-on cards or just because of BIOS settings. This patch addresses the problem by using the ACPI device name with "i2c-" prefix for ACPI enumerated I2C slaves. For them device name "x-00yz" becomes "i2c-INTABCD:ij" after this patch. Signed-off-by: Jarkko Nikula --- drivers/i2c/i2c-core.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 75ba860..9126c2e 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -48,6 +48,7 @@ #include #include #include +#include #include #include "i2c-core.h" @@ -618,6 +619,24 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter); +static void i2c_dev_set_name(struct i2c_adapter *adap, +struct i2c_client *client) +{ + if (ACPI_HANDLE(&client->dev)) { + struct acpi_device *adev; + if (!acpi_bus_get_device(ACPI_HANDLE(&client->dev), &adev)) { + dev_set_name(&client->dev, "i2c-%s", +dev_name(&adev->dev)); + return; + } + } + + /* For 10-bit clients, add an arbitrary offset to avoid collisions */ + dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), +client->addr | ((client->flags & I2C_CLIENT_TEN) +? 0xa000 : 0)); +} + /** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device @@ -676,10 +695,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; ACPI_HANDLE_SET(&client->dev, info->acpi_node.handle); - /* For 10-bit clients, add an arbitrary offset to avoid collisions */ - dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), -client->addr | ((client->flags & I2C_CLIENT_TEN) -? 0xa000 : 0)); + i2c_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err; -- 1.8.4.rc3 -- 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
[PATCHv2 0/3] I2C and SPI dev_name change for ACPI enumerated slaves
Hi Second version of the set that changes I2C and SPI slave device names to be generated from stable ACPI device names on ACPI 5 based systems instead of using bus numbers which could change. Slave device name change goes as "x-00yz" -> "i2c-INTABCD:ij" "spix.y" -> "spi-INTABCD:ij" This version adds patch to include/acpi/acpi_bus.h that allow us to remove #if IS_ENABLED(CONFIG_ACPI) checks that were added in the first version. Set goes on top linux-pm/linux-next commit e56b4d2. First version here: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067737.html Jarkko Nikula (3): ACPI: Expose struct acpi_device and acpi_bus_get_device() to non-ACPI builds i2c: Use stable dev_name for ACPI enumerated I2C slaves spi: Use stable dev_name for ACPI enumerated SPI slaves drivers/i2c/i2c-core.c | 24 drivers/spi/spi.c | 20 +--- include/acpi/acpi_bus.h | 9 +++-- 3 files changed, 44 insertions(+), 9 deletions(-) -- 1.8.4.rc3 -- 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
[PATCHv2 3/3] spi: Use stable dev_name for ACPI enumerated SPI slaves
Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order. This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij". Signed-off-by: Jarkko Nikula --- drivers/spi/spi.c | 20 +--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 740f9dd..4c0c801 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -38,6 +38,7 @@ #include #include #include +#include static void spidev_release(struct device *dev) { @@ -352,6 +353,21 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device); +void spi_dev_set_name(struct spi_device *spi) +{ + if (ACPI_HANDLE(&spi->dev)) { + struct acpi_device *adev; + if (!acpi_bus_get_device(ACPI_HANDLE(&spi->dev), &adev)) { + dev_set_name(&spi->dev, "spi-%s", +dev_name(&adev->dev)); + return; + } + } + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), +spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -378,9 +394,7 @@ int spi_add_device(struct spi_device *spi) } /* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi); /* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash -- 1.8.4.rc3 -- 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
[PATCHv2 1/3] ACPI: Expose struct acpi_device and acpi_bus_get_device() to non-ACPI builds
We may find use for struct acpi_device and acpi_bus_get_device() in generic code without wanting to add #if IS_ENABLED(CONFIG_ACPI) churn there. Code can use ACPI_HANDLE() test to let compiler optimize out code blocks that are not used in !CONFIG_ACPI builds but structure definitions and function stubs must be available. This patch moves CONFIG_ACPI test so that struct acpi_device definition is available for all builds that include acpi_bus.h and provides a stub for acpi_bus_get_device(). Signed-off-by: Jarkko Nikula --- include/acpi/acpi_bus.h | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 15100f6..232b37d3 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -66,8 +66,6 @@ bool acpi_ata_match(acpi_handle handle); bool acpi_bay_match(acpi_handle handle); bool acpi_dock_match(acpi_handle handle); -#ifdef CONFIG_ACPI - #include #define ACPI_BUS_FILE_ROOT "acpi" @@ -315,6 +313,8 @@ struct acpi_device { void (*remove)(struct acpi_device *); }; +#if IS_ENABLED(CONFIG_ACPI) + static inline void *acpi_driver_data(struct acpi_device *d) { return d->driver_data; @@ -532,6 +532,11 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) static inline int register_acpi_bus_type(void *bus) { return 0; } static inline int unregister_acpi_bus_type(void *bus) { return 0; } +static inline int acpi_bus_get_device(acpi_handle handle, + struct acpi_device **device) +{ + return -ENODEV; +} #endif /* CONFIG_ACPI */ -- 1.8.4.rc3 -- 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: [RFC 1/2] i2c: Use stable dev_name for ACPI enumerated I2C slaves
Hi Rafael On 10/25/2013 05:08 PM, Rafael J. Wysocki wrote: On Friday, October 25, 2013 04:30:23 PM Jarkko Nikula wrote: >>> >> Hmm, not only. Referencing to dev field in struct acpi_device by >> dev_name(&adev->dev) here will fail too. > > Well, something is not quite right here. > > One of the *reasons* for having ACPI_HANDLE() defined this way is to > avoid using explicit CONFIG_ACPI checks, so if that doesn't work, > then all of that becomes a bit pointless. > One possible thing to do is to let structure definitions to be available for non-ACPI builds. Then compiler won't fail on structure access which will be anyway optimized away by later compiler stages. With a quick test below vmlinux section sizes don't change for couple non-ACPI build tests and allow to get rid off IS_ENABLED(CONFIG_ACPI) test in this patch. It's very minimal as it only moves the CONFIG_ACPI test just after the struct acpi_device definition and defines acpi_bus_get_device for non-ACPI case. What I don't know how consistent it is as there are still couple structure definitions under CONFIG_ACPI, doesn't touch other acpi headers and requires to include acpi_bus.h in driver (or move acpi_bus.h include in linux/acpi.h currently under CONFIG_ACPI). -- Jarkko diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index d901982..7ab7870 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -66,8 +66,6 @@ bool acpi_ata_match(acpi_handle handle); bool acpi_bay_match(acpi_handle handle); bool acpi_dock_match(acpi_handle handle); -#ifdef CONFIG_ACPI - #include #define ACPI_BUS_FILE_ROOT"acpi" @@ -314,6 +312,8 @@ struct acpi_device { void (*remove)(struct acpi_device *); }; +#if IS_ENABLED(CONFIG_ACPI) + static inline void *acpi_driver_data(struct acpi_device *d) { return d->driver_data; @@ -531,6 +531,8 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) static inline int register_acpi_bus_type(void *bus) { return 0; } static inline int unregister_acpi_bus_type(void *bus) { return 0; } +static inline int acpi_bus_get_device(acpi_handle handle, + struct acpi_device **device) { return 0; } #endif/* CONFIG_ACPI */ -- 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: [RFC 1/2] i2c: Use stable dev_name for ACPI enumerated I2C slaves
On 10/25/2013 04:18 PM, Rafael J. Wysocki wrote: On Friday, October 25, 2013 03:55:29 PM Jarkko Nikula wrote: Well, acpi_bus_get_device() is not available for non-ACPI builds and at least the gcc I used for test build didn't optimize that block away. Well, it should. ACPI_HANDLE() translates to (NULL) if CONFIG_ACPI is not defined and that causes the check to become "if (NULL)" which should always be dropped by the compiler. My very vague memory says the same. I don't know is this gcc version or flag dependent behavior. I got the build error from both i386 build using gcc 4.8.1 and arm build by using make ARCH=arm omap2plus_defconfig and gcc-4.7-arm-linux-gnueabi 4.7.2-4. Does providing a stub acpi_bus_get_device() for !CONFIG_ACPI actually help? Hmm, not only. Referencing to dev field in struct acpi_device by dev_name(&adev->dev) here will fail too. -- Jarkko -- 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: [RFC 1/2] i2c: Use stable dev_name for ACPI enumerated I2C slaves
Hi On 10/25/2013 03:52 PM, Rafael J. Wysocki wrote: +static void i2c_dev_set_name(struct i2c_adapter *adap, +struct i2c_client *client) +{ +#if IS_ENABLED(CONFIG_ACPI) + if (ACPI_HANDLE(&client->dev)) { ACPI_HANDLE() already contains an "is CONFIG_ACPI enabled?" check, so the #if around the if (ACPI_HANDLE()) {} is redundant. Similarly for the SPI patch. Well, acpi_bus_get_device() is not available for non-ACPI builds and at least the gcc I used for test build didn't optimize that block away. -- Jarkko -- 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
[RFC 1/2] i2c: Use stable dev_name for ACPI enumerated I2C slaves
Current I2C adapter id - client address "x-00yy" based device naming scheme is not always stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This is problematic in PC kind of platforms where I2C adapter numbers can change due variable amount of bus controllers, probe order, add-on cards or just because of BIOS settings. This patch addresses the problem by using the ACPI device name with "i2c-" prefix for ACPI enumerated I2C slaves. For them device name "x-00yz" becomes "i2c-INTABCD:ij" after this patch. Signed-off-by: Jarkko Nikula --- I'm not sure when would acpi_bus_get_device fail and how realistic is that here. I put minimalistic error handling here which just falls back to old adapter-addr naming scheme. --- drivers/i2c/i2c-core.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 111b2c6..8d6f3e5 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -613,6 +613,25 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter); +static void i2c_dev_set_name(struct i2c_adapter *adap, +struct i2c_client *client) +{ +#if IS_ENABLED(CONFIG_ACPI) + if (ACPI_HANDLE(&client->dev)) { + struct acpi_device *adev; + if (!acpi_bus_get_device(ACPI_HANDLE(&client->dev), &adev)) { + dev_set_name(&client->dev, "i2c-%s", +dev_name(&adev->dev)); + return; + } + } +#endif + /* For 10-bit clients, add an arbitrary offset to avoid collisions */ + dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), +client->addr | ((client->flags & I2C_CLIENT_TEN) +? 0xa000 : 0)); +} + /** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device @@ -671,10 +690,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; ACPI_HANDLE_SET(&client->dev, info->acpi_node.handle); - /* For 10-bit clients, add an arbitrary offset to avoid collisions */ - dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), -client->addr | ((client->flags & I2C_CLIENT_TEN) -? 0xa000 : 0)); + i2c_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err; -- 1.8.4.rc3 -- 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
[RFC 0/2] I2C and SPI dev_name change for ACPI enumerated slaves
Hi We've run into problem of changing I2C device names while developing ALSA SoC drivers for x86 based systems. Changing device names makes more difficult to match devices by their name. Which is what we use within ASoC subsystem. These changing names comes from changing adapter/bus numbering which could occur due variable amount of bus controllers, probe order, add-on cards or different BIOS settings. Patches here try to solve the problem on ACPI 5 based systems by using stable ACPI device name with a "i2c-"/"spi-" prefix for I2C/SPI slave device names. Both patches are independent from each other and can go through their own subsystems. Jarkko Nikula (2): i2c: Use stable dev_name for ACPI enumerated I2C slaves spi: Use stable dev_name for ACPI enumerated SPI slaves drivers/i2c/i2c-core.c | 24 drivers/spi/spi.c | 21 ++--- 2 files changed, 38 insertions(+), 7 deletions(-) -- 1.8.4.rc3 -- 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
[RFC 2/2] spi: Use stable dev_name for ACPI enumerated SPI slaves
Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem. This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order. This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij". Signed-off-by: Jarkko Nikula --- "may not be" and "This can be problem" since I don't know how likely is to have changes in SPI bus configuration in PC kind of devices. I assume yes as this is already problem with I2C devices. --- drivers/spi/spi.c | 21 ++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8d05acc..2ca997f 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -345,6 +345,23 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device); +void spi_dev_set_name(struct spi_device *spi) +{ +#if IS_ENABLED(CONFIG_ACPI) + if (ACPI_HANDLE(&spi->dev)) { + struct acpi_device *adev; + if (!acpi_bus_get_device(ACPI_HANDLE(&spi->dev), &adev)) { + dev_set_name(&spi->dev, "spi-%s", +dev_name(&adev->dev)); + return; + } + } +#endif + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), +spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -371,9 +388,7 @@ int spi_add_device(struct spi_device *spi) } /* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi); /* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash -- 1.8.4.rc3 -- 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: Fix modalias for ACPI enumerated I2C devices
On 10/16/2013 03:04 AM, Grant Likely wrote: On Wed, Oct 16, 2013 at 12:47 AM, Rafael J. Wysocki wrote: On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote: On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote: On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: I have a question about the device "uevent" and "modalias" sysfs attributes. what is the relationship between these two? Am I right to say that, if there is the "MODALIAS" field in uevent file, this field must be consistent with the content in "modalias" attribute? Well, if it isn't, it's pretty pointless, right? static int platform_uevent(struct device *dev, struct kobj_uevent_env *env) { struct platform_device *pdev = to_platform_device(dev); int rc; /* Some devices have extra OF data and an OF-style MODALIAS */ rc = of_device_uevent_modalias(dev, env); if (rc != -ENODEV) return rc; add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX, pdev->name); return 0; } This means that the OF-style MODALIAS is not shown in "modalias" sysfs attribute. is this a bug? Here is an example from one DT based system: cat /sys/bus/platform/devices/4807.i2c/uevent DRIVER=omap_i2c OF_NAME=i2c OF_FULLNAME=/ocp/i2c@4807 OF_COMPATIBLE_0=ti,omap4-i2c OF_COMPATIBLE_N=1 MODALIAS=of:Ni2cTCti,omap4-i2c cat /sys/bus/platform/devices/4807.i2c/modalias platform:4807.i2c And a device on that I2C bus: cat /sys/bus/platform/devices/rtc.11/uevent DRIVER=twl_rtc OF_NAME=rtc OF_FULLNAME=/ocp/i2c@4807/twl@48/rtc OF_COMPATIBLE_0=ti,twl4030-rtc OF_COMPATIBLE_N=1 MODALIAS=of:NrtcTCti,twl4030-rtc cat /sys/bus/platform/devices/rtc.11/modalias platform:rtc.11 Unfortunately I cannot debug above example further at the moment is there failing or needless modprobe calls. Maybe device tree experts know better? -- Jarkko -- 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] ACPI: add module autoloading support for ACPI enumerated devices
Hi On 10/14/2013 02:16 PM, Zhang Rui wrote: On Mon, 2013-10-14 at 13:14 +0300, Mika Westerberg wrote: For the three patches. I wonder if we should do the same for SPI bus as well? yes, you are right. Actually, I should check for buses that supports acpi_driver_match_device() and fix them all. Will add it in next version. You could have my pre- "Tested-by: Jarkko Nikula ". With your set I see bunch of failing modprobe calls from udev for i2c:INTABCD:xy and platform:INTDCBA:xy modaliases gone. -- Jarkko -- 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: Fix modalias for ACPI enumerated I2C devices
On 10/14/2013 12:23 PM, Zhang Rui wrote: Hi, On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote: There is a minor fault about ACPI enumerated I2C devices with their modalias attribute. Now modalias is set by device instance not by hardware ID. For example "i2c:INTABCD:00", "i2c:INTABCD:01" etc. This means each device instance gets different modalias which does match with generated modules.alias. Currently this is not problem as matching can happen also with "acpi:INTABCD" modalias. IMO, this is not the proper fix for the modalias problem because ACPI enumerated I2C device may have compatible ids. Instead, we should export all the compatible ids as the modules alias of the ACPI enumerated I2C device. can you please take a look at the patch I sent out earlier? https://patchwork.kernel.org/patch/3034991/ https://patchwork.kernel.org/patch/3035041/ https://patchwork.kernel.org/patch/3035021/ I see. This makes sense as it avoids that same device has two different modaliases from both acpi and other subsystem. How about modalias nodes in sysfs, should they also reflect what is matching uvent? -- Jarkko -- 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: Fix modalias for ACPI enumerated I2C devices
On 10/12/2013 07:18 PM, Mika Westerberg wrote: If we have two ACPI enumerated devices, they have following modalias: i2c-device0: i2c:INTABCD:00 acpi:INTABCD i2c-device1: i2c:INTABCD:01 acpi:INTABCD Likelihood that some random I2C driver has INTABCD:00 or INTABCD:01 ids in their list is minimal. However, when you turn it to this: i2c-device0: i2c:INTABCD acpi:INTABCD i2c-device1: i2c:INTABCD acpi:INTABCD It might be possible that we get a match that isn't supposed to happen. Well, OK it is pretty remote but anyway :-) Well, name conflicts could occur of course but still I don't think we should generate illegal or wrong modaliases. I'm not an udev expert but I suppose trying to load nonexisting drivers (i2c_INTABCD:xy) could slow booting a little and perhaps pollute needlessly error log compared to if it can see that driver is already loaded or tries to load the same driver again. I don't think name conflicts can pose too big risk as they are trivial to fix in sources and can be queued to stable too. -- Jarkko -- 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: Fix modalias for ACPI enumerated I2C devices
On 10/12/2013 08:04 AM, Mika Westerberg wrote: On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote: I think that this is intentional. We don't want that the i2c modalias matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI IDs that are added to the driver to match with the ACPI device. Well, I'm not really sure this was intentional, but I wonder how other bus types work in that respect? We have the same for platform bus, if that's what you are asking. Do we? I don't recall seeing per device modaliases on other platforms on their platform buses. And actually I don't see that happening in drivers/base/platform.c: platform_uevent() either where just pdev->name is used but not pdev->id (which is used with pdev->name for dev_set_name()). This makes me thinking that perhaps "pdevinfo.name = dev_name(&adev->dev);" in drivers/acpi/acpi_platform.c: acpi_create_platform_device() should be fixed too as now modalias for ACPI registered platform devices differ from platform devices that are registered in other subsystems (e.g. regulatory, pcspkr, alarmtimer, etc devices)? I can send a patch for that. -- Jarkko -- 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: Fix modalias for ACPI enumerated I2C devices
There is a minor fault about ACPI enumerated I2C devices with their modalias attribute. Now modalias is set by device instance not by hardware ID. For example "i2c:INTABCD:00", "i2c:INTABCD:01" etc. This means each device instance gets different modalias which does match with generated modules.alias. Currently this is not problem as matching can happen also with "acpi:INTABCD" modalias. Fix this by using ACPI hardware ID. Signed-off-by: Jarkko Nikula Cc: Mika Westerberg --- Generated on top of v3.12-rc4-29-g0e7a3ed. --- 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 29d3f04..6dd0c53 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -,7 +,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, if (ret < 0 || !info.addr) return AE_OK; - strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type)); + strlcpy(info.type, acpi_device_hid(adev), sizeof(info.type)); if (!i2c_new_device(adapter, &info)) { dev_err(&adapter->dev, "failed to add I2C device %s from ACPI\n", -- 1.8.4.rc3 -- 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-omap: Double clear of ARDY status in IRQ handler
On Fri, 11 Feb 2011 12:15:18 +0530 Keerthy wrote: > From: Richard woodruff > > ProDB00017052 - ARDY interrupt reasserted after being cleared. > This errata caused intermittent i2c instabilty(1 error per 3 hours) on several > customer platforms. After applying the workaround the intermittent errors were > not seen. This is not captured in the usual errata documents. > > The workaround is to have a double clear of ARDY status in > irq handler. > > Signed-off-by: Richard woodruff > Signed-off-by: Keerthy > --- > drivers/i2c/busses/i2c-omap.c |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 445de08..9bcefae 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -851,7 +851,8 @@ complete: > OMAP_I2C_STAT_AL)) { > omap_i2c_ack_stat(dev, stat & > (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR | > - OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); > + OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR | > + OMAP_I2C_STAT_ARDY)); Most probably this is correct but I was just wondering are there any use for ARDY interrupt (register access ready) anyway and is the ARDY here caused by omap_i2c_write_reg for STAT_NACK in a few lines above. Just my 2 cents about possible further patches (while this having the priority as fixes the issue). -- Jarkko -- 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: omap: fix OOPS in omap_i2c_unidle() during probe
On Tue, 11 May 2010 10:01:22 +0300 Mika Westerberg wrote: > I believe this is already in mainline: > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=7c6bd2010fced38444c9fd658f4c6ce61bd185bf > Ah, yeah. I was using the sound-2.6.git tree which is not yet updated any newer than 2.6.34-rc4 and that's why I didn't see it. Sorry about line noise. -- Jarkko -- 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: omap: fix OOPS in omap_i2c_unidle() during probe
On Fri, 16 Apr 2010 12:47:50 +0300 Grazvydas Ignotas wrote: > >> Tested-by: Paul Walmsley > Uhm why does this patch (or Tony's version) doesn't reach mainline for > so long? It's critical for all OMAPs except the very old ones. > > If it helps: > Tested-by: Grazvydas Ignotas > > > Tested-by: Jarkko Nikula Ping? Mike or Tony, can you resend the patch just in case. Reminder: 2.6.34 doesn't boot on OMAP without this patch. -- Jarkko -- 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: omap: fix OOPS in omap_i2c_unidle() during probe
On Wed, 31 Mar 2010 04:27:41 -0600 (MDT) Paul Walmsley wrote: > > This patch moves register shift setting before any register accesses are > > done. > > > > Signed-off-by: Mika Westerberg > > Cc: Cory Maccarrone > > Tested-by: Paul Walmsley > BTW, Tony had the same fix buried in an another thread: http://marc.info/?l=linux-omap&m=126826012627677&w=2 You could add my tested by as well to which one goes in but the fix should go for 2.6.34 as the mainline doesn't boot otherwise on OMAP. Tested-by: Jarkko Nikula -- 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