mc13xxx: add I2C support, V5
Hi, This series (against mfd-2.6/for-next) changes the mc13xxx driver to use regmap and adds I2C support. It has a compile dependency on regmap/for-next, as the spi driver uses the recently added pad_bits config field. Patch 2/4 has: Reviewed-by: Mark Brown Patch 4/4 has: Signed-off-by: Oskar Schirmer Changes since: V4: - Fix compile error when CONFIG_OF enabled - Select REGMAP_I2C and REGMAP_SPI for appropriate drivers. V3: - spi driver uses padded register format (reads should actually work now!) - mc13873 removed from I2C driver - fix memory leak on probe error Cheers, Marc -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH v5 1/4] mfd: mc13xxx-core: Prepare for separate spi and i2c backends.
This patch abstracts the bus specific operations from the driver core. Generic init and cleanup is consolidated into mc13xxx_common_*. spi specific functions are renamed to reflect such. (The irq member of the mc13xxx struct is no longer redundant, it's used to store the irq for cleanup time). Signed-off-by: Marc Reilly --- drivers/mfd/mc13xxx-core.c | 187 ++-- 1 files changed, 112 insertions(+), 75 deletions(-) diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c index 7122386..0753402 100644 --- a/drivers/mfd/mc13xxx-core.c +++ b/drivers/mfd/mc13xxx-core.c @@ -22,8 +22,18 @@ #include #include +enum mc13xxx_id { + MC13XXX_ID_MC13783, + MC13XXX_ID_MC13892, + MC13XXX_ID_INVALID, +}; + struct mc13xxx { struct spi_device *spidev; + + struct device *dev; + enum mc13xxx_id ictype; + struct mutex lock; int irq; int flags; @@ -144,36 +154,32 @@ struct mc13xxx { void mc13xxx_lock(struct mc13xxx *mc13xxx) { if (!mutex_trylock(&mc13xxx->lock)) { - dev_dbg(&mc13xxx->spidev->dev, "wait for %s from %pf\n", + dev_dbg(mc13xxx->dev, "wait for %s from %pf\n", __func__, __builtin_return_address(0)); mutex_lock(&mc13xxx->lock); } - dev_dbg(&mc13xxx->spidev->dev, "%s from %pf\n", + dev_dbg(mc13xxx->dev, "%s from %pf\n", __func__, __builtin_return_address(0)); } EXPORT_SYMBOL(mc13xxx_lock); void mc13xxx_unlock(struct mc13xxx *mc13xxx) { - dev_dbg(&mc13xxx->spidev->dev, "%s from %pf\n", + dev_dbg(mc13xxx->dev, "%s from %pf\n", __func__, __builtin_return_address(0)); mutex_unlock(&mc13xxx->lock); } EXPORT_SYMBOL(mc13xxx_unlock); #define MC13XXX_REGOFFSET_SHIFT 25 -int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val) +static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx, + unsigned int offset, u32 *val) { struct spi_transfer t; struct spi_message m; int ret; - BUG_ON(!mutex_is_locked(&mc13xxx->lock)); - - if (offset > MC13XXX_NUMREGS) - return -EINVAL; - *val = offset << MC13XXX_REGOFFSET_SHIFT; memset(&t, 0, sizeof(t)); @@ -195,26 +201,17 @@ int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val) *val &= 0xff; - dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] -> 0x%06x\n", offset, *val); - return 0; } -EXPORT_SYMBOL(mc13xxx_reg_read); -int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val) +static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, + u32 val) { u32 buf; struct spi_transfer t; struct spi_message m; int ret; - BUG_ON(!mutex_is_locked(&mc13xxx->lock)); - - dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] <- 0x%06x\n", offset, val); - - if (offset > MC13XXX_NUMREGS || val > 0xff) - return -EINVAL; - buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val; memset(&t, 0, sizeof(t)); @@ -235,6 +232,34 @@ int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val) return 0; } + +int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val) +{ + int ret; + + BUG_ON(!mutex_is_locked(&mc13xxx->lock)); + + if (offset > MC13XXX_NUMREGS) + return -EINVAL; + + ret = mc13xxx_spi_reg_read(mc13xxx, offset, val); + dev_vdbg(mc13xxx->dev, "[0x%02x] -> 0x%06x\n", offset, *val); + + return ret; +} +EXPORT_SYMBOL(mc13xxx_reg_read); + +int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val) +{ + BUG_ON(!mutex_is_locked(&mc13xxx->lock)); + + dev_vdbg(mc13xxx->dev, "[0x%02x] <- 0x%06x\n", offset, val); + + if (offset > MC13XXX_NUMREGS || val > 0xff) + return -EINVAL; + + return mc13xxx_spi_reg_write(mc13xxx, offset, val); +} EXPORT_SYMBOL(mc13xxx_reg_write); int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset, @@ -439,7 +464,7 @@ static int mc13xxx_irq_handle(struct mc13xxx *mc13xxx, if (handled == IRQ_HANDLED) num_handled++; } else { - dev_err(&mc13xxx->spidev->dev, + dev_err(mc13xxx->dev, "BUG: irq %u but no handler\n", baseirq + irq); @@ -475,25 +500,23 @@ static irqreturn_t mc13xxx_irq_thread(int irq, void *data) return IRQ_RETVAL(handled); } -enum mc13xxx_id { - MC13XXX_ID_MC13783, - MC13XXX_ID_MC13892, - MC13XXX_ID_INVALID, -}; - static const char *mc13xxx_chipname[] = { [MC13XXX_ID_MC13783] = "mc13783",
[PATCH v5 3/4] mfd: mc13xxx-core: Move spi specific code into separate module.
All spi specific code is moved into a new module. The mc13xxx struct moves to a new local include file by necessity. A new config choice selects the SPI bus type support and by default is value of SPI_MASTER to remain compatible with existing configs. Signed-off-by: Marc Reilly --- drivers/mfd/Kconfig| 17 - drivers/mfd/Makefile |1 + drivers/mfd/mc13xxx-core.c | 148 ++-- drivers/mfd/mc13xxx-spi.c | 140 + drivers/mfd/mc13xxx.h | 45 + 5 files changed, 204 insertions(+), 147 deletions(-) create mode 100644 drivers/mfd/mc13xxx-spi.c create mode 100644 drivers/mfd/mc13xxx.h diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 4e92513..fe0a0ab 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -576,14 +576,23 @@ config MFD_MC13XXX depends on SPI_MASTER select MFD_CORE select MFD_MC13783 - select REGMAP_SPI help - Support for the Freescale (Atlas) PMIC and audio CODECs - MC13783 and MC13892. - This driver provides common support for accessing the device, + Enable support for the Freescale MC13783 and MC13892 PMICs. + This driver provides common support for accessing the device, additional drivers must be enabled in order to use the functionality of the device. +if MFD_MC13XXX + +config MFD_MC13XXX_SPI + tristate "MC13xxx SPI interface" if SPI_MASTER + default SPI_MASTER + select REGMAP_SPI + help + Select this if your MC13xxx is connected via an SPI bus. + +endif + config ABX500_CORE bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions" default y if ARCH_U300 || ARCH_U8500 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index b953bab..2dc66ed 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -53,6 +53,7 @@ obj-$(CONFIG_TWL6030_PWM) += twl6030-pwm.o obj-$(CONFIG_TWL6040_CORE) += twl6040-core.o twl6040-irq.o obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o +obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o obj-$(CONFIG_MFD_CORE) += mfd-core.o diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c index 3223625..5a60273 100644 --- a/drivers/mfd/mc13xxx-core.c +++ b/drivers/mfd/mc13xxx-core.c @@ -15,36 +15,13 @@ #include #include #include -#include #include #include #include #include #include -#include -#include -enum mc13xxx_id { - MC13XXX_ID_MC13783, - MC13XXX_ID_MC13892, - MC13XXX_ID_INVALID, -}; - -struct mc13xxx { - struct regmap *regmap; - - struct device *dev; - enum mc13xxx_id ictype; - - struct mutex lock; - int irq; - int flags; - - irq_handler_t irqhandler[MC13XXX_NUM_IRQ]; - void *irqdata[MC13XXX_NUM_IRQ]; - - int adcflags; -}; +#include "mc13xxx.h" #define MC13XXX_IRQSTAT0 0 #define MC13XXX_IRQSTAT0_ADCDONEI (1 << 0) @@ -151,8 +128,6 @@ struct mc13xxx { #define MC13XXX_ADC2 45 -#define MC13XXX_NUMREGS 0x3f - void mc13xxx_lock(struct mc13xxx *mc13xxx) { if (!mutex_trylock(&mc13xxx->lock)) { @@ -667,90 +642,7 @@ static inline int mc13xxx_probe_flags_dt(struct mc13xxx *mc13xxx) } #endif -static const struct spi_device_id mc13xxx_device_id[] = { - { - .name = "mc13783", - .driver_data = MC13XXX_ID_MC13783, - }, { - .name = "mc13892", - .driver_data = MC13XXX_ID_MC13892, - }, { - /* sentinel */ - } -}; -MODULE_DEVICE_TABLE(spi, mc13xxx_device_id); - -static const struct of_device_id mc13xxx_dt_ids[] = { - { .compatible = "fsl,mc13783", .data = (void *) MC13XXX_ID_MC13783, }, - { .compatible = "fsl,mc13892", .data = (void *) MC13XXX_ID_MC13892, }, - { /* sentinel */ } -}; -MODULE_DEVICE_TABLE(of, mc13xxx_dt_ids); - -static struct regmap_config mc13xxx_regmap_spi_config = { - .reg_bits = 7, - .pad_bits = 1, - .val_bits = 24, - - .max_register = MC13XXX_NUMREGS, - - .cache_type = REGCACHE_NONE, -}; - -static int mc13xxx_common_init(struct mc13xxx *mc13xxx, - struct mc13xxx_platform_data *pdata, int irq); - -static void mc13xxx_common_cleanup(struct mc13xxx *mc13xxx); - -static int mc13xxx_spi_probe(struct spi_device *spi) -{ - const struct of_device_id *of_id; - struct spi_driver *sdrv = to_spi_driver(spi->dev.driver); - struct mc13xxx *mc13xxx; - struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev); - int ret; - - of_id = of_match_device(mc13xxx_dt_ids, &spi->dev); - if (of_id) - sdrv->id_table = &mc13xxx_device_id[(enum mc13xxx_id) of_id->data]; - - mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL); - if (!mc13xxx) - return -ENOMEM; - - dev_set_drvdata(
[PATCH v5 2/4] mfd: mc13xxx-core: use regmap for register access
This change converts the mc13xxx core to use regmap rather than direct spi r/w. The spidev member of mc13xxx struct becomes redundant and is removed. Extra debugging aids are added to mc13xxx_reg_rmw. Mutex init is moved to before regmap init. Signed-off-by: Marc Reilly --- drivers/mfd/Kconfig|1 + drivers/mfd/mc13xxx-core.c | 113 +-- 2 files changed, 35 insertions(+), 79 deletions(-) diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index cd13e9f..4e92513 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -576,6 +576,7 @@ config MFD_MC13XXX depends on SPI_MASTER select MFD_CORE select MFD_MC13783 + select REGMAP_SPI help Support for the Freescale (Atlas) PMIC and audio CODECs MC13783 and MC13892. diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c index 0753402..3223625 100644 --- a/drivers/mfd/mc13xxx-core.c +++ b/drivers/mfd/mc13xxx-core.c @@ -21,6 +21,8 @@ #include #include #include +#include +#include enum mc13xxx_id { MC13XXX_ID_MC13783, @@ -29,7 +31,7 @@ enum mc13xxx_id { }; struct mc13xxx { - struct spi_device *spidev; + struct regmap *regmap; struct device *dev; enum mc13xxx_id ictype; @@ -172,67 +174,6 @@ void mc13xxx_unlock(struct mc13xxx *mc13xxx) } EXPORT_SYMBOL(mc13xxx_unlock); -#define MC13XXX_REGOFFSET_SHIFT 25 -static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx, - unsigned int offset, u32 *val) -{ - struct spi_transfer t; - struct spi_message m; - int ret; - - *val = offset << MC13XXX_REGOFFSET_SHIFT; - - memset(&t, 0, sizeof(t)); - - t.tx_buf = val; - t.rx_buf = val; - t.len = sizeof(u32); - - spi_message_init(&m); - spi_message_add_tail(&t, &m); - - ret = spi_sync(mc13xxx->spidev, &m); - - /* error in message.status implies error return from spi_sync */ - BUG_ON(!ret && m.status); - - if (ret) - return ret; - - *val &= 0xff; - - return 0; -} - -static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, - u32 val) -{ - u32 buf; - struct spi_transfer t; - struct spi_message m; - int ret; - - buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val; - - memset(&t, 0, sizeof(t)); - - t.tx_buf = &buf; - t.rx_buf = &buf; - t.len = sizeof(u32); - - spi_message_init(&m); - spi_message_add_tail(&t, &m); - - ret = spi_sync(mc13xxx->spidev, &m); - - BUG_ON(!ret && m.status); - - if (ret) - return ret; - - return 0; -} - int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val) { int ret; @@ -242,7 +183,7 @@ int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val) if (offset > MC13XXX_NUMREGS) return -EINVAL; - ret = mc13xxx_spi_reg_read(mc13xxx, offset, val); + ret = regmap_read(mc13xxx->regmap, offset, val); dev_vdbg(mc13xxx->dev, "[0x%02x] -> 0x%06x\n", offset, *val); return ret; @@ -258,25 +199,19 @@ int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val) if (offset > MC13XXX_NUMREGS || val > 0xff) return -EINVAL; - return mc13xxx_spi_reg_write(mc13xxx, offset, val); + return regmap_write(mc13xxx->regmap, offset, val); } EXPORT_SYMBOL(mc13xxx_reg_write); int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset, u32 mask, u32 val) { - int ret; - u32 valread; - + BUG_ON(!mutex_is_locked(&mc13xxx->lock)); BUG_ON(val & ~mask); + dev_vdbg(mc13xxx->dev, "[0x%02x] <- 0x%06x (mask: 0x%06x)\n", + offset, val, mask); - ret = mc13xxx_reg_read(mc13xxx, offset, &valread); - if (ret) - return ret; - - valread = (valread & ~mask) | val; - - return mc13xxx_reg_write(mc13xxx, offset, valread); + return regmap_update_bits(mc13xxx->regmap, offset, mask, val); } EXPORT_SYMBOL(mc13xxx_reg_rmw); @@ -706,7 +641,7 @@ static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format) #ifdef CONFIG_OF static int mc13xxx_probe_flags_dt(struct mc13xxx *mc13xxx) { - struct device_node *np = mc13xxx->dev.of_node; + struct device_node *np = mc13xxx->dev->of_node; if (!np) return -ENODEV; @@ -752,6 +687,16 @@ static const struct of_device_id mc13xxx_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, mc13xxx_dt_ids); +static struct regmap_config mc13xxx_regmap_spi_config = { + .reg_bits = 7, + .pad_bits = 1, + .val_bits = 24, + + .max_register = MC13XXX_NUMREGS, + + .cache_type = REGCACHE_NONE, +}; + static int mc13xxx_common_init(struct mc13xxx *mc13xxx,
[PATCH v5 4/4] mfd: mc13xxx: Add i2c driver
Adds support for mc13xxx family ICs connected via i2c. Signed-off-by: Marc Reilly --- drivers/mfd/Kconfig |9 +++- drivers/mfd/Makefile |1 + drivers/mfd/mc13xxx-i2c.c | 128 + 3 files changed, 137 insertions(+), 1 deletions(-) create mode 100644 drivers/mfd/mc13xxx-i2c.c diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index fe0a0ab..7f4d85a 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -573,7 +573,7 @@ config MFD_MC13783 config MFD_MC13XXX tristate "Support Freescale MC13783 and MC13892" - depends on SPI_MASTER + depends on SPI_MASTER || I2C select MFD_CORE select MFD_MC13783 help @@ -591,6 +591,13 @@ config MFD_MC13XXX_SPI help Select this if your MC13xxx is connected via an SPI bus. +config MFD_MC13XXX_I2C + tristate "MC13xxx I2C interface" if I2C + default I2C + select REGMAP_I2C + help + Select this if your MC13xxx is connected via an I2C bus. + endif config ABX500_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 2dc66ed..6d4566f 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -54,6 +54,7 @@ obj-$(CONFIG_TWL6040_CORE)+= twl6040-core.o twl6040-irq.o obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o +obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o obj-$(CONFIG_MFD_CORE) += mfd-core.o diff --git a/drivers/mfd/mc13xxx-i2c.c b/drivers/mfd/mc13xxx-i2c.c new file mode 100644 index 000..d22501d --- /dev/null +++ b/drivers/mfd/mc13xxx-i2c.c @@ -0,0 +1,128 @@ +/* + * Copyright 2009-2010 Creative Product Design + * Marc Reilly m...@cpdesign.com.au + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "mc13xxx.h" + +static const struct i2c_device_id mc13xxx_i2c_device_id[] = { + { + .name = "mc13892", + .driver_data = MC13XXX_ID_MC13892, + }, { + /* sentinel */ + } +}; +MODULE_DEVICE_TABLE(i2c, mc13xxx_i2c_device_id); + +static const struct of_device_id mc13xxx_dt_ids[] = { + { + .compatible = "fsl,mc13892", + .data = (void *) &mc13xxx_i2c_device_id[0], + }, { + /* sentinel */ + } +}; +MODULE_DEVICE_TABLE(of, mc13xxx_dt_ids); + +static struct regmap_config mc13xxx_regmap_i2c_config = { + .reg_bits = 8, + .val_bits = 24, + + .max_register = MC13XXX_NUMREGS, + + .cache_type = REGCACHE_NONE, +}; + +static int mc13xxx_i2c_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + const struct of_device_id *of_id; + struct i2c_driver *idrv = to_i2c_driver(client->dev.driver); + struct mc13xxx *mc13xxx; + struct mc13xxx_platform_data *pdata = dev_get_platdata(&client->dev); + int ret; + + of_id = of_match_device(mc13xxx_dt_ids, &client->dev); + if (of_id) + idrv->id_table = (const struct i2c_device_id*) of_id->data; + + mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL); + if (!mc13xxx) + return -ENOMEM; + + dev_set_drvdata(&client->dev, mc13xxx); + + mc13xxx->dev = &client->dev; + mutex_init(&mc13xxx->lock); + + mc13xxx->regmap = regmap_init_i2c(client, &mc13xxx_regmap_i2c_config); + if (IS_ERR(mc13xxx->regmap)) { + ret = PTR_ERR(mc13xxx->regmap); + dev_err(mc13xxx->dev, "Failed to initialize register map: %d\n", + ret); + dev_set_drvdata(&client->dev, NULL); + kfree(mc13xxx); + return ret; + } + + ret = mc13xxx_common_init(mc13xxx, pdata, client->irq); + + if (ret == 0 && (id->driver_data != mc13xxx->ictype)) + dev_warn(mc13xxx->dev, + "device id doesn't match auto detection!\n"); + + return ret; +} + +static int __devexit mc13xxx_i2c_remove(struct i2c_client *client) +{ + struct mc13xxx *mc13xxx = dev_get_drvdata(&client->dev); + + mc13xxx_common_cleanup(mc13xxx); + + return 0; +} + +static struct i2c_driver mc13xxx_i2c_driver = { + .id_table = mc13xxx_i2c_device_id, + .driver = { + .owner = THIS_MODULE, + .name = "mc13xxx", + .of_match_table = mc13xxx_dt_ids, + }, + .probe = mc13xxx_i2c_probe, + .remove = __devexit_p(mc13xxx_i2c_remove), +}; + +static int __init mc13xxx_i2c_init(void) +{ + return i2c_add_driver(&mc13xxx_i2c_driver); +} +subsys_initcall(mc13xxx_i2c_init); + +static void __exit mc13xxx_i2c_exit(
Re: mc13xxx: add I2C support, V5
Hi Marc, On Sun, Apr 01, 2012 at 04:41:35PM +1000, Marc Reilly wrote: > This series (against mfd-2.6/for-next) changes the mc13xxx driver to use > regmap and adds I2C support. > It has a compile dependency on regmap/for-next, as the spi driver uses the > recently added pad_bits config field. > > Patch 2/4 has: > Reviewed-by: Mark Brown > Patch 4/4 has: > Signed-off-by: Oskar Schirmer You didn't add these tags to the respective patches. (And I think Signed-off-by: Oskar is wrong. Maybe this should be an Ack?) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: mc13xxx: add I2C support, V5
Hi Uwe, > On Sun, Apr 01, 2012 at 04:41:35PM +1000, Marc Reilly wrote: > > This series (against mfd-2.6/for-next) changes the mc13xxx driver to use > > regmap and adds I2C support. > > It has a compile dependency on regmap/for-next, as the spi driver uses > > the recently added pad_bits config field. > > > > Patch 2/4 has: > > Reviewed-by: Mark Brown > > Patch 4/4 has: > > Signed-off-by: Oskar Schirmer > > You didn't add these tags to the respective patches. (And I think > Signed-off-by: Oskar is wrong. Maybe this should be an Ack?) Whats the best/proper way to do this? Do I need to rebase the series and add them with each relevant commit, or just add them manually to the patches? Or option C... For Oskar's tag, I just copied it from an email from the previous thread. Cheers, Marc -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
RE: mc13xxx: add I2C support, V5
Hi Marc, thanks for V5. > This series (against mfd-2.6/for-next) changes the mc13xxx driver to use > regmap and adds I2C support. > It has a compile dependency on regmap/for-next, as the spi driver uses the > recently added pad_bits config field. If included in your plans to relocate the current version of patches for version 3.4? Cheers, Alex -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH/RFC] ARM: amba: Remove AMBA level regulator support
The AMBA bus regulator support is being used to model on/off switches for power domains which isn't terribly idiomatic for modern kernels with the generic power domain code and creates integration problems on platforms which don't use regulators for their power domains as it's hard to tell the difference between a regulator that is needed but failed to be provided and one that isn't supposed to be there (though DT does make that easier). Platforms that wish to use the regulator API to manage their power domains can indirect via the power domain interface. The impact should be minimal since currently there are no mainline systems which actually provide a vcore regulator so none need updating. Signed-off-by: Mark Brown --- RFC because there's some disagreement about this. drivers/amba/bus.c | 42 +- drivers/spi/spi-pl022.c |2 -- include/linux/amba/bus.h |8 3 files changed, 1 insertions(+), 51 deletions(-) diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index 01c2cf4..cc27322 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -247,8 +247,7 @@ static int amba_pm_restore(struct device *dev) /* * Hooks to provide runtime PM of the pclk (bus clock). It is safe to * enable/disable the bus clock at runtime PM suspend/resume as this - * does not result in loss of context. However, disabling vcore power - * would do, so we leave that to the driver. + * does not result in loss of context. */ static int amba_pm_runtime_suspend(struct device *dev) { @@ -354,39 +353,6 @@ static void amba_put_disable_pclk(struct amba_device *pcdev) clk_put(pclk); } -static int amba_get_enable_vcore(struct amba_device *pcdev) -{ - struct regulator *vcore = regulator_get(&pcdev->dev, "vcore"); - int ret; - - pcdev->vcore = vcore; - - if (IS_ERR(vcore)) { - /* It is OK not to supply a vcore regulator */ - if (PTR_ERR(vcore) == -ENODEV) - return 0; - return PTR_ERR(vcore); - } - - ret = regulator_enable(vcore); - if (ret) { - regulator_put(vcore); - pcdev->vcore = ERR_PTR(-ENODEV); - } - - return ret; -} - -static void amba_put_disable_vcore(struct amba_device *pcdev) -{ - struct regulator *vcore = pcdev->vcore; - - if (!IS_ERR(vcore)) { - regulator_disable(vcore); - regulator_put(vcore); - } -} - /* * These are the device model conversion veneers; they convert the * device model structures to our more specific structures. @@ -399,10 +365,6 @@ static int amba_probe(struct device *dev) int ret; do { - ret = amba_get_enable_vcore(pcdev); - if (ret) - break; - ret = amba_get_enable_pclk(pcdev); if (ret) break; @@ -420,7 +382,6 @@ static int amba_probe(struct device *dev) pm_runtime_put_noidle(dev); amba_put_disable_pclk(pcdev); - amba_put_disable_vcore(pcdev); } while (0); return ret; @@ -442,7 +403,6 @@ static int amba_remove(struct device *dev) pm_runtime_put_noidle(dev); amba_put_disable_pclk(pcdev); - amba_put_disable_vcore(pcdev); return ret; } diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 96f0da6..09c925a 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -2195,7 +2195,6 @@ static int pl022_runtime_suspend(struct device *dev) struct pl022 *pl022 = dev_get_drvdata(dev); clk_disable(pl022->clk); - amba_vcore_disable(pl022->adev); return 0; } @@ -2204,7 +2203,6 @@ static int pl022_runtime_resume(struct device *dev) { struct pl022 *pl022 = dev_get_drvdata(dev); - amba_vcore_enable(pl022->adev); clk_enable(pl022->clk); return 0; diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h index 7847e19..63a59ac 100644 --- a/include/linux/amba/bus.h +++ b/include/linux/amba/bus.h @@ -19,7 +19,6 @@ #include #include #include -#include #define AMBA_NR_IRQS 2 #define AMBA_CID 0xb105f00d @@ -30,7 +29,6 @@ struct amba_device { struct device dev; struct resource res; struct clk *pclk; - struct regulator*vcore; u64 dma_mask; unsigned intperiphid; unsigned intirq[AMBA_NR_IRQS]; @@ -75,12 +73,6 @@ void amba_release_regions(struct amba_device *); #define amba_pclk_disable(d) \ do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0) -#define amba_vcore_enable(d) \ - (IS_ERR((d)->vcore) ? 0 : regulator_enable((d)->vcore)) - -#define amba_vcore_disable(d) \ - do { if (!IS_ERR((d)->vcore)) regulator_disable((d)->vcore); } while (0)
Re: [PATCH/RFC] ARM: amba: Remove AMBA level regulator support
On Sun, Apr 1, 2012 at 5:29 PM, Mark Brown wrote: > The AMBA bus regulator support is being used to model on/off switches > for power domains which isn't terribly idiomatic for modern kernels with > the generic power domain code and creates integration problems on platforms > which don't use regulators for their power domains as it's hard to tell > the difference between a regulator that is needed but failed to be provided > and one that isn't supposed to be there (though DT does make that easier). > > Platforms that wish to use the regulator API to manage their power domains > can indirect via the power domain interface. I don't see how this solves the problem of AMBA PrimeCell probing. If you look at the code in bus.c, you can see that it acquires and enables the regulator - as it does with the clock. The reason is that is does this *before* the device can probe, since it needs to map up the memory to read out some magic values at the end to figure out what device it is in the first place. We need the current code replaced with something that enables a power domain before probe instead, then implement these power domains for the in-kernel AMBA devices that need it. Is the default behaviour of power domains such that they will be enabled as soon as devices are registered but before any buses probe()? Because that is what is needed in this case. (AMBA devices are special in this way: no other ARM things support auto-detection of devices using magic numbers, basically the DT stuff came about because noone was using a thing like this.) > The impact should be minimal since currently there are no mainline > systems which actually provide a vcore regulator so none need updating. Oh yes there are: drivers/mfd/db8500-prcmu.c This driver registers a number of voltage domain regulators, among those: static struct regulator_consumer_supply db8500_vape_consumers[] = { (...) REGULATOR_SUPPLY("vcore", "sdi0"), REGULATOR_SUPPLY("vcore", "sdi1"), REGULATOR_SUPPLY("vcore", "sdi2"), REGULATOR_SUPPLY("vcore", "sdi3"), REGULATOR_SUPPLY("vcore", "sdi4"), (...) REGULATOR_SUPPLY("vcore", "uart0"), REGULATOR_SUPPLY("vcore", "uart1"), REGULATOR_SUPPLY("vcore", "uart2"), (I know the supplies should probably be moved up to the platform but there they are.) The first array is MMC controllers and the second is a number of UARTs. IIRC the machine will not boot (i.e. these drivers cannot even probe) without these regulators in place, so they get enabled by the AMBA bus code. So we need something that not just removes stuff from the AMBA bus, but also adds a better power domain mechanism and fixes up taking the above regulators. That said I'm all for replacing it - but I'd need to figure out the details on how to do that. We do have code for ux500 power domains. If it will will be enough to handle this I can try to hack it up and submit it. Yours, Linus Walleij -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH/RFC v2] ARM: amba: Remove AMBA level regulator support
The AMBA bus regulator support is being used to model on/off switches for power domains which isn't terribly idiomatic for modern kernels with the generic power domain code and creates integration problems on platforms which don't use regulators for their power domains as it's hard to tell the difference between a regulator that is needed but failed to be provided and one that isn't supposed to be there (though DT does make that easier). Platforms that wish to use the regulator API to manage their power domains can indirect via the power domain interface. This feature is only used with the vape supply of the db8500 PRCMU driver which supplies the UARTs and MMC controllers, none of which have support for managing vcore at runtime in mainline (only pl022 SPI controller does). Update that supply to have an always_on constraint until the power domain support for the system is updated so that it is enabled for these users, this is likely to have no impact on practical systems as probably at least one of these devices will be active and cause AMBA to hold the supply on anyway. Signed-off-by: Mark Brown --- Updated to add the always_on constraint for db8500-prcmu as discussed in the changelog. drivers/amba/bus.c | 42 +- drivers/mfd/db8500-prcmu.c |1 + drivers/spi/spi-pl022.c|2 -- include/linux/amba/bus.h |8 4 files changed, 2 insertions(+), 51 deletions(-) diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index 01c2cf4..cc27322 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -247,8 +247,7 @@ static int amba_pm_restore(struct device *dev) /* * Hooks to provide runtime PM of the pclk (bus clock). It is safe to * enable/disable the bus clock at runtime PM suspend/resume as this - * does not result in loss of context. However, disabling vcore power - * would do, so we leave that to the driver. + * does not result in loss of context. */ static int amba_pm_runtime_suspend(struct device *dev) { @@ -354,39 +353,6 @@ static void amba_put_disable_pclk(struct amba_device *pcdev) clk_put(pclk); } -static int amba_get_enable_vcore(struct amba_device *pcdev) -{ - struct regulator *vcore = regulator_get(&pcdev->dev, "vcore"); - int ret; - - pcdev->vcore = vcore; - - if (IS_ERR(vcore)) { - /* It is OK not to supply a vcore regulator */ - if (PTR_ERR(vcore) == -ENODEV) - return 0; - return PTR_ERR(vcore); - } - - ret = regulator_enable(vcore); - if (ret) { - regulator_put(vcore); - pcdev->vcore = ERR_PTR(-ENODEV); - } - - return ret; -} - -static void amba_put_disable_vcore(struct amba_device *pcdev) -{ - struct regulator *vcore = pcdev->vcore; - - if (!IS_ERR(vcore)) { - regulator_disable(vcore); - regulator_put(vcore); - } -} - /* * These are the device model conversion veneers; they convert the * device model structures to our more specific structures. @@ -399,10 +365,6 @@ static int amba_probe(struct device *dev) int ret; do { - ret = amba_get_enable_vcore(pcdev); - if (ret) - break; - ret = amba_get_enable_pclk(pcdev); if (ret) break; @@ -420,7 +382,6 @@ static int amba_probe(struct device *dev) pm_runtime_put_noidle(dev); amba_put_disable_pclk(pcdev); - amba_put_disable_vcore(pcdev); } while (0); return ret; @@ -442,7 +403,6 @@ static int amba_remove(struct device *dev) pm_runtime_put_noidle(dev); amba_put_disable_pclk(pcdev); - amba_put_disable_vcore(pcdev); return ret; } diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c index ebc1e86..5be3248 100644 --- a/drivers/mfd/db8500-prcmu.c +++ b/drivers/mfd/db8500-prcmu.c @@ -2788,6 +2788,7 @@ static struct regulator_init_data db8500_regulators[DB8500_NUM_REGULATORS] = { .constraints = { .name = "db8500-vape", .valid_ops_mask = REGULATOR_CHANGE_STATUS, + .always_on = true, }, .consumer_supplies = db8500_vape_consumers, .num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers), diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 96f0da6..09c925a 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -2195,7 +2195,6 @@ static int pl022_runtime_suspend(struct device *dev) struct pl022 *pl022 = dev_get_drvdata(dev); clk_disable(pl022->clk); - amba_vcore_disable(pl022->adev); return 0; } @@ -2204,7 +2203,6 @@ static int pl022_runtime_resume(struct device *dev) { struct pl022 *pl022 = dev_get_drvdata(dev); - amb
Re: [PATCH/RFC v2] ARM: amba: Remove AMBA level regulator support
On Sun, Apr 1, 2012 at 8:58 PM, Mark Brown wrote: > diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c > index ebc1e86..5be3248 100644 > --- a/drivers/mfd/db8500-prcmu.c > +++ b/drivers/mfd/db8500-prcmu.c > @@ -2788,6 +2788,7 @@ static struct regulator_init_data > db8500_regulators[DB8500_NUM_REGULATORS] = { > .constraints = { > .name = "db8500-vape", > .valid_ops_mask = REGULATOR_CHANGE_STATUS, > + .always_on = true, > }, > .consumer_supplies = db8500_vape_consumers, > .num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers), Combined with the PL022 patch this causes a power regression since the PL022 is hereafter always on. But I guess if I fix a power domain patch to accomplish much the same things then nothing is really lost... And I do like the change, if for nothing else so for the fact that it eventually pushes to power domains what belongs there, so: Acked-by: Linus Walleij But to the defence: power domain code was not in the kernel when the AMBA "vcore" regulator was introduced so how else could we do it... except for inventing power domains... Yours, Linus Walleij -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH/RFC] ARM: amba: Remove AMBA level regulator support
On Sun, Apr 1, 2012 at 22:52, Linus Walleij wrote: > On Sun, Apr 1, 2012 at 5:29 PM, Mark Brown > wrote: >> The impact should be minimal since currently there are no mainline >> systems which actually provide a vcore regulator so none need updating. > > Oh yes there are: > drivers/mfd/db8500-prcmu.c > > This driver registers a number of voltage domain regulators, > among those: > > static struct regulator_consumer_supply db8500_vape_consumers[] = { > (...) > REGULATOR_SUPPLY("vcore", "sdi0"), > REGULATOR_SUPPLY("vcore", "sdi1"), > REGULATOR_SUPPLY("vcore", "sdi2"), > REGULATOR_SUPPLY("vcore", "sdi3"), > REGULATOR_SUPPLY("vcore", "sdi4"), > (...) > REGULATOR_SUPPLY("vcore", "uart0"), > REGULATOR_SUPPLY("vcore", "uart1"), > REGULATOR_SUPPLY("vcore", "uart2"), > > IIRC the machine will not boot (i.e. these drivers cannot even > probe) without these regulators in place, so they get enabled by > the AMBA bus code. These vcore "regulators" do nothing but increment a variable which is write-only in mainline, so the machine will boot and drivers will probe fine with Mark's patch. -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH/RFC v2] ARM: amba: Remove AMBA level regulator support
On Mon, Apr 2, 2012 at 00:52, Linus Walleij wrote: > On Sun, Apr 1, 2012 at 8:58 PM, Mark Brown > wrote: > >> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c >> index ebc1e86..5be3248 100644 >> --- a/drivers/mfd/db8500-prcmu.c >> +++ b/drivers/mfd/db8500-prcmu.c >> @@ -2788,6 +2788,7 @@ static struct regulator_init_data >> db8500_regulators[DB8500_NUM_REGULATORS] = { >> .constraints = { >> .name = "db8500-vape", >> .valid_ops_mask = REGULATOR_CHANGE_STATUS, >> + .always_on = true, >> }, >> .consumer_supplies = db8500_vape_consumers, >> .num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers), > > Combined with the PL022 patch this causes a power regression since > the PL022 is hereafter always on. How can there be a "power regression" here? This is the only user of the vcore regulator, and, apart from the fact that its disable routine only decrements an essentialy write-only variable, it is shared which several devices which already never disable it. -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Grosses Soldes de printemps chez Cdiscount : jusqu'à -95 %
Pour voir le message, veuillez utiliser un lecteur de mail compatible HTML Lien miroir : http://m10-fr.com/mc10_m/YT04JmI9ODgxNSZjPTE5NjE2NDEmZD0yMDEyLTA0LTAxIDIyOjIwOjAxJmU9MSZoPTg4MTQmZj04ODE1Jmc9ODgxNQ== Lien de désinscription : http://m10-fr.com/mc10_unsub/YT04JmI9ODgxNSZjPTE5NjE2NDEmZD0yMDEyLTA0LTAxIDIyOjIwOjAxJmU9MSZoPTg4MTQmZj04ODE1Jmc9ODgxNQ== -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH/RFC v2] ARM: amba: Remove AMBA level regulator support
On Sun, Apr 1, 2012 at 9:39 PM, Rabin Vincent wrote: > On Mon, Apr 2, 2012 at 00:52, Linus Walleij wrote: >> Combined with the PL022 patch this causes a power regression since >> the PL022 is hereafter always on. > > How can there be a "power regression" here? This is the only user of > the vcore regulator, and, apart from the fact that its disable routine > only decrements an essentialy write-only variable, it is shared which > several devices which already never disable it. Yes true... Hm I hope something else in mainline will increase refcount to vape so it's not disabled at regulator_has_full_constraints()? Better test it, then I'll see. Mark: as Rabin says the v1 patch is probably fine, are you pushing this to ARM SoC or into Russell's patch tracker? Yours, Linus Walleij -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH/RFC v2] ARM: amba: Remove AMBA level regulator support
On Sun, Apr 1, 2012 at 11:27 PM, Mark Brown wrote: > On Sun, Apr 01, 2012 at 09:22:50PM +0200, Linus Walleij wrote: > >> Combined with the PL022 patch this causes a power regression since >> the PL022 is hereafter always on. > > I guess this code isn't in mainline, though? In that case you can > always add a revert of this commit to your out of tree patches if you > need to. No, we can sure live with it... Out-of-mainline we do use power domains so that's what we should do instead. It currently looks like this: http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f=arch/arm/mach-ux500/pm/runtime.c;hb=HEAD It's a really nice piece of code but uses some out-of-tree features, the most obvious one is "atomic regulators" (which are exactly that). >> But to the defence: power domain code was not in the kernel >> when the AMBA "vcore" regulator was introduced so how else >> could we do it... except for inventing power domains... > > Which might've happened sooner if we'd noticed :) There were some other > platforms doing similar things but they mostly used the clock API since > it was always entirely platform code until 3.4 so they're less intrusive > into the generic code. Yeah ... but this sounds familiar, (searching searching) Yes! We did ask on the lists if regulators were proper for modeling power domains in 2008: http://marc.info/?l=linux-arm-kernel&m=121580531500758&w=2 But I should've pushed for a proper answer ... Yours, Linus Walleij -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: mc13xxx: add I2C support, V5
Hello Marc, On Sun, Apr 01, 2012 at 07:39:49PM +1000, Marc Reilly wrote: > > On Sun, Apr 01, 2012 at 04:41:35PM +1000, Marc Reilly wrote: > > > This series (against mfd-2.6/for-next) changes the mc13xxx driver to use > > > regmap and adds I2C support. > > > It has a compile dependency on regmap/for-next, as the spi driver uses > > > the recently added pad_bits config field. > > > > > > Patch 2/4 has: > > > Reviewed-by: Mark Brown > > > Patch 4/4 has: > > > Signed-off-by: Oskar Schirmer > > > > You didn't add these tags to the respective patches. (And I think > > Signed-off-by: Oskar is wrong. Maybe this should be an Ack?) > > Whats the best/proper way to do this? Do I need to rebase the series and add > them with each relevant commit, or just add them manually to the patches? Or > option C... Rebasing is ok, still more as you didn't publish your tree (at least I'm not aware of it). The tags should definitly make it into the final history. When you submit the patches via email it doesn't matter if you add them to your git tree or only to the mail (as you tree doesn't matter for the final result). > For Oskar's tag, I just copied it from an email from the previous thread. Yeah, I saw it. That was already wrong. S-o-b is only for the author(s) of a patch and the people forwarding it for inclusion (either via mail or via a git tree). A S-o-b certifies that the patch's copyright is suitable for the kernel. It doesn't say anything about its technical impact. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general