* [RFC PATCH] regmap: smbus: add support for regmap over SMBus
@ 2014-04-12 8:45 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-12 8:45 UTC (permalink / raw)
To: Mark Brown, Greg Kroah-Hartman
Cc: Maxime Ripard, Shuge, kevin, Chen-Yu Tsai, Hans de Goede,
Carlo Caione, linux-arm-kernel, linux-kernel, dev,
Boris BREZILLON
SMBus is a subset of the I2C protocol, oftenly used to access registers on
external devices.
I2C adpaters are able to access SMBus devices thanks to the SMBus
emulation layer. In the other hand SMBus adapters may not provide
regular I2C transfers, and thus you may not be able to expose a regmap
if your device is connected to such kind of adapter.
Hence why we need this regmap over SMBus implementation.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark,
I know you were not in favor of the smbus regmap (according to this thread
[1]), but I now hit a point where I need this functionnality.
The A31 SoC have an SMBus adatper which do not support regular I2C transfers
and on some boards (if not all) the AXP221 PMIC is connected to thi adapter.
Moreover the AXP221 really looks like the AXP209 whose driver is being
submitting ([2]), and this driver uses regmap to expose PMIC's registers.
I'd like to reuse the driver, and the only thing I need to do so is to add
support for regmap over SMBus, and patch the axp20x driver to register an
smbus regmap instead of an i2c one.
Let me now if you still think regmap over SMBus should not be supported, or
if you've find a better way to achieve my goal.
Best Regards,
Boris
[1] http://comments.gmane.org/gmane.linux.kernel/1167755
drivers/base/regmap/Kconfig | 5 +-
drivers/base/regmap/Makefile | 1 +
drivers/base/regmap/regmap-smbus.c | 364 +++++++++++++++++++++++++++++++++++++
3 files changed, 369 insertions(+), 1 deletion(-)
create mode 100644 drivers/base/regmap/regmap-smbus.c
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 4251570..450b4c1 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@
# subsystems should select the appropriate symbols.
config REGMAP
- default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
+ default y if (REGMAP_I2C || REGMAP_SMBUS || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
select LZO_COMPRESS
select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
@@ -12,6 +12,9 @@ config REGMAP
config REGMAP_I2C
tristate
+config REGMAP_SMBUS
+ tristate
+
config REGMAP_SPI
tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index a7c670b..e752b9c 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_REGMAP) += regmap.o regcache.o
obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
+obj-$(CONFIG_REGMAP_SMBUS) += regmap-smbus.o
obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
diff --git a/drivers/base/regmap/regmap-smbus.c b/drivers/base/regmap/regmap-smbus.c
new file mode 100644
index 0000000..c8b8075
--- /dev/null
+++ b/drivers/base/regmap/regmap-smbus.c
@@ -0,0 +1,364 @@
+/*
+ * Register map access API - SMBus support
+ *
+ * Copyright 2014 Free Electrons
+ *
+ * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ * 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 <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+struct regmap_smbus_context {
+ struct i2c_client *i2c;
+ enum regmap_smbus_transfer_type transfer_type;
+ int val_bytes;
+};
+
+static int regmap_smbus_write(void *context, const void *data, size_t count)
+{
+ struct regmap_smbus_context *ctx = context;
+ int ret = 0;
+ u8 reg = *(u8 *)data++;
+
+ count--;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
+ *(u8 *)data++);
+
+ count--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c, reg,
+ *(u16 *)data++);
+
+ reg += 2;
+ count -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_gather_write(void *context,
+ const void *reg, size_t reg_size,
+ const void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c,
+ smbus_reg++,
+ *(u8 *)val++);
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c,
+ smbus_reg,
+ *(u16 *)val++);
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (val_size > I2C_SMBUS_BLOCK_MAX)
+ return -ENOTSUPP;
+
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 buffer[I2C_SMBUS_BLOCK_MAX];
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_byte_data(ctx->i2c, smbus_reg++);
+ if (ret >= 0)
+ *((u8 *)val++) = ret;
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_word_data(ctx->i2c, smbus_reg);
+ if (ret >= 0)
+ *(u16 *)val++ = ret;
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_block_data(ctx->i2c,
+ smbus_reg,
+ buffer);
+ if (ret >= 0 && ret < ctx->val_bytes) {
+ ret = -EIO;
+ break;
+ }
+
+ memcpy(val, buffer, ctx->val_bytes);
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ return -ENOTSUPP;
+
+ ret = i2c_smbus_read_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (u8 *)val);
+ if (ret >= 0 && ret < val_size) {
+ ret = -EIO;
+ break;
+ }
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void regmap_smbus_free_context(void *context)
+{
+ kfree(context);
+}
+
+struct regmap_smbus_context *regmap_smbus_gen_context(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx;
+ int val_bytes = 0;
+
+ if (config->reg_bits != 8 || config->pad_bits != 0)
+ return ERR_PTR(-ENOTSUPP);
+
+ switch (transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ if (config->val_bits != 8)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ if (config->val_bits != 16)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BLOCK_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_I2C_BLOCK))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ default:
+ return ERR_PTR(-ENOTSUPP);
+ }
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return ERR_PTR(-ENOMEM);
+
+ ctx->i2c = i2c;
+ ctx->transfer_type = transfer_type;
+ ctx->val_bytes = val_bytes;
+
+ return ctx;
+}
+
+static struct regmap_bus regmap_smbus = {
+ .write = regmap_smbus_write,
+ .gather_write = regmap_smbus_gather_write,
+ .read = regmap_smbus_read,
+ .free_context = regmap_smbus_free_context,
+};
+
+/**
+ * regmap_init_smbus(): Initialise register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_smbus);
+
+/**
+ * devm_regmap_init_smbus(): Initialise managed register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap. The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return devm_regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_smbus);
+
+MODULE_LICENSE("GPL");
--
1.8.3.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC PATCH] regmap: smbus: add support for regmap over SMBus
@ 2014-04-12 8:45 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-12 8:45 UTC (permalink / raw)
To: linux-arm-kernel
SMBus is a subset of the I2C protocol, oftenly used to access registers on
external devices.
I2C adpaters are able to access SMBus devices thanks to the SMBus
emulation layer. In the other hand SMBus adapters may not provide
regular I2C transfers, and thus you may not be able to expose a regmap
if your device is connected to such kind of adapter.
Hence why we need this regmap over SMBus implementation.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark,
I know you were not in favor of the smbus regmap (according to this thread
[1]), but I now hit a point where I need this functionnality.
The A31 SoC have an SMBus adatper which do not support regular I2C transfers
and on some boards (if not all) the AXP221 PMIC is connected to thi adapter.
Moreover the AXP221 really looks like the AXP209 whose driver is being
submitting ([2]), and this driver uses regmap to expose PMIC's registers.
I'd like to reuse the driver, and the only thing I need to do so is to add
support for regmap over SMBus, and patch the axp20x driver to register an
smbus regmap instead of an i2c one.
Let me now if you still think regmap over SMBus should not be supported, or
if you've find a better way to achieve my goal.
Best Regards,
Boris
[1] http://comments.gmane.org/gmane.linux.kernel/1167755
drivers/base/regmap/Kconfig | 5 +-
drivers/base/regmap/Makefile | 1 +
drivers/base/regmap/regmap-smbus.c | 364 +++++++++++++++++++++++++++++++++++++
3 files changed, 369 insertions(+), 1 deletion(-)
create mode 100644 drivers/base/regmap/regmap-smbus.c
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 4251570..450b4c1 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@
# subsystems should select the appropriate symbols.
config REGMAP
- default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
+ default y if (REGMAP_I2C || REGMAP_SMBUS || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
select LZO_COMPRESS
select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
@@ -12,6 +12,9 @@ config REGMAP
config REGMAP_I2C
tristate
+config REGMAP_SMBUS
+ tristate
+
config REGMAP_SPI
tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index a7c670b..e752b9c 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_REGMAP) += regmap.o regcache.o
obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
+obj-$(CONFIG_REGMAP_SMBUS) += regmap-smbus.o
obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
diff --git a/drivers/base/regmap/regmap-smbus.c b/drivers/base/regmap/regmap-smbus.c
new file mode 100644
index 0000000..c8b8075
--- /dev/null
+++ b/drivers/base/regmap/regmap-smbus.c
@@ -0,0 +1,364 @@
+/*
+ * Register map access API - SMBus support
+ *
+ * Copyright 2014 Free Electrons
+ *
+ * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ * 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 <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+struct regmap_smbus_context {
+ struct i2c_client *i2c;
+ enum regmap_smbus_transfer_type transfer_type;
+ int val_bytes;
+};
+
+static int regmap_smbus_write(void *context, const void *data, size_t count)
+{
+ struct regmap_smbus_context *ctx = context;
+ int ret = 0;
+ u8 reg = *(u8 *)data++;
+
+ count--;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
+ *(u8 *)data++);
+
+ count--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c, reg,
+ *(u16 *)data++);
+
+ reg += 2;
+ count -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_gather_write(void *context,
+ const void *reg, size_t reg_size,
+ const void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c,
+ smbus_reg++,
+ *(u8 *)val++);
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c,
+ smbus_reg,
+ *(u16 *)val++);
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (val_size > I2C_SMBUS_BLOCK_MAX)
+ return -ENOTSUPP;
+
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 buffer[I2C_SMBUS_BLOCK_MAX];
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_byte_data(ctx->i2c, smbus_reg++);
+ if (ret >= 0)
+ *((u8 *)val++) = ret;
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_word_data(ctx->i2c, smbus_reg);
+ if (ret >= 0)
+ *(u16 *)val++ = ret;
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_block_data(ctx->i2c,
+ smbus_reg,
+ buffer);
+ if (ret >= 0 && ret < ctx->val_bytes) {
+ ret = -EIO;
+ break;
+ }
+
+ memcpy(val, buffer, ctx->val_bytes);
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ return -ENOTSUPP;
+
+ ret = i2c_smbus_read_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (u8 *)val);
+ if (ret >= 0 && ret < val_size) {
+ ret = -EIO;
+ break;
+ }
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void regmap_smbus_free_context(void *context)
+{
+ kfree(context);
+}
+
+struct regmap_smbus_context *regmap_smbus_gen_context(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx;
+ int val_bytes = 0;
+
+ if (config->reg_bits != 8 || config->pad_bits != 0)
+ return ERR_PTR(-ENOTSUPP);
+
+ switch (transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ if (config->val_bits != 8)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ if (config->val_bits != 16)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BLOCK_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_I2C_BLOCK))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ default:
+ return ERR_PTR(-ENOTSUPP);
+ }
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return ERR_PTR(-ENOMEM);
+
+ ctx->i2c = i2c;
+ ctx->transfer_type = transfer_type;
+ ctx->val_bytes = val_bytes;
+
+ return ctx;
+}
+
+static struct regmap_bus regmap_smbus = {
+ .write = regmap_smbus_write,
+ .gather_write = regmap_smbus_gather_write,
+ .read = regmap_smbus_read,
+ .free_context = regmap_smbus_free_context,
+};
+
+/**
+ * regmap_init_smbus(): Initialise register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_smbus);
+
+/**
+ * devm_regmap_init_smbus(): Initialise managed register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap. The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return devm_regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_smbus);
+
+MODULE_LICENSE("GPL");
--
1.8.3.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [RFC PATCH] regmap: smbus: add support for regmap over SMBus
2014-04-12 8:45 ` Boris BREZILLON
@ 2014-04-12 9:02 ` Boris BREZILLON
-1 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-12 9:02 UTC (permalink / raw)
To: Mark Brown, Greg Kroah-Hartman
Cc: Maxime Ripard, Shuge, kevin, Chen-Yu Tsai, Hans de Goede,
Carlo Caione, linux-arm-kernel, linux-kernel, dev
On 12/04/2014 10:45, Boris BREZILLON wrote:
> SMBus is a subset of the I2C protocol, oftenly used to access registers on
> external devices.
>
> I2C adpaters are able to access SMBus devices thanks to the SMBus
> emulation layer. In the other hand SMBus adapters may not provide
> regular I2C transfers, and thus you may not be able to expose a regmap
> if your device is connected to such kind of adapter.
> Hence why we need this regmap over SMBus implementation.
>
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
> Hello Mark,
>
> I know you were not in favor of the smbus regmap (according to this thread
> [1]), but I now hit a point where I need this functionnality.
>
> The A31 SoC have an SMBus adatper which do not support regular I2C transfers
> and on some boards (if not all) the AXP221 PMIC is connected to thi adapter.
> Moreover the AXP221 really looks like the AXP209 whose driver is being
> submitting ([2]), and this driver uses regmap to expose PMIC's registers.
>
> I'd like to reuse the driver, and the only thing I need to do so is to add
> support for regmap over SMBus, and patch the axp20x driver to register an
> smbus regmap instead of an i2c one.
>
> Let me now if you still think regmap over SMBus should not be supported, or
> if you've find a better way to achieve my goal.
>
> Best Regards,
>
> Boris
>
> [1] http://comments.gmane.org/gmane.linux.kernel/1167755
Oops, I forgot to add the 2nd reference link:
[2] http://www.spinics.net/lists/linux-input/msg30806.html
> drivers/base/regmap/Kconfig | 5 +-
> drivers/base/regmap/Makefile | 1 +
> drivers/base/regmap/regmap-smbus.c | 364 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 369 insertions(+), 1 deletion(-)
> create mode 100644 drivers/base/regmap/regmap-smbus.c
>
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> index 4251570..450b4c1 100644
> --- a/drivers/base/regmap/Kconfig
> +++ b/drivers/base/regmap/Kconfig
> @@ -3,7 +3,7 @@
> # subsystems should select the appropriate symbols.
>
> config REGMAP
> - default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
> + default y if (REGMAP_I2C || REGMAP_SMBUS || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
> select LZO_COMPRESS
> select LZO_DECOMPRESS
> select IRQ_DOMAIN if REGMAP_IRQ
> @@ -12,6 +12,9 @@ config REGMAP
> config REGMAP_I2C
> tristate
>
> +config REGMAP_SMBUS
> + tristate
> +
> config REGMAP_SPI
> tristate
>
> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> index a7c670b..e752b9c 100644
> --- a/drivers/base/regmap/Makefile
> +++ b/drivers/base/regmap/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_REGMAP) += regmap.o regcache.o
> obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
> obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
> obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
> +obj-$(CONFIG_REGMAP_SMBUS) += regmap-smbus.o
> obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
> obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
> obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
> diff --git a/drivers/base/regmap/regmap-smbus.c b/drivers/base/regmap/regmap-smbus.c
> new file mode 100644
> index 0000000..c8b8075
> --- /dev/null
> +++ b/drivers/base/regmap/regmap-smbus.c
> @@ -0,0 +1,364 @@
> +/*
> + * Register map access API - SMBus support
> + *
> + * Copyright 2014 Free Electrons
> + *
> + * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
> + *
> + * 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 <linux/regmap.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +struct regmap_smbus_context {
> + struct i2c_client *i2c;
> + enum regmap_smbus_transfer_type transfer_type;
> + int val_bytes;
> +};
> +
> +static int regmap_smbus_write(void *context, const void *data, size_t count)
> +{
> + struct regmap_smbus_context *ctx = context;
> + int ret = 0;
> + u8 reg = *(u8 *)data++;
> +
> + count--;
> +
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
> + *(u8 *)data++);
> +
> + count--;
> + }
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_word_data(ctx->i2c, reg,
> + *(u16 *)data++);
> +
> + reg += 2;
> + count -= 2;
> + }
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_block_data(ctx->i2c,
> + reg,
> + ctx->val_bytes,
> + (const u8 *)data);
> +
> + reg += ctx->val_bytes;
> + count -= ctx->val_bytes;
> + data += ctx->val_bytes;
> + }
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
> + reg,
> + ctx->val_bytes,
> + (const u8 *)data);
> +
> + reg += ctx->val_bytes;
> + count -= ctx->val_bytes;
> + data += ctx->val_bytes;
> + }
> + break;
> +
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int regmap_smbus_gather_write(void *context,
> + const void *reg, size_t reg_size,
> + const void *val, size_t val_size)
> +{
> + struct regmap_smbus_context *ctx = context;
> + u8 smbus_reg;
> + int ret = 0;
> +
> + if (reg_size != 1)
> + return -ENOTSUPP;
> +
> + smbus_reg = *(u8 *)reg;
> +
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_byte_data(ctx->i2c,
> + smbus_reg++,
> + *(u8 *)val++);
> +
> + val_size--;
> + }
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_word_data(ctx->i2c,
> + smbus_reg,
> + *(u16 *)val++);
> +
> + smbus_reg += 2;
> + val_size -= 2;
> + }
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_block_data(ctx->i2c,
> + smbus_reg,
> + ctx->val_bytes,
> + (const u8 *)val);
> +
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + if (val_size > I2C_SMBUS_BLOCK_MAX)
> + return -ENOTSUPP;
> +
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
> + smbus_reg,
> + ctx->val_bytes,
> + (const u8 *)val);
> +
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int regmap_smbus_read(void *context,
> + const void *reg, size_t reg_size,
> + void *val, size_t val_size)
> +{
> + struct regmap_smbus_context *ctx = context;
> + u8 buffer[I2C_SMBUS_BLOCK_MAX];
> + u8 smbus_reg;
> + int ret = 0;
> +
> + if (reg_size != 1)
> + return -ENOTSUPP;
> +
> + smbus_reg = *(u8 *)reg;
> +
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (val_size && ret >= 0) {
> + ret = i2c_smbus_read_byte_data(ctx->i2c, smbus_reg++);
> + if (ret >= 0)
> + *((u8 *)val++) = ret;
> +
> + val_size--;
> + }
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + while (val_size && ret >= 0) {
> + ret = i2c_smbus_read_word_data(ctx->i2c, smbus_reg);
> + if (ret >= 0)
> + *(u16 *)val++ = ret;
> +
> + smbus_reg += 2;
> + val_size -= 2;
> + }
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + while (val_size && ret >= 0) {
> + ret = i2c_smbus_read_block_data(ctx->i2c,
> + smbus_reg,
> + buffer);
> + if (ret >= 0 && ret < ctx->val_bytes) {
> + ret = -EIO;
> + break;
> + }
> +
> + memcpy(val, buffer, ctx->val_bytes);
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + while (val_size && ret >= 0) {
> + return -ENOTSUPP;
> +
> + ret = i2c_smbus_read_i2c_block_data(ctx->i2c,
> + smbus_reg,
> + ctx->val_bytes,
> + (u8 *)val);
> + if (ret >= 0 && ret < val_size) {
> + ret = -EIO;
> + break;
> + }
> +
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + default:
> + return -ENOTSUPP;
> + }
> +
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void regmap_smbus_free_context(void *context)
> +{
> + kfree(context);
> +}
> +
> +struct regmap_smbus_context *regmap_smbus_gen_context(struct i2c_client *i2c,
> + const struct regmap_config *config,
> + enum regmap_smbus_transfer_type transfer_type)
> +{
> + struct regmap_smbus_context *ctx;
> + int val_bytes = 0;
> +
> + if (config->reg_bits != 8 || config->pad_bits != 0)
> + return ERR_PTR(-ENOTSUPP);
> +
> + switch (transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + if (config->val_bits != 8)
> + return ERR_PTR(-EINVAL);
> +
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + if (config->val_bits != 16)
> + return ERR_PTR(-EINVAL);
> +
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
> + return ERR_PTR(-EINVAL);
> +
> + val_bytes = DIV_ROUND_UP(config->val_bits, 8);
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_BLOCK_DATA))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
> + return ERR_PTR(-EINVAL);
> +
> + val_bytes = DIV_ROUND_UP(config->val_bits, 8);
> +
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_I2C_BLOCK))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + default:
> + return ERR_PTR(-ENOTSUPP);
> + }
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return ERR_PTR(-ENOMEM);
> +
> + ctx->i2c = i2c;
> + ctx->transfer_type = transfer_type;
> + ctx->val_bytes = val_bytes;
> +
> + return ctx;
> +}
> +
> +static struct regmap_bus regmap_smbus = {
> + .write = regmap_smbus_write,
> + .gather_write = regmap_smbus_gather_write,
> + .read = regmap_smbus_read,
> + .free_context = regmap_smbus_free_context,
> +};
> +
> +/**
> + * regmap_init_smbus(): Initialise register map
> + *
> + * @i2c: Device that will be interacted with
> + * @config: Configuration for register map
> + * @transfer_type: SMBUS transfer type
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer to
> + * a struct regmap.
> + */
> +struct regmap *regmap_init_smbus(struct i2c_client *i2c,
> + const struct regmap_config *config,
> + enum regmap_smbus_transfer_type transfer_type)
> +{
> + struct regmap_smbus_context *ctx =
> + regmap_smbus_gen_context(i2c, config, transfer_type);
> +
> + if (IS_ERR(ctx))
> + return ERR_PTR(PTR_ERR(ctx));
> +
> + return regmap_init(&i2c->dev, ®map_smbus, ctx, config);
> +}
> +EXPORT_SYMBOL_GPL(regmap_init_smbus);
> +
> +/**
> + * devm_regmap_init_smbus(): Initialise managed register map
> + *
> + * @i2c: Device that will be interacted with
> + * @config: Configuration for register map
> + * @transfer_type: SMBUS transfer type
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a struct regmap. The regmap will be automatically freed by the
> + * device management code.
> + */
> +struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
> + const struct regmap_config *config,
> + enum regmap_smbus_transfer_type transfer_type)
> +{
> + struct regmap_smbus_context *ctx =
> + regmap_smbus_gen_context(i2c, config, transfer_type);
> +
> + if (IS_ERR(ctx))
> + return ERR_PTR(PTR_ERR(ctx));
> +
> + return devm_regmap_init(&i2c->dev, ®map_smbus, ctx, config);
> +}
> +EXPORT_SYMBOL_GPL(devm_regmap_init_smbus);
> +
> +MODULE_LICENSE("GPL");
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH] regmap: smbus: add support for regmap over SMBus
@ 2014-04-12 9:02 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-12 9:02 UTC (permalink / raw)
To: linux-arm-kernel
On 12/04/2014 10:45, Boris BREZILLON wrote:
> SMBus is a subset of the I2C protocol, oftenly used to access registers on
> external devices.
>
> I2C adpaters are able to access SMBus devices thanks to the SMBus
> emulation layer. In the other hand SMBus adapters may not provide
> regular I2C transfers, and thus you may not be able to expose a regmap
> if your device is connected to such kind of adapter.
> Hence why we need this regmap over SMBus implementation.
>
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
> Hello Mark,
>
> I know you were not in favor of the smbus regmap (according to this thread
> [1]), but I now hit a point where I need this functionnality.
>
> The A31 SoC have an SMBus adatper which do not support regular I2C transfers
> and on some boards (if not all) the AXP221 PMIC is connected to thi adapter.
> Moreover the AXP221 really looks like the AXP209 whose driver is being
> submitting ([2]), and this driver uses regmap to expose PMIC's registers.
>
> I'd like to reuse the driver, and the only thing I need to do so is to add
> support for regmap over SMBus, and patch the axp20x driver to register an
> smbus regmap instead of an i2c one.
>
> Let me now if you still think regmap over SMBus should not be supported, or
> if you've find a better way to achieve my goal.
>
> Best Regards,
>
> Boris
>
> [1] http://comments.gmane.org/gmane.linux.kernel/1167755
Oops, I forgot to add the 2nd reference link:
[2] http://www.spinics.net/lists/linux-input/msg30806.html
> drivers/base/regmap/Kconfig | 5 +-
> drivers/base/regmap/Makefile | 1 +
> drivers/base/regmap/regmap-smbus.c | 364 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 369 insertions(+), 1 deletion(-)
> create mode 100644 drivers/base/regmap/regmap-smbus.c
>
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> index 4251570..450b4c1 100644
> --- a/drivers/base/regmap/Kconfig
> +++ b/drivers/base/regmap/Kconfig
> @@ -3,7 +3,7 @@
> # subsystems should select the appropriate symbols.
>
> config REGMAP
> - default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
> + default y if (REGMAP_I2C || REGMAP_SMBUS || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
> select LZO_COMPRESS
> select LZO_DECOMPRESS
> select IRQ_DOMAIN if REGMAP_IRQ
> @@ -12,6 +12,9 @@ config REGMAP
> config REGMAP_I2C
> tristate
>
> +config REGMAP_SMBUS
> + tristate
> +
> config REGMAP_SPI
> tristate
>
> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> index a7c670b..e752b9c 100644
> --- a/drivers/base/regmap/Makefile
> +++ b/drivers/base/regmap/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_REGMAP) += regmap.o regcache.o
> obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
> obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
> obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
> +obj-$(CONFIG_REGMAP_SMBUS) += regmap-smbus.o
> obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
> obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
> obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
> diff --git a/drivers/base/regmap/regmap-smbus.c b/drivers/base/regmap/regmap-smbus.c
> new file mode 100644
> index 0000000..c8b8075
> --- /dev/null
> +++ b/drivers/base/regmap/regmap-smbus.c
> @@ -0,0 +1,364 @@
> +/*
> + * Register map access API - SMBus support
> + *
> + * Copyright 2014 Free Electrons
> + *
> + * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
> + *
> + * 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 <linux/regmap.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +struct regmap_smbus_context {
> + struct i2c_client *i2c;
> + enum regmap_smbus_transfer_type transfer_type;
> + int val_bytes;
> +};
> +
> +static int regmap_smbus_write(void *context, const void *data, size_t count)
> +{
> + struct regmap_smbus_context *ctx = context;
> + int ret = 0;
> + u8 reg = *(u8 *)data++;
> +
> + count--;
> +
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
> + *(u8 *)data++);
> +
> + count--;
> + }
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_word_data(ctx->i2c, reg,
> + *(u16 *)data++);
> +
> + reg += 2;
> + count -= 2;
> + }
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_block_data(ctx->i2c,
> + reg,
> + ctx->val_bytes,
> + (const u8 *)data);
> +
> + reg += ctx->val_bytes;
> + count -= ctx->val_bytes;
> + data += ctx->val_bytes;
> + }
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
> + reg,
> + ctx->val_bytes,
> + (const u8 *)data);
> +
> + reg += ctx->val_bytes;
> + count -= ctx->val_bytes;
> + data += ctx->val_bytes;
> + }
> + break;
> +
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int regmap_smbus_gather_write(void *context,
> + const void *reg, size_t reg_size,
> + const void *val, size_t val_size)
> +{
> + struct regmap_smbus_context *ctx = context;
> + u8 smbus_reg;
> + int ret = 0;
> +
> + if (reg_size != 1)
> + return -ENOTSUPP;
> +
> + smbus_reg = *(u8 *)reg;
> +
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_byte_data(ctx->i2c,
> + smbus_reg++,
> + *(u8 *)val++);
> +
> + val_size--;
> + }
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_word_data(ctx->i2c,
> + smbus_reg,
> + *(u16 *)val++);
> +
> + smbus_reg += 2;
> + val_size -= 2;
> + }
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_block_data(ctx->i2c,
> + smbus_reg,
> + ctx->val_bytes,
> + (const u8 *)val);
> +
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + if (val_size > I2C_SMBUS_BLOCK_MAX)
> + return -ENOTSUPP;
> +
> + while (val_size && !ret) {
> + ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
> + smbus_reg,
> + ctx->val_bytes,
> + (const u8 *)val);
> +
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int regmap_smbus_read(void *context,
> + const void *reg, size_t reg_size,
> + void *val, size_t val_size)
> +{
> + struct regmap_smbus_context *ctx = context;
> + u8 buffer[I2C_SMBUS_BLOCK_MAX];
> + u8 smbus_reg;
> + int ret = 0;
> +
> + if (reg_size != 1)
> + return -ENOTSUPP;
> +
> + smbus_reg = *(u8 *)reg;
> +
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (val_size && ret >= 0) {
> + ret = i2c_smbus_read_byte_data(ctx->i2c, smbus_reg++);
> + if (ret >= 0)
> + *((u8 *)val++) = ret;
> +
> + val_size--;
> + }
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + while (val_size && ret >= 0) {
> + ret = i2c_smbus_read_word_data(ctx->i2c, smbus_reg);
> + if (ret >= 0)
> + *(u16 *)val++ = ret;
> +
> + smbus_reg += 2;
> + val_size -= 2;
> + }
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + while (val_size && ret >= 0) {
> + ret = i2c_smbus_read_block_data(ctx->i2c,
> + smbus_reg,
> + buffer);
> + if (ret >= 0 && ret < ctx->val_bytes) {
> + ret = -EIO;
> + break;
> + }
> +
> + memcpy(val, buffer, ctx->val_bytes);
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + while (val_size && ret >= 0) {
> + return -ENOTSUPP;
> +
> + ret = i2c_smbus_read_i2c_block_data(ctx->i2c,
> + smbus_reg,
> + ctx->val_bytes,
> + (u8 *)val);
> + if (ret >= 0 && ret < val_size) {
> + ret = -EIO;
> + break;
> + }
> +
> + smbus_reg += ctx->val_bytes;
> + val_size -= ctx->val_bytes;
> + val += ctx->val_bytes;
> + }
> + break;
> +
> + default:
> + return -ENOTSUPP;
> + }
> +
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void regmap_smbus_free_context(void *context)
> +{
> + kfree(context);
> +}
> +
> +struct regmap_smbus_context *regmap_smbus_gen_context(struct i2c_client *i2c,
> + const struct regmap_config *config,
> + enum regmap_smbus_transfer_type transfer_type)
> +{
> + struct regmap_smbus_context *ctx;
> + int val_bytes = 0;
> +
> + if (config->reg_bits != 8 || config->pad_bits != 0)
> + return ERR_PTR(-ENOTSUPP);
> +
> + switch (transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + if (config->val_bits != 8)
> + return ERR_PTR(-EINVAL);
> +
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + case REGMAP_SMBUS_WORD_TRANSFER:
> + if (config->val_bits != 16)
> + return ERR_PTR(-EINVAL);
> +
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + case REGMAP_SMBUS_BLOCK_TRANSFER:
> + if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
> + return ERR_PTR(-EINVAL);
> +
> + val_bytes = DIV_ROUND_UP(config->val_bits, 8);
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_BLOCK_DATA))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
> + return ERR_PTR(-EINVAL);
> +
> + val_bytes = DIV_ROUND_UP(config->val_bits, 8);
> +
> + if (!i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_I2C_BLOCK))
> + return ERR_PTR(-ENOTSUPP);
> + break;
> +
> + default:
> + return ERR_PTR(-ENOTSUPP);
> + }
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return ERR_PTR(-ENOMEM);
> +
> + ctx->i2c = i2c;
> + ctx->transfer_type = transfer_type;
> + ctx->val_bytes = val_bytes;
> +
> + return ctx;
> +}
> +
> +static struct regmap_bus regmap_smbus = {
> + .write = regmap_smbus_write,
> + .gather_write = regmap_smbus_gather_write,
> + .read = regmap_smbus_read,
> + .free_context = regmap_smbus_free_context,
> +};
> +
> +/**
> + * regmap_init_smbus(): Initialise register map
> + *
> + * @i2c: Device that will be interacted with
> + * @config: Configuration for register map
> + * @transfer_type: SMBUS transfer type
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer to
> + * a struct regmap.
> + */
> +struct regmap *regmap_init_smbus(struct i2c_client *i2c,
> + const struct regmap_config *config,
> + enum regmap_smbus_transfer_type transfer_type)
> +{
> + struct regmap_smbus_context *ctx =
> + regmap_smbus_gen_context(i2c, config, transfer_type);
> +
> + if (IS_ERR(ctx))
> + return ERR_PTR(PTR_ERR(ctx));
> +
> + return regmap_init(&i2c->dev, ®map_smbus, ctx, config);
> +}
> +EXPORT_SYMBOL_GPL(regmap_init_smbus);
> +
> +/**
> + * devm_regmap_init_smbus(): Initialise managed register map
> + *
> + * @i2c: Device that will be interacted with
> + * @config: Configuration for register map
> + * @transfer_type: SMBUS transfer type
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a struct regmap. The regmap will be automatically freed by the
> + * device management code.
> + */
> +struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
> + const struct regmap_config *config,
> + enum regmap_smbus_transfer_type transfer_type)
> +{
> + struct regmap_smbus_context *ctx =
> + regmap_smbus_gen_context(i2c, config, transfer_type);
> +
> + if (IS_ERR(ctx))
> + return ERR_PTR(PTR_ERR(ctx));
> +
> + return devm_regmap_init(&i2c->dev, ®map_smbus, ctx, config);
> +}
> +EXPORT_SYMBOL_GPL(devm_regmap_init_smbus);
> +
> +MODULE_LICENSE("GPL");
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-12 8:45 ` Boris BREZILLON
@ 2014-04-14 13:08 ` Boris BREZILLON
-1 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-14 13:08 UTC (permalink / raw)
To: Mark Brown
Cc: Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin, Chen-Yu Tsai,
Hans de Goede, Carlo Caione, linux-arm-kernel, linux-kernel, dev,
Boris BREZILLON
SMBus is a subset of the I2C protocol, oftenly used to access registers on
external devices.
I2C adapters are able to access SMBus devices thanks to the SMBus
emulation layer. In the other hand SMBus adapters may not provide
regular I2C transfers, and thus you may not be able to expose a regmap
if your device is connected to such kind of adapter.
Hence why we need this regmap over SMBus implementation.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark,
Sorry for the noise, but I forgot to add the changes in regmap.h in my
previous commit.
Best Regards,
Boris
drivers/base/regmap/Kconfig | 5 +-
drivers/base/regmap/Makefile | 1 +
drivers/base/regmap/regmap-smbus.c | 364 +++++++++++++++++++++++++++++++++++++
include/linux/regmap.h | 13 ++
4 files changed, 382 insertions(+), 1 deletion(-)
create mode 100644 drivers/base/regmap/regmap-smbus.c
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 4251570..450b4c1 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@
# subsystems should select the appropriate symbols.
config REGMAP
- default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
+ default y if (REGMAP_I2C || REGMAP_SMBUS || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
select LZO_COMPRESS
select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
@@ -12,6 +12,9 @@ config REGMAP
config REGMAP_I2C
tristate
+config REGMAP_SMBUS
+ tristate
+
config REGMAP_SPI
tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index a7c670b..e752b9c 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_REGMAP) += regmap.o regcache.o
obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
+obj-$(CONFIG_REGMAP_SMBUS) += regmap-smbus.o
obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
diff --git a/drivers/base/regmap/regmap-smbus.c b/drivers/base/regmap/regmap-smbus.c
new file mode 100644
index 0000000..c8b8075
--- /dev/null
+++ b/drivers/base/regmap/regmap-smbus.c
@@ -0,0 +1,364 @@
+/*
+ * Register map access API - SMBus support
+ *
+ * Copyright 2014 Free Electrons
+ *
+ * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ * 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 <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+struct regmap_smbus_context {
+ struct i2c_client *i2c;
+ enum regmap_smbus_transfer_type transfer_type;
+ int val_bytes;
+};
+
+static int regmap_smbus_write(void *context, const void *data, size_t count)
+{
+ struct regmap_smbus_context *ctx = context;
+ int ret = 0;
+ u8 reg = *(u8 *)data++;
+
+ count--;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
+ *(u8 *)data++);
+
+ count--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c, reg,
+ *(u16 *)data++);
+
+ reg += 2;
+ count -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_gather_write(void *context,
+ const void *reg, size_t reg_size,
+ const void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c,
+ smbus_reg++,
+ *(u8 *)val++);
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c,
+ smbus_reg,
+ *(u16 *)val++);
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (val_size > I2C_SMBUS_BLOCK_MAX)
+ return -ENOTSUPP;
+
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 buffer[I2C_SMBUS_BLOCK_MAX];
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_byte_data(ctx->i2c, smbus_reg++);
+ if (ret >= 0)
+ *((u8 *)val++) = ret;
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_word_data(ctx->i2c, smbus_reg);
+ if (ret >= 0)
+ *(u16 *)val++ = ret;
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_block_data(ctx->i2c,
+ smbus_reg,
+ buffer);
+ if (ret >= 0 && ret < ctx->val_bytes) {
+ ret = -EIO;
+ break;
+ }
+
+ memcpy(val, buffer, ctx->val_bytes);
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ return -ENOTSUPP;
+
+ ret = i2c_smbus_read_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (u8 *)val);
+ if (ret >= 0 && ret < val_size) {
+ ret = -EIO;
+ break;
+ }
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void regmap_smbus_free_context(void *context)
+{
+ kfree(context);
+}
+
+struct regmap_smbus_context *regmap_smbus_gen_context(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx;
+ int val_bytes = 0;
+
+ if (config->reg_bits != 8 || config->pad_bits != 0)
+ return ERR_PTR(-ENOTSUPP);
+
+ switch (transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ if (config->val_bits != 8)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ if (config->val_bits != 16)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BLOCK_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_I2C_BLOCK))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ default:
+ return ERR_PTR(-ENOTSUPP);
+ }
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return ERR_PTR(-ENOMEM);
+
+ ctx->i2c = i2c;
+ ctx->transfer_type = transfer_type;
+ ctx->val_bytes = val_bytes;
+
+ return ctx;
+}
+
+static struct regmap_bus regmap_smbus = {
+ .write = regmap_smbus_write,
+ .gather_write = regmap_smbus_gather_write,
+ .read = regmap_smbus_read,
+ .free_context = regmap_smbus_free_context,
+};
+
+/**
+ * regmap_init_smbus(): Initialise register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_smbus);
+
+/**
+ * devm_regmap_init_smbus(): Initialise managed register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap. The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return devm_regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_smbus);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 85691b9..34ef2c7 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -317,6 +317,13 @@ struct regmap_bus {
enum regmap_endian val_format_endian_default;
};
+enum regmap_smbus_transfer_type {
+ REGMAP_SMBUS_BYTE_TRANSFER,
+ REGMAP_SMBUS_WORD_TRANSFER,
+ REGMAP_SMBUS_BLOCK_TRANSFER,
+ REGMAP_SMBUS_I2C_BLOCK_TRANSFER,
+};
+
struct regmap *regmap_init(struct device *dev,
const struct regmap_bus *bus,
void *bus_context,
@@ -325,6 +332,9 @@ int regmap_attach_dev(struct device *dev, struct regmap *map,
const struct regmap_config *config);
struct regmap *regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config);
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type);
struct regmap *regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
struct regmap *regmap_init_spmi_base(struct spmi_device *dev,
@@ -341,6 +351,9 @@ struct regmap *devm_regmap_init(struct device *dev,
const struct regmap_config *config);
struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config);
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type);
struct regmap *devm_regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
struct regmap *devm_regmap_init_spmi_base(struct spmi_device *dev,
--
1.8.3.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-14 13:08 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-14 13:08 UTC (permalink / raw)
To: linux-arm-kernel
SMBus is a subset of the I2C protocol, oftenly used to access registers on
external devices.
I2C adapters are able to access SMBus devices thanks to the SMBus
emulation layer. In the other hand SMBus adapters may not provide
regular I2C transfers, and thus you may not be able to expose a regmap
if your device is connected to such kind of adapter.
Hence why we need this regmap over SMBus implementation.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark,
Sorry for the noise, but I forgot to add the changes in regmap.h in my
previous commit.
Best Regards,
Boris
drivers/base/regmap/Kconfig | 5 +-
drivers/base/regmap/Makefile | 1 +
drivers/base/regmap/regmap-smbus.c | 364 +++++++++++++++++++++++++++++++++++++
include/linux/regmap.h | 13 ++
4 files changed, 382 insertions(+), 1 deletion(-)
create mode 100644 drivers/base/regmap/regmap-smbus.c
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 4251570..450b4c1 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@
# subsystems should select the appropriate symbols.
config REGMAP
- default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
+ default y if (REGMAP_I2C || REGMAP_SMBUS || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
select LZO_COMPRESS
select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
@@ -12,6 +12,9 @@ config REGMAP
config REGMAP_I2C
tristate
+config REGMAP_SMBUS
+ tristate
+
config REGMAP_SPI
tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index a7c670b..e752b9c 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_REGMAP) += regmap.o regcache.o
obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
+obj-$(CONFIG_REGMAP_SMBUS) += regmap-smbus.o
obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
diff --git a/drivers/base/regmap/regmap-smbus.c b/drivers/base/regmap/regmap-smbus.c
new file mode 100644
index 0000000..c8b8075
--- /dev/null
+++ b/drivers/base/regmap/regmap-smbus.c
@@ -0,0 +1,364 @@
+/*
+ * Register map access API - SMBus support
+ *
+ * Copyright 2014 Free Electrons
+ *
+ * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ * 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 <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+struct regmap_smbus_context {
+ struct i2c_client *i2c;
+ enum regmap_smbus_transfer_type transfer_type;
+ int val_bytes;
+};
+
+static int regmap_smbus_write(void *context, const void *data, size_t count)
+{
+ struct regmap_smbus_context *ctx = context;
+ int ret = 0;
+ u8 reg = *(u8 *)data++;
+
+ count--;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
+ *(u8 *)data++);
+
+ count--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c, reg,
+ *(u16 *)data++);
+
+ reg += 2;
+ count -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (count > 0 && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ reg,
+ ctx->val_bytes,
+ (const u8 *)data);
+
+ reg += ctx->val_bytes;
+ count -= ctx->val_bytes;
+ data += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_gather_write(void *context,
+ const void *reg, size_t reg_size,
+ const void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_byte_data(ctx->i2c,
+ smbus_reg++,
+ *(u8 *)val++);
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_word_data(ctx->i2c,
+ smbus_reg,
+ *(u16 *)val++);
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (val_size > I2C_SMBUS_BLOCK_MAX)
+ return -ENOTSUPP;
+
+ while (val_size && !ret) {
+ ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (const u8 *)val);
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return ret;
+}
+
+static int regmap_smbus_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ struct regmap_smbus_context *ctx = context;
+ u8 buffer[I2C_SMBUS_BLOCK_MAX];
+ u8 smbus_reg;
+ int ret = 0;
+
+ if (reg_size != 1)
+ return -ENOTSUPP;
+
+ smbus_reg = *(u8 *)reg;
+
+ switch (ctx->transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_byte_data(ctx->i2c, smbus_reg++);
+ if (ret >= 0)
+ *((u8 *)val++) = ret;
+
+ val_size--;
+ }
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_word_data(ctx->i2c, smbus_reg);
+ if (ret >= 0)
+ *(u16 *)val++ = ret;
+
+ smbus_reg += 2;
+ val_size -= 2;
+ }
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ ret = i2c_smbus_read_block_data(ctx->i2c,
+ smbus_reg,
+ buffer);
+ if (ret >= 0 && ret < ctx->val_bytes) {
+ ret = -EIO;
+ break;
+ }
+
+ memcpy(val, buffer, ctx->val_bytes);
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ while (val_size && ret >= 0) {
+ return -ENOTSUPP;
+
+ ret = i2c_smbus_read_i2c_block_data(ctx->i2c,
+ smbus_reg,
+ ctx->val_bytes,
+ (u8 *)val);
+ if (ret >= 0 && ret < val_size) {
+ ret = -EIO;
+ break;
+ }
+
+ smbus_reg += ctx->val_bytes;
+ val_size -= ctx->val_bytes;
+ val += ctx->val_bytes;
+ }
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void regmap_smbus_free_context(void *context)
+{
+ kfree(context);
+}
+
+struct regmap_smbus_context *regmap_smbus_gen_context(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx;
+ int val_bytes = 0;
+
+ if (config->reg_bits != 8 || config->pad_bits != 0)
+ return ERR_PTR(-ENOTSUPP);
+
+ switch (transfer_type) {
+ case REGMAP_SMBUS_BYTE_TRANSFER:
+ if (config->val_bits != 8)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_WORD_TRANSFER:
+ if (config->val_bits != 16)
+ return ERR_PTR(-EINVAL);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BLOCK_DATA))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+ if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+ return ERR_PTR(-EINVAL);
+
+ val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+
+ if (!i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_I2C_BLOCK))
+ return ERR_PTR(-ENOTSUPP);
+ break;
+
+ default:
+ return ERR_PTR(-ENOTSUPP);
+ }
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return ERR_PTR(-ENOMEM);
+
+ ctx->i2c = i2c;
+ ctx->transfer_type = transfer_type;
+ ctx->val_bytes = val_bytes;
+
+ return ctx;
+}
+
+static struct regmap_bus regmap_smbus = {
+ .write = regmap_smbus_write,
+ .gather_write = regmap_smbus_gather_write,
+ .read = regmap_smbus_read,
+ .free_context = regmap_smbus_free_context,
+};
+
+/**
+ * regmap_init_smbus(): Initialise register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_smbus);
+
+/**
+ * devm_regmap_init_smbus(): Initialise managed register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap. The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type)
+{
+ struct regmap_smbus_context *ctx =
+ regmap_smbus_gen_context(i2c, config, transfer_type);
+
+ if (IS_ERR(ctx))
+ return ERR_PTR(PTR_ERR(ctx));
+
+ return devm_regmap_init(&i2c->dev, ®map_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_smbus);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 85691b9..34ef2c7 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -317,6 +317,13 @@ struct regmap_bus {
enum regmap_endian val_format_endian_default;
};
+enum regmap_smbus_transfer_type {
+ REGMAP_SMBUS_BYTE_TRANSFER,
+ REGMAP_SMBUS_WORD_TRANSFER,
+ REGMAP_SMBUS_BLOCK_TRANSFER,
+ REGMAP_SMBUS_I2C_BLOCK_TRANSFER,
+};
+
struct regmap *regmap_init(struct device *dev,
const struct regmap_bus *bus,
void *bus_context,
@@ -325,6 +332,9 @@ int regmap_attach_dev(struct device *dev, struct regmap *map,
const struct regmap_config *config);
struct regmap *regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config);
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type);
struct regmap *regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
struct regmap *regmap_init_spmi_base(struct spmi_device *dev,
@@ -341,6 +351,9 @@ struct regmap *devm_regmap_init(struct device *dev,
const struct regmap_config *config);
struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config);
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ enum regmap_smbus_transfer_type transfer_type);
struct regmap *devm_regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
struct regmap *devm_regmap_init_spmi_base(struct spmi_device *dev,
--
1.8.3.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-14 13:08 ` Boris BREZILLON
@ 2014-04-14 21:04 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-14 21:04 UTC (permalink / raw)
To: Boris BREZILLON
Cc: Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin, Chen-Yu Tsai,
Hans de Goede, Carlo Caione, linux-arm-kernel, linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 964 bytes --]
On Mon, Apr 14, 2014 at 03:08:05PM +0200, Boris BREZILLON wrote:
> SMBus is a subset of the I2C protocol, oftenly used to access registers on
> external devices.
This is basically fine. However...
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
> + *(u8 *)data++);
> +
> + count--;
> + }
> + break;
The transfer type gets set once per device at init time so why not just
parameterise based on val_bytes?
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
> + reg,
> + ctx->val_bytes,
> + (const u8 *)data);
Fix the const correctness of the API rather than casting.
> + reg += ctx->val_bytes;
> + count -= ctx->val_bytes;
> + data += ctx->val_bytes;
> + }
I'm assuming this will only be used if val_bytes isn't 1 or 2?
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-14 21:04 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-14 21:04 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 14, 2014 at 03:08:05PM +0200, Boris BREZILLON wrote:
> SMBus is a subset of the I2C protocol, oftenly used to access registers on
> external devices.
This is basically fine. However...
> + switch (ctx->transfer_type) {
> + case REGMAP_SMBUS_BYTE_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
> + *(u8 *)data++);
> +
> + count--;
> + }
> + break;
The transfer type gets set once per device at init time so why not just
parameterise based on val_bytes?
> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> + while (count > 0 && !ret) {
> + ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
> + reg,
> + ctx->val_bytes,
> + (const u8 *)data);
Fix the const correctness of the API rather than casting.
> + reg += ctx->val_bytes;
> + count -= ctx->val_bytes;
> + data += ctx->val_bytes;
> + }
I'm assuming this will only be used if val_bytes isn't 1 or 2?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140414/69c7a190/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-14 21:04 ` Mark Brown
@ 2014-04-15 7:36 ` Boris BREZILLON
-1 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-15 7:36 UTC (permalink / raw)
To: Mark Brown
Cc: Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin, Chen-Yu Tsai,
Hans de Goede, Carlo Caione, linux-arm-kernel, linux-kernel, dev
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 14/04/2014 23:04, Mark Brown wrote:
> On Mon, Apr 14, 2014 at 03:08:05PM +0200, Boris BREZILLON wrote:
>> SMBus is a subset of the I2C protocol, oftenly used to access
registers on
>> external devices.
>
> This is basically fine. However...
>
>> + switch (ctx->transfer_type) { + case
>> REGMAP_SMBUS_BYTE_TRANSFER: + while (count > 0 && !ret) {
>> + ret = i2c_smbus_write_byte_data(ctx->i2c, reg++, +
>> *(u8 *)data++); + + count--; + } +
>> break;
>
> The transfer type gets set once per device at init time so why not
> just parameterise based on val_bytes?
Actually, you may want to transfer 1 byte registers using the block
method (if your device only support block transfers). This depends on
the device being accessed and what it supports, but I'm not sure we can
assume 1 byte registers will always be transferred using SMBUS byte
transfers.
>
>
>> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: + while (count
>> > 0 && !ret) { + ret =
>> i2c_smbus_write_i2c_block_data(ctx->i2c, +
>> reg, + ctx->val_bytes, +
>> (const u8 *)data);
>
> Fix the const correctness of the API rather than casting.
The API is correct because the i2c_smbus_write_i2c_block does not modify
the data pointer.
Just removing the const keyword in the cast should be enough, because
you can safely cast a non const pointer to a const one.
>
>
>> + reg += ctx->val_bytes; + count -=
>> ctx->val_bytes; + data += ctx->val_bytes; + }
>
> I'm assuming this will only be used if val_bytes isn't 1 or 2?
As explained earlier, this depends on the device being accessed, but it
might be used for smaller transfers (1 or 2 bytes).
Thanks for the review.
Best Regards,
Boris
- --
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJTTOFkAAoJEHLimErDqMKyhlcH/jeKnphnxL9Sc+OKsd1sbvZ4
dgjwm0HVy/oQO4dwGHbGXDrvoqdcuPKjKJv0An1RAW0u8CBQHCHbn2xQZtHkrNhK
C4g+8ws3R050ppDAAvFGgnx2bIoS8+Elbcd0GNCQW+xYU/xOySH9qgPitFF7PgGw
X9wgM+o4x6W2Lo/NOxmkXeZVWl3UrfUL24UTXfqysJK6ciYtW7GDP1uInfK2eH1G
RL7eD7LanEH95bk8eQtVfZZzcoAZCo1ri2z6iPf2WS4Nep84fyAr6Q+B1Ltmhsus
QXZgXWnVY45ZYFRNjMX8axCUUoJf7DSn9ayi3Gil1i7fLmCPKhpxc8HNxWvIMkk=
=FFGW
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-15 7:36 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-15 7:36 UTC (permalink / raw)
To: linux-arm-kernel
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 14/04/2014 23:04, Mark Brown wrote:
> On Mon, Apr 14, 2014 at 03:08:05PM +0200, Boris BREZILLON wrote:
>> SMBus is a subset of the I2C protocol, oftenly used to access
registers on
>> external devices.
>
> This is basically fine. However...
>
>> + switch (ctx->transfer_type) { + case
>> REGMAP_SMBUS_BYTE_TRANSFER: + while (count > 0 && !ret) {
>> + ret = i2c_smbus_write_byte_data(ctx->i2c, reg++, +
>> *(u8 *)data++); + + count--; + } +
>> break;
>
> The transfer type gets set once per device at init time so why not
> just parameterise based on val_bytes?
Actually, you may want to transfer 1 byte registers using the block
method (if your device only support block transfers). This depends on
the device being accessed and what it supports, but I'm not sure we can
assume 1 byte registers will always be transferred using SMBUS byte
transfers.
>
>
>> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: + while (count
>> > 0 && !ret) { + ret =
>> i2c_smbus_write_i2c_block_data(ctx->i2c, +
>> reg, + ctx->val_bytes, +
>> (const u8 *)data);
>
> Fix the const correctness of the API rather than casting.
The API is correct because the i2c_smbus_write_i2c_block does not modify
the data pointer.
Just removing the const keyword in the cast should be enough, because
you can safely cast a non const pointer to a const one.
>
>
>> + reg += ctx->val_bytes; + count -=
>> ctx->val_bytes; + data += ctx->val_bytes; + }
>
> I'm assuming this will only be used if val_bytes isn't 1 or 2?
As explained earlier, this depends on the device being accessed, but it
might be used for smaller transfers (1 or 2 bytes).
Thanks for the review.
Best Regards,
Boris
- --
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJTTOFkAAoJEHLimErDqMKyhlcH/jeKnphnxL9Sc+OKsd1sbvZ4
dgjwm0HVy/oQO4dwGHbGXDrvoqdcuPKjKJv0An1RAW0u8CBQHCHbn2xQZtHkrNhK
C4g+8ws3R050ppDAAvFGgnx2bIoS8+Elbcd0GNCQW+xYU/xOySH9qgPitFF7PgGw
X9wgM+o4x6W2Lo/NOxmkXeZVWl3UrfUL24UTXfqysJK6ciYtW7GDP1uInfK2eH1G
RL7eD7LanEH95bk8eQtVfZZzcoAZCo1ri2z6iPf2WS4Nep84fyAr6Q+B1Ltmhsus
QXZgXWnVY45ZYFRNjMX8axCUUoJf7DSn9ayi3Gil1i7fLmCPKhpxc8HNxWvIMkk=
=FFGW
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-15 7:36 ` Boris BREZILLON
@ 2014-04-15 10:09 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 10:09 UTC (permalink / raw)
To: Boris BREZILLON
Cc: Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin, Chen-Yu Tsai,
Hans de Goede, Carlo Caione, linux-arm-kernel, linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]
On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
> On 14/04/2014 23:04, Mark Brown wrote:
> > The transfer type gets set once per device at init time so why not
> > just parameterise based on val_bytes?
> Actually, you may want to transfer 1 byte registers using the block
> method (if your device only support block transfers). This depends on
> the device being accessed and what it supports, but I'm not sure we can
> assume 1 byte registers will always be transferred using SMBUS byte
> transfers.
OK, so if this a realistic issue then it seems like it's better to
implement three different buses - there is not really any common code
between the various paths. This would also mean that you avoid having
gather write when it can't be implemented.
The code is also not validating the lengths for two byte values.
> >> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: + while (count
> >> > 0 && !ret) { + ret =
> >> i2c_smbus_write_i2c_block_data(ctx->i2c, +
> >> reg, + ctx->val_bytes, +
> >> (const u8 *)data);
> > Fix the const correctness of the API rather than casting.
> The API is correct because the i2c_smbus_write_i2c_block does not modify
> the data pointer.
> Just removing the const keyword in the cast should be enough, because
> you can safely cast a non const pointer to a const one.
If you need to cast away from void at all something is going wrong (and
we appear to be always passing in write buffers as const too, I can't
remember which operation this was though).
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-15 10:09 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 10:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
> On 14/04/2014 23:04, Mark Brown wrote:
> > The transfer type gets set once per device at init time so why not
> > just parameterise based on val_bytes?
> Actually, you may want to transfer 1 byte registers using the block
> method (if your device only support block transfers). This depends on
> the device being accessed and what it supports, but I'm not sure we can
> assume 1 byte registers will always be transferred using SMBUS byte
> transfers.
OK, so if this a realistic issue then it seems like it's better to
implement three different buses - there is not really any common code
between the various paths. This would also mean that you avoid having
gather write when it can't be implemented.
The code is also not validating the lengths for two byte values.
> >> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: + while (count
> >> > 0 && !ret) { + ret =
> >> i2c_smbus_write_i2c_block_data(ctx->i2c, +
> >> reg, + ctx->val_bytes, +
> >> (const u8 *)data);
> > Fix the const correctness of the API rather than casting.
> The API is correct because the i2c_smbus_write_i2c_block does not modify
> the data pointer.
> Just removing the const keyword in the cast should be enough, because
> you can safely cast a non const pointer to a const one.
If you need to cast away from void at all something is going wrong (and
we appear to be always passing in write buffers as const too, I can't
remember which operation this was though).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140415/101e2bae/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-15 10:09 ` Mark Brown
@ 2014-04-15 11:54 ` Boris BREZILLON
-1 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-15 11:54 UTC (permalink / raw)
To: Mark Brown
Cc: Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin, Chen-Yu Tsai,
Hans de Goede, Carlo Caione, linux-arm-kernel, linux-kernel, dev
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 15/04/2014 12:09, Mark Brown wrote:
> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>> On 14/04/2014 23:04, Mark Brown wrote:
>
>>> The transfer type gets set once per device at init time so why not
>>> just parameterise based on val_bytes?
>
>> Actually, you may want to transfer 1 byte registers using the block
>> method (if your device only support block transfers). This depends on
>> the device being accessed and what it supports, but I'm not sure we can
>> assume 1 byte registers will always be transferred using SMBUS byte
>> transfers.
>
> OK, so if this a realistic issue then it seems like it's better to
> implement three different buses - there is not really any common code
> between the various paths.
Okay, I'll create 4 different busses (one for each access type).
BTW, should I keep these implementations in the same source file
(regmap-smbus.c) ?
And, should I keep one method to register an smbus regmap or should I
provide one method per access type and get rid of the
regmap_smbus_transfer_type enum ?
> This would also mean that you avoid having
> gather write when it can't be implemented.
Correct me if I'm wrong, but they all support gather write.
>
>
> The code is also not validating the lengths for two byte values.
I'm not sure I get this one.
Do you mean I should check that val_size is a 2 byte multiple ?
If this is what you meant, then I should also check it for block transfers.
>
>
>>>> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: + while (count
>>>>> 0 && !ret) { + ret =
>>>> i2c_smbus_write_i2c_block_data(ctx->i2c, +
>>>> reg, + ctx->val_bytes, +
>>>> (const u8 *)data);
>
>>> Fix the const correctness of the API rather than casting.
>
>> The API is correct because the i2c_smbus_write_i2c_block does not modify
>> the data pointer.
>> Just removing the const keyword in the cast should be enough, because
>> you can safely cast a non const pointer to a const one.
>
> If you need to cast away from void at all something is going wrong (and
> we appear to be always passing in write buffers as const too, I can't
> remember which operation this was though).
You're right, removing the cast when passing the val argument to the i2c
block transfer functions works.
Best Regards,
Boris
- --
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJTTR3aAAoJEHLimErDqMKy3w0H/07ZyPXLB5YB3irMT0+eOqtS
spxzeZDv3OKT2Rggsz7wAjkRhFbFT9rAMtGJyrmo2js7CPh5cXEg+oWWeTeLpqjF
xeCyJW6yoihI0JbF2fsYlkuLqGIKrFYTeGzSWgP1DlLdxwJ//lxqQrayOMzd5exS
KbQglfzUEVJ5W5n65CGjmNBYW2/QWasAQGwzgypv3gWVLfQzd+n/vCnHBvn81bfe
Ry8nTMEOLpV3rJSGiNZZQL1TIsDiAUQuxKh+n4mc2ntIgLZcb2l6mjGA/36ziQaA
Gfpc1zTGTQWaY4ZdQuvljAEOl2yBUZrkuf+ZVrv26l6saIQv9ekxSCmVRP/CmbY=
=Bt4h
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-15 11:54 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-15 11:54 UTC (permalink / raw)
To: linux-arm-kernel
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 15/04/2014 12:09, Mark Brown wrote:
> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>> On 14/04/2014 23:04, Mark Brown wrote:
>
>>> The transfer type gets set once per device at init time so why not
>>> just parameterise based on val_bytes?
>
>> Actually, you may want to transfer 1 byte registers using the block
>> method (if your device only support block transfers). This depends on
>> the device being accessed and what it supports, but I'm not sure we can
>> assume 1 byte registers will always be transferred using SMBUS byte
>> transfers.
>
> OK, so if this a realistic issue then it seems like it's better to
> implement three different buses - there is not really any common code
> between the various paths.
Okay, I'll create 4 different busses (one for each access type).
BTW, should I keep these implementations in the same source file
(regmap-smbus.c) ?
And, should I keep one method to register an smbus regmap or should I
provide one method per access type and get rid of the
regmap_smbus_transfer_type enum ?
> This would also mean that you avoid having
> gather write when it can't be implemented.
Correct me if I'm wrong, but they all support gather write.
>
>
> The code is also not validating the lengths for two byte values.
I'm not sure I get this one.
Do you mean I should check that val_size is a 2 byte multiple ?
If this is what you meant, then I should also check it for block transfers.
>
>
>>>> + case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: + while (count
>>>>> 0 && !ret) { + ret =
>>>> i2c_smbus_write_i2c_block_data(ctx->i2c, +
>>>> reg, + ctx->val_bytes, +
>>>> (const u8 *)data);
>
>>> Fix the const correctness of the API rather than casting.
>
>> The API is correct because the i2c_smbus_write_i2c_block does not modify
>> the data pointer.
>> Just removing the const keyword in the cast should be enough, because
>> you can safely cast a non const pointer to a const one.
>
> If you need to cast away from void at all something is going wrong (and
> we appear to be always passing in write buffers as const too, I can't
> remember which operation this was though).
You're right, removing the cast when passing the val argument to the i2c
block transfer functions works.
Best Regards,
Boris
- --
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJTTR3aAAoJEHLimErDqMKy3w0H/07ZyPXLB5YB3irMT0+eOqtS
spxzeZDv3OKT2Rggsz7wAjkRhFbFT9rAMtGJyrmo2js7CPh5cXEg+oWWeTeLpqjF
xeCyJW6yoihI0JbF2fsYlkuLqGIKrFYTeGzSWgP1DlLdxwJ//lxqQrayOMzd5exS
KbQglfzUEVJ5W5n65CGjmNBYW2/QWasAQGwzgypv3gWVLfQzd+n/vCnHBvn81bfe
Ry8nTMEOLpV3rJSGiNZZQL1TIsDiAUQuxKh+n4mc2ntIgLZcb2l6mjGA/36ziQaA
Gfpc1zTGTQWaY4ZdQuvljAEOl2yBUZrkuf+ZVrv26l6saIQv9ekxSCmVRP/CmbY=
=Bt4h
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-15 11:54 ` Boris BREZILLON
@ 2014-04-15 12:10 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 12:10 UTC (permalink / raw)
To: Boris BREZILLON
Cc: Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin, Chen-Yu Tsai,
Hans de Goede, Carlo Caione, linux-arm-kernel, linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]
On Tue, Apr 15, 2014 at 01:54:02PM +0200, Boris BREZILLON wrote:
> On 15/04/2014 12:09, Mark Brown wrote:
> > OK, so if this a realistic issue then it seems like it's better to
> > implement three different buses - there is not really any common code
> > between the various paths.
> Okay, I'll create 4 different busses (one for each access type).
> BTW, should I keep these implementations in the same source file
> (regmap-smbus.c) ?
> And, should I keep one method to register an smbus regmap or should I
> provide one method per access type and get rid of the
> regmap_smbus_transfer_type enum ?
Do something that looks tasteful in implementation.
> > This would also mean that you avoid having
> > gather write when it can't be implemented.
> Correct me if I'm wrong, but they all support gather write.
Everything except for the block transfers is writing one register at a
time, actually looking again everything that can only write one register
at a time ought not to be implementing the buffer based stuff at all and
should instead be hooked into reg_read() and reg_write() - it's not able
to implement the API properly in any of the cases, it's unpacking the
buffer which the upper level code already supports.
> > The code is also not validating the lengths for two byte values.
> I'm not sure I get this one.
> Do you mean I should check that val_size is a 2 byte multiple ?
> If this is what you meant, then I should also check it for block transfers.
Yes.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-15 12:10 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 12:10 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 15, 2014 at 01:54:02PM +0200, Boris BREZILLON wrote:
> On 15/04/2014 12:09, Mark Brown wrote:
> > OK, so if this a realistic issue then it seems like it's better to
> > implement three different buses - there is not really any common code
> > between the various paths.
> Okay, I'll create 4 different busses (one for each access type).
> BTW, should I keep these implementations in the same source file
> (regmap-smbus.c) ?
> And, should I keep one method to register an smbus regmap or should I
> provide one method per access type and get rid of the
> regmap_smbus_transfer_type enum ?
Do something that looks tasteful in implementation.
> > This would also mean that you avoid having
> > gather write when it can't be implemented.
> Correct me if I'm wrong, but they all support gather write.
Everything except for the block transfers is writing one register at a
time, actually looking again everything that can only write one register
at a time ought not to be implementing the buffer based stuff at all and
should instead be hooked into reg_read() and reg_write() - it's not able
to implement the API properly in any of the cases, it's unpacking the
buffer which the upper level code already supports.
> > The code is also not validating the lengths for two byte values.
> I'm not sure I get this one.
> Do you mean I should check that val_size is a 2 byte multiple ?
> If this is what you meant, then I should also check it for block transfers.
Yes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140415/63eff8cd/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-15 11:54 ` Boris BREZILLON
@ 2014-04-15 12:25 ` Lars-Peter Clausen
-1 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 12:25 UTC (permalink / raw)
To: Boris BREZILLON
Cc: Mark Brown, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>
>
> On 15/04/2014 12:09, Mark Brown wrote:
>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>> On 14/04/2014 23:04, Mark Brown wrote:
>
>>>> The transfer type gets set once per device at init time so why not
>>>> just parameterise based on val_bytes?
>
>>> Actually, you may want to transfer 1 byte registers using the block
>>> method (if your device only support block transfers). This depends on
>>> the device being accessed and what it supports, but I'm not sure we can
>>> assume 1 byte registers will always be transferred using SMBUS byte
>>> transfers.
>
>> OK, so if this a realistic issue then it seems like it's better to
>> implement three different buses - there is not really any common code
>> between the various paths.
>
> Okay, I'll create 4 different busses (one for each access type).
> BTW, should I keep these implementations in the same source file
> (regmap-smbus.c) ?
> And, should I keep one method to register an smbus regmap or should I
> provide one method per access type and get rid of the
> regmap_smbus_transfer_type enum ?
I don't think we should leave the decision which bus to use to the driver.
Neither should the driver have to choose whether to use smbus or native I2C.
We want to use native I2C when available, because it is more efficient than
going through the smbus emulation layer. On the other hand we want to
automatically switch to smbus when native I2C is not available and the
device can work fine with smbus. Also I'm afraid that we'll otherwise soon
see code popping up like:
if (of_property_read_bool(np, "use-smbus-regmap"))
regmap = regmap_init_smbus(...)
else
regmap = regmap_init_i2c(...)
My suggestion is that in regmap_init_i2c() you check the capabilities of the
I2C adapter. If it supports native I2C you setup the regmap with the
regmap_i2c struct just as it does right now. If the adapter does not support
native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
struct, and if you can fallback to smbus choose the bus depending on the
config's val_bytes.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-15 12:25 ` Lars-Peter Clausen
0 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 12:25 UTC (permalink / raw)
To: linux-arm-kernel
On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>
>
> On 15/04/2014 12:09, Mark Brown wrote:
>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>> On 14/04/2014 23:04, Mark Brown wrote:
>
>>>> The transfer type gets set once per device at init time so why not
>>>> just parameterise based on val_bytes?
>
>>> Actually, you may want to transfer 1 byte registers using the block
>>> method (if your device only support block transfers). This depends on
>>> the device being accessed and what it supports, but I'm not sure we can
>>> assume 1 byte registers will always be transferred using SMBUS byte
>>> transfers.
>
>> OK, so if this a realistic issue then it seems like it's better to
>> implement three different buses - there is not really any common code
>> between the various paths.
>
> Okay, I'll create 4 different busses (one for each access type).
> BTW, should I keep these implementations in the same source file
> (regmap-smbus.c) ?
> And, should I keep one method to register an smbus regmap or should I
> provide one method per access type and get rid of the
> regmap_smbus_transfer_type enum ?
I don't think we should leave the decision which bus to use to the driver.
Neither should the driver have to choose whether to use smbus or native I2C.
We want to use native I2C when available, because it is more efficient than
going through the smbus emulation layer. On the other hand we want to
automatically switch to smbus when native I2C is not available and the
device can work fine with smbus. Also I'm afraid that we'll otherwise soon
see code popping up like:
if (of_property_read_bool(np, "use-smbus-regmap"))
regmap = regmap_init_smbus(...)
else
regmap = regmap_init_i2c(...)
My suggestion is that in regmap_init_i2c() you check the capabilities of the
I2C adapter. If it supports native I2C you setup the regmap with the
regmap_i2c struct just as it does right now. If the adapter does not support
native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
struct, and if you can fallback to smbus choose the bus depending on the
config's val_bytes.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-15 12:25 ` Lars-Peter Clausen
@ 2014-04-15 12:40 ` Boris BREZILLON
-1 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-15 12:40 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Mark Brown, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
On 15/04/2014 14:25, Lars-Peter Clausen wrote:
> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>>
>>
>> On 15/04/2014 12:09, Mark Brown wrote:
>>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>>> On 14/04/2014 23:04, Mark Brown wrote:
>>
>>>>> The transfer type gets set once per device at init time so why not
>>>>> just parameterise based on val_bytes?
>>
>>>> Actually, you may want to transfer 1 byte registers using the block
>>>> method (if your device only support block transfers). This depends on
>>>> the device being accessed and what it supports, but I'm not sure we
>>>> can
>>>> assume 1 byte registers will always be transferred using SMBUS byte
>>>> transfers.
>>
>>> OK, so if this a realistic issue then it seems like it's better to
>>> implement three different buses - there is not really any common code
>>> between the various paths.
>>
>> Okay, I'll create 4 different busses (one for each access type).
>> BTW, should I keep these implementations in the same source file
>> (regmap-smbus.c) ?
>> And, should I keep one method to register an smbus regmap or should I
>> provide one method per access type and get rid of the
>> regmap_smbus_transfer_type enum ?
>
> I don't think we should leave the decision which bus to use to the
> driver. Neither should the driver have to choose whether to use smbus
> or native I2C. We want to use native I2C when available, because it is
> more efficient than going through the smbus emulation layer. On the
> other hand we want to automatically switch to smbus when native I2C is
> not available and the device can work fine with smbus. Also I'm afraid
> that we'll otherwise soon see code popping up like:
>
> if (of_property_read_bool(np, "use-smbus-regmap"))
> regmap = regmap_init_smbus(...)
> else
> regmap = regmap_init_i2c(...)
>
> My suggestion is that in regmap_init_i2c() you check the capabilities
> of the I2C adapter. If it supports native I2C you setup the regmap
> with the regmap_i2c struct just as it does right now. If the adapter
> does not support native I2C, check if the device can be supported by
> smbus (reg_bytes == 8 && val_bytes % 8 == 0). For each type of smbus
> operations have one regmap_bus struct, and if you can fallback to
> smbus choose the bus depending on the config's val_bytes.
>
What if the device only support SMBus block transfers, but the adapter
support both regular I2C and SMBus block transfers (don't know if such a
device exist :-)) ?
My point is that I'm not sure the core remap-i2c code can decide whether
to use SMBus or I2C transfer only from val_bytes value, but I might be
wrong.
Best Regards,
Boris
> - Lars
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-15 12:40 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-15 12:40 UTC (permalink / raw)
To: linux-arm-kernel
On 15/04/2014 14:25, Lars-Peter Clausen wrote:
> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>>
>>
>> On 15/04/2014 12:09, Mark Brown wrote:
>>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>>> On 14/04/2014 23:04, Mark Brown wrote:
>>
>>>>> The transfer type gets set once per device at init time so why not
>>>>> just parameterise based on val_bytes?
>>
>>>> Actually, you may want to transfer 1 byte registers using the block
>>>> method (if your device only support block transfers). This depends on
>>>> the device being accessed and what it supports, but I'm not sure we
>>>> can
>>>> assume 1 byte registers will always be transferred using SMBUS byte
>>>> transfers.
>>
>>> OK, so if this a realistic issue then it seems like it's better to
>>> implement three different buses - there is not really any common code
>>> between the various paths.
>>
>> Okay, I'll create 4 different busses (one for each access type).
>> BTW, should I keep these implementations in the same source file
>> (regmap-smbus.c) ?
>> And, should I keep one method to register an smbus regmap or should I
>> provide one method per access type and get rid of the
>> regmap_smbus_transfer_type enum ?
>
> I don't think we should leave the decision which bus to use to the
> driver. Neither should the driver have to choose whether to use smbus
> or native I2C. We want to use native I2C when available, because it is
> more efficient than going through the smbus emulation layer. On the
> other hand we want to automatically switch to smbus when native I2C is
> not available and the device can work fine with smbus. Also I'm afraid
> that we'll otherwise soon see code popping up like:
>
> if (of_property_read_bool(np, "use-smbus-regmap"))
> regmap = regmap_init_smbus(...)
> else
> regmap = regmap_init_i2c(...)
>
> My suggestion is that in regmap_init_i2c() you check the capabilities
> of the I2C adapter. If it supports native I2C you setup the regmap
> with the regmap_i2c struct just as it does right now. If the adapter
> does not support native I2C, check if the device can be supported by
> smbus (reg_bytes == 8 && val_bytes % 8 == 0). For each type of smbus
> operations have one regmap_bus struct, and if you can fallback to
> smbus choose the bus depending on the config's val_bytes.
>
What if the device only support SMBus block transfers, but the adapter
support both regular I2C and SMBus block transfers (don't know if such a
device exist :-)) ?
My point is that I'm not sure the core remap-i2c code can decide whether
to use SMBus or I2C transfer only from val_bytes value, but I might be
wrong.
Best Regards,
Boris
> - Lars
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
2014-04-15 12:25 ` Lars-Peter Clausen
@ 2014-04-15 12:56 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 12:56 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Boris BREZILLON, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]
On Tue, Apr 15, 2014 at 02:25:15PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
> >And, should I keep one method to register an smbus regmap or should I
> >provide one method per access type and get rid of the
> >regmap_smbus_transfer_type enum ?
> I don't think we should leave the decision which bus to use to the driver.
> Neither should the driver have to choose whether to use smbus or native I2C.
> We want to use native I2C when available, because it is more efficient than
> going through the smbus emulation layer. On the other hand we want to
> automatically switch to smbus when native I2C is not available and the
> device can work fine with smbus. Also I'm afraid that we'll otherwise soon
> see code popping up like:
I have to say I'd misread the commit message (due to the multiple copies
that arrived before I actually reviewed it I think I just skimmed it and
went into incremental review) and was reviewing this as being for
*device* limitations not controller ones. That also matches with what
Boris was saying about not being able to decide to use the word or byte
at a time operations automatically, though the commit message does say
it's about controller limitations.
> My suggestion is that in regmap_init_i2c() you check the capabilities of the
> I2C adapter. If it supports native I2C you setup the regmap with the
> regmap_i2c struct just as it does right now. If the adapter does not support
> native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
> val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
> struct, and if you can fallback to smbus choose the bus depending on the
> config's val_bytes.
That'd definitely be useful but potentially orthogonal, we can also do
both and expose smbus explicitly with I2C falling back to it
transparently.
If the device *is* limited to smbus and the controller supports both
(some do) I'd naively have expected that the native smbus support would
do a little better - otherwise why bother using it? We could identify
the constraint set automatically for I2C devices though it's more for
the client driver to specify.
This means there's two changes to consider here:
- Providing APIs for registering actual smbus devices as a convenience
for devices with that constraint, regardless of how that is done
behind the scenes.
- Having the I2C implementation automatically use the smbus APIs if
it can and either the controller is smbus only or it makes sense to
do so for optimisation.
both of which are independently useful.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
@ 2014-04-15 12:56 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 12:56 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 15, 2014 at 02:25:15PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
> >And, should I keep one method to register an smbus regmap or should I
> >provide one method per access type and get rid of the
> >regmap_smbus_transfer_type enum ?
> I don't think we should leave the decision which bus to use to the driver.
> Neither should the driver have to choose whether to use smbus or native I2C.
> We want to use native I2C when available, because it is more efficient than
> going through the smbus emulation layer. On the other hand we want to
> automatically switch to smbus when native I2C is not available and the
> device can work fine with smbus. Also I'm afraid that we'll otherwise soon
> see code popping up like:
I have to say I'd misread the commit message (due to the multiple copies
that arrived before I actually reviewed it I think I just skimmed it and
went into incremental review) and was reviewing this as being for
*device* limitations not controller ones. That also matches with what
Boris was saying about not being able to decide to use the word or byte
at a time operations automatically, though the commit message does say
it's about controller limitations.
> My suggestion is that in regmap_init_i2c() you check the capabilities of the
> I2C adapter. If it supports native I2C you setup the regmap with the
> regmap_i2c struct just as it does right now. If the adapter does not support
> native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
> val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
> struct, and if you can fallback to smbus choose the bus depending on the
> config's val_bytes.
That'd definitely be useful but potentially orthogonal, we can also do
both and expose smbus explicitly with I2C falling back to it
transparently.
If the device *is* limited to smbus and the controller supports both
(some do) I'd naively have expected that the native smbus support would
do a little better - otherwise why bother using it? We could identify
the constraint set automatically for I2C devices though it's more for
the client driver to specify.
This means there's two changes to consider here:
- Providing APIs for registering actual smbus devices as a convenience
for devices with that constraint, regardless of how that is done
behind the scenes.
- Having the I2C implementation automatically use the smbus APIs if
it can and either the controller is smbus only or it makes sense to
do so for optimisation.
both of which are independently useful.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140415/980758da/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
2014-04-15 12:40 ` Boris BREZILLON
@ 2014-04-15 13:08 ` Lars-Peter Clausen
-1 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 13:08 UTC (permalink / raw)
To: Boris BREZILLON
Cc: Mark Brown, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
On 04/15/2014 02:40 PM, Boris BREZILLON wrote:
>
> On 15/04/2014 14:25, Lars-Peter Clausen wrote:
>> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>>>
>>>
>>> On 15/04/2014 12:09, Mark Brown wrote:
>>>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>>>> On 14/04/2014 23:04, Mark Brown wrote:
>>>
>>>>>> The transfer type gets set once per device at init time so why not
>>>>>> just parameterise based on val_bytes?
>>>
>>>>> Actually, you may want to transfer 1 byte registers using the block
>>>>> method (if your device only support block transfers). This depends on
>>>>> the device being accessed and what it supports, but I'm not sure we
>>>>> can
>>>>> assume 1 byte registers will always be transferred using SMBUS byte
>>>>> transfers.
>>>
>>>> OK, so if this a realistic issue then it seems like it's better to
>>>> implement three different buses - there is not really any common code
>>>> between the various paths.
>>>
>>> Okay, I'll create 4 different busses (one for each access type).
>>> BTW, should I keep these implementations in the same source file
>>> (regmap-smbus.c) ?
>>> And, should I keep one method to register an smbus regmap or should I
>>> provide one method per access type and get rid of the
>>> regmap_smbus_transfer_type enum ?
>>
>> I don't think we should leave the decision which bus to use to the
>> driver. Neither should the driver have to choose whether to use smbus
>> or native I2C. We want to use native I2C when available, because it is
>> more efficient than going through the smbus emulation layer. On the
>> other hand we want to automatically switch to smbus when native I2C is
>> not available and the device can work fine with smbus. Also I'm afraid
>> that we'll otherwise soon see code popping up like:
>>
>> if (of_property_read_bool(np, "use-smbus-regmap"))
>> regmap = regmap_init_smbus(...)
>> else
>> regmap = regmap_init_i2c(...)
>>
>> My suggestion is that in regmap_init_i2c() you check the capabilities
>> of the I2C adapter. If it supports native I2C you setup the regmap
>> with the regmap_i2c struct just as it does right now. If the adapter
>> does not support native I2C, check if the device can be supported by
>> smbus (reg_bytes == 8 && val_bytes % 8 == 0). For each type of smbus
>> operations have one regmap_bus struct, and if you can fallback to
>> smbus choose the bus depending on the config's val_bytes.
>>
>
> What if the device only support SMBus block transfers, but the adapter
> support both regular I2C and SMBus block transfers (don't know if such a
> device exist :-)) ?
SMBus block transfers prefix the transfer with the number of bytes that are
in this transfer. We do not support this currently in the I2C backend (but
we could). If there should ever be a driver that needs support for SMBus
block transfers this could easily be added. For now I think it is safe to
ignore this.
>
> My point is that I'm not sure the core remap-i2c code can decide whether
> to use SMBus or I2C transfer only from val_bytes value, but I might be
> wrong.
It must be able to figure out what to do based on the config. If it is not
we probably need to extend the config. E.g. if we should ever need to
support devices which require SMBus block transfers we could add a flag that
tells regmap that this device want's all messages prefixed with the length
of the message.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus
@ 2014-04-15 13:08 ` Lars-Peter Clausen
0 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 13:08 UTC (permalink / raw)
To: linux-arm-kernel
On 04/15/2014 02:40 PM, Boris BREZILLON wrote:
>
> On 15/04/2014 14:25, Lars-Peter Clausen wrote:
>> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>>>
>>>
>>> On 15/04/2014 12:09, Mark Brown wrote:
>>>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>>>> On 14/04/2014 23:04, Mark Brown wrote:
>>>
>>>>>> The transfer type gets set once per device at init time so why not
>>>>>> just parameterise based on val_bytes?
>>>
>>>>> Actually, you may want to transfer 1 byte registers using the block
>>>>> method (if your device only support block transfers). This depends on
>>>>> the device being accessed and what it supports, but I'm not sure we
>>>>> can
>>>>> assume 1 byte registers will always be transferred using SMBUS byte
>>>>> transfers.
>>>
>>>> OK, so if this a realistic issue then it seems like it's better to
>>>> implement three different buses - there is not really any common code
>>>> between the various paths.
>>>
>>> Okay, I'll create 4 different busses (one for each access type).
>>> BTW, should I keep these implementations in the same source file
>>> (regmap-smbus.c) ?
>>> And, should I keep one method to register an smbus regmap or should I
>>> provide one method per access type and get rid of the
>>> regmap_smbus_transfer_type enum ?
>>
>> I don't think we should leave the decision which bus to use to the
>> driver. Neither should the driver have to choose whether to use smbus
>> or native I2C. We want to use native I2C when available, because it is
>> more efficient than going through the smbus emulation layer. On the
>> other hand we want to automatically switch to smbus when native I2C is
>> not available and the device can work fine with smbus. Also I'm afraid
>> that we'll otherwise soon see code popping up like:
>>
>> if (of_property_read_bool(np, "use-smbus-regmap"))
>> regmap = regmap_init_smbus(...)
>> else
>> regmap = regmap_init_i2c(...)
>>
>> My suggestion is that in regmap_init_i2c() you check the capabilities
>> of the I2C adapter. If it supports native I2C you setup the regmap
>> with the regmap_i2c struct just as it does right now. If the adapter
>> does not support native I2C, check if the device can be supported by
>> smbus (reg_bytes == 8 && val_bytes % 8 == 0). For each type of smbus
>> operations have one regmap_bus struct, and if you can fallback to
>> smbus choose the bus depending on the config's val_bytes.
>>
>
> What if the device only support SMBus block transfers, but the adapter
> support both regular I2C and SMBus block transfers (don't know if such a
> device exist :-)) ?
SMBus block transfers prefix the transfer with the number of bytes that are
in this transfer. We do not support this currently in the I2C backend (but
we could). If there should ever be a driver that needs support for SMBus
block transfers this could easily be added. For now I think it is safe to
ignore this.
>
> My point is that I'm not sure the core remap-i2c code can decide whether
> to use SMBus or I2C transfer only from val_bytes value, but I might be
> wrong.
It must be able to figure out what to do based on the config. If it is not
we probably need to extend the config. E.g. if we should ever need to
support devices which require SMBus block transfers we could add a flag that
tells regmap that this device want's all messages prefixed with the length
of the message.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
2014-04-15 12:56 ` Mark Brown
@ 2014-04-15 13:34 ` Lars-Peter Clausen
-1 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 13:34 UTC (permalink / raw)
To: Mark Brown
Cc: Boris BREZILLON, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
On 04/15/2014 02:56 PM, Mark Brown wrote:
[...]
>> My suggestion is that in regmap_init_i2c() you check the capabilities of the
>> I2C adapter. If it supports native I2C you setup the regmap with the
>> regmap_i2c struct just as it does right now. If the adapter does not support
>> native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
>> val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
>> struct, and if you can fallback to smbus choose the bus depending on the
>> config's val_bytes.
>
> That'd definitely be useful but potentially orthogonal, we can also do
> both and expose smbus explicitly with I2C falling back to it
> transparently.
>
> If the device *is* limited to smbus and the controller supports both
> (some do) I'd naively have expected that the native smbus support would
> do a little better - otherwise why bother using it? We could identify
> the constraint set automatically for I2C devices though it's more for
> the client driver to specify.
>
> This means there's two changes to consider here:
>
> - Providing APIs for registering actual smbus devices as a convenience
> for devices with that constraint, regardless of how that is done
> behind the scenes.
>
> - Having the I2C implementation automatically use the smbus APIs if
> it can and either the controller is smbus only or it makes sense to
> do so for optimisation.
>
> both of which are independently useful.
>
I don't think it makes sense to expose smbus explicitly. We can already
describe smbus restricted devices just fine with the current regmap_config
and already support them with the current I2C regmap implementation. Using
native I2C to access these devices will be more efficient than going through
the smbus emulation layer. The smbus emulation layer essentially does the
same as we do in regmap, so using the smbus emulation layer through regmap
means doing the same thing twice.
What we currently do not support is devices which are restricted to block
transfers and I think adding support for this should be handled by a
separate patchset. As far as I understood it this patchset is about making
it possible to use smbus controllers with regmap devices which are smbus
compatible.
As I see it there are currently 3 cases:
1) Device is strictly smbus only and the controller supports native smbus
=> Use smbus
2) The device is smbus compatible but has extensions (e.g. support for multi
register writes) and the controller supports only smbus.
=> Use smbus
3) For every other case
=> Use native I2C.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
@ 2014-04-15 13:34 ` Lars-Peter Clausen
0 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 13:34 UTC (permalink / raw)
To: linux-arm-kernel
On 04/15/2014 02:56 PM, Mark Brown wrote:
[...]
>> My suggestion is that in regmap_init_i2c() you check the capabilities of the
>> I2C adapter. If it supports native I2C you setup the regmap with the
>> regmap_i2c struct just as it does right now. If the adapter does not support
>> native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
>> val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
>> struct, and if you can fallback to smbus choose the bus depending on the
>> config's val_bytes.
>
> That'd definitely be useful but potentially orthogonal, we can also do
> both and expose smbus explicitly with I2C falling back to it
> transparently.
>
> If the device *is* limited to smbus and the controller supports both
> (some do) I'd naively have expected that the native smbus support would
> do a little better - otherwise why bother using it? We could identify
> the constraint set automatically for I2C devices though it's more for
> the client driver to specify.
>
> This means there's two changes to consider here:
>
> - Providing APIs for registering actual smbus devices as a convenience
> for devices with that constraint, regardless of how that is done
> behind the scenes.
>
> - Having the I2C implementation automatically use the smbus APIs if
> it can and either the controller is smbus only or it makes sense to
> do so for optimisation.
>
> both of which are independently useful.
>
I don't think it makes sense to expose smbus explicitly. We can already
describe smbus restricted devices just fine with the current regmap_config
and already support them with the current I2C regmap implementation. Using
native I2C to access these devices will be more efficient than going through
the smbus emulation layer. The smbus emulation layer essentially does the
same as we do in regmap, so using the smbus emulation layer through regmap
means doing the same thing twice.
What we currently do not support is devices which are restricted to block
transfers and I think adding support for this should be handled by a
separate patchset. As far as I understood it this patchset is about making
it possible to use smbus controllers with regmap devices which are smbus
compatible.
As I see it there are currently 3 cases:
1) Device is strictly smbus only and the controller supports native smbus
=> Use smbus
2) The device is smbus compatible but has extensions (e.g. support for multi
register writes) and the controller supports only smbus.
=> Use smbus
3) For every other case
=> Use native I2C.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
2014-04-15 13:34 ` Lars-Peter Clausen
@ 2014-04-15 16:46 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 16:46 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Boris BREZILLON, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]
On Tue, Apr 15, 2014 at 03:34:55PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 02:56 PM, Mark Brown wrote:
> > - Providing APIs for registering actual smbus devices as a convenience
> > for devices with that constraint, regardless of how that is done
> > behind the scenes.
> > - Having the I2C implementation automatically use the smbus APIs if
> > it can and either the controller is smbus only or it makes sense to
> > do so for optimisation.
> I don't think it makes sense to expose smbus explicitly. We can already
> describe smbus restricted devices just fine with the current regmap_config
> and already support them with the current I2C regmap implementation. Using
Please note what I said about convenience - if lots of devices have
exactly the same set of constraints to set up it's possible sensible to
have a standard way of specifying them. It's going to be a bit easier
to just say it's smbus than to remember exactly what that means.
> native I2C to access these devices will be more efficient than going through
> the smbus emulation layer. The smbus emulation layer essentially does the
> same as we do in regmap, so using the smbus emulation layer through regmap
> means doing the same thing twice.
Right, that's why I said it's probably only worth it if the controller
does smbus natively.
> As I see it there are currently 3 cases:
> 1) Device is strictly smbus only and the controller supports native smbus
> => Use smbus
> 2) The device is smbus compatible but has extensions (e.g. support for multi
> register writes) and the controller supports only smbus.
> => Use smbus
> 3) For every other case
> => Use native I2C.
It's not clear to me that if the controller supports smbus we shouldn't
use it; presumably it's adding some value to have written the code to
take advantage of it. That would mean another case for device is smbus
only and controller has explicit smbus support.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
@ 2014-04-15 16:46 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 16:46 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 15, 2014 at 03:34:55PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 02:56 PM, Mark Brown wrote:
> > - Providing APIs for registering actual smbus devices as a convenience
> > for devices with that constraint, regardless of how that is done
> > behind the scenes.
> > - Having the I2C implementation automatically use the smbus APIs if
> > it can and either the controller is smbus only or it makes sense to
> > do so for optimisation.
> I don't think it makes sense to expose smbus explicitly. We can already
> describe smbus restricted devices just fine with the current regmap_config
> and already support them with the current I2C regmap implementation. Using
Please note what I said about convenience - if lots of devices have
exactly the same set of constraints to set up it's possible sensible to
have a standard way of specifying them. It's going to be a bit easier
to just say it's smbus than to remember exactly what that means.
> native I2C to access these devices will be more efficient than going through
> the smbus emulation layer. The smbus emulation layer essentially does the
> same as we do in regmap, so using the smbus emulation layer through regmap
> means doing the same thing twice.
Right, that's why I said it's probably only worth it if the controller
does smbus natively.
> As I see it there are currently 3 cases:
> 1) Device is strictly smbus only and the controller supports native smbus
> => Use smbus
> 2) The device is smbus compatible but has extensions (e.g. support for multi
> register writes) and the controller supports only smbus.
> => Use smbus
> 3) For every other case
> => Use native I2C.
It's not clear to me that if the controller supports smbus we shouldn't
use it; presumably it's adding some value to have written the code to
take advantage of it. That would mean another case for device is smbus
only and controller has explicit smbus support.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140415/43481013/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
2014-04-15 16:46 ` Mark Brown
@ 2014-04-15 19:18 ` Lars-Peter Clausen
-1 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 19:18 UTC (permalink / raw)
To: Mark Brown
Cc: Boris BREZILLON, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
On 04/15/2014 06:46 PM, Mark Brown wrote:
> On Tue, Apr 15, 2014 at 03:34:55PM +0200, Lars-Peter Clausen wrote:
>> On 04/15/2014 02:56 PM, Mark Brown wrote:
>
>>> - Providing APIs for registering actual smbus devices as a convenience
>>> for devices with that constraint, regardless of how that is done
>>> behind the scenes.
>
>>> - Having the I2C implementation automatically use the smbus APIs if
>>> it can and either the controller is smbus only or it makes sense to
>>> do so for optimisation.
>
>> I don't think it makes sense to expose smbus explicitly. We can already
>> describe smbus restricted devices just fine with the current regmap_config
>> and already support them with the current I2C regmap implementation. Using
>
> Please note what I said about convenience - if lots of devices have
> exactly the same set of constraints to set up it's possible sensible to
> have a standard way of specifying them. It's going to be a bit easier
> to just say it's smbus than to remember exactly what that means.
Maybe, but the only shared constraint is reg_bits = 8 (and if it is pure
smbus use_single_rw = true).
>
>> native I2C to access these devices will be more efficient than going through
>> the smbus emulation layer. The smbus emulation layer essentially does the
>> same as we do in regmap, so using the smbus emulation layer through regmap
>> means doing the same thing twice.
>
> Right, that's why I said it's probably only worth it if the controller
> does smbus natively.
>
>> As I see it there are currently 3 cases:
>
>> 1) Device is strictly smbus only and the controller supports native smbus
>> => Use smbus
>> 2) The device is smbus compatible but has extensions (e.g. support for multi
>> register writes) and the controller supports only smbus.
>> => Use smbus
>> 3) For every other case
>> => Use native I2C.
>
> It's not clear to me that if the controller supports smbus we shouldn't
> use it; presumably it's adding some value to have written the code to
> take advantage of it. That would mean another case for device is smbus
> only and controller has explicit smbus support.
Potentially yes, but not necessarily in the first version of the driver.
It's a bit of an exotic case, there seem to be only two I2C controller
drivers in the Linux kernel that have support for both raw I2C and native
SMBus and neither of them don't seem to be fairly recent (i2c-powermac.c and
i2c-pasemi.c). And on the other hand most devices are SMBus compatible have
extensions like being able to write/read multiple register in one transfer
if raw I2C access is available, which is something we want to make use of if
available.
And the other thing to consider is that a client driver currently has no way
to query whether the controller driver supports native SMBus or only
emulated SMBus. This is surely something that can be added to the I2C core,
but this is necessarily something that we require before any SMBus support
is added to regmap.
So in conclusion I think the best way forward is to create a patch that
checks if native I2C is available, and if not falls back to either
SMBUS_{WRITE,READ}_BYTE or SMBUS_{WRITE,READ}_WORD operations (Depending on
whether val_bits is 8 or 16). Such a patch, I think, has the most effect for
the least amount of work since it is rather simple and self contained and
allows all devices which are SMBus compatible to be used with SMBus-only
controllers. Everything else discussed in this thread (optimizations for
special cases, convince helpers) can then be implemented on top of that.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
@ 2014-04-15 19:18 ` Lars-Peter Clausen
0 siblings, 0 replies; 40+ messages in thread
From: Lars-Peter Clausen @ 2014-04-15 19:18 UTC (permalink / raw)
To: linux-arm-kernel
On 04/15/2014 06:46 PM, Mark Brown wrote:
> On Tue, Apr 15, 2014 at 03:34:55PM +0200, Lars-Peter Clausen wrote:
>> On 04/15/2014 02:56 PM, Mark Brown wrote:
>
>>> - Providing APIs for registering actual smbus devices as a convenience
>>> for devices with that constraint, regardless of how that is done
>>> behind the scenes.
>
>>> - Having the I2C implementation automatically use the smbus APIs if
>>> it can and either the controller is smbus only or it makes sense to
>>> do so for optimisation.
>
>> I don't think it makes sense to expose smbus explicitly. We can already
>> describe smbus restricted devices just fine with the current regmap_config
>> and already support them with the current I2C regmap implementation. Using
>
> Please note what I said about convenience - if lots of devices have
> exactly the same set of constraints to set up it's possible sensible to
> have a standard way of specifying them. It's going to be a bit easier
> to just say it's smbus than to remember exactly what that means.
Maybe, but the only shared constraint is reg_bits = 8 (and if it is pure
smbus use_single_rw = true).
>
>> native I2C to access these devices will be more efficient than going through
>> the smbus emulation layer. The smbus emulation layer essentially does the
>> same as we do in regmap, so using the smbus emulation layer through regmap
>> means doing the same thing twice.
>
> Right, that's why I said it's probably only worth it if the controller
> does smbus natively.
>
>> As I see it there are currently 3 cases:
>
>> 1) Device is strictly smbus only and the controller supports native smbus
>> => Use smbus
>> 2) The device is smbus compatible but has extensions (e.g. support for multi
>> register writes) and the controller supports only smbus.
>> => Use smbus
>> 3) For every other case
>> => Use native I2C.
>
> It's not clear to me that if the controller supports smbus we shouldn't
> use it; presumably it's adding some value to have written the code to
> take advantage of it. That would mean another case for device is smbus
> only and controller has explicit smbus support.
Potentially yes, but not necessarily in the first version of the driver.
It's a bit of an exotic case, there seem to be only two I2C controller
drivers in the Linux kernel that have support for both raw I2C and native
SMBus and neither of them don't seem to be fairly recent (i2c-powermac.c and
i2c-pasemi.c). And on the other hand most devices are SMBus compatible have
extensions like being able to write/read multiple register in one transfer
if raw I2C access is available, which is something we want to make use of if
available.
And the other thing to consider is that a client driver currently has no way
to query whether the controller driver supports native SMBus or only
emulated SMBus. This is surely something that can be added to the I2C core,
but this is necessarily something that we require before any SMBus support
is added to regmap.
So in conclusion I think the best way forward is to create a patch that
checks if native I2C is available, and if not falls back to either
SMBUS_{WRITE,READ}_BYTE or SMBUS_{WRITE,READ}_WORD operations (Depending on
whether val_bits is 8 or 16). Such a patch, I think, has the most effect for
the least amount of work since it is rather simple and self contained and
allows all devices which are SMBus compatible to be used with SMBus-only
controllers. Everything else discussed in this thread (optimizations for
special cases, convince helpers) can then be implemented on top of that.
- Lars
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
2014-04-15 19:18 ` Lars-Peter Clausen
@ 2014-04-15 22:38 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 22:38 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Boris BREZILLON, Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin,
Chen-Yu Tsai, Hans de Goede, Carlo Caione, linux-arm-kernel,
linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 2372 bytes --]
On Tue, Apr 15, 2014 at 09:18:11PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 06:46 PM, Mark Brown wrote:
> >Please note what I said about convenience - if lots of devices have
> >exactly the same set of constraints to set up it's possible sensible to
> >have a standard way of specifying them. It's going to be a bit easier
> >to just say it's smbus than to remember exactly what that means.
> Maybe, but the only shared constraint is reg_bits = 8 (and if it is pure
> smbus use_single_rw = true).
I thought there was some other stuff at the edge of the protocol, though
perhaps I misremember. We do also have the thing with implementing the
bulk write differently.
> It's a bit of an exotic case, there seem to be only two I2C controller
> drivers in the Linux kernel that have support for both raw I2C and native
> SMBus and neither of them don't seem to be fairly recent (i2c-powermac.c and
> i2c-pasemi.c). And on the other hand most devices are SMBus compatible have
Blackfin was another - I didn't look that far to be honest since the
first few that I found did it.
> extensions like being able to write/read multiple register in one transfer
> if raw I2C access is available, which is something we want to make use of if
> available.
Sure, if the device supports it.
> And the other thing to consider is that a client driver currently has no way
> to query whether the controller driver supports native SMBus or only
> emulated SMBus. This is surely something that can be added to the I2C core,
> but this is necessarily something that we require before any SMBus support
> is added to regmap.
Right, that'll take a couple of minutes though.
> So in conclusion I think the best way forward is to create a patch that
> checks if native I2C is available, and if not falls back to either
> SMBUS_{WRITE,READ}_BYTE or SMBUS_{WRITE,READ}_WORD operations (Depending on
> whether val_bits is 8 or 16). Such a patch, I think, has the most effect for
> the least amount of work since it is rather simple and self contained and
> allows all devices which are SMBus compatible to be used with SMBus-only
> controllers. Everything else discussed in this thread (optimizations for
> special cases, convince helpers) can then be implemented on top of that.
Indeed, like I've been saying there's several orthogonal things going on
here and that's one of them.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v2] regmap: smbus: add support for regmap over smbus
@ 2014-04-15 22:38 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-15 22:38 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Apr 15, 2014 at 09:18:11PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 06:46 PM, Mark Brown wrote:
> >Please note what I said about convenience - if lots of devices have
> >exactly the same set of constraints to set up it's possible sensible to
> >have a standard way of specifying them. It's going to be a bit easier
> >to just say it's smbus than to remember exactly what that means.
> Maybe, but the only shared constraint is reg_bits = 8 (and if it is pure
> smbus use_single_rw = true).
I thought there was some other stuff at the edge of the protocol, though
perhaps I misremember. We do also have the thing with implementing the
bulk write differently.
> It's a bit of an exotic case, there seem to be only two I2C controller
> drivers in the Linux kernel that have support for both raw I2C and native
> SMBus and neither of them don't seem to be fairly recent (i2c-powermac.c and
> i2c-pasemi.c). And on the other hand most devices are SMBus compatible have
Blackfin was another - I didn't look that far to be honest since the
first few that I found did it.
> extensions like being able to write/read multiple register in one transfer
> if raw I2C access is available, which is something we want to make use of if
> available.
Sure, if the device supports it.
> And the other thing to consider is that a client driver currently has no way
> to query whether the controller driver supports native SMBus or only
> emulated SMBus. This is surely something that can be added to the I2C core,
> but this is necessarily something that we require before any SMBus support
> is added to regmap.
Right, that'll take a couple of minutes though.
> So in conclusion I think the best way forward is to create a patch that
> checks if native I2C is available, and if not falls back to either
> SMBUS_{WRITE,READ}_BYTE or SMBUS_{WRITE,READ}_WORD operations (Depending on
> whether val_bits is 8 or 16). Such a patch, I think, has the most effect for
> the least amount of work since it is rather simple and self contained and
> allows all devices which are SMBus compatible to be used with SMBus-only
> controllers. Everything else discussed in this thread (optimizations for
> special cases, convince helpers) can then be implemented on top of that.
Indeed, like I've been saying there's several orthogonal things going on
here and that's one of them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140415/1baf6253/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
2014-04-15 22:38 ` Mark Brown
@ 2014-04-16 8:16 ` Boris BREZILLON
-1 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-16 8:16 UTC (permalink / raw)
To: Mark Brown, Lars-Peter Clausen
Cc: Greg Kroah-Hartman, Maxime Ripard, Shuge, kevin, Chen-Yu Tsai,
Hans de Goede, Carlo Caione, linux-arm-kernel, linux-kernel, dev,
Boris BREZILLON
Some I2C adapters are only compatible with the SMBus protocol and do not
support standard I2C transfers.
Fallback to SMBus transfers if we encounter such kind of adapters.
The transfer type is chosen according to the val_bits field in the regmap
config.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark, Lars,
Would this patch be acceptable ?
I'm still not convinced that we can decide which kind of protocol should
be used based only on the adapter functionalities and the val_bytes field.
But doing this as a first step should work in my case (my adapter only
supports SMBus byte transfers :-)).
Note that this patch has not been tested yet. I'll test it and resend a
proper version, if you agree on the implementation.
Best Regards,
Boris
drivers/base/regmap/regmap-i2c.c | 101 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 99 insertions(+), 2 deletions(-)
diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index ebd1895..80051c5 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -14,6 +14,69 @@
#include <linux/i2c.h>
#include <linux/module.h>
+
+static int regmap_smbus_byte_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+ int ret;
+
+ if (reg > 0xff)
+ return -EINVAL;
+
+ ret = i2c_smbus_read_byte_data(i2c, reg);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+
+ return 0;
+}
+
+static int regmap_smbus_byte_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+
+ if (val > 0xff || reg > 0xff)
+ return -EINVAL;
+
+ return i2c_smbus_write_byte_data(i2c, reg, val);
+}
+
+static int regmap_smbus_word_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+ int ret;
+
+ if (reg > 0xffff)
+ return -EINVAL;
+
+ ret = i2c_smbus_read_word_data(i2c, reg);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+
+ return 0;
+}
+
+static int regmap_smbus_word_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+
+ if (val > 0xffff || reg > 0xffff)
+ return -EINVAL;
+
+ return i2c_smbus_write_word_data(i2c, reg, val);
+}
+
static int regmap_i2c_write(void *context, const void *data, size_t count)
{
struct device *dev = context;
@@ -97,6 +160,28 @@ static struct regmap_bus regmap_i2c = {
.read = regmap_i2c_read,
};
+const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
+ struct regmap_config *config)
+{
+ if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
+ return ®map_i2c;
+ } else if (config->val_bits == 16 &&
+ i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA)) {
+ config->reg_read = regmap_smbus_word_reg_read;
+ config->reg_write = regmap_smbus_word_reg_write;
+ return NULL;
+ } else if (config->val_bits == 8 &&
+ i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA)) {
+ config->reg_read = regmap_smbus_byte_reg_read;
+ config->reg_write = regmap_smbus_byte_reg_write;
+ return NULL;
+ }
+
+ return ERR_PTR(-ENOTSUPP);
+}
+
/**
* regmap_init_i2c(): Initialise register map
*
@@ -109,7 +194,13 @@ static struct regmap_bus regmap_i2c = {
struct regmap *regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config)
{
- return regmap_init(&i2c->dev, ®map_i2c, &i2c->dev, config);
+ struct regmap_config upd_conf = *config;
+ const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, &upd_conf);
+
+ if (IS_ERR(bus))
+ return ERR_PTR(PTR_ERR(bus));
+
+ return regmap_init(&i2c->dev, bus, &i2c->dev, &upd_conf);
}
EXPORT_SYMBOL_GPL(regmap_init_i2c);
@@ -126,7 +217,13 @@ EXPORT_SYMBOL_GPL(regmap_init_i2c);
struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config)
{
- return devm_regmap_init(&i2c->dev, ®map_i2c, &i2c->dev, config);
+ struct regmap_config upd_conf = *config;
+ const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, &upd_conf);
+
+ if (IS_ERR(bus))
+ return ERR_PTR(PTR_ERR(bus));
+
+ return devm_regmap_init(&i2c->dev, bus, &i2c->dev, &upd_conf);
}
EXPORT_SYMBOL_GPL(devm_regmap_init_i2c);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
@ 2014-04-16 8:16 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-16 8:16 UTC (permalink / raw)
To: linux-arm-kernel
Some I2C adapters are only compatible with the SMBus protocol and do not
support standard I2C transfers.
Fallback to SMBus transfers if we encounter such kind of adapters.
The transfer type is chosen according to the val_bits field in the regmap
config.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark, Lars,
Would this patch be acceptable ?
I'm still not convinced that we can decide which kind of protocol should
be used based only on the adapter functionalities and the val_bytes field.
But doing this as a first step should work in my case (my adapter only
supports SMBus byte transfers :-)).
Note that this patch has not been tested yet. I'll test it and resend a
proper version, if you agree on the implementation.
Best Regards,
Boris
drivers/base/regmap/regmap-i2c.c | 101 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 99 insertions(+), 2 deletions(-)
diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index ebd1895..80051c5 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -14,6 +14,69 @@
#include <linux/i2c.h>
#include <linux/module.h>
+
+static int regmap_smbus_byte_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+ int ret;
+
+ if (reg > 0xff)
+ return -EINVAL;
+
+ ret = i2c_smbus_read_byte_data(i2c, reg);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+
+ return 0;
+}
+
+static int regmap_smbus_byte_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+
+ if (val > 0xff || reg > 0xff)
+ return -EINVAL;
+
+ return i2c_smbus_write_byte_data(i2c, reg, val);
+}
+
+static int regmap_smbus_word_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+ int ret;
+
+ if (reg > 0xffff)
+ return -EINVAL;
+
+ ret = i2c_smbus_read_word_data(i2c, reg);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+
+ return 0;
+}
+
+static int regmap_smbus_word_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct device *dev = context;
+ struct i2c_client *i2c = to_i2c_client(dev);
+
+ if (val > 0xffff || reg > 0xffff)
+ return -EINVAL;
+
+ return i2c_smbus_write_word_data(i2c, reg, val);
+}
+
static int regmap_i2c_write(void *context, const void *data, size_t count)
{
struct device *dev = context;
@@ -97,6 +160,28 @@ static struct regmap_bus regmap_i2c = {
.read = regmap_i2c_read,
};
+const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
+ struct regmap_config *config)
+{
+ if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
+ return ®map_i2c;
+ } else if (config->val_bits == 16 &&
+ i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA)) {
+ config->reg_read = regmap_smbus_word_reg_read;
+ config->reg_write = regmap_smbus_word_reg_write;
+ return NULL;
+ } else if (config->val_bits == 8 &&
+ i2c_check_functionality(i2c->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA)) {
+ config->reg_read = regmap_smbus_byte_reg_read;
+ config->reg_write = regmap_smbus_byte_reg_write;
+ return NULL;
+ }
+
+ return ERR_PTR(-ENOTSUPP);
+}
+
/**
* regmap_init_i2c(): Initialise register map
*
@@ -109,7 +194,13 @@ static struct regmap_bus regmap_i2c = {
struct regmap *regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config)
{
- return regmap_init(&i2c->dev, ®map_i2c, &i2c->dev, config);
+ struct regmap_config upd_conf = *config;
+ const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, &upd_conf);
+
+ if (IS_ERR(bus))
+ return ERR_PTR(PTR_ERR(bus));
+
+ return regmap_init(&i2c->dev, bus, &i2c->dev, &upd_conf);
}
EXPORT_SYMBOL_GPL(regmap_init_i2c);
@@ -126,7 +217,13 @@ EXPORT_SYMBOL_GPL(regmap_init_i2c);
struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config)
{
- return devm_regmap_init(&i2c->dev, ®map_i2c, &i2c->dev, config);
+ struct regmap_config upd_conf = *config;
+ const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, &upd_conf);
+
+ if (IS_ERR(bus))
+ return ERR_PTR(PTR_ERR(bus));
+
+ return devm_regmap_init(&i2c->dev, bus, &i2c->dev, &upd_conf);
}
EXPORT_SYMBOL_GPL(devm_regmap_init_i2c);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
2014-04-16 8:16 ` Boris BREZILLON
@ 2014-04-16 17:06 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-16 17:06 UTC (permalink / raw)
To: Boris BREZILLON
Cc: Lars-Peter Clausen, Greg Kroah-Hartman, Maxime Ripard, Shuge,
kevin, Chen-Yu Tsai, Hans de Goede, Carlo Caione,
linux-arm-kernel, linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 652 bytes --]
On Wed, Apr 16, 2014 at 10:16:10AM +0200, Boris BREZILLON wrote:
> + if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
> + return ®map_i2c;
> + } else if (config->val_bits == 16 &&
> + i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA)) {
> + config->reg_read = regmap_smbus_word_reg_read;
> + config->reg_write = regmap_smbus_word_reg_write;
> + return NULL;
This all looks good to me except we shouldn't be modifying the config
struct (it's supposed to be const). We should instead add the ability
for the bus to set these ops - that'll also be useful for things like
AC'97.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
@ 2014-04-16 17:06 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-16 17:06 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Apr 16, 2014 at 10:16:10AM +0200, Boris BREZILLON wrote:
> + if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
> + return ®map_i2c;
> + } else if (config->val_bits == 16 &&
> + i2c_check_functionality(i2c->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA)) {
> + config->reg_read = regmap_smbus_word_reg_read;
> + config->reg_write = regmap_smbus_word_reg_write;
> + return NULL;
This all looks good to me except we shouldn't be modifying the config
struct (it's supposed to be const). We should instead add the ability
for the bus to set these ops - that'll also be useful for things like
AC'97.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140416/575bc83b/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
2014-04-16 17:06 ` Mark Brown
@ 2014-04-16 17:16 ` Boris BREZILLON
-1 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-16 17:16 UTC (permalink / raw)
To: Mark Brown
Cc: Lars-Peter Clausen, Greg Kroah-Hartman, Maxime Ripard, Shuge,
kevin, Chen-Yu Tsai, Hans de Goede, Carlo Caione,
linux-arm-kernel, linux-kernel, dev
Hello Mark,
On 16/04/2014 19:06, Mark Brown wrote:
> On Wed, Apr 16, 2014 at 10:16:10AM +0200, Boris BREZILLON wrote:
>
>> + if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
>> + return ®map_i2c;
>> + } else if (config->val_bits == 16 &&
>> + i2c_check_functionality(i2c->adapter,
>> + I2C_FUNC_SMBUS_WORD_DATA)) {
>> + config->reg_read = regmap_smbus_word_reg_read;
>> + config->reg_write = regmap_smbus_word_reg_write;
>> + return NULL;
> This all looks good to me except we shouldn't be modifying the config
> struct (it's supposed to be const). We should instead add the ability
> for the bus to set these ops - that'll also be useful for things like
> AC'97.
Actually I do not modify the passed config struct, I copy the config
values into a local instance and then modify this local instance
appropriately.
But I agree that having a proper way to overload config parameters would
be better.
Any suggestion on how this should be done ?
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
@ 2014-04-16 17:16 ` Boris BREZILLON
0 siblings, 0 replies; 40+ messages in thread
From: Boris BREZILLON @ 2014-04-16 17:16 UTC (permalink / raw)
To: linux-arm-kernel
Hello Mark,
On 16/04/2014 19:06, Mark Brown wrote:
> On Wed, Apr 16, 2014 at 10:16:10AM +0200, Boris BREZILLON wrote:
>
>> + if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
>> + return ®map_i2c;
>> + } else if (config->val_bits == 16 &&
>> + i2c_check_functionality(i2c->adapter,
>> + I2C_FUNC_SMBUS_WORD_DATA)) {
>> + config->reg_read = regmap_smbus_word_reg_read;
>> + config->reg_write = regmap_smbus_word_reg_write;
>> + return NULL;
> This all looks good to me except we shouldn't be modifying the config
> struct (it's supposed to be const). We should instead add the ability
> for the bus to set these ops - that'll also be useful for things like
> AC'97.
Actually I do not modify the passed config struct, I copy the config
values into a local instance and then modify this local instance
appropriately.
But I agree that having a proper way to overload config parameters would
be better.
Any suggestion on how this should be done ?
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
2014-04-16 17:16 ` Boris BREZILLON
@ 2014-04-16 21:00 ` Mark Brown
-1 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-16 21:00 UTC (permalink / raw)
To: Boris BREZILLON
Cc: Lars-Peter Clausen, Greg Kroah-Hartman, Maxime Ripard, Shuge,
kevin, Chen-Yu Tsai, Hans de Goede, Carlo Caione,
linux-arm-kernel, linux-kernel, dev
[-- Attachment #1: Type: text/plain, Size: 610 bytes --]
On Wed, Apr 16, 2014 at 07:16:14PM +0200, Boris BREZILLON wrote:
> Actually I do not modify the passed config struct, I copy the config
> values into a local instance and then modify this local instance
> appropriately.
> But I agree that having a proper way to overload config parameters would
> be better.
> Any suggestion on how this should be done ?
I think just adding them as operations in the bus struct should work,
though I only had a fairly quick scan through so I might have missed a
case where it's more than just the init that needs updating to take
account of there being a bus with these ops.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C
@ 2014-04-16 21:00 ` Mark Brown
0 siblings, 0 replies; 40+ messages in thread
From: Mark Brown @ 2014-04-16 21:00 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Apr 16, 2014 at 07:16:14PM +0200, Boris BREZILLON wrote:
> Actually I do not modify the passed config struct, I copy the config
> values into a local instance and then modify this local instance
> appropriately.
> But I agree that having a proper way to overload config parameters would
> be better.
> Any suggestion on how this should be done ?
I think just adding them as operations in the bus struct should work,
though I only had a fairly quick scan through so I might have missed a
case where it's more than just the init that needs updating to take
account of there being a bus with these ops.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140416/d3bbe62f/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2014-04-16 21:00 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-12 8:45 [RFC PATCH] regmap: smbus: add support for regmap over SMBus Boris BREZILLON
2014-04-12 8:45 ` Boris BREZILLON
2014-04-12 9:02 ` Boris BREZILLON
2014-04-12 9:02 ` Boris BREZILLON
2014-04-14 13:08 ` [RFC PATCH v2] " Boris BREZILLON
2014-04-14 13:08 ` Boris BREZILLON
2014-04-14 21:04 ` Mark Brown
2014-04-14 21:04 ` Mark Brown
2014-04-15 7:36 ` Boris BREZILLON
2014-04-15 7:36 ` Boris BREZILLON
2014-04-15 10:09 ` Mark Brown
2014-04-15 10:09 ` Mark Brown
2014-04-15 11:54 ` Boris BREZILLON
2014-04-15 11:54 ` Boris BREZILLON
2014-04-15 12:10 ` Mark Brown
2014-04-15 12:10 ` Mark Brown
2014-04-15 12:25 ` Lars-Peter Clausen
2014-04-15 12:25 ` Lars-Peter Clausen
2014-04-15 12:40 ` Boris BREZILLON
2014-04-15 12:40 ` Boris BREZILLON
2014-04-15 13:08 ` Lars-Peter Clausen
2014-04-15 13:08 ` Lars-Peter Clausen
2014-04-15 12:56 ` [RFC PATCH v2] regmap: smbus: add support for regmap over smbus Mark Brown
2014-04-15 12:56 ` Mark Brown
2014-04-15 13:34 ` Lars-Peter Clausen
2014-04-15 13:34 ` Lars-Peter Clausen
2014-04-15 16:46 ` Mark Brown
2014-04-15 16:46 ` Mark Brown
2014-04-15 19:18 ` Lars-Peter Clausen
2014-04-15 19:18 ` Lars-Peter Clausen
2014-04-15 22:38 ` Mark Brown
2014-04-15 22:38 ` Mark Brown
2014-04-16 8:16 ` [PATCH] regmap: i2c: fallback to SMBus if the adapter does not support standard I2C Boris BREZILLON
2014-04-16 8:16 ` Boris BREZILLON
2014-04-16 17:06 ` Mark Brown
2014-04-16 17:06 ` Mark Brown
2014-04-16 17:16 ` Boris BREZILLON
2014-04-16 17:16 ` Boris BREZILLON
2014-04-16 21:00 ` Mark Brown
2014-04-16 21:00 ` Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.