Re: [PATCH v3 6/6] mfd: mc13xxx: Add i2c driver
Hi Marc,
On Wed, Mar 14, 2012 at 5:43 PM, Marc Reilly wrote:
> +
> +static const struct i2c_device_id mc13xxx_i2c_device_id[] = {
> + {
> + .name = "mc13783",
> + .driver_data = MC13XXX_ID_MC13783
mc13783 does not have i2c interface.
--
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 v3 6/6] mfd: mc13xxx: Add i2c driver
Adds support for mc13xxx family ICs connected via i2c.
Signed-off-by: Marc Reilly
---
drivers/mfd/Kconfig |8 +++-
drivers/mfd/Makefile |1 +
drivers/mfd/mc13xxx-i2c.c | 128 +
3 files changed, 136 insertions(+), 1 deletions(-)
create mode 100644 drivers/mfd/mc13xxx-i2c.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 501fee5..3a039ff 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
@@ -590,6 +590,12 @@ 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
+ 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..1fb89af
--- /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 = "mc13783",
+ .driver_data = MC13XXX_ID_MC13783
+ }, {
+ .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,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_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 =
+ &mc13xxx_i2c_device_id[(enum mc13xxx_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);
+ 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
[PATCH v3 2/6] regmap: Add support for device with 24 data bits.
Add support for devices with 24 data bits.
Signed-off-by: Marc Reilly
---
drivers/base/regmap/regmap.c | 23 +++
1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 62ef0df..880e3a0 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -122,6 +122,15 @@ static void regmap_format_16(void *buf, unsigned int val)
b[0] = cpu_to_be16(val);
}
+static void regmap_format_24(void *buf, unsigned int val)
+{
+ u8 *b = buf;
+
+ b[0] = val >> 16;
+ b[1] = val >> 8;
+ b[2] = val;
+}
+
static unsigned int regmap_parse_8(void *buf)
{
u8 *b = buf;
@@ -138,6 +147,16 @@ static unsigned int regmap_parse_16(void *buf)
return b[0];
}
+static unsigned int regmap_parse_24(void *buf)
+{
+ u8 *b = buf;
+ unsigned int ret = b[2];
+ ret |= ((unsigned int)b[1]) << 8;
+ ret |= ((unsigned int)b[0]) << 16;
+
+ return ret;
+}
+
/**
* regmap_init(): Initialise register map
*
@@ -240,6 +259,10 @@ struct regmap *regmap_init(struct device *dev,
map->format.format_val = regmap_format_16;
map->format.parse_val = regmap_parse_16;
break;
+ case 24:
+ map->format.format_val = regmap_format_24;
+ map->format.parse_val = regmap_parse_24;
+ break;
}
if (!map->format.format_write &&
--
1.7.3.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 v3 4/6] 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/mc13xxx-core.c | 109 ---
1 files changed, 31 insertions(+), 78 deletions(-)
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 0753402..9804572 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);
@@ -752,6 +687,15 @@ 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,
+ .val_bits = 25,
+
+ .max_register = MC13XXX_NUMREGS,
+
+ .cache_type = REGCACHE_NONE,
+};
+
static int mc13xxx_common_init(struct mc13xxx *mc13xxx,
struct mc13xxx_platform_data *pdata, int irq);
@@ -776,10 +720,18 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
dev_set_drvdata(&spi->dev, mc13xxx);
spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
spi->bits_per_word = 32;
- spi_setup(spi);
mc13xxx->dev = &spi->dev;
- mc13xxx->spidev = spi;
+ mutex_init(&mc13xxx->lock);
+
+ mc13xxx->regmap = regmap_init_spi(spi, &mc13xxx_regmap_spi_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(&spi->dev, NULL);
+ return ret;
+ }
ret = mc13xxx_common_init(mc13xxx, pda
[PATCH v3 5/6] 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| 15 -
drivers/mfd/Makefile |1 +
drivers/mfd/mc13xxx-core.c | 146 ++--
drivers/mfd/mc13xxx-spi.c | 138 +
drivers/mfd/mc13xxx.h | 45 ++
5 files changed, 201 insertions(+), 144 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 cd13e9f..501fee5 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -577,12 +577,21 @@ config MFD_MC13XXX
select MFD_CORE
select MFD_MC13783
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
+ 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 9804572..31dbf91 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,88 +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,
- .val_bits = 25,
-
- .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(&spi->dev, mc13xxx);
- spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
- spi->bits_per_word = 32;
-
-
[PATCH v3 3/6] 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 v3 1/6] regmap: add support for 7_25 format
This adds support for devices with 7 address bits and 25 data bits.
The initial intended user for this are the mc13xxx mfds in spi mode.
(The 25th data bit is actually a dummy bit)
Signed-off-by: Marc Reilly
---
drivers/base/regmap/regmap.c | 11 +++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index be10a4f..62ef0df 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -100,6 +100,14 @@ static void regmap_format_10_14_write(struct regmap *map,
out[0] = reg >> 2;
}
+static void regmap_format_7_25_write(struct regmap *map,
+ unsigned int reg, unsigned int val)
+{
+ __be32 *out = map->work_buf;
+
+ *out = (reg << 25) | val;
+}
+
static void regmap_format_8(void *buf, unsigned int val)
{
u8 *b = buf;
@@ -193,6 +201,9 @@ struct regmap *regmap_init(struct device *dev,
case 9:
map->format.format_write = regmap_format_7_9_write;
break;
+ case 25:
+ map->format.format_write = regmap_format_7_25_write;
+ break;
default:
goto err_map;
}
--
1.7.3.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
mc13xxx: add I2C support (now with regmap), V3
Hi, This series (against mfd-2.6/for-next) changes the mc13xxx driver to use regmap and adds I2C support. It generally goes about this the same way as the previous 2 versions, but now uses regmap. I am unable to test SPI functionality, but the I2C stuff works. It would be great if someone could test the SPI side again. The first 2 patches add required support to regmap for the mc13xxx register format(s), I've included them here for convenience. @Mark B: I also have some versions of these that apply against regmap/for-next. Cheers, Marc -- 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/3] i2c-s3c2410: Rework device type handling
On Tue, Mar 13, 2012 at 05:54:38PM +0100, Karol Lewandowski wrote: > - replace s3c24xx_i2c_type enum with plain unsigned int that can >hold not only device type but also hw revision-specific quirks Would it not be clearer to just have explicit flags for the quirks (eg, as a set of bitfield flags)? -- 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: Add support for 16bit register access
On Wednesday 14 March 2012 09:19:22 Bhupesh SHARMA wrote:
> > > > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > > > @@ -164,7 +164,14 @@ static char *abort_sources[] = {
> > > >
> > > > u32 dw_readl(struct dw_i2c_dev *dev, int offset)
> > > > {
> > > >
> > > > - u32 value = readl(dev->base + offset);
> > > > + u32 value;
> > > > +
> > > > + if (dev->access_16bit) {
> > > > + value = readw(dev->base + offset) |
> > > > + (readw(dev->base + offset + 2) << 16);
> > > > + } else {
> > > > + value = readl(dev->base + offset);
> > > > + }
> > >
> > > We can do away with '{' parenthesis as these are single line
> > > of code inside both the 'if-else' blocks.
> >
> > It's not a single-line statement. The first block extends spans 2
> > lines. At
> > least that how I interpret this CodingStyle recommendation.
>
> ???
> Breaking a single line of code into two for readability does not
> make them two separate executable statements.
>
> As per CodingStyle recommendation:
> Do not unnecessarily use braces where a single statement will do.
>
> if (condition)
> action();
>
> So, please modify your patch as braces here waste precious screen
> space and reduce readability.
I have no strong preferences here. I it helps getting this patch accepted,
I'll remove those braces.
> > > > if (dev->swab)
> > > >
> > > > return swab32(value);
> > > >
> > > > @@ -177,7 +184,12 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b,
> >
> > int
> >
> > > > offset)
> > > >
> > > > if (dev->swab)
> > > >
> > > > b = swab32(b);
> > > >
> > > > - writel(b, dev->base + offset);
> > > > + if (dev->access_16bit) {
> > > > + writew((u16)b, dev->base + offset);
> > > > + writew((u16)(b >> 16), dev->base + offset + 2);
> > > > + } else {
> > > > + writel(b, dev->base + offset);
> > > > + }
> > > >
> > > > }
> > > >
> > > > static u32
> > > >
> > > > @@ -258,6 +270,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> > > >
> > > > reg = DW_IC_COMP_TYPE_VALUE;
> > > >
> > > > }
> > > >
> > > > + /* Configure register access mode 16bit */
> > > > + if (reg == (DW_IC_COMP_TYPE_VALUE & 0x)) {
> > > > + dev->access_16bit = 1;
> > >
> > > Can we use a suitable macro for 0x?
> >
> > Hmmm. Wouldn't that make it more complex? 0x is easy to
> > understand. A
> > marco would "hide" this value. I would prefer to keep the value.
>
> Using a macro doesn't make things 'more complex', but more readable.
> Magic numbers must be avoided at all cost. A better
> named MACRO is always better (for anyone else reading the code)
> than a magic number like 0x.
I really don't share your point of view here. I feel that in this case, the
number is much better readable than a macro.
> > > Also, if dev->access_16bit is bool we can simply set:
> > > dev->access_16bit = true;
> > >
> > > more on that below...
> > >
> > > > + reg = DW_IC_COMP_TYPE_VALUE;
> > > > + }
> > > > +
> > > >
> > > > if (reg != DW_IC_COMP_TYPE_VALUE) {
> > > >
> > > > dev_err(dev->dev, "Unknown Synopsys component type: "
> > > >
> > > > "0x%08x\n", reg);
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-designware-core.h
> > > > b/drivers/i2c/busses/i2c-designware-core.h
> > > > index 02d1a2d..f5af101 100644
> > > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > > @@ -83,6 +83,7 @@ struct dw_i2c_dev {
> > > >
> > > > u32 abort_source;
> > > > int irq;
> > > > int swab;
> > > >
> > > > + int access_16bit;
> > >
> > > ...
> > > int?? Probably we are better off with making this as bool.
> >
> > I'm not a big fan of bool's. But I have no strong preference here. My
> > reasoning here was consistency. As we already have "int swab" for a
> > similar
> > issue.
>
> If we have not done it earlier, doesn't mean that we repeat the same
> mistake again. There is no reason to take access_16bit as an int when a
> bool will suffice.
>
> This wastes storage and on some platforms (which have real crunch of
> memory), such saving is critical.
Again, I have no big problem changing this to bool.
> > So basically, I would prefer to not make the changes you suggested. But
> > in the
> > end its the decision of the maintainer(s).
>
> Linux is a collaborative world and patches can be reviewed by
> literally anyone :)
Sure.
> I am sure the maintainer(s) will find my comments worth adding in your
> patch..
Might be. But who is the maintainer of this driver?
Thanks,
Stefan
--
To unsubscribe from this list: send t
RE: [PATCH] i2c: designware: Add support for 16bit register access
> -Original Message-
> From: Jean Delvare [mailto:kh...@linux-fr.org]
> Sent: Wednesday, March 14, 2012 2:14 PM
> To: Bhupesh SHARMA
> Cc: Stefan Roese; linux-i2c@vger.kernel.org; spear-devel; ben-
> li...@fluff.org
> Subject: Re: [PATCH] i2c: designware: Add support for 16bit register
> access
>
> On Wed, 14 Mar 2012 16:19:22 +0800, Bhupesh SHARMA wrote:
> > > -Original Message-
> > > From: Stefan Roese [mailto:s...@denx.de]
> > > Sent: Wednesday, March 14, 2012 1:28 PM
> > > To: Bhupesh SHARMA
> > > Cc: linux-i2c@vger.kernel.org; spear-devel; ben-li...@fluff.org
> > > Subject: Re: [PATCH] i2c: designware: Add support for 16bit
> register
> > > access
> > >
> > > On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > > > > -Original Message-
> > > > > From: Stefan Roese [mailto:s...@denx.de]
> > > > > Sent: Tuesday, March 13, 2012 9:24 PM
> > > > > To: linux-i2c@vger.kernel.org
> > > > > Cc: spear-devel; ben-li...@fluff.org
> > > > > Subject: [PATCH] i2c: designware: Add support for 16bit
> register access
> > > > > (...)
> > > > > @@ -258,6 +270,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> > > > >
> > > > > reg = DW_IC_COMP_TYPE_VALUE;
> > > > >
> > > > > }
> > > > >
> > > > > + /* Configure register access mode 16bit */
> > > > > + if (reg == (DW_IC_COMP_TYPE_VALUE & 0x)) {
> > > > > + dev->access_16bit = 1;
> > > >
> > > > Can we use a suitable macro for 0x?
> > >
> > > Hmmm. Wouldn't that make it more complex? 0x is easy to
> > > understand. A
> > > marco would "hide" this value. I would prefer to keep the value.
> >
> > Using a macro doesn't make things 'more complex', but more readable.
> > Magic numbers must be avoided at all cost. A better
> > named MACRO is always better (for anyone else reading the code)
> > than a magic number like 0x.
>
> I beg to disagree. Quite strongly, even.
>
> For one thing, "at all cost" never holds in the real world. You always
> have to make compromises.
>
> For another, it only makes sense to define constants for values that
> have a specific meaning. For example bits in a bit vector, or bit
> masks. Here this is clearly not the case, so I fully agree with Stefan
> that a define would make the code _less_ readable.
Well you can have your opinion, let's wait for the
maintainer's words..
> > > > > (...)
> > > > > diff --git a/drivers/i2c/busses/i2c-designware-core.h
> > > > > b/drivers/i2c/busses/i2c-designware-core.h
> > > > > index 02d1a2d..f5af101 100644
> > > > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > > > @@ -83,6 +83,7 @@ struct dw_i2c_dev {
> > > > >
> > > > > u32 abort_source;
> > > > > int irq;
> > > > > int swab;
> > > > >
> > > > > + int access_16bit;
> > > >
> > > > ...
> > > > int?? Probably we are better off with making this as bool.
> > >
> > > I'm not a big fan of bool's. But I have no strong preference here.
> My
> > > reasoning here was consistency. As we already have "int swab" for a
> > > similar
> > > issue.
> >
> > If we have not done it earlier, doesn't mean that we repeat the same
> > mistake again. There is no reason to take access_16bit as an int when
> a bool
> > will suffice.
> >
> > This wastes storage and on some platforms (which have real crunch of
> memory),
> > such saving is critical.
> >
> > > So basically, I would prefer to not make the changes you suggested.
> But
> > > in the
> > > end its the decision of the maintainer(s).
> > >
> >
> > Linux is a collaborative world and patches can be reviewed by
> > literally anyone :)
>
> Likewise, everyone can send patches to address issues that bother them.
> Want these ints turned into bools? Stop yelling at Stefan, and do it
> yourself. If it is so critical, no doubt you'll find reviewers to ack
> your patch and maintainers to apply it. And you'll even get the fame.
You missed the point completely..
We have done a thing in 'X' way but later we realize that 'Y'
is a better way of doing it and when we bring out something new
we still use the 'X' way and claim that it was always done that way
doesn't make sense. It's better to use 'Y' way for everything new
and gradually shift older stuff too..
> > I am sure the maintainer(s) will find my comments worth adding in
> your patch..
>
> I'm not in charge of this specific driver, so I can't speak for the
> maintainer. And you shouldn't either...
>
Yes of-course.
Let's wait for the maintainer(s) words.
Regards,
Bhupesh
--
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: Add support for 16bit register access
On Wed, 14 Mar 2012 16:19:22 +0800, Bhupesh SHARMA wrote:
> > -Original Message-
> > From: Stefan Roese [mailto:s...@denx.de]
> > Sent: Wednesday, March 14, 2012 1:28 PM
> > To: Bhupesh SHARMA
> > Cc: linux-i2c@vger.kernel.org; spear-devel; ben-li...@fluff.org
> > Subject: Re: [PATCH] i2c: designware: Add support for 16bit register
> > access
> >
> > On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > > > -Original Message-
> > > > From: Stefan Roese [mailto:s...@denx.de]
> > > > Sent: Tuesday, March 13, 2012 9:24 PM
> > > > To: linux-i2c@vger.kernel.org
> > > > Cc: spear-devel; ben-li...@fluff.org
> > > > Subject: [PATCH] i2c: designware: Add support for 16bit register access
> > > > (...)
> > > > @@ -258,6 +270,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> > > >
> > > > reg = DW_IC_COMP_TYPE_VALUE;
> > > >
> > > > }
> > > >
> > > > + /* Configure register access mode 16bit */
> > > > + if (reg == (DW_IC_COMP_TYPE_VALUE & 0x)) {
> > > > + dev->access_16bit = 1;
> > >
> > > Can we use a suitable macro for 0x?
> >
> > Hmmm. Wouldn't that make it more complex? 0x is easy to
> > understand. A
> > marco would "hide" this value. I would prefer to keep the value.
>
> Using a macro doesn't make things 'more complex', but more readable.
> Magic numbers must be avoided at all cost. A better
> named MACRO is always better (for anyone else reading the code)
> than a magic number like 0x.
I beg to disagree. Quite strongly, even.
For one thing, "at all cost" never holds in the real world. You always
have to make compromises.
For another, it only makes sense to define constants for values that
have a specific meaning. For example bits in a bit vector, or bit
masks. Here this is clearly not the case, so I fully agree with Stefan
that a define would make the code _less_ readable.
> > > > (...)
> > > > diff --git a/drivers/i2c/busses/i2c-designware-core.h
> > > > b/drivers/i2c/busses/i2c-designware-core.h
> > > > index 02d1a2d..f5af101 100644
> > > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > > @@ -83,6 +83,7 @@ struct dw_i2c_dev {
> > > >
> > > > u32 abort_source;
> > > > int irq;
> > > > int swab;
> > > >
> > > > + int access_16bit;
> > >
> > > ...
> > > int?? Probably we are better off with making this as bool.
> >
> > I'm not a big fan of bool's. But I have no strong preference here. My
> > reasoning here was consistency. As we already have "int swab" for a
> > similar
> > issue.
>
> If we have not done it earlier, doesn't mean that we repeat the same
> mistake again. There is no reason to take access_16bit as an int when a bool
> will suffice.
>
> This wastes storage and on some platforms (which have real crunch of memory),
> such saving is critical.
>
> > So basically, I would prefer to not make the changes you suggested. But
> > in the
> > end its the decision of the maintainer(s).
> >
>
> Linux is a collaborative world and patches can be reviewed by
> literally anyone :)
Likewise, everyone can send patches to address issues that bother them.
Want these ints turned into bools? Stop yelling at Stefan, and do it
yourself. If it is so critical, no doubt you'll find reviewers to ack
your patch and maintainers to apply it. And you'll even get the fame.
> I am sure the maintainer(s) will find my comments worth adding in your
> patch..
I'm not in charge of this specific driver, so I can't speak for the
maintainer. And you shouldn't either...
Thanks,
--
Jean Delvare
--
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-algo-bit: Fix spurious SCL timeouts under heavy load
When the system is under heavy load, there can be a significant delay
between the getscl() and time_after() calls inside sclhi(). That delay
may cause the time_after() check to trigger after SCL has gone high,
causing sclhi() to return -ETIMEDOUT.
To fix the problem, double check that SCL is still low after the
timeout has been reached, before deciding to return -ETIMEDOUT.
Signed-off-by: Ville Syrjälä
---
I can easily reproduce these spurious timeouts on my HP-compaq nc6000
laptop with the radeon kms driver. It's enough to have a -j2 kernel
build running, and simultaneosly issue xrandr commands in a
terminal. Calling xrandr will cause the driver to re-read the EDID
from the display. A significant number of the EDID reads will fail.
With this fix I have yet to see any failed EDID reads.
drivers/i2c/algos/i2c-algo-bit.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index 525c734..d25112e 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -104,9 +104,11 @@ static int sclhi(struct i2c_algo_bit_data *adap)
* are processing data internally.
*/
if (time_after(jiffies, start + adap->timeout))
- return -ETIMEDOUT;
+ break;
cond_resched();
}
+ if (!getscl(adap))
+ return -ETIMEDOUT;
#ifdef DEBUG
if (jiffies != start && i2c_debug >= 3)
pr_debug("i2c-algo-bit: needed %ld jiffies for SCL to go "
--
1.7.3.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
[DEBUG PATCH] Print a message when a spurious i2c SCL timeout occurs.
A quick hack to verify that the fix works as intended...
Do not apply.
---
drivers/i2c/algos/i2c-algo-bit.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index d25112e..3f547b5 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -89,6 +89,7 @@ static inline void scllo(struct i2c_algo_bit_data *adap)
static int sclhi(struct i2c_algo_bit_data *adap)
{
unsigned long start;
+ bool timeout = false;
setscl(adap, 1);
@@ -104,11 +105,16 @@ static int sclhi(struct i2c_algo_bit_data *adap)
* are processing data internally.
*/
if (time_after(jiffies, start + adap->timeout))
+ {
+ timeout = true;
break;
+ }
cond_resched();
}
if (!getscl(adap))
return -ETIMEDOUT;
+ if (timeout)
+ printk(KERN_CRIT "spurious i2c scl timeout\n");
#ifdef DEBUG
if (jiffies != start && i2c_debug >= 3)
pr_debug("i2c-algo-bit: needed %ld jiffies for SCL to go "
--
1.7.3.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] i2c multiplexer driver for Proliant microserver N36L
Hi Thomas, > With the updated patch and the modified sensors-detect I have been > able to probe all ports and have not encountered the power issue > again. If no further issues are identified I will seperate the patch > into stages and submit for review. I confirm that your patch works well on my proliant n36l and solve the power issue. Thank you for rewrote my patch. Regards, Eddi -- 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: Add support for 16bit register access
Hi Stefan,
> -Original Message-
> From: Stefan Roese [mailto:s...@denx.de]
> Sent: Wednesday, March 14, 2012 1:28 PM
> To: Bhupesh SHARMA
> Cc: linux-i2c@vger.kernel.org; spear-devel; ben-li...@fluff.org
> Subject: Re: [PATCH] i2c: designware: Add support for 16bit register
> access
>
> Hi Bhupesh,
>
> On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > > -Original Message-
> > > From: Stefan Roese [mailto:s...@denx.de]
> > > Sent: Tuesday, March 13, 2012 9:24 PM
> > > To: linux-i2c@vger.kernel.org
> > > Cc: spear-devel; ben-li...@fluff.org
> > > Subject: [PATCH] i2c: designware: Add support for 16bit register
> access
> > >
> > > The STM SPEAr platform can only access the i2c controller register
> > > via 16bit read/write functions. This patch adds support to
> > > automatically detect this 16bit access mode.
> > >
> > > Signed-off-by: Stefan Roese
> > > ---
> > >
> > > drivers/i2c/busses/i2c-designware-core.c | 22
> --
> > > drivers/i2c/busses/i2c-designware-core.h |1 +
> > > 2 files changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > > b/drivers/i2c/busses/i2c-designware-core.c
> > > index df87992..d1facbc 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.c
> > > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > > @@ -164,7 +164,14 @@ static char *abort_sources[] = {
> > >
> > > u32 dw_readl(struct dw_i2c_dev *dev, int offset)
> > > {
> > >
> > > - u32 value = readl(dev->base + offset);
> > > + u32 value;
> > > +
> > > + if (dev->access_16bit) {
> > > + value = readw(dev->base + offset) |
> > > + (readw(dev->base + offset + 2) << 16);
> > > + } else {
> > > + value = readl(dev->base + offset);
> > > + }
> >
> > We can do away with '{' parenthesis as these are single line
> > of code inside both the 'if-else' blocks.
>
> It's not a single-line statement. The first block extends spans 2
> lines. At
> least that how I interpret this CodingStyle recommendation.
???
Breaking a single line of code into two for readability does not
make them two separate executable statements.
As per CodingStyle recommendation:
Do not unnecessarily use braces where a single statement will do.
if (condition)
action();
So, please modify your patch as braces here waste precious screen
space and reduce readability.
> > > if (dev->swab)
> > >
> > > return swab32(value);
> > >
> > > @@ -177,7 +184,12 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b,
> int
> > > offset)
> > >
> > > if (dev->swab)
> > >
> > > b = swab32(b);
> > >
> > > - writel(b, dev->base + offset);
> > > + if (dev->access_16bit) {
> > > + writew((u16)b, dev->base + offset);
> > > + writew((u16)(b >> 16), dev->base + offset + 2);
> > > + } else {
> > > + writel(b, dev->base + offset);
> > > + }
> > >
> > > }
> > >
> > > static u32
> > >
> > > @@ -258,6 +270,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> > >
> > > reg = DW_IC_COMP_TYPE_VALUE;
> > >
> > > }
> > >
> > > + /* Configure register access mode 16bit */
> > > + if (reg == (DW_IC_COMP_TYPE_VALUE & 0x)) {
> > > + dev->access_16bit = 1;
> >
> > Can we use a suitable macro for 0x?
>
> Hmmm. Wouldn't that make it more complex? 0x is easy to
> understand. A
> marco would "hide" this value. I would prefer to keep the value.
Using a macro doesn't make things 'more complex', but more readable.
Magic numbers must be avoided at all cost. A better
named MACRO is always better (for anyone else reading the code)
than a magic number like 0x.
> > Also, if dev->access_16bit is bool we can simply set:
> > dev->access_16bit = true;
> >
> > more on that below...
> >
> > > + reg = DW_IC_COMP_TYPE_VALUE;
> > > + }
> > > +
> > >
> > > if (reg != DW_IC_COMP_TYPE_VALUE) {
> > >
> > > dev_err(dev->dev, "Unknown Synopsys component type: "
> > >
> > > "0x%08x\n", reg);
> > >
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.h
> > > b/drivers/i2c/busses/i2c-designware-core.h
> > > index 02d1a2d..f5af101 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > @@ -83,6 +83,7 @@ struct dw_i2c_dev {
> > >
> > > u32 abort_source;
> > > int irq;
> > > int swab;
> > >
> > > + int access_16bit;
> >
> > ...
> > int?? Probably we are better off with making this as bool.
>
> I'm not a big fan of bool's. But I have no strong preference here. My
> reasoning here was consistency. As we already have "int swab" for a
> similar
> issue.
If we have not done it earlier, doesn't mean that we repeat the same
mistake again. There is no reason to take access_16bit as an int when a bool
will suffice.
This wastes storage and on some platforms (which have real crunch of memory)
Re: [PATCH] i2c: designware: Add support for 16bit register access
Hi Bhupesh,
On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > -Original Message-
> > From: Stefan Roese [mailto:s...@denx.de]
> > Sent: Tuesday, March 13, 2012 9:24 PM
> > To: linux-i2c@vger.kernel.org
> > Cc: spear-devel; ben-li...@fluff.org
> > Subject: [PATCH] i2c: designware: Add support for 16bit register access
> >
> > The STM SPEAr platform can only access the i2c controller register
> > via 16bit read/write functions. This patch adds support to
> > automatically detect this 16bit access mode.
> >
> > Signed-off-by: Stefan Roese
> > ---
> >
> > drivers/i2c/busses/i2c-designware-core.c | 22 --
> > drivers/i2c/busses/i2c-designware-core.h |1 +
> > 2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > b/drivers/i2c/busses/i2c-designware-core.c
> > index df87992..d1facbc 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -164,7 +164,14 @@ static char *abort_sources[] = {
> >
> > u32 dw_readl(struct dw_i2c_dev *dev, int offset)
> > {
> >
> > - u32 value = readl(dev->base + offset);
> > + u32 value;
> > +
> > + if (dev->access_16bit) {
> > + value = readw(dev->base + offset) |
> > + (readw(dev->base + offset + 2) << 16);
> > + } else {
> > + value = readl(dev->base + offset);
> > + }
>
> We can do away with '{' parenthesis as these are single line
> of code inside both the 'if-else' blocks.
It's not a single-line statement. The first block extends spans 2 lines. At
least that how I interpret this CodingStyle recommendation.
> > if (dev->swab)
> >
> > return swab32(value);
> >
> > @@ -177,7 +184,12 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int
> > offset)
> >
> > if (dev->swab)
> >
> > b = swab32(b);
> >
> > - writel(b, dev->base + offset);
> > + if (dev->access_16bit) {
> > + writew((u16)b, dev->base + offset);
> > + writew((u16)(b >> 16), dev->base + offset + 2);
> > + } else {
> > + writel(b, dev->base + offset);
> > + }
> >
> > }
> >
> > static u32
> >
> > @@ -258,6 +270,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> >
> > reg = DW_IC_COMP_TYPE_VALUE;
> >
> > }
> >
> > + /* Configure register access mode 16bit */
> > + if (reg == (DW_IC_COMP_TYPE_VALUE & 0x)) {
> > + dev->access_16bit = 1;
>
> Can we use a suitable macro for 0x?
Hmmm. Wouldn't that make it more complex? 0x is easy to understand. A
marco would "hide" this value. I would prefer to keep the value.
> Also, if dev->access_16bit is bool we can simply set:
> dev->access_16bit = true;
>
> more on that below...
>
> > + reg = DW_IC_COMP_TYPE_VALUE;
> > + }
> > +
> >
> > if (reg != DW_IC_COMP_TYPE_VALUE) {
> >
> > dev_err(dev->dev, "Unknown Synopsys component type: "
> >
> > "0x%08x\n", reg);
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-core.h
> > b/drivers/i2c/busses/i2c-designware-core.h
> > index 02d1a2d..f5af101 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.h
> > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > @@ -83,6 +83,7 @@ struct dw_i2c_dev {
> >
> > u32 abort_source;
> > int irq;
> > int swab;
> >
> > + int access_16bit;
>
> ...
> int?? Probably we are better off with making this as bool.
I'm not a big fan of bool's. But I have no strong preference here. My
reasoning here was consistency. As we already have "int swab" for a similar
issue.
So basically, I would prefer to not make the changes you suggested. But in the
end its the decision of the maintainer(s).
Thanks,
Stefan
--
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
